diff --git a/caffold/src/agent/claude/served_tools.rs b/caffold/src/agent/claude/served_tools.rs index afa48cec..0821c880 100644 --- a/caffold/src/agent/claude/served_tools.rs +++ b/caffold/src/agent/claude/served_tools.rs @@ -203,6 +203,7 @@ fn tool_ask(message: &Value, request_id: &str, mcp_id: &Value) -> Result for NoteContent { + fn from(content: String) -> Self { + Self::Text(content) + } +} + /// A Notes tool call whose arguments have been checked. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum NotesToolCall { @@ -19,12 +36,12 @@ pub(crate) enum NotesToolCall { }, CreateNote { name: String, - content: String, + content: NoteContent, directory_id: Option, }, UpdateNoteContent { note_id: String, - content: String, + content: NoteContent, expected_content_version: i64, }, RenameNote { @@ -55,6 +72,17 @@ pub(crate) enum NotesToolCall { }, } +impl NotesToolCall { + pub(crate) fn needs_working_directory(&self) -> bool { + match self { + Self::CreateNote { content, .. } | Self::UpdateNoteContent { content, .. } => { + matches!(content, NoteContent::File(path) if !Path::new(path).is_absolute()) + } + _ => false, + } + } +} + pub(crate) struct NotesToolSpec { pub(crate) name: &'static str, pub(crate) description: &'static str, @@ -81,7 +109,7 @@ pub(crate) fn notes_tool_call( CREATE_NOTE => parse::(tool, arguments).and_then(|arguments| { Ok(NotesToolCall::CreateNote { name: required_text("name", arguments.name)?, - content: arguments.content, + content: note_content(arguments.content, arguments.content_path)?, directory_id: optional_text("directoryId", arguments.directory_id)?, }) }), @@ -95,7 +123,7 @@ pub(crate) fn notes_tool_call( } Ok(NotesToolCall::UpdateNoteContent { note_id: required_text("noteId", arguments.note_id)?, - content: arguments.content, + content: note_content(arguments.content, arguments.content_path)?, expected_content_version: arguments.expected_content_version, }) }) @@ -161,7 +189,17 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { [ NotesToolSpec { name: LIST_NOTES, - description: "List what one Caffold Note directory directly holds: its directories, each with how many directories and Notes it directly holds, and its Notes, with ids, names, and last change times but no Note content. Omit directoryId for the top of the Notes tree, and pass a listed directory's id to look one level down. Caffold Notes are durable records shared by every Caffold task and kept after a task ends; they are not the current task's plan documents under .caffold/plans/current. Find and read Caffold Notes only through these Notes tools, not in the Caffold app, a browser, or on screen.", + description: concat!( + "List what one Caffold Note directory directly holds: its directories, ", + "each with how many directories and Notes it directly holds, ", + "and its Notes, with ids, names, and last change times but no Note content. ", + "Omit directoryId for the top of the Notes tree, ", + "and pass a listed directory's id to look one level down. ", + "Caffold Notes are durable records shared by every Caffold task and kept after a task ends; ", + "they are not the current task's plan documents under .caffold/plans/current. ", + "Find and read Caffold Notes only through these Notes tools, ", + "not in the Caffold app, a browser, or on screen.", + ), input_schema: object_schema( json!({ "directoryId": text_schema("The Note directory to list. Omit it for the top of the Notes tree."), @@ -171,7 +209,12 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: READ_NOTE, - description: "Read one Caffold Note: its name, directory, Markdown content, and contentVersion. Pass that contentVersion to update_note_content. Find and read Caffold Notes only through these Notes tools, not in the Caffold app, a browser, or on screen.", + description: concat!( + "Read one Caffold Note: its name, directory, Markdown content, and contentVersion. ", + "Pass that contentVersion to update_note_content. ", + "Find and read Caffold Notes only through these Notes tools, ", + "not in the Caffold app, a browser, or on screen.", + ), input_schema: object_schema( json!({ "noteId": text_schema("The id of the Note to read.") }), &["noteId"], @@ -179,30 +222,71 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: CREATE_NOTE, - description: "Create a Caffold Note only when the user explicitly asks to save something as a note. Notes are durable records shared by every Caffold task; keep the current task's own plan in .caffold/plans/current instead. Omit directoryId to place the Note at the top of the Notes tree.", - input_schema: object_schema( + description: concat!( + "Create a Caffold Note only when the user explicitly asks to save something as a note. ", + "Notes are durable records shared by every Caffold task; ", + "keep the current task's own plan in .caffold/plans/current instead. ", + "Omit directoryId to place the Note at the top of the Notes tree. ", + "Supply exactly one of content or contentPath. ", + "Use contentPath when the content is an existing file, such as a finished plan. ", + "Relative paths start in this Task's working directory; absolute paths are accepted. ", + "After following symlinks, the file must be inside the server's filesystem root, ", + "which may include files outside this Task's directory. ", + "The file must be regular, UTF-8, and at most 1 MiB (1,048,576 bytes); ", + "its content is stored unchanged.", + ), + input_schema: note_content_schema( json!({ "name": text_schema("The Note's name."), "content": content_schema("The Note's Markdown content."), + "contentPath": text_schema(concat!( + "An existing UTF-8 file to use as the Note's content. ", + "Relative to this Task's working directory, ", + "or an absolute path within the server's filesystem root. ", + "Maximum 1 MiB. ", + "Supply this or content, exactly one.", + )), "directoryId": text_schema("The Note directory to create the Note in. Omit it for the top of the Notes tree."), }), - &["name", "content"], + &["name"], ), }, NotesToolSpec { name: UPDATE_NOTE_CONTENT, - description: "Replace a Caffold Note's entire Markdown content only when the user explicitly asks to change that Note. Caffold keeps no history, so the replaced content cannot be recovered. Pass the contentVersion read_note returned: if the Note changed since then nothing is written and the result gives the current contentVersion, so read the Note again, reapply the change, and retry.", - input_schema: object_schema( + description: concat!( + "Replace a Caffold Note's entire Markdown content ", + "only when the user explicitly asks to change that Note. ", + "Caffold keeps no history, so the replaced content cannot be recovered. ", + "Pass the contentVersion read_note returned: ", + "if the Note changed since then nothing is written ", + "and the result gives the current contentVersion, ", + "so read the Note again, reapply the change, and retry. ", + "Supply exactly one of content or contentPath. ", + "Use contentPath when the complete new content is an existing file. ", + "Relative paths start in this Task's working directory; absolute paths are accepted. ", + "After following symlinks, the file must be inside the server's filesystem root, ", + "which may include files outside this Task's directory. ", + "The file must be regular, UTF-8, and at most 1 MiB (1,048,576 bytes); ", + "its content is stored unchanged.", + ), + input_schema: note_content_schema( json!({ "noteId": text_schema("The id of the Note to change."), "content": content_schema("The Note's complete new Markdown content."), + "contentPath": text_schema(concat!( + "An existing UTF-8 file containing the Note's complete new content. ", + "Relative to this Task's working directory, ", + "or an absolute path within the server's filesystem root. ", + "Maximum 1 MiB. ", + "Supply this or content, exactly one.", + )), "expectedContentVersion": { "type": "integer", "minimum": 1, "description": "The contentVersion read_note returned for the content this change starts from.", }, }), - &["noteId", "content", "expectedContentVersion"], + &["noteId", "expectedContentVersion"], ), }, NotesToolSpec { @@ -218,7 +302,10 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: MOVE_NOTE, - description: "Move a Caffold Note into another Note directory only when the user explicitly asks. Omit directoryId to move it to the top of the Notes tree.", + description: concat!( + "Move a Caffold Note into another Note directory only when the user explicitly asks. ", + "Omit directoryId to move it to the top of the Notes tree.", + ), input_schema: object_schema( json!({ "noteId": text_schema("The id of the Note to move."), @@ -229,7 +316,10 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: DELETE_NOTE, - description: "Delete a Caffold Note only when the user explicitly asks. Caffold keeps no history, so a deleted Note cannot be recovered.", + description: concat!( + "Delete a Caffold Note only when the user explicitly asks. ", + "Caffold keeps no history, so a deleted Note cannot be recovered.", + ), input_schema: object_schema( json!({ "noteId": text_schema("The id of the Note to delete.") }), &["noteId"], @@ -237,7 +327,11 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: CREATE_NOTE_DIRECTORY, - description: "Create a directory in the Caffold Notes tree only when the user explicitly asks to create or organize Notes. Omit parentDirectoryId to create it at the top of the Notes tree.", + description: concat!( + "Create a directory in the Caffold Notes tree ", + "only when the user explicitly asks to create or organize Notes. ", + "Omit parentDirectoryId to create it at the top of the Notes tree.", + ), input_schema: object_schema( json!({ "name": text_schema("The directory's name."), @@ -259,7 +353,12 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: MOVE_NOTE_DIRECTORY, - description: "Move a directory in the Caffold Notes tree, with everything in it, only when the user explicitly asks. A directory cannot move into itself or one of its subdirectories. Omit parentDirectoryId to move it to the top of the Notes tree.", + description: concat!( + "Move a directory in the Caffold Notes tree, with everything in it, ", + "only when the user explicitly asks. ", + "A directory cannot move into itself or one of its subdirectories. ", + "Omit parentDirectoryId to move it to the top of the Notes tree.", + ), input_schema: object_schema( json!({ "directoryId": text_schema("The id of the Note directory to move."), @@ -270,7 +369,13 @@ pub(crate) fn notes_tool_specs() -> [NotesToolSpec; 11] { }, NotesToolSpec { name: DELETE_NOTE_DIRECTORY, - description: "Delete an empty directory from the Caffold Notes tree only when the user explicitly asks. A directory that still holds Notes or directories is not deleted; delete or move what it holds first. Caffold keeps no history, so a deleted directory cannot be recovered.", + description: concat!( + "Delete an empty directory from the Caffold Notes tree ", + "only when the user explicitly asks. ", + "A directory that still holds Notes or directories is not deleted; ", + "delete or move what it holds first. ", + "Caffold keeps no history, so a deleted directory cannot be recovered.", + ), input_schema: object_schema( json!({ "directoryId": text_schema("The id of the empty Note directory to delete.") }), &["directoryId"], @@ -307,7 +412,10 @@ struct NoteArguments { #[serde(rename_all = "camelCase", deny_unknown_fields)] struct CreateNoteArguments { name: String, - content: String, + #[serde(default, deserialize_with = "present_string")] + content: Option, + #[serde(default, deserialize_with = "present_string")] + content_path: Option, directory_id: Option, } @@ -315,7 +423,10 @@ struct CreateNoteArguments { #[serde(rename_all = "camelCase", deny_unknown_fields)] struct UpdateNoteContentArguments { note_id: String, - content: String, + #[serde(default, deserialize_with = "present_string")] + content: Option, + #[serde(default, deserialize_with = "present_string")] + content_path: Option, expected_content_version: i64, } @@ -384,6 +495,33 @@ fn optional_text(field: &str, value: Option) -> Result, S value.map(|value| required_text(field, value)).transpose() } +fn present_string<'de, D: serde::Deserializer<'de>>( + deserializer: D, +) -> Result, D::Error> { + String::deserialize(deserializer).map(Some) +} + +fn note_content( + content: Option, + content_path: Option, +) -> Result { + match (content, content_path) { + (Some(content), None) => Ok(NoteContent::Text(content)), + (None, Some(path)) if !path.is_empty() => Ok(NoteContent::File(path)), + (None, Some(_)) => Err("`contentPath` must be a non-empty file path.".to_string()), + _ => Err("Supply exactly one of `content` or `contentPath`.".to_string()), + } +} + +fn note_content_schema(properties: Value, required: &[&str]) -> Value { + let mut schema = object_schema(properties, required); + schema["oneOf"] = json!([ + { "required": ["content"] }, + { "required": ["contentPath"] }, + ]); + schema +} + fn object_schema(properties: Value, required: &[&str]) -> Value { json!({ "type": "object", @@ -492,6 +630,102 @@ mod tests { assert!(notes_tool_call("", &Value::Null).is_none()); } + #[test] + fn content_input_requires_exactly_one_string_and_keeps_file_paths_verbatim() { + for (tool, base) in [ + ("create_note", json!({"name": "File"})), + ( + "update_note_content", + json!({"noteId": "note", "expectedContentVersion": 1}), + ), + ] { + for invalid in [ + json!({}), + json!({"content": "", "contentPath": "file.md"}), + json!({"content": null}), + json!({"contentPath": null}), + json!({"content": "x", "contentPath": null}), + json!({"content": null, "contentPath": "file.md"}), + json!({"contentPath": ""}), + json!({"contentPath": 1}), + json!({"content": []}), + ] { + let mut arguments = base.clone(); + arguments + .as_object_mut() + .unwrap() + .extend(invalid.as_object().unwrap().clone()); + assert!( + call(tool, arguments.clone()).is_err(), + "{tool}: {arguments}" + ); + } + for (input, expected, needs_cwd) in [ + ( + json!({"content": ""}), + NoteContent::Text(String::new()), + false, + ), + ( + json!({"contentPath": " ../shared/file.md "}), + NoteContent::File(" ../shared/file.md ".to_string()), + true, + ), + ( + json!({"contentPath": "/workspace/file.md"}), + NoteContent::File("/workspace/file.md".to_string()), + false, + ), + ] { + let mut arguments = base.clone(); + arguments + .as_object_mut() + .unwrap() + .extend(input.as_object().unwrap().clone()); + let parsed = call(tool, arguments).unwrap(); + assert_eq!(parsed.needs_working_directory(), needs_cwd); + match parsed { + NotesToolCall::CreateNote { content, .. } + | NotesToolCall::UpdateNoteContent { content, .. } => { + assert_eq!(content, expected) + } + _ => panic!("expected a writing tool"), + } + } + } + } + + #[test] + fn writing_schemas_offer_exclusive_content_sources_and_document_the_file_boundary() { + for spec in notes_tool_specs() + .into_iter() + .filter(|spec| matches!(spec.name, "create_note" | "update_note_content")) + { + assert_eq!( + spec.input_schema["oneOf"], + json!([{"required": ["content"]}, {"required": ["contentPath"]}]) + ); + assert!( + !spec.input_schema["required"] + .as_array() + .unwrap() + .contains(&json!("content")) + ); + assert_eq!( + spec.input_schema["properties"]["contentPath"]["minLength"], + 1 + ); + for phrase in [ + "Use contentPath", + "Task's working directory", + "server's filesystem root", + "1,048,576 bytes", + ] { + assert!(spec.description.contains(phrase), "{}: {phrase}", spec.name); + } + } + } + #[test] fn arguments_become_trimmed_calls() { assert_eq!( @@ -515,7 +749,7 @@ mod tests { ), Ok(NotesToolCall::CreateNote { name: "Decisions".to_string(), - content: String::new(), + content: String::new().into(), directory_id: Some("dir".to_string()), }) ); @@ -526,7 +760,7 @@ mod tests { ), Ok(NotesToolCall::UpdateNoteContent { note_id: "note".to_string(), - content: " kept as written ".to_string(), + content: " kept as written ".to_string().into(), expected_content_version: 3, }) ); diff --git a/caffold/src/app/notes.rs b/caffold/src/app/notes.rs index 26ae1cff..d65f788f 100644 --- a/caffold/src/app/notes.rs +++ b/caffold/src/app/notes.rs @@ -5,7 +5,11 @@ //! browser only reads. Directories and Notes live in two tables, so each rule //! that spans both is checked here inside one store transaction. -use std::collections::{HashMap, HashSet}; +use std::{ + collections::{HashMap, HashSet}, + path::{Path as FilePath, PathBuf}, + sync::Arc, +}; use axum::{ Json, Router, @@ -20,12 +24,15 @@ use uuid::Uuid; use super::error::ApiError; use crate::{ agent::notes_tools::NotesToolCall, + fs::RootedFs, task_store::{ Note, NoteContentUpdate, NoteDirectory, NoteSummary, TaskStore, TaskStoreError, TaskStoreTables, }, }; +mod content; + pub(in crate::app) fn router(store: TaskStore) -> Router { Router::new() .route("/api/notes", get(notes_level)) @@ -39,10 +46,21 @@ pub(in crate::app) async fn execute_tool( store: TaskStore, thread_id: String, call: NotesToolCall, + fs: Option>, + task_cwd: Option, ) -> Result { - tokio::task::spawn_blocking(move || apply_tool(&store, &thread_id, call, now_ms())) - .await - .map_err(|error| format!("Caffold's Notes worker failed: {error}"))? + tokio::task::spawn_blocking(move || { + apply_tool( + &store, + &thread_id, + call, + now_ms(), + fs.as_deref(), + task_cwd.as_deref(), + ) + }) + .await + .map_err(|error| format!("Caffold's Notes worker failed: {error}"))? } #[derive(Clone)] @@ -342,6 +360,8 @@ fn apply_tool( thread_id: &str, call: NotesToolCall, now_ms: u64, + fs: Option<&RootedFs>, + task_cwd: Option<&FilePath>, ) -> Result { match call { NotesToolCall::ListNotes { directory_id } => list_notes(store, directory_id.as_deref()), @@ -356,7 +376,7 @@ fn apply_tool( note_id: Uuid::new_v4().to_string(), directory_id, name, - content, + content: content::resolve(content, fs, task_cwd)?, content_version: 1, created_by_thread_id: thread_id.to_string(), updated_by_thread_id: thread_id.to_string(), @@ -371,7 +391,7 @@ fn apply_tool( } => update_note_content( store, ¬e_id, - &content, + &content::resolve(content, fs, task_cwd)?, expected_content_version, thread_id, now_ms, @@ -723,8 +743,179 @@ mod tests { }; use tower::ServiceExt; - use super::*; + use super::{apply_tool as apply_with_context, *}; + use crate::agent::notes_tools::{MAX_NOTE_FILE_BYTES, NoteContent, notes_tool_call}; use crate::task_store::{ManagedThread, RunBy}; + use std::fs; + + fn apply_tool( + store: &TaskStore, + thread_id: &str, + call: NotesToolCall, + now_ms: u64, + ) -> Result { + apply_with_context(store, thread_id, call, now_ms, None, None) + } + + #[tokio::test] + async fn file_input_round_trips_through_the_store_and_failures_preserve_persisted_notes() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("root"); + fs::create_dir_all(root.join("backend")).unwrap(); + fs::create_dir(root.join("shared")).unwrap(); + let fs = Arc::new(RootedFs::new(&root).unwrap()); + let cwd = fs.root().join("backend"); + let path = fs.root().join("shared/result.md"); + let original = "\u{feff}# 결과\r\n\0 spaces \r\n"; + fs::write(&path, original).unwrap(); + let store_path = temp.path().join("notes.redb"); + let store = TaskStore::redb(&store_path).unwrap(); + let parse = |tool: &str, args: Value| notes_tool_call(tool, &args).unwrap().unwrap(); + let created = execute_tool( + store.clone(), + "writer".to_string(), + parse( + "create_note", + json!({"name": "Result", "contentPath": "../shared/result.md"}), + ), + Some(fs.clone()), + Some(cwd.clone()), + ) + .await + .unwrap(); + let created: Value = serde_json::from_str(&created).unwrap(); + assert!(created.get("content").is_none()); + let note_id = created["noteId"].as_str().unwrap(); + let initial = store.read(|tables| tables.note(note_id)).unwrap().unwrap(); + assert_eq!(initial.content.as_bytes(), original.as_bytes()); + assert_eq!(initial.content_version, 1); + assert_eq!(initial.created_by_thread_id, "writer"); + + fs::write(&path, "# Updated\n").unwrap(); + let updated = execute_tool( + store.clone(), + "editor".to_string(), + parse( + "update_note_content", + json!({"noteId": note_id, "contentPath": path, "expectedContentVersion": 1}), + ), + Some(fs.clone()), + None, + ) + .await + .unwrap(); + assert_eq!( + serde_json::from_str::(&updated).unwrap()["contentVersion"], + 2 + ); + let expected = store.read(|tables| tables.note(note_id)).unwrap().unwrap(); + assert_eq!(expected.content, "# Updated\n"); + assert_eq!(expected.updated_by_thread_id, "editor"); + let stale = execute_tool( + store.clone(), + "stale".to_string(), + parse( + "update_note_content", + json!({"noteId": note_id, "contentPath": path, "expectedContentVersion": 1}), + ), + Some(fs.clone()), + None, + ) + .await + .unwrap_err(); + assert!(stale.contains("contentVersion is now 2"), "{stale}"); + + fs::write(root.join("invalid.md"), [0xff]).unwrap(); + fs::write( + root.join("large.md"), + vec![b'a'; MAX_NOTE_FILE_BYTES as usize + 1], + ) + .unwrap(); + fs::write(temp.path().join("outside.md"), "outside").unwrap(); + for path in [ + root.join("missing.md"), + root.join("invalid.md"), + root.join("large.md"), + root.join("backend"), + temp.path().join("outside.md"), + ] { + for (tool, arguments) in [ + ( + "create_note", + json!({"name": "Rejected", "contentPath": path}), + ), + ( + "update_note_content", + json!({"noteId": note_id, "contentPath": path, "expectedContentVersion": 2}), + ), + ] { + assert!( + execute_tool( + store.clone(), + "rejected".to_string(), + parse(tool, arguments), + Some(fs.clone()), + None + ) + .await + .is_err() + ); + assert_eq!( + store.read(|tables| tables.note_summaries()).unwrap().len(), + 1 + ); + assert_eq!( + store.read(|tables| tables.note(note_id)).unwrap().unwrap(), + expected + ); + } + } + drop(store); + let reopened = TaskStore::redb(&store_path).unwrap(); + assert_eq!( + reopened + .read(|tables| tables.note(note_id)) + .unwrap() + .unwrap(), + expected + ); + } + + #[test] + fn content_read_from_a_file_cannot_overwrite_a_note_changed_before_the_write() { + let temp = tempfile::tempdir().unwrap(); + let fs = RootedFs::new(temp.path()).unwrap(); + fs::write(temp.path().join("file.md"), "from file").unwrap(); + let store = TaskStore::memory().unwrap(); + let created = apply( + &store, + "writer", + NotesToolCall::CreateNote { + name: "Result".to_string(), + content: "original".to_string().into(), + directory_id: None, + }, + 1, + ); + let note_id = created["noteId"].as_str().unwrap(); + let prepared = content::resolve( + NoteContent::File("file.md".to_string()), + Some(&fs), + Some(fs.root()), + ) + .unwrap(); + update_note_content(&store, note_id, "other writer", 1, "other", 2).unwrap(); + let before = store.read(|tables| tables.note(note_id)).unwrap().unwrap(); + assert!( + update_note_content(&store, note_id, &prepared, 1, "late writer", 3) + .unwrap_err() + .contains("contentVersion is now 2") + ); + assert_eq!( + store.read(|tables| tables.note(note_id)).unwrap().unwrap(), + before + ); + } fn apply(store: &TaskStore, thread_id: &str, call: NotesToolCall, now_ms: u64) -> Value { let answer = apply_tool(store, thread_id, call, now_ms).unwrap(); @@ -763,7 +954,7 @@ mod tests { "task-a", NotesToolCall::CreateNote { name: "Storage".to_string(), - content: "# Storage\n".to_string(), + content: "# Storage\n".to_string().into(), directory_id: Some(decisions.clone()), }, 2_000, @@ -814,7 +1005,7 @@ mod tests { "task-b", NotesToolCall::UpdateNoteContent { note_id: note_id.clone(), - content: "# Storage\n\nUse redb.\n".to_string(), + content: "# Storage\n\nUse redb.\n".to_string().into(), expected_content_version: 1, }, 4_000, @@ -825,7 +1016,7 @@ mod tests { "task-c", NotesToolCall::UpdateNoteContent { note_id: note_id.clone(), - content: "lost".to_string(), + content: "lost".to_string().into(), expected_content_version: 1, }, 5_000, @@ -926,7 +1117,7 @@ mod tests { "task", NotesToolCall::CreateNote { name: name.to_string(), - content: String::new(), + content: String::new().into(), directory_id: directory.map(str::to_string), }, 1_000, @@ -973,7 +1164,7 @@ mod tests { "task", NotesToolCall::CreateNote { name: name.to_string(), - content: String::new(), + content: String::new().into(), directory_id: None, }, 1_000, @@ -1017,7 +1208,7 @@ mod tests { "task", NotesToolCall::CreateNote { name: "Loose".to_string(), - content: String::new(), + content: String::new().into(), directory_id: None, }, 1_000, @@ -1088,7 +1279,7 @@ mod tests { }, NotesToolCall::UpdateNoteContent { note_id: "gone".to_string(), - content: "x".to_string(), + content: "x".to_string().into(), expected_content_version: 1, }, NotesToolCall::RenameNote { @@ -1113,7 +1304,7 @@ mod tests { let calls = [ NotesToolCall::CreateNote { name: "Orphan".to_string(), - content: String::new(), + content: String::new().into(), directory_id: Some("gone".to_string()), }, create_directory_call("Orphan", Some("gone")), @@ -1224,7 +1415,7 @@ mod tests { "task-writer", NotesToolCall::CreateNote { name: "Storage".to_string(), - content: "# Storage\n".to_string(), + content: "# Storage\n".to_string().into(), directory_id: Some(directory.clone()), }, 2_000, diff --git a/caffold/src/app/notes/content.rs b/caffold/src/app/notes/content.rs new file mode 100644 index 00000000..429374a0 --- /dev/null +++ b/caffold/src/app/notes/content.rs @@ -0,0 +1,371 @@ +//! Resolve Task-relative Note input against the server's filesystem boundary. +//! Symlinks are resolved once; descriptor-relative opens then refuse replaced +//! symlinks in every component, including the path to the server root. + +use std::{ + fs::File, + io::{self, Read}, + path::{Component, Path}, +}; + +use rustix::fs::{AtFlags, FileType, Mode, OFlags, open, openat, statat}; + +use crate::{ + agent::notes_tools::{MAX_NOTE_FILE_BYTES, NoteContent}, + fs::RootedFs, +}; + +pub(super) fn resolve( + content: NoteContent, + fs: Option<&RootedFs>, + task_cwd: Option<&Path>, +) -> Result { + let path = match content { + NoteContent::Text(text) => return Ok(text), + NoteContent::File(path) => path, + }; + let fs = fs.ok_or_else(|| "Caffold's Notes file access is unavailable.".to_string())?; + let requested = Path::new(&path); + let candidate = if requested.is_absolute() { + requested.to_path_buf() + } else { + let cwd = task_cwd.filter(|cwd| cwd.is_absolute()).ok_or_else(|| { + "Caffold could not determine this Task's working directory for `contentPath`. Retry when the Task directory is available, or use an absolute path inside the server's filesystem root.".to_string() + })?; + cwd.join(requested) + }; + let absolute = candidate.canonicalize().map_err(|error| { + format!( + "Caffold could not resolve `contentPath` `{path}` inside the server's filesystem root: {error}. Check that the file exists, is readable, and stays inside that root." + ) + })?; + // Keep the canonical path as OS components: a symlink target can contain + // non-UTF-8 names even though the tool's supplied path is a JSON string. + let logical = absolute.strip_prefix(fs.root()).map_err(|_| { + format!("`contentPath` `{path}` resolves outside the server's filesystem root. Choose a file inside that root.") + })?; + let file = open_resolved(fs.root(), logical).map_err(|error| { + format!( + "Caffold could not safely open `contentPath` `{path}` as a regular file: {error}. Check permissions and retry if the path changed." + ) + })?; + let metadata = file + .metadata() + .map_err(|error| format!("Caffold could not inspect `contentPath` `{path}`: {error}."))?; + if !metadata.is_file() { + return Err(format!("`contentPath` `{path}` must name a regular file.")); + } + if metadata.len() > MAX_NOTE_FILE_BYTES { + return Err(too_large(&path)); + } + read_opened(file, metadata.len(), &path) +} + +fn open_resolved(root: &Path, logical: &Path) -> io::Result { + let mut directory = open("/", directory_flags(), Mode::empty())?; + // Both paths come from RootedFs canonicalization. Starting at / also + // refuses a symlink installed in an ancestor of the configured root. + for component in root + .components() + .chain(logical.parent().into_iter().flat_map(Path::components)) + { + match component { + Component::RootDir => {} + Component::Normal(name) => { + directory = openat(&directory, name, directory_flags(), Mode::empty())?; + } + _ => return Err(io::Error::other("the resolved file path changed; retry")), + } + } + let name = logical + .file_name() + .ok_or_else(|| io::Error::other("not a regular file"))?; + let metadata = statat(&directory, name, AtFlags::SYMLINK_NOFOLLOW)?; + if FileType::from_raw_mode(metadata.st_mode) != FileType::RegularFile { + return Err(io::Error::other( + "not a regular file, or the path changed; retry", + )); + } + // NONBLOCK keeps a regular file swapped for a FIFO from blocking open. + // The opened handle is checked again before reading any bytes. + Ok(File::from(openat( + &directory, + name, + OFlags::RDONLY | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::NONBLOCK, + Mode::empty(), + )?)) +} + +fn directory_flags() -> OFlags { + // Traversal needs search permission, not permission to list a directory. + #[cfg(target_os = "macos")] + let access = OFlags::from_bits_retain(libc::O_SEARCH as _); + #[cfg(target_os = "linux")] + let access = OFlags::PATH; + #[cfg(not(any(target_os = "macos", target_os = "linux")))] + let access = OFlags::RDONLY; + access | OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::DIRECTORY +} + +fn read_opened(file: File, checked_size: u64, path: &str) -> Result { + let mut bytes = Vec::with_capacity(checked_size as usize); + file.take(MAX_NOTE_FILE_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|error| format!("Caffold could not read `contentPath` `{path}`: {error}."))?; + if bytes.len() as u64 > MAX_NOTE_FILE_BYTES { + return Err(too_large(path)); + } + String::from_utf8(bytes).map_err(|_| { + format!("`contentPath` `{path}` is not valid UTF-8. Save the file as UTF-8 and retry.") + }) +} + +fn too_large(path: &str) -> String { + format!( + "`contentPath` `{path}` exceeds the 1 MiB limit ({MAX_NOTE_FILE_BYTES} bytes). Use a smaller file; nothing was written." + ) +} + +#[cfg(test)] +mod tests { + use std::{ + fs, + os::unix::{ + fs::{PermissionsExt, symlink}, + net::UnixListener, + }, + process::Command, + }; + + use super::*; + + #[test] + fn task_relative_and_absolute_paths_read_siblings_and_preserve_every_utf8_byte() { + let temp = tempfile::tempdir().unwrap(); + let rooted = RootedFs::new(temp.path()).unwrap(); + let cwd = rooted.root().join("backend"); + fs::create_dir(&cwd).unwrap(); + fs::create_dir(rooted.root().join("shared")).unwrap(); + let expected = "\u{feff}# 한글\r\n\0 keep trailing spaces \r\n"; + let file = rooted.root().join("shared/ result.md "); + fs::write(&file, expected).unwrap(); + symlink(&file, cwd.join("linked.md")).unwrap(); + for path in [ + "../shared/ result.md ".to_string(), + file.display().to_string(), + "linked.md".to_string(), + ] { + let content = resolve(NoteContent::File(path), Some(&rooted), Some(&cwd)).unwrap(); + assert_eq!(content.as_bytes(), expected.as_bytes()); + } + assert_eq!( + resolve( + NoteContent::File(file.display().to_string()), + Some(&rooted), + None + ) + .unwrap(), + expected + ); + assert!( + resolve( + NoteContent::File("linked.md".to_string()), + Some(&rooted), + None + ) + .unwrap_err() + .contains("working directory") + ); + assert!( + resolve( + NoteContent::File("linked.md".to_string()), + Some(&rooted), + Some(Path::new("backend")) + ) + .is_err() + ); + assert!( + resolve(NoteContent::File(file.display().to_string()), None, None) + .unwrap_err() + .contains("unavailable") + ); + assert_eq!( + resolve(NoteContent::Text("inline".to_string()), None, None).unwrap(), + "inline" + ); + let global = RootedFs::from_filesystem_root().unwrap(); + assert_eq!( + resolve( + NoteContent::File(file.display().to_string()), + Some(&global), + None + ) + .unwrap(), + expected + ); + } + + #[test] + fn files_outside_the_server_root_and_non_regular_files_are_refused() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("root"); + let sibling = temp.path().join("root-other"); + fs::create_dir(&root).unwrap(); + fs::create_dir(&sibling).unwrap(); + fs::write(sibling.join("secret.md"), "outside").unwrap(); + symlink(&sibling, root.join("outside")).unwrap(); + symlink(root.join("missing"), root.join("broken")).unwrap(); + let rooted = RootedFs::new(&root).unwrap(); + let socket = root.join("socket"); + let _listener = UnixListener::bind(&socket).unwrap(); + assert!( + Command::new("mkfifo") + .arg(root.join("fifo")) + .status() + .unwrap() + .success() + ); + fs::write(root.join("invalid.md"), [0xff]).unwrap(); + fs::write(root.join("private.md"), "private").unwrap(); + fs::set_permissions(root.join("private.md"), fs::Permissions::from_mode(0o0)).unwrap(); + for path in ["../root-other/secret.md", "outside/secret.md"] { + let error = resolve( + NoteContent::File(path.to_string()), + Some(&rooted), + Some(&root), + ) + .unwrap_err(); + assert!(error.contains("outside the server"), "{error}"); + } + for path in [ + "missing", + "broken", + ".", + "socket", + "fifo", + "private.md", + "invalid.md", + ] { + let error = resolve( + NoteContent::File(path.to_string()), + Some(&rooted), + Some(&root), + ) + .unwrap_err(); + assert!(error.contains("contentPath"), "{path}: {error}"); + if path == "invalid.md" { + assert!(error.contains("not valid UTF-8"), "{error}"); + } + } + } + + #[test] + fn size_limits_apply_to_bytes_including_files_that_grow_after_the_size_check() { + let temp = tempfile::tempdir().unwrap(); + let rooted = RootedFs::new(temp.path()).unwrap(); + let path = rooted.root().join("file.md"); + for size in [ + 0, + MAX_NOTE_FILE_BYTES - 1, + MAX_NOTE_FILE_BYTES, + MAX_NOTE_FILE_BYTES + 1, + ] { + fs::write(&path, vec![b'a'; size as usize]).unwrap(); + let answer = resolve( + NoteContent::File(path.display().to_string()), + Some(&rooted), + None, + ); + if size <= MAX_NOTE_FILE_BYTES { + assert_eq!(answer.unwrap().len() as u64, size); + } else { + assert!(answer.unwrap_err().contains("1048576 bytes")); + } + } + fs::write(&path, "한".repeat(MAX_NOTE_FILE_BYTES as usize / 3 + 1)).unwrap(); + assert!( + resolve( + NoteContent::File(path.display().to_string()), + Some(&rooted), + None + ) + .is_err() + ); + fs::write(&path, "").unwrap(); + let opened = File::open(&path).unwrap(); + let checked_size = opened.metadata().unwrap().len(); + fs::write(&path, vec![b'a'; MAX_NOTE_FILE_BYTES as usize + 1]).unwrap(); + assert!( + read_opened(opened, checked_size, "file.md") + .unwrap_err() + .contains("1 MiB") + ); + } + + #[test] + fn resolved_paths_cannot_be_replaced_with_symlinks_before_opening() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("root"); + let outside = temp.path().join("outside"); + fs::create_dir_all(root.join("docs")).unwrap(); + fs::create_dir(&outside).unwrap(); + fs::write(root.join("docs/file.md"), "inside").unwrap(); + fs::write(outside.join("file.md"), "outside").unwrap(); + let root = root.canonicalize().unwrap(); + // Replace a checked parent, then a checked leaf, then the server root. + fs::rename(root.join("docs"), root.join("held")).unwrap(); + symlink(&outside, root.join("docs")).unwrap(); + assert!(open_resolved(&root, Path::new("docs/file.md")).is_err()); + fs::rename(root.join("held/file.md"), root.join("held/original.md")).unwrap(); + symlink(outside.join("file.md"), root.join("held/file.md")).unwrap(); + assert!(open_resolved(&root, Path::new("held/file.md")).is_err()); + let opened = open_resolved(&root, Path::new("held/original.md")).unwrap(); + fs::rename(root.join("held/original.md"), root.join("held/saved.md")).unwrap(); + symlink(outside.join("file.md"), root.join("held/original.md")).unwrap(); + assert_eq!(read_opened(opened, 6, "file.md").unwrap(), "inside"); + fs::rename(&root, temp.path().join("held-root")).unwrap(); + symlink(&outside, &root).unwrap(); + assert!(open_resolved(&root, Path::new("file.md")).is_err()); + } + + #[test] + fn readable_files_do_not_require_permission_to_list_their_parent_directory() { + let temp = tempfile::tempdir().unwrap(); + let rooted = RootedFs::new(temp.path()).unwrap(); + let directory = rooted.root().join("search-only"); + fs::create_dir(&directory).unwrap(); + fs::write(directory.join("file.md"), "readable").unwrap(); + fs::set_permissions(&directory, fs::Permissions::from_mode(0o100)).unwrap(); + let result = resolve( + NoteContent::File("search-only/file.md".to_string()), + Some(&rooted), + Some(rooted.root()), + ); + fs::set_permissions(&directory, fs::Permissions::from_mode(0o700)).unwrap(); + assert_eq!(result.unwrap(), "readable"); + } + + // This macOS volume refuses creating non-UTF-8 filenames (EILSEQ). + // Linux exercises byte-preserving path components at the real FS boundary. + #[cfg(target_os = "linux")] + #[test] + fn a_utf8_symlink_can_name_a_file_with_non_utf8_os_components() { + use std::{ffi::OsString, os::unix::ffi::OsStringExt}; + + let temp = tempfile::tempdir().unwrap(); + let rooted = RootedFs::new(temp.path()).unwrap(); + let raw_name = OsString::from_vec(vec![b'f', 0xff]); + fs::write(rooted.root().join(&raw_name), "selected file").unwrap(); + fs::write(rooted.root().join("f�"), "different file").unwrap(); + symlink(raw_name, rooted.root().join("link.md")).unwrap(); + assert_eq!( + resolve( + NoteContent::File("link.md".to_string()), + Some(&rooted), + Some(rooted.root()) + ) + .unwrap(), + "selected file" + ); + } +} diff --git a/caffold/src/app/tasks.rs b/caffold/src/app/tasks.rs index 4ba73010..e4a9873c 100644 --- a/caffold/src/app/tasks.rs +++ b/caffold/src/app/tasks.rs @@ -140,6 +140,7 @@ impl TaskState { task_store.clone(), shutdown.clone(), ); + let task_runtime = task_runtime.with_notes_fs(fs.clone()); let task_runtime = match codex_mcp { Some(bindings) => task_runtime.with_codex_mcp(bindings), None => task_runtime, diff --git a/caffold/src/app/tasks/codex_mcp.rs b/caffold/src/app/tasks/codex_mcp.rs index 13e2e412..c0a714db 100644 --- a/caffold/src/app/tasks/codex_mcp.rs +++ b/caffold/src/app/tasks/codex_mcp.rs @@ -179,11 +179,13 @@ async fn serve_mcp(State(host): State, headers: HeaderMap, body: B #[cfg(test)] mod tests { + use crate::fs::RootedFs; use anyhow::Context; use axum::{ body::{Body, to_bytes}, http::Request, }; + use std::{fs, net::SocketAddr}; use tower::ServiceExt; use super::*; @@ -998,6 +1000,9 @@ mod tests { #[tokio::test] async fn a_bound_connection_keeps_notes_for_its_task_without_asking_codex() { + let root = tempfile::tempdir().unwrap(); + let file = root.path().join("findings.md"); + fs::write(&file, "# Findings from file\n").unwrap(); let host = memory_host("http://127.0.0.1:5177".to_string()); let store = TaskStore::memory().unwrap(); store @@ -1015,7 +1020,8 @@ mod tests { TaskEvents::default(), store.clone(), shutdown, - ); + ) + .with_notes_fs(Arc::new(RootedFs::new(root.path()).unwrap())); runtime.install_test_client(1, client.clone()).await; host.attach_runtime(runtime); let managed = bind_thread(&host, "thread_1").await; @@ -1042,7 +1048,23 @@ mod tests { .expect("the Note is stored"); assert_eq!(note.created_by_thread_id, "thread_1"); - let refused = response_json(bound_request(&host, &unmanaged, create_note).await).await; + let from_file = json!({ + "jsonrpc": "2.0", "id": 7, "method": "tools/call", + "params": {"name": "create_note", "arguments": {"name": "File", "contentPath": file}}, + }); + let created = response_json(bound_request(&host, &managed, from_file.clone()).await).await; + assert_eq!(created["result"]["isError"], false, "{created}"); + let answer: Value = + serde_json::from_str(created["result"]["content"][0]["text"].as_str().unwrap()) + .unwrap(); + let note = store + .read(|tables| tables.note(answer["noteId"].as_str().unwrap())) + .unwrap() + .unwrap(); + assert_eq!(note.content, "# Findings from file\n"); + assert_eq!(note.created_by_thread_id, "thread_1"); + + let refused = response_json(bound_request(&host, &unmanaged, from_file).await).await; assert_eq!(refused["result"]["isError"], true); assert_eq!( refused["result"]["content"][0]["text"], @@ -1050,7 +1072,7 @@ mod tests { ); assert_eq!( store.read(|tables| tables.note_summaries()).unwrap().len(), - 1 + 2 ); assert!(client.mock_requests().await.is_empty()); } @@ -1291,6 +1313,67 @@ mod tests { assert!(client.mock_requests().await.is_empty()); } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + #[ignore = "requires an installed authenticated Codex CLI; no model turn is submitted"] + async fn live_codex_mcp_accepts_note_file_input_through_the_installed_cli() { + use anyhow::{Result, ensure}; + + let root = tempfile::tempdir().unwrap(); + let cwd = root.path().join("task"); + fs::create_dir(&cwd).unwrap(); + fs::write(root.path().join("result.md"), "\u{feff}# File\r\n").unwrap(); + let (host, _, shutdown, server) = start_live_mcp_host( + SocketAddr::from(([127, 0, 0, 1], 0)), + &root.path().join("mcp"), + ) + .await + .unwrap(); + let store = TaskStore::memory().unwrap(); + let (runtime_shutdown, _) = broadcast::channel(1); + let runtime = TaskRuntime::new( + ClaudeClient::mock().0, + GrokClient::unreachable(), + TaskSessions::default(), + TaskEvents::default(), + store.clone(), + runtime_shutdown, + ) + .with_notes_fs(Arc::new(RootedFs::new(root.path()).unwrap())); + host.attach_runtime(runtime.clone()); + let installation = inspect_codex_installation().await.unwrap(); + let client = + CodexThreadClient::start_with_installation_and_mcp(&installation, host.bindings()) + .await + .unwrap(); + runtime.install_test_client(1, client.clone()).await; + let mut thread_id = None; + let result: Result<()> = async { + let started = client.start_thread(cwd.to_str().unwrap(), None, NORMAL_SERVICE_TIER_ID).await?; + thread_id = Some(started.thread_id.clone()); + store.claim(ManagedThread::new(&started.thread_id, RunBy::Codex, None, None, None), 1)?; + let created = client.call_mcp_tool_for_test(&started.thread_id, "create_note", json!({"name": "Live file", "contentPath": "../result.md"})).await?; + ensure!(created["isError"] != true, "{created}"); + let answer: Value = serde_json::from_str(created["content"][0]["text"].as_str().context("Note result text")?)?; + let note_id = answer["noteId"].as_str().context("created Note id")?; + let note = store.read(|tables| tables.note(note_id))?.context("stored Note")?; + ensure!(note.content == "\u{feff}# File\r\n"); + let updated = client.call_mcp_tool_for_test(&started.thread_id, "update_note_content", json!({"noteId": note_id, "contentPath": root.path().join("result.md"), "expectedContentVersion": 1})).await?; + ensure!(updated["isError"] != true, "{updated}"); + let stale = client.call_mcp_tool_for_test(&started.thread_id, "update_note_content", json!({"noteId": note_id, "contentPath": root.path().join("result.md"), "expectedContentVersion": 1})).await?; + ensure!(stale["isError"] == true, "{stale}"); + ensure!(store.read(|tables| tables.note(note_id))?.context("updated Note")?.content_version == 2); + Ok(()) + }.await; + let cleanup = match thread_id { + Some(thread_id) => client.delete_thread(&thread_id).await, + None => Ok(()), + }; + client.shutdown().await; + stop_live_mcp_host(shutdown, server).await; + result.expect("real Codex CLI forwards file input and version conflicts"); + cleanup.expect("delete the isolated live test thread"); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[ignore = "requires an authenticated installed Codex app-server"] async fn live_codex_mcp_survives_backend_replacement_and_runtime_reattachment() { diff --git a/caffold/src/app/tasks/grok_mcp.rs b/caffold/src/app/tasks/grok_mcp.rs index 25af751c..1db3e1ca 100644 --- a/caffold/src/app/tasks/grok_mcp.rs +++ b/caffold/src/app/tasks/grok_mcp.rs @@ -572,7 +572,10 @@ mod tests { /// which Task wrote them. #[tokio::test] async fn a_grok_task_keeps_notes_through_its_own_address() { + use std::fs; + let root = tempfile::tempdir().unwrap(); + fs::write(root.path().join("bridge.md"), "# Bridge\n").unwrap(); let (state, leader, _memory, host) = task_state_with_grok( RootedFs::new(root.path()).unwrap(), CodexThreadClient::mock(Vec::new()), @@ -617,7 +620,7 @@ mod tests { call( 2, "create_note", - json!({ "name": "Bridge", "content": "# Bridge\n" }), + json!({ "name": "Bridge", "contentPath": "bridge.md" }), ), ) .await, diff --git a/caffold/src/app/tasks/runtime.rs b/caffold/src/app/tasks/runtime.rs index dd94f4fa..d21b0be9 100644 --- a/caffold/src/app/tasks/runtime.rs +++ b/caffold/src/app/tasks/runtime.rs @@ -23,6 +23,7 @@ use crate::{ agent::{Driver, TokenUsage}, app::jev::PermissionReviewer, app::tasks::sessions::{SessionSnapshot, TaskSessions}, + fs::RootedFs, task_store::{ManagedThread, RunBy, TaskStore}, }; @@ -61,6 +62,7 @@ pub(in crate::app::tasks) struct TaskRuntime { sessions: TaskSessions, events: TaskEvents, task_store: TaskStore, + notes_fs: Option>, lifecycle: Option, push: Option, /// Who answers a permission request before a person is asked to. @@ -217,6 +219,7 @@ impl TaskRuntime { sessions, events, task_store, + notes_fs: None, lifecycle: None, push: None, approvals: Arc::new(Mutex::new(HashMap::new())), @@ -232,6 +235,11 @@ impl TaskRuntime { self } + pub(super) fn with_notes_fs(mut self, fs: Arc) -> Self { + self.notes_fs = Some(fs); + self + } + pub(in crate::app::tasks) fn startup(&self) { self.watch_claude(); self.watch_grok(); diff --git a/caffold/src/app/tasks/runtime/claude_bridge.rs b/caffold/src/app/tasks/runtime/claude_bridge.rs index 41fd9de0..cfc6ea9e 100644 --- a/caffold/src/app/tasks/runtime/claude_bridge.rs +++ b/caffold/src/app/tasks/runtime/claude_bridge.rs @@ -955,7 +955,10 @@ mod tests { #[tokio::test] async fn the_agent_keeps_a_note_that_records_its_task() { + use std::fs; + let root = tempfile::tempdir().unwrap(); + fs::write(root.path().join("findings.md"), "# Findings\n").unwrap(); let (state, runner) = watched(root.path()).await; state .task_store @@ -968,7 +971,7 @@ mod tests { notes_call( 5, "create_note", - json!({ "name": "Findings", "content": "# Findings\n" }), + json!({ "name": "Findings", "contentPath": "findings.md" }), ), ) .await; @@ -984,6 +987,7 @@ mod tests { .unwrap() .expect("the Note is stored"); assert_eq!(note.name, "Findings"); + assert_eq!(note.content, "# Findings\n"); assert_eq!(note.created_by_thread_id, SESSION); } diff --git a/caffold/src/app/tasks/runtime/server_requests.rs b/caffold/src/app/tasks/runtime/server_requests.rs index 820865e3..ae5ff1e6 100644 --- a/caffold/src/app/tasks/runtime/server_requests.rs +++ b/caffold/src/app/tasks/runtime/server_requests.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc}; +use std::{collections::HashMap, fmt::Display, path::PathBuf, sync::Arc}; use serde::Deserialize; use serde_json::{Value as JsonValue, json}; @@ -25,9 +25,9 @@ use crate::app::tasks::{ TaskEventPosition, TaskEventRecord, approval_requested_event, approval_resolved_event, now_ms, }, - worktrees::IsolateOutcome, + worktrees::{IsolateOutcome, inspect_ready_worktree}, }; -use crate::task_store::{ManagedThread, RunBy, TaskStoreError}; +use crate::task_store::{ManagedThread, ManagedWorktreeState, RunBy, TaskStoreError}; /// An approval waiting for an answer. /// @@ -1133,10 +1133,83 @@ impl TaskRuntime { thread_id: &str, call: NotesToolCall, ) -> Result { - if self.managed_thread(thread_id).await?.is_none() { + let Some(managed) = self.managed_thread(thread_id).await? else { return Err("Caffold only serves Notes to a task that it manages.".to_string()); + }; + let cwd = if call.needs_working_directory() { + Some(self.notes_working_directory(&managed).await?) + } else { + None + }; + notes::execute_tool( + self.task_store.clone(), + thread_id.to_string(), + call, + self.notes_fs.clone(), + cwd, + ) + .await + } + + /// Relative Note input follows the Task's effective cwd, while RootedFs + /// separately owns the server-wide file access boundary. + async fn notes_working_directory(&self, managed: &ManagedThread) -> Result { + let store = self.task_store.clone(); + let thread_id = managed.thread_id.clone(); + let worktree = tokio::task::spawn_blocking(move || { + let Some(worktree) = store + .worktree_for_thread(&thread_id) + .map_err(|error| error.to_string())? + else { + return Ok(None); + }; + if worktree.state != ManagedWorktreeState::Ready { + return Err("the managed worktree is not ready".to_string()); + } + inspect_ready_worktree(&worktree) + .map(|checkout| Some(checkout.path)) + .map_err(|error| error.to_string()) + }) + .await + .map_err(notes_cwd_unavailable)? + .map_err(notes_cwd_unavailable)?; + if let Some(path) = worktree { + return Ok(path); + } + let cwd = match &managed.run_by { + RunBy::Codex => { + let connection = self.connection().await.map_err(notes_cwd_unavailable)?; + connection + .client + .read_thread(&managed.thread_id) + .await + .map(|thread| thread.cwd) + .map_err(|error| error.to_string()) + } + RunBy::Claude { .. } => self + .claude + .working_directory(&managed.thread_id) + .await + .ok_or_else(|| "the Claude session's directory is unavailable".to_string()), + RunBy::Grok { .. } => self + .grok + .working_directory(&managed.thread_id) + .await + .map_err(|error| error.to_string()), } - notes::execute_tool(self.task_store.clone(), thread_id.to_string(), call).await + .map_err(notes_cwd_unavailable)?; + let cwd = PathBuf::from(cwd); + tokio::task::spawn_blocking(move || { + if cwd.is_absolute() && cwd.is_dir() { + Ok(cwd) + } else { + Err(notes_cwd_unavailable( + "the Task directory is not an available absolute directory", + )) + } + }) + .await + .map_err(notes_cwd_unavailable)? } async fn managed_thread(&self, thread_id: &str) -> Result, String> { @@ -1187,6 +1260,12 @@ impl TaskRuntime { } } +fn notes_cwd_unavailable(reason: impl Display) -> String { + format!( + "Caffold could not determine this Task's working directory for `contentPath`: {reason}. Restore the Task directory or use an absolute path inside the server's filesystem root." + ) +} + /// Why an approval stopped being answerable, when nobody answered it here. /// /// The agent answering the request itself and the turn moving on are different @@ -1247,7 +1326,7 @@ fn approval_id_from_request(request_id: &JsonValue, params: &JsonValue) -> Strin #[cfg(test)] mod tests { - use std::{path::Path, process::Command, sync::Arc, sync::Mutex as StdMutex}; + use std::{fs, path::Path, process::Command, sync::Arc, sync::Mutex as StdMutex}; use tokio::sync::broadcast; @@ -1265,7 +1344,9 @@ mod tests { }, app::terminal::TaskTerminals, fs::RootedFs, - task_store::{ManagedThread, PushSubscriptionInput, RunBy, TaskStore}, + task_store::{ + CheckoutAnchor, ManagedThread, ManagedWorktree, PushSubscriptionInput, RunBy, TaskStore, + }, }; fn initialize_repository(path: &Path) { @@ -1409,6 +1490,147 @@ mod tests { ) } + #[tokio::test] + async fn notes_relative_paths_use_the_codex_task_cwd_and_allow_server_root_siblings() { + let root = tempfile::tempdir().unwrap(); + let project = root.path().join("project"); + initialize_repository(&project); + let cwd = project.join("backend"); + fs::create_dir(&cwd).unwrap(); + fs::write(project.join("result.md"), "wrong Git root").unwrap(); + fs::write(cwd.join("result.md"), "Task cwd").unwrap(); + fs::write(root.path().join("shared.md"), "server sibling").unwrap(); + let store = TaskStore::memory().unwrap(); + store + .claim( + ManagedThread::new("notes-codex", RunBy::Codex, None, None, None), + 1, + ) + .unwrap(); + let thread = json!({"thread": {"id": "notes-codex", "preview": "Notes", "status": {"type": "idle"}, "cwd": cwd, "createdAt": 1.0, "updatedAt": 1.0, "turns": []}}); + let mut missing_cwd = thread.clone(); + missing_cwd["thread"]["cwd"] = json!(root.path().join("missing-directory")); + let client = CodexThreadClient::mock(vec![ + MockCodexResponse::ok("thread/read", thread.clone()), + MockCodexResponse::ok("thread/read", thread), + MockCodexResponse::ok("thread/read", missing_cwd), + ]); + let runtime = test_runtime(store.clone()) + .with_notes_fs(Arc::new(RootedFs::new(root.path()).unwrap())); + runtime.install_test_client(1, client.clone()).await; + for (path, expected) in [ + ("result.md", "Task cwd"), + ("../../shared.md", "server sibling"), + ] { + let answer = runtime + .execute_mcp_tool( + "notes-codex", + "create_note", + json!({"name": "Result", "contentPath": path}), + ) + .await + .unwrap(); + let answer: JsonValue = serde_json::from_str(&answer).unwrap(); + let note = store + .read(|tables| tables.note(answer["noteId"].as_str().unwrap())) + .unwrap() + .unwrap(); + assert_eq!(note.content, expected); + } + let refused = runtime + .execute_mcp_tool( + "notes-codex", + "create_note", + json!({"name": "Unavailable cwd", "contentPath": "shared.md"}), + ) + .await + .unwrap_err(); + assert!(refused.contains("working directory"), "{refused}"); + assert_eq!( + store.read(|tables| tables.note_summaries()).unwrap().len(), + 2 + ); + assert_eq!(client.mock_requests().await.len(), 3); + } + + #[tokio::test] + async fn notes_refuse_an_unknown_relative_cwd_but_absolute_input_needs_no_session() { + let root = tempfile::tempdir().unwrap(); + let file = root.path().join("result.md"); + fs::write(&file, "absolute file").unwrap(); + let store = TaskStore::memory().unwrap(); + store + .claim( + ManagedThread::new( + "notes-claude", + RunBy::Claude { + cwd: root.path().display().to_string(), + }, + None, + None, + None, + ), + 1, + ) + .unwrap(); + let runtime = test_runtime(store.clone()) + .with_notes_fs(Arc::new(RootedFs::new(root.path()).unwrap())); + let error = runtime + .execute_mcp_tool( + "notes-claude", + "create_note", + json!({"name": "Relative", "contentPath": "result.md"}), + ) + .await + .unwrap_err(); + assert!(error.contains("working directory"), "{error}"); + assert!( + store + .read(|tables| tables.note_summaries()) + .unwrap() + .is_empty() + ); + store + .create_worktree(ManagedWorktree { + worktree_id: "not-ready".to_string(), + thread_id: Some("notes-claude".to_string()), + repository_git_dir: root.path().join(".git").display().to_string(), + worktree_path: root.path().join("worktree").display().to_string(), + state: ManagedWorktreeState::Creating, + checkout_anchor: Some(CheckoutAnchor { + branch_name: "feat/notes".to_string(), + head_sha: "fixture-head".to_string(), + }), + created_at_ms: 1, + updated_at_ms: 1, + }) + .unwrap(); + let refused = runtime + .execute_mcp_tool( + "notes-claude", + "create_note", + json!({"name": "Not ready", "contentPath": "result.md"}), + ) + .await + .unwrap_err(); + assert!( + refused.contains("managed worktree is not ready"), + "{refused}" + ); + runtime + .execute_mcp_tool( + "notes-claude", + "create_note", + json!({"name": "Absolute", "contentPath": file}), + ) + .await + .unwrap(); + assert_eq!( + store.read(|tables| tables.note_summaries()).unwrap().len(), + 1 + ); + } + #[tokio::test] async fn clarification_requests_do_not_create_approval_or_user_events() { let events = TaskEvents::default(); @@ -2858,7 +3080,7 @@ mod tests { } #[tokio::test] - async fn isolate_tool_prepares_the_same_task_without_requesting_source_changes() { + async fn isolate_tool_prepares_the_same_task_and_notes_follow_its_worktree() { let root = tempfile::tempdir().unwrap(); let source = root.path().join("source"); initialize_repository(&source); @@ -2905,7 +3127,8 @@ mod tests { store.clone(), shutdown, ) - .with_lifecycle(lifecycle); + .with_lifecycle(lifecycle) + .with_notes_fs(fs.clone()); let thread_read = json!({ "thread": { "id": "thread_source", @@ -2955,6 +3178,27 @@ mod tests { ) ); + fs::write(source.join("result.md"), "old source cwd").unwrap(); + fs::write( + Path::new(&records[0].worktree_path).join("result.md"), + "managed worktree", + ) + .unwrap(); + let created = runtime + .execute_mcp_tool( + "thread_source", + "create_note", + json!({"name": "Worktree", "contentPath": "result.md"}), + ) + .await + .unwrap(); + let created: JsonValue = serde_json::from_str(&created).unwrap(); + let note = store + .read(|tables| tables.note(created["noteId"].as_str().unwrap())) + .unwrap() + .unwrap(); + assert_eq!(note.content, "managed worktree"); + let stored_before_switch = records[0].clone(); let switched = Command::new("git") .arg("-C") @@ -2988,6 +3232,20 @@ mod tests { store.worktree(&records[0].worktree_id).unwrap().unwrap(), stored_before_switch ); + fs::rename(&records[0].worktree_path, root.path().join("held-worktree")).unwrap(); + let refused = runtime + .execute_mcp_tool( + "thread_source", + "create_note", + json!({"name": "Missing worktree", "contentPath": "result.md"}), + ) + .await + .unwrap_err(); + assert!(refused.contains("managed worktree"), "{refused}"); + assert_eq!( + store.read(|tables| tables.note_summaries()).unwrap().len(), + 1 + ); } #[test] diff --git a/caffold/tests/claude_live.rs b/caffold/tests/claude_live.rs index a3bf6b07..d714e378 100644 --- a/caffold/tests/claude_live.rs +++ b/caffold/tests/claude_live.rs @@ -560,6 +560,50 @@ async fn restarting_the_claude_runtime_ends_its_sessions_and_tasks_resume() { .await; } +#[tokio::test(flavor = "multi_thread")] +#[ignore = "requires an authenticated Claude CLI and spends model usage"] +async fn the_agent_saves_and_updates_a_note_from_existing_files() { + let backend = Backend::start().await; + let task = backend + .start_task( + "Save the existing file one.txt as a Caffold Note named 'Live file import'. \ + Call mcp__caffold__create_note with contentPath set to one.txt. \ + Do not read or copy the file content into the tool arguments. Do not modify files. \ + After the tool succeeds, reply with done.", + MODEL, + ) + .await; + task.wait_for(TurnState::Idle, Duration::from_secs(180)) + .await; + let tree = backend.get("/api/notes").await.expect("Notes tree"); + let note = tree["notes"] + .as_array() + .unwrap() + .iter() + .find(|note| note["name"] == "Live file import") + .expect("the agent created the Note through file input"); + let note_id = note["id"].as_str().unwrap(); + let route = format!("/api/notes/{note_id}"); + assert_eq!( + backend.get(&route).await.unwrap()["content"], + "the first note" + ); + task.say( + "Replace the Caffold Note 'Live file import' with the contents of two.txt. \ + Read the Note with read_note for its contentVersion, then call update_note_content \ + with contentPath set to two.txt and expectedContentVersion from that read. \ + Do not read or copy the source file into arguments. Do not modify files. \ + After the tool succeeds, reply with done.", + ) + .await; + task.wait_for(TurnState::Idle, Duration::from_secs(180)) + .await; + assert_eq!( + backend.get(&route).await.unwrap()["content"], + "the second note" + ); +} + #[tokio::test(flavor = "multi_thread")] #[ignore = "requires an authenticated Claude CLI and spends model usage"] async fn the_agent_renames_its_task_when_asked_to_in_the_conversation() { diff --git a/docs/architecture/agent-runtimes.md b/docs/architecture/agent-runtimes.md index 098cd3bc..17dc9b9b 100644 --- a/docs/architecture/agent-runtimes.md +++ b/docs/architecture/agent-runtimes.md @@ -628,9 +628,27 @@ names, descriptions, input schemas, and argument checks to Codex's and Grok's addresses and to Claude's in-process server. A Notes call does the same thing whichever agent makes it: it runs only for a Task Caffold manages, reads or changes Caffold-owned Notes through the application, and records that Task on -a Note it creates or changes. It never touches the agent's conversation or -process. The pre-MCP Codex -dynamic-tool path serves no Notes tools. +a Note it creates or changes. The pre-MCP Codex dynamic-tool path serves no +Notes tools. + +`create_note` and `update_note_content` accept exactly one of `content` or +`contentPath`. Existing files use `contentPath` so the agent need not send their +text back through the tool. Relative paths start in the calling Task's effective +working directory: its ready managed worktree, or the current directory owned +by its agent. An unavailable relative-path base fails without a fallback. +Absolute filesystem paths require no Task cwd lookup. Both forms allow files +outside the Task directory when their resolved targets remain inside the +server's configured `RootedFs` root, including the default `/` root. + +File input accepts regular UTF-8 files up to 1 MiB (1,048,576 bytes), including +empty files, and preserves their bytes. The reader resolves symlinks against +the server boundary and opens the resolved components through directory handles +without following replacement symlinks. It checks the opened file and bounds +the read even if the file grows. File reads finish outside the Notes store +transaction; the update's `expectedContentVersion` is checked inside the write +transaction. A failed read or version conflict leaves the Note unchanged. +Inline content keeps its existing size behavior. Success returns the Note id +and content version without echoing the content. The application handles both requests through the same Task and Git lifecycle. Only delivery and cwd movement differ. Codex accepts a new cwd for the next