From 4f38f7d5b06215ef6c33790d45994ac81df4c97e Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 6 Oct 2026 14:18:34 +0900 Subject: [PATCH] refactor(worker): share file contracts across Tools and WIP providers Flatten shared typed Tool arguments and derive WIP parameters and checkout allowlists from core schemas. Workspace settings uses shared read/write/edit processing within its captured-read and canonical tree-CAS commit path. Identical settings edits now reject before read or commit; provider response contracts and unknown-outcome handling stay intact. T-707 validation: root cargo check; fs-operation (58); tools (74 lib, 39 integration, 1 doctest); worker (802 lib, 109 integration); workdir --features http-client (125); yoi-workspace-server --lib workspace_config (19); cargo fmt --all -- --check; git diff --check HEAD. --- crates/tools/src/checkout.rs | 24 +++-- crates/tools/src/edit.rs | 61 ++++++----- crates/tools/src/error.rs | 40 +++++++ crates/tools/src/read.rs | 70 ++++-------- crates/tools/src/write.rs | 14 ++- crates/worker/src/checkout.rs | 13 +-- crates/worker/src/checkout_race_tests.rs | 69 ++++++++++++ .../builtin/workspace_config/content.rs | 100 +++++++++++------- .../workspace_config/production_tests.rs | 45 ++++++++ crates/worker/src/file_operation.rs | 43 ++++++++ crates/worker/src/lib.rs | 1 + docs/development/file-operations.md | 49 +++++++++ 12 files changed, 388 insertions(+), 141 deletions(-) create mode 100644 crates/worker/src/file_operation.rs create mode 100644 docs/development/file-operations.md diff --git a/crates/tools/src/checkout.rs b/crates/tools/src/checkout.rs index 1bfcc254e..73e59e5e5 100644 --- a/crates/tools/src/checkout.rs +++ b/crates/tools/src/checkout.rs @@ -36,13 +36,19 @@ pub async fn execute_checkout_tool( arguments: Value, ctx: ToolExecutionContext, ) -> Result { + // Native file arguments come from the same definitions used by ordinary + // Tools and both WIP projections. Route fields are intentionally not in them. + let file_schema = fs_operation::text::TextOperation::from_name(&tool_name.to_lowercase(), true) + .map(|operation| operation.argument_schema()); + let file_fields: Vec<&str> = file_schema + .as_ref() + .and_then(|schema| schema.as_value()["properties"].as_object()) + .map(|properties| properties.keys().map(String::as_str).collect()) + .unwrap_or_default(); let (capability, allowed): (_, &[&str]) = match tool_name { - "Read" => (WorkdirSessionCapability::Read, &["offset", "limit"]), - "Edit" => ( - WorkdirSessionCapability::Edit, - &["old_string", "new_string", "replace_all"], - ), - "Write" => (WorkdirSessionCapability::Write, &["content"]), + "Read" => (WorkdirSessionCapability::Read, &file_fields), + "Edit" => (WorkdirSessionCapability::Edit, &file_fields), + "Write" => (WorkdirSessionCapability::Write, &file_fields), "Create" => (WorkdirSessionCapability::Write, &["path", "content"]), "Glob" => (WorkdirSessionCapability::Glob, &["pattern", "path"]), "Grep" => ( @@ -110,7 +116,8 @@ pub async fn execute_checkout_tool( } _ => { let params: crate::write::WriteParams = decode(arguments, tool_name)?; - crate::write::execute_write(target, tracker, params.content, None, ctx).await + crate::write::execute_write(target, tracker, params.write.content, None, ctx) + .await } } } @@ -185,8 +192,7 @@ pub async fn execute_checkout_tool( } fn decode(arguments: Value, name: &str) -> Result { - serde_json::from_value(arguments) - .map_err(|error| ToolError::InvalidArgument(format!("invalid {name} input: {error}"))) + crate::error::decode_file_value(arguments, name) } fn stale_search() -> ToolError { diff --git a/crates/tools/src/edit.rs b/crates/tools/src/edit.rs index 698b139b0..805a83087 100644 --- a/crates/tools/src/edit.rs +++ b/crates/tools/src/edit.rs @@ -23,13 +23,8 @@ pub(crate) struct EditParams { pub target_workdir: Option, /// Logical path relative to the bound Workdir root. pub file_path: String, - /// String to replace. Must be unique in the file unless `replace_all` is true. - pub old_string: String, - /// Replacement string. Must differ from `old_string`. - pub new_string: String, - /// Replace all occurrences. Defaults to false. - #[serde(default)] - pub replace_all: bool, + #[serde(flatten)] + pub edit: fs_operation::text::EditArgs, } pub(crate) struct EditTool { @@ -44,8 +39,7 @@ impl Tool for EditTool { input_json: &str, ctx: agen::tool::ToolExecutionContext, ) -> Result { - let params: EditParams = serde_json::from_str(input_json) - .map_err(|e| ToolError::InvalidArgument(format!("invalid Edit input: {e}")))?; + let params: EditParams = crate::error::decode_file_input(input_json, "Edit")?; let selected = crate::routing::resolve_session( &self.router, @@ -78,25 +72,19 @@ pub(crate) async fn execute_edit( ctx: agen::tool::ToolExecutionContext, ) -> Result { let path = &target.path; - tracing::debug!(path = %path, replace_all = params.replace_all, "Edit"); - if params.old_string.is_empty() { - return Err(ToolError::InvalidArgument( - "old_string must not be empty".into(), - )); - } - if params.old_string == params.new_string { - return Err(ToolError::InvalidArgument( - "old_string and new_string are identical".into(), - )); - } + tracing::debug!(path = %path, replace_all = params.edit.replace_all, "Edit"); + params + .edit + .validate(Default::default()) + .map_err(crate::error::text_error)?; let mutation_key = PathBuf::from(path.as_str()); let _mutation_permit = tracker.acquire_mutation(&mutation_key, &ctx).await; let expected_hash = tracker.expected_workdir_hash(path)?; let (result, validator) = target .edit( - params.old_string.clone(), - params.new_string.clone(), - params.replace_all, + params.edit.old_string.clone(), + params.edit.new_string.clone(), + params.edit.replace_all, expected_hash, ) .await?; @@ -105,8 +93,8 @@ pub(crate) async fn execute_edit( path, result.content_hash, replacements, - params.new_string.lines().count(), - params.old_string.lines().count(), + params.edit.new_string.lines().count(), + params.edit.old_string.lines().count(), ); let summary = format!( "Edited {} ({} replacement{})", @@ -114,7 +102,7 @@ pub(crate) async fn execute_edit( replacements, if replacements == 1 { "" } else { "s" } ); - let preview = make_preview(¶ms.new_string, ¶ms.new_string); + let preview = make_preview(¶ms.edit.new_string, ¶ms.edit.new_string); Ok(crate::checkout::CheckoutToolOutput { output: ToolOutput { summary, @@ -200,6 +188,27 @@ mod tests { .unwrap(); } + #[tokio::test] + async fn edit_shared_validation_rejects_identical_text_before_read_or_mutation() { + let (dir, fs, tracker) = setup(); + let file = dir.path().join("a.txt"); + std::fs::write(&file, "same").unwrap(); + let (_, tool) = edit_tool(fs, tracker.clone())(); + let error = tool + .execute( + &serde_json::json!({"file_path":"a.txt","old_string":"same","new_string":"same"}) + .to_string(), + Default::default(), + ) + .await + .unwrap_err(); + assert!( + matches!(error, ToolError::InvalidArgument(message) if message == "old_string and new_string are identical") + ); + assert_eq!(std::fs::read_to_string(file).unwrap(), "same"); + assert_eq!(tracker.change_stat(), crate::ChangeStat::default()); + } + #[tokio::test] async fn edit_unique_replacement() { let (dir, fs, tracker) = setup(); diff --git a/crates/tools/src/error.rs b/crates/tools/src/error.rs index dff6d1e08..b25025b7c 100644 --- a/crates/tools/src/error.rs +++ b/crates/tools/src/error.rs @@ -58,11 +58,51 @@ impl From for ToolError { } } +/// Pure text failures are pre-dispatch argument failures, never unknown outcomes. +pub(crate) fn text_error(error: fs_operation::text::TextError) -> ToolError { + ToolError::InvalidArgument(error.to_string()) +} + +pub(crate) fn decode_file_input( + input: &str, + name: &str, +) -> Result { + fs_operation::text::decode_json(input).map_err(|error| decode_error(error, name)) +} + +pub(crate) fn decode_file_value( + value: serde_json::Value, + name: &str, +) -> Result { + fs_operation::text::decode(value).map_err(|error| decode_error(error, name)) +} + +fn decode_error(error: fs_operation::text::TextError, name: &str) -> ToolError { + match error { + fs_operation::text::TextError::Decode(error) => { + ToolError::InvalidArgument(format!("invalid {name} input: {error}")) + } + error => text_error(error), + } +} + #[cfg(test)] mod tests { use super::*; use workdir::http::{WorkdirTransportError, WorkdirTransportErrorCode}; + #[test] + fn flattened_file_tool_arguments_keep_raw_json_duplicate_rejection() { + let error = decode_file_input::( + r#"{"file_path":"a.txt","old_string":"a","old_string":"b","new_string":"c"}"#, + "Edit", + ) + .unwrap_err(); + assert!( + matches!(error, ToolError::InvalidArgument(message) if message.contains("duplicate field `old_string`")) + ); + } + #[test] fn checkout_outcome_unknown_remains_execution_failure() { let error = ToolError::from(ToolsError::WorkdirSession( diff --git a/crates/tools/src/read.rs b/crates/tools/src/read.rs index a77be007b..f9d49ab1f 100644 --- a/crates/tools/src/read.rs +++ b/crates/tools/src/read.rs @@ -26,12 +26,8 @@ pub(crate) struct ReadParams { pub target_workdir: Option, /// Workdir-relative path, or an absolute path covered by readable scope. pub file_path: String, - /// 0-based line offset from the start. Defaults to 0. - #[serde(default)] - pub offset: Option, - /// Maximum number of lines to return. Defaults to 2000. - #[serde(default)] - pub limit: Option, + #[serde(flatten)] + pub read: fs_operation::text::LineReadArgs, } pub(crate) struct ReadTool { @@ -46,8 +42,7 @@ impl Tool for ReadTool { input_json: &str, _ctx: agen::tool::ToolExecutionContext, ) -> Result { - let params: ReadParams = serde_json::from_str(input_json) - .map_err(|e| ToolError::InvalidArgument(format!("invalid Read input: {e}")))?; + let params: ReadParams = crate::error::decode_file_input(input_json, "Read")?; let selected = crate::routing::resolve_session( &self.router, params.target_workdir.as_deref(), @@ -75,15 +70,21 @@ pub(crate) async fn execute_read( tracker: Tracker, params: ReadParams, ) -> Result { - let offset = params.offset.unwrap_or(0); - let limit = params.limit.unwrap_or(DEFAULT_LIMIT).max(1); + let (offset, limit) = params.read.range(DEFAULT_LIMIT); let path = &target.path; tracing::debug!(path = %path, offset, limit, "Read"); let (result, validator) = target.read(offset, limit, PROVIDER_BYTE_LIMIT).await?; tracker.record_workdir_observation(path, result.content_hash, result.total_lines); - let text = String::from_utf8_lossy(&result.bytes).into_owned(); + // Source/response bounds were enforced by the streaming provider. Lossy + // UTF-8 conversion is the existing Tool presentation contract; rendering + // below independently bounds its possible byte expansion. + let text = fs_operation::text::read( + String::from_utf8_lossy(&result.bytes).into_owned(), + Default::default(), + ) + .map_err(crate::error::text_error)?; let rendered = render_provider_read( - &text, + &text.content, result.start_line, result.total_lines, result.truncated, @@ -111,50 +112,19 @@ pub(crate) async fn execute_read( }) } -struct Rendered { - body: String, - line_count: usize, - total_lines: usize, - truncated: bool, -} - fn render_provider_read( text: &str, start_line: usize, total_lines: usize, truncated: bool, -) -> Rendered { - const MARKER: &str = "\n[truncated at rendered output byte limit]\n"; - // The numbered presentation is bounded too, not only provider source bytes. - // This fits the native wire limit even with JSON's worst-case escaping. - let budget = PROVIDER_BYTE_LIMIT - MARKER.len(); - let mut body = String::with_capacity(text.len().min(PROVIDER_BYTE_LIMIT)); - let mut line_count = 0; - let mut output_truncated = false; - for (index, line) in text.lines().enumerate() { - let numbered = format!("{:>6}\t{}\n", start_line + index + 1, line); - let remaining = budget.saturating_sub(body.len()); - if remaining > 0 { - line_count += 1; - } - if numbered.len() > remaining { - let mut end = remaining; - while !numbered.is_char_boundary(end) { - end -= 1; - } - body.push_str(&numbered[..end]); - body.push_str(MARKER); - output_truncated = true; - break; - } - body.push_str(&numbered); - } - Rendered { - body, - line_count, +) -> fs_operation::text::Rendered { + fs_operation::text::render_numbered( + text, + start_line, total_lines, - truncated: start_line > 0 || truncated || output_truncated, - } + truncated, + PROVIDER_BYTE_LIMIT, + ) } /// Factory for the `Read` tool bound to one compatibility session. diff --git a/crates/tools/src/write.rs b/crates/tools/src/write.rs index 858f91f44..c6f083cba 100644 --- a/crates/tools/src/write.rs +++ b/crates/tools/src/write.rs @@ -22,8 +22,8 @@ pub(crate) struct WriteParams { pub target_workdir: Option, /// Logical path relative to the bound Workdir root. pub file_path: String, - /// Full content to write. Overwrites any existing content. - pub content: String, + #[serde(flatten)] + pub write: fs_operation::text::WriteArgs, } pub(crate) struct WriteTool { @@ -38,8 +38,7 @@ impl Tool for WriteTool { input_json: &str, ctx: agen::tool::ToolExecutionContext, ) -> Result { - let params: WriteParams = serde_json::from_str(input_json) - .map_err(|e| ToolError::InvalidArgument(format!("invalid Write input: {e}")))?; + let params: WriteParams = crate::error::decode_file_input(input_json, "Write")?; let selected = crate::routing::resolve_session( &self.router, @@ -57,7 +56,7 @@ impl Tool for WriteTool { validator: None, }, tracker, - params.content, + params.write.content, None, ctx, ) @@ -74,6 +73,11 @@ pub(crate) async fn execute_write( create_path: Option, ctx: agen::tool::ToolExecutionContext, ) -> Result { + // Provider size/access checks still run inside the checked save. The shared + // core owns text semantics, not the provider's bounds or lifecycle. + let content = fs_operation::text::write(content, Default::default()) + .map_err(crate::error::text_error)? + .content; let path = create_path.as_ref().unwrap_or(&target.path); tracing::debug!(path = %path, bytes = content.len(), "Write"); let mutation_key = PathBuf::from(path.as_str()); diff --git a/crates/worker/src/checkout.rs b/crates/worker/src/checkout.rs index 1602ce23b..dedc6e0d4 100644 --- a/crates/worker/src/checkout.rs +++ b/crates/worker/src/checkout.rs @@ -241,28 +241,21 @@ impl Provider { ops.push(operation( "read", "Read text with line offsets and numbered output", - vec![ - parameter("offset", false, TypeExpr::Integer), - parameter("limit", false, TypeExpr::Integer), - ], + crate::file_operation::parameters("read", true).unwrap(), )); } if has(Cap::Edit, "Edit") { ops.push(operation( "edit", "Replace a unique string (requires prior Read)", - vec![ - parameter("old_string", true, TypeExpr::String), - parameter("new_string", true, TypeExpr::String), - parameter("replace_all", false, TypeExpr::Boolean), - ], + crate::file_operation::parameters("edit", true).unwrap(), )); } if has(Cap::Write, "Write") { ops.push(operation( "write", "Overwrite this existing file (requires prior Read)", - vec![parameter("content", true, TypeExpr::String)], + crate::file_operation::parameters("write", true).unwrap(), )); } } else { diff --git a/crates/worker/src/checkout_race_tests.rs b/crates/worker/src/checkout_race_tests.rs index b4e42397c..954ce0977 100644 --- a/crates/worker/src/checkout_race_tests.rs +++ b/crates/worker/src/checkout_race_tests.rs @@ -193,6 +193,75 @@ fn provider(router: Arc) -> Arc { }) } +#[tokio::test] +async fn checkout_wip_file_operations_share_core_and_keep_prior_read_boundary() { + let dir = TempDir::new().unwrap(); + std::fs::write(dir.path().join("text.txt"), "旧 旧\n").unwrap(); + let router = Arc::new(WorkdirSessionRouter::new()); + router + .attach( + WorkdirAttachmentAlias::new("main").unwrap(), + HookedSession::new(&dir), + ) + .unwrap(); + let provider = provider(router); + let route = format!("{}/text.txt", checkout_root("main")); + let projection = provider.node(&route).await.unwrap().unwrap(); + let invalid = BTreeMap::from([ + ("old_string".into(), Value::String("旧".into())), + ("new_string".into(), Value::String("旧".into())), + ]); + assert!( + matches!(projection.handler.call("edit", &invalid, context()).await, + Err(WipOperationError::Protocol(error)) if error.code == ProtocolErrorCode::InvalidArguments) + ); + let edit = BTreeMap::from([ + ("old_string".into(), Value::String("旧".into())), + ("new_string".into(), Value::String("新".into())), + ("replace_all".into(), Value::Boolean(true)), + ]); + // Valid shared arguments do not bypass the provider's Read requirement. + assert!( + projection + .handler + .call("edit", &edit, context()) + .await + .is_err() + ); + assert_eq!( + std::fs::read_to_string(dir.path().join("text.txt")).unwrap(), + "旧 旧\n" + ); + projection + .handler + .call("read", &BTreeMap::new(), context()) + .await + .unwrap_or_else(|_| panic!("shared read failed")); + projection + .handler + .call("edit", &edit, context()) + .await + .unwrap_or_else(|_| panic!("shared edit failed after read")); + assert_eq!( + std::fs::read_to_string(dir.path().join("text.txt")).unwrap(), + "新 新\n" + ); + let projection = provider.node(&route).await.unwrap().unwrap(); + projection + .handler + .call( + "write", + &BTreeMap::from([("content".into(), Value::String("written\n".into()))]), + context(), + ) + .await + .unwrap_or_else(|_| panic!("shared write failed after read/edit")); + assert_eq!( + std::fs::read_to_string(dir.path().join("text.txt")).unwrap(), + "written\n" + ); +} + fn deadline_provider(router: Arc) -> Arc { let mut p = provider(router); let settings = Arc::get_mut(&mut p).unwrap(); diff --git a/crates/worker/src/feature/builtin/workspace_config/content.rs b/crates/worker/src/feature/builtin/workspace_config/content.rs index 59929eea8..3fccc35f8 100644 --- a/crates/worker/src/feature/builtin/workspace_config/content.rs +++ b/crates/worker/src/feature/builtin/workspace_config/content.rs @@ -332,9 +332,8 @@ fn descriptor(names: &[String]) -> InterfaceDescriptor { name: name.clone(), documentation: None, parameters: match name.as_str() { "attach" => vec![parameter("access", false, TypeExpr::String)], - "read" | "delete" => vec![], - "write" => vec![parameter("content", true, TypeExpr::String)], - "edit" => vec![parameter("old_string", true, TypeExpr::String), parameter("new_string", true, TypeExpr::String), parameter("replace_all", false, TypeExpr::Boolean)], + "read" | "write" | "edit" => crate::file_operation::parameters(name, false).unwrap(), + "delete" => vec![], "create" => vec![parameter("path", false, TypeExpr::String), parameter("content", true, TypeExpr::String), parameter("content_type", false, TypeExpr::String)], "apply_changes" => vec![parameter("changes", true, TypeExpr::Json)], _ => vec![], @@ -388,6 +387,40 @@ fn text(arguments: &BTreeMap, key: &str) -> Result Err(invalid()), } } +fn text_limits() -> fs_operation::text::TextLimits { + fs_operation::text::TextLimits { + max_input_bytes: Some(MAX_TEXT_BYTES), + max_output_bytes: Some(MAX_TEXT_BYTES), + max_replacements: None, + } +} +fn decode_text( + arguments: &BTreeMap, +) -> Result { + let value = Value::Record(arguments.clone()); + fs_operation::text::decode(crate::wip::wip_to_json(&value).map_err(|_| invalid())?) + .map_err(text_error) +} +fn text_error(error: fs_operation::text::TextError) -> WipOperationError { + use fs_operation::text::TextError; + match error { + TextError::OutputTooLarge { .. } + | TextError::Overflow + | TextError::ReplacementLimitExceeded { .. } => failure( + ProtocolErrorCode::ResourceLimitExceeded, + "Workspace config edit exceeds text limit", + ), + TextError::IdenticalStrings => failure( + ProtocolErrorCode::InvalidArguments, + "old_string and new_string are identical", + ), + TextError::Decode(_) + | TextError::EmptyOldString + | TextError::NotFound + | TextError::MultipleMatches { .. } + | TextError::InputTooLarge { .. } => invalid(), + } +} fn content_type(value: Option<&Value>) -> Result { match value { None => Ok(ConfigContentType::Decodal), @@ -422,6 +455,10 @@ impl ConfigHandler { "Invalid Workspace config read response", )); } + let mut response = response; + response.content = fs_operation::text::read(response.content, text_limits()) + .map_err(text_error)? + .content; Ok(response) } @@ -582,6 +619,7 @@ impl ConfigHandler { .await; } if operation == "read" { + let _: fs_operation::text::ReadArgs = decode_text(arguments)?; let result = self.read().await?; // Validator goes to Client state, not the model operation result. let value = json_to_wip(&json!({ @@ -601,46 +639,26 @@ impl ConfigHandler { )); } let changes = match operation { - "write" => vec![Change::Update { - path: self.node.path.clone(), - content: text(arguments, "content")?, - }], - "edit" => { - let original = self.read().await?.content; - let old = text(arguments, "old_string")?; - let new = text(arguments, "new_string")?; - let all = match arguments.get("replace_all") { - None => false, - Some(Value::Boolean(all)) => *all, - _ => return Err(invalid()), - }; - if old.is_empty() { - return Err(invalid()); - } - let count = original.matches(&old).count(); - if count == 0 || (!all && count != 1) { - return Err(invalid()); - } - let count = if all { count } else { 1 }; - let size = original - .len() - .checked_sub(count.saturating_mul(old.len())) - .and_then(|n| n.checked_add(count.checked_mul(new.len())?)) - .filter(|n| *n <= MAX_TEXT_BYTES); - if size.is_none() { - return Err(failure( - ProtocolErrorCode::ResourceLimitExceeded, - "Workspace config edit exceeds text limit", - )); - } - let content = if all { - original.replace(&old, &new) - } else { - original.replacen(&old, &new, 1) - }; + "write" => { + let args: fs_operation::text::WriteArgs = decode_text(arguments)?; + let result = + fs_operation::text::write(args.content, text_limits()).map_err(text_error)?; vec![Change::Update { path: self.node.path.clone(), - content, + content: result.content, + }] + } + "edit" => { + let args: fs_operation::text::EditArgs = decode_text(arguments)?; + // Reject basic invalid input before reading or entering the commit + // path. The captured read and whole-tree CAS remain bound here. + args.validate(text_limits()).map_err(text_error)?; + let original = self.read().await?.content; + let result = fs_operation::text::edit(&original, &args, text_limits()) + .map_err(text_error)?; + vec![Change::Update { + path: self.node.path.clone(), + content: result.content, }] } "delete" => vec![Change::Delete { diff --git a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs index 29e61dcc5..4c6107af2 100644 --- a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs +++ b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs @@ -857,6 +857,51 @@ async fn workspace_config_production_backend_failure_is_not_hidden_as_unauthoriz assert!(!message.contains("/private/host/config")); } +#[tokio::test] +async fn workspace_config_edit_shared_argument_rejection_never_commits() { + let client = Router::new(); + let (feature, runtime) = runtime(client.clone()); + feature.attach("initial", None).await.unwrap(); + let path = "/workspace-config/main.dcdl"; + observe_interface(&runtime, path, true).await; + for (args, expected) in [ + ( + json!({"old_string":"old","new_string":"old"}), + "old_string and new_string are identical", + ), + ( + json!({"old_string":"old","new_string":"new","replace_all":"true"}), + "expected boolean", + ), + ] { + let error = call(&runtime, path, "edit", args).await.unwrap_err(); + assert!(error.to_string().contains(expected), "{error:?}"); + let state = client.state.lock().unwrap(); + assert!(state.commits.is_empty()); + assert_eq!(state.revision, 1); + assert_eq!(state.entries["main.dcdl"].1, "model = old"); + // Argument-only rejection must not even fetch a preimage. + assert!( + state + .requests + .iter() + .all(|request| !request.path.ends_with("/read")) + ); + } + // Rejection has not made the observation stale or poisoned the commit path. + call( + &runtime, + path, + "edit", + json!({"old_string":"old","new_string":"新"}), + ) + .await + .unwrap(); + let state = client.state.lock().unwrap(); + assert_eq!(state.commits.len(), 1); + assert_eq!(state.entries["main.dcdl"].1, "model = 新"); +} + #[tokio::test] async fn workspace_config_production_ungranted_root_does_not_poison_worldspace_discovery() { let client = Router::new(); diff --git a/crates/worker/src/file_operation.rs b/crates/worker/src/file_operation.rs new file mode 100644 index 000000000..57a5f359f --- /dev/null +++ b/crates/worker/src/file_operation.rs @@ -0,0 +1,43 @@ +//! WIP presentation of the provider-neutral file argument contracts. Providers +//! choose which read surface to publish; routing and preconditions are not args. +use fs_operation::text::TextOperation; +use wip_protocol::{ParameterDeclaration, TypeExpr}; + +pub(crate) fn parameters(name: &str, line_read: bool) -> Option> { + let schema = TextOperation::from_name(name, line_read)?.argument_schema(); + Some(schema_parameters(&schema)) +} + +fn schema_parameters(schema: &schemars::Schema) -> Vec { + let schema = schema.as_value(); + let required = schema["required"].as_array(); + schema["properties"] + .as_object() + .into_iter() + .flat_map(|properties| properties.iter()) + .map(|(name, property)| { + // These core arguments are primitives. Optional Tool read fields + // accept null; WIP keeps its existing optional integer declaration. + let kind = property["type"].as_str().or_else(|| { + property["type"] + .as_array()? + .iter() + .filter_map(|v| v.as_str()) + .find(|t| *t != "null") + }); + let r#type = match kind { + Some("string") => TypeExpr::String, + Some("boolean") => TypeExpr::Boolean, + Some("integer") => TypeExpr::Integer, + _ => panic!("file operation arguments must be primitive: {name}"), + }; + ParameterDeclaration { + name: name.clone(), + required: required + .is_some_and(|items| items.iter().any(|item| item.as_str() == Some(name))), + documentation: None, + r#type, + } + }) + .collect() +} diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 66160db84..ae5baddae 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -5,6 +5,7 @@ pub mod controller; pub mod discovery; pub mod entrypoint; pub mod feature; +mod file_operation; pub mod fs_view; pub mod hook; pub(crate) mod in_flight; diff --git a/docs/development/file-operations.md b/docs/development/file-operations.md new file mode 100644 index 000000000..f86d1eb24 --- /dev/null +++ b/docs/development/file-operations.md @@ -0,0 +1,49 @@ +# Shared text file operations + +`fs-operation::text` owns the provider-neutral read/edit/write argument contracts, +JSON decoding, argument validation, pure text outcomes and byte-bound calculations. +The argument schemas also supply WIP parameter declarations. Ordinary Tools flatten +these arguments alongside their own Workdir routing fields; checkout WIP uses the +same Tool execution functions, not a third editing implementation. + +Edit requires a nonempty `old_string`, a different `new_string`, and a unique +non-overlapping match unless `replace_all` is true. Limits count UTF-8 bytes, are +supplied by each provider, and are checked (including arithmetic/capacity overflow) +before allocating a replacement result. Outcomes carry replacement count and byte +size independently of persistence metadata. + +## Provider-owned differences + +- Workdir read exposes optional line offset/limit and numbered, bounded output. + Workspace settings read remains a whole-text read with no arguments, preserving + content type and digest. Line range defaults and numbered rendering live in the + core but are only selected by the Workdir adapter. +- Workdir routing, attachment generation, Scope and permission identity remain in + Worker/Tools/Workdir. Read history and content hashes stay in the existing + attachment-scoped tracker. Descriptor-protected reads and checked edits/writes + retain their existing validator, identity and save boundary in the provider. +- Workspace settings grants, access mode and connection lifetime remain in Backend. + The settings adapter binds the read to captured entry metadata and generates a + canonical commit using the same tree revision/digest. Decodal validation, + formatting and atomic multi-file persistence remain in the canonical Backend + commit. Content type, create/delete/rename/apply_changes and attach are settings + operations, not ordinary OS file operations. + +The core does not perform I/O, authorize anything, or own revision/existence +mirrors. Providers invoke pure processing inside their existing consistency +boundary; it must not be used to turn a checked edit into an unchecked +Read–Write sequence. + +## External contracts + +Operation names, argument shapes and successful responses remain unchanged. +Workspace settings Edit now rejects identical old/new strings before reading or +committing (the intended behavior change). Pure text failures are explicitly +mapped to Tool argument errors or WIP `InvalidArguments`/`ResourceLimitExceeded` +by the adapters. A mutation's post-dispatch transport or response failure is still +`OutcomeUnknown`; the core does not retry it or reinterpret it as a validation +rejection. + +Pure policy matrices belong in `fs-operation::text` tests. Provider tests cover +entrypoint wiring and the existing authorization, observation, checked-save, +configuration CAS and unknown-outcome boundaries rather than copying that matrix.