diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 5066a333d79..1ae9fe7c1db 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -467,6 +467,15 @@ Rejected API fallback arguments return the same bounded schema locations as monitor calls, with no submitted values. Correct the arguments before retrying; validation does not dispatch an HTTP request or relax route permissions. +This validation feedback applies to every authorized native tool, including +`paperclip_search_assigned_tools`, `paperclip_call_assigned_tool`, and direct app +tools. Invalid arguments return repair guidance and at most three locations in +the authorized input schema, within 512 bytes. Feedback excludes submitted values +and unknown input keys. A rejected call creates no execution receipt; corrected +arguments can be submitted again. `retryable: false` prevents automatic replay +of the invalid call. Unauthorized tools still receive a generic rejection, and +corrected calls retain the same permission and approval checks. + A native provider may omit `continuation` (or send `null`) when it reports `yielded` after saving a monitor. The controller reads current authorized task context and fills the canonical `monitor` continuation only if that same task, diff --git a/maps/agent-access-map/gateways-profiles.md b/maps/agent-access-map/gateways-profiles.md index a360688d066..69455f2230f 100644 --- a/maps/agent-access-map/gateways-profiles.md +++ b/maps/agent-access-map/gateways-profiles.md @@ -97,6 +97,15 @@ approvals and revocation checks remain independent. Regression coverage lives in [assigned-tool tests](../../server/src/services/native-runtime/assigned-mcp-tools.test.ts); these fixtures do not certify a live external-client journey. +Native assigned tools return bounded input-schema repair guidance after argument +validation fails. Rejection creates no execution receipt and discloses no submitted +values or unknown keys. Corrected calls keep the same authorization checks. +[Runner regressions](../../packages/paperclip-runner/runner/crates/runner-core/src/tests/provider_assigned_tool_validation.rs) +cover discovery repair and unauthorized calls; the +[Codex transport fixture](../../packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_assigned_tool_validation.rs) +covers rejection, corrected dispatch, and completion with a simulated provider. +This does not establish live model or GitHub access. + Remote invocation regressions preserve access to authorized sibling tools after a fetch failure, timeout, non-authentication HTTP error, or JSON-RPC rejection. HTTP 401/403 and insufficient-scope errors keep their connection gates. The diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs index 8568cb65f28..49860a9e06d 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs @@ -682,6 +682,9 @@ fn run() -> Result<(), Box> { .iter() .any(|value| value == "--finish-turn-with-pending-tool"); let require_dynamic_tool = args.iter().any(|value| value == "--require-dynamic-tool"); + let repair_assigned_discovery = args + .iter() + .any(|value| value == "--repair-assigned-discovery"); let require_completion_contract = args .iter() .any(|value| value == "--require-completion-contract"); @@ -934,6 +937,45 @@ fn run() -> Result<(), Box> { )?; continue; } + if repair_assigned_discovery + && message.get("method").is_none() + && message.get("id") == Some(&json!("discovery-request")) + { + let text = message + .pointer("/result/contentItems/0/text") + .and_then(Value::as_str) + .ok_or("discovery response omitted content")?; + let result: Value = serde_json::from_str(text)?; + if message.pointer("/result/success") == Some(&json!(false)) { + let hint = result + .pointer("/error/message") + .and_then(Value::as_str) + .ok_or("discovery rejection omitted guidance")?; + if !hint.contains("Correct the arguments and retry") + || !hint.contains("/properties/query/type") + || hint.contains("PRIVATE_QUERY") + { + return Err("discovery rejection lacks safe repair guidance".into()); + } + log_call(call_log.as_deref(), "discovery:repair-guidance")?; + send( + json!({"id":"discovery-request", "method":"item/tool/call", "params":{ + "threadId":state.thread_id, "turnId":state.active_turn_id, + "callId":"assigned-discovery", "tool":"paperclip_search_assigned_tools", + "arguments":{"query":"github"} + }}), + )?; + } else { + if message.pointer("/result/success") != Some(&json!(true)) + || result != json!({"tools":[{"name":"app_github_read_pull_request"}]}) + { + return Err("corrected discovery did not return assigned tools".into()); + } + log_call(call_log.as_deref(), "discovery:success")?; + finish_turn(&state_path, &mut state, "completed")?; + } + continue; + } if message.get("method").is_none() && message.get("id") == Some(&json!("tool-request-1")) { if message.pointer("/result/success") == Some(&json!(false)) { log_call(call_log.as_deref(), "tool-response:failure")?; @@ -1656,6 +1698,14 @@ fn run() -> Result<(), Box> { }))?; } else if exit_after_turn_start { return Ok(()); + } else if repair_assigned_discovery { + send( + json!({"id":"discovery-request", "method":"item/tool/call", "params":{ + "threadId":state.thread_id, "turnId":provider_turn_id, + "callId":"assigned-discovery", "tool":"paperclip_search_assigned_tools", + "arguments":{"query":{"PRIVATE_QUERY":"private search"}} + }}), + )?; } else if emit_tool_call { send(json!({ "id": "tool-request-1", diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs index 660042ee790..dd9a9e8ebeb 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs @@ -4999,7 +4999,7 @@ mod tests { } #[test] - fn invalid_tool_results_expose_only_reserved_schema_guidance() { + fn invalid_tool_results_expose_authorized_schema_guidance() { let finish_result = schema_rejection( "paperclip_finish", json!({"secretSubmittedValue": "must-not-appear"}), @@ -5052,10 +5052,9 @@ mod tests { assert_eq!(monitor_result.result["error"]["retryable"], false); let ordinary_result = schema_rejection("get_task_context", json!({})); - assert_eq!( - ordinary_result.result["error"]["message"], - GENERIC_INVALID_TOOL_CALL_MESSAGE - ); + let ordinary_message = ordinary_result.result["error"]["message"].as_str().unwrap(); + assert!(ordinary_message.contains("Correct the arguments and retry")); + assert!(ordinary_message.contains("/required (missing \"requiredField\")")); let unauthorized_error = ProviderToolBridge::default() .begin_call( @@ -6543,3 +6542,7 @@ mod tests { #[cfg(test)] #[path = "tests/provider_api_validation.rs"] mod api_validation_tests; + +#[cfg(test)] +#[path = "tests/provider_assigned_tool_validation.rs"] +mod assigned_tool_validation_tests; diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/provider_bridge.rs b/packages/paperclip-runner/runner/crates/runner-core/src/provider_bridge.rs index 09cb2fe08da..ffbdeed071e 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/provider_bridge.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/provider_bridge.rs @@ -44,6 +44,7 @@ const QUESTION_INPUT_SCHEMA_HINT: &str = "Invalid request_human_input arguments. const BLOCK_INPUT_SCHEMA_HINT: &str = "Invalid paperclip_block arguments. Required fields: reportedWorkDisposition=blocked, summary, completionClaim, evidence, verification, and blocker. blocker must include reasonCode, owner, unblockAction, and scope."; const MONITOR_INPUT_SCHEMA_HINT: &str = "Invalid set_task_monitor arguments. Supply idempotencyKey and monitor: {nextCheckAt: , notes: }. Put all scheduling fields inside monitor, or use monitor: null to clear. Omit taskId for the current task. Correct the arguments and retry."; const API_INPUT_SCHEMA_HINT: &str = "Invalid call_api arguments. For responseText, use GET with offsetBytes >= 0 and responseText.limitBytes from 4 through 24576 bytes (default 24576). Follow nextOffsetBytes; null means complete. Correct the arguments and retry."; +const TOOL_INPUT_SCHEMA_HINT: &str = "Invalid tool arguments. The tool was not executed. Match its authorized input schema. Correct the arguments and retry."; #[derive(Clone, Debug, Deserialize, Serialize, PartialEq)] #[serde(rename_all = "camelCase")] @@ -248,7 +249,7 @@ impl ProviderBridgeError { "request_human_input" => Some(QUESTION_INPUT_SCHEMA_HINT), "set_task_monitor" => Some(MONITOR_INPUT_SCHEMA_HINT), "call_api" => Some(API_INPUT_SCHEMA_HINT), - _ => None, + _ => Some(TOOL_INPUT_SCHEMA_HINT), }; Self { message: format!("provider arguments for {operation_id} failed JSON Schema validation"), @@ -257,8 +258,6 @@ impl ProviderBridgeError { } fn with_schema_locations(mut self, validator: &jsonschema::Validator, input: &Value) -> Self { - // Only the reserved tools above have a provider-safe hint. Report bounded paths - // into the authorized schema, never the submitted values or unknown keys. if let Some(message) = self.safe_provider_message.as_mut() { let locations = validator .iter_errors(input) @@ -1612,7 +1611,9 @@ mod tests { ); let other = ProviderBridgeError::input_schema_validation("some_other_tool") .with_schema_locations(&validator, &json!({"a": 1})); - assert_eq!(other.safe_provider_message(), None); + let message = other.safe_provider_message().unwrap(); + assert!(message.starts_with(TOOL_INPUT_SCHEMA_HINT)); + assert!(message.contains("/properties/a/type")); } #[test] diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/tests/provider_assigned_tool_validation.rs b/packages/paperclip-runner/runner/crates/runner-core/src/tests/provider_assigned_tool_validation.rs new file mode 100644 index 00000000000..ed3960c4dbb --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/src/tests/provider_assigned_tool_validation.rs @@ -0,0 +1,144 @@ +use super::*; +use crate::provider_bridge::AuthorizedTool; + +fn authorized_bridge(operation_id: &str, input_schema: Value) -> ProviderToolBridge { + let operations = vec![AuthorizedTool { + operation_id: operation_id.to_owned(), + version: 1, + description: "Search assigned tools.".to_owned(), + input_schema, + response_schema: json!({"type": "object"}), + }]; + let mut bridge = ProviderToolBridge::default(); + bridge + .prepare(AuthorizedToolSet { + schema: TOOL_SET_SCHEMA.to_owned(), + schema_version: 1, + catalog_digest: authorized_tool_catalog_digest(&operations).unwrap(), + operations, + }) + .unwrap(); + bridge +} + +#[test] +fn assigned_tool_discovery_rejection_guides_repair_without_retaining_a_receipt() { + let operation_id = "paperclip_search_assigned_tools"; + let mut bridge = authorized_bridge( + operation_id, + json!({ + "type": "object", "additionalProperties": false, + "properties": { + "query": {"type": "string", "maxLength": 200}, + "offset": {"type": "integer", "minimum": 0}, + "limit": {"type": "integer", "minimum": 1, "maximum": 20}, + "schemaTool": {"type": "string"}, + "schemaOffset": {"type": "integer", "minimum": 0} + }, + "required": ["query"] + }), + ); + let initial_state = serde_json::to_value(&bridge).unwrap(); + for (input, location) in [ + (json!({}), "/required (missing \"query\")"), + (json!({"query": null}), "/properties/query/type"), + ( + json!({"query": "PRIVATE_QUERY", "offset": null}), + "/properties/offset/type", + ), + ( + json!({"query": "PRIVATE_QUERY", "limit": 21}), + "/properties/limit/maximum", + ), + ( + json!({"query": "PRIVATE_QUERY".repeat(100)}), + "/properties/query/maxLength", + ), + ( + json!({"query": "PRIVATE_QUERY", "PRIVATE_FIELD": "PRIVATE_VALUE"}), + "/additionalProperties", + ), + ] { + let error = bridge + .begin_call("discovery".to_owned(), operation_id.to_owned(), input) + .unwrap_err(); + let result = + invalid_tool_call_result("discovery".to_owned(), operation_id.to_owned(), &error); + let message = result.result["error"]["message"].as_str().unwrap(); + assert!( + message.contains("Correct the arguments and retry"), + "{message}" + ); + assert!(message.contains(location), "{message}"); + for private in ["PRIVATE_QUERY", "PRIVATE_FIELD", "PRIVATE_VALUE"] { + assert!(!result.result.to_string().contains(private)); + } + assert!(message.len() <= 512); + assert!(!message.chars().any(char::is_control)); + assert!(result.is_error); + assert_eq!(result.result["error"]["code"], "invalid_tool_call"); + assert_eq!(result.result["error"]["retryable"], false); + assert!(!bridge.has_call_receipt("discovery")); + assert_eq!(serde_json::to_value(&bridge).unwrap(), initial_state); + } + + let input = json!({"query": "github"}); + bridge + .begin_call( + "discovery".to_owned(), + operation_id.to_owned(), + input.clone(), + ) + .unwrap(); + assert_eq!(bridge.pending_calls().count(), 1); + let result = ToolResult { + call_id: "discovery".to_owned(), + operation_id: operation_id.to_owned(), + result: json!({"tools": [{"name": "app_github_read_pull_request"}]}), + is_error: false, + }; + bridge.apply_result(result.clone()).unwrap(); + assert_eq!(bridge.pending_calls().count(), 0); + assert_eq!( + bridge + .replay_result("discovery", operation_id, &input) + .unwrap(), + Some(result) + ); + assert!(bridge + .begin_call("discovery".to_owned(), operation_id.to_owned(), input) + .is_err()); +} + +#[test] +fn arbitrary_assigned_tool_gets_schema_guidance_only_when_authorized() { + let operation_id = "app_custom_connection_search"; + let schema = json!({ + "type": "object", "required": ["query"], + "properties": {"query": {"type": "string"}}, + "additionalProperties": false + }); + let mut bridge = authorized_bridge(operation_id, schema); + let error = bridge + .begin_call("custom".to_owned(), operation_id.to_owned(), json!({})) + .unwrap_err(); + let result = invalid_tool_call_result("custom".to_owned(), operation_id.to_owned(), &error); + assert!(result.result["error"]["message"] + .as_str() + .unwrap() + .contains("/required (missing \"query\")")); + + let denied = bridge + .begin_call( + "denied".to_owned(), + "app_unassigned".to_owned(), + json!({"PRIVATE_FIELD": "PRIVATE_VALUE"}), + ) + .unwrap_err(); + assert_eq!( + invalid_tool_call_result("denied".to_owned(), "app_unassigned".to_owned(), &denied).result + ["error"]["message"], + GENERIC_INVALID_TOOL_CALL_MESSAGE + ); + assert!(!bridge.has_call_receipt("denied")); +} diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs index 3cf7eb5de78..07c7912635c 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs @@ -19,6 +19,9 @@ use paperclip_runner_core::provider_bridge::{ use paperclip_runner_core::provider_events::normalize_codex_notification; use serde_json::{json, Value}; +#[path = "support/codex_assigned_tool_validation.rs"] +mod assigned_tool_validation; + static NEXT_DIRECTORY: AtomicU64 = AtomicU64::new(1); static RECEIPT_LIMIT_TEST_LOCK: Mutex<()> = Mutex::new(()); diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_assigned_tool_validation.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_assigned_tool_validation.rs new file mode 100644 index 00000000000..fe029b8ae2d --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_assigned_tool_validation.rs @@ -0,0 +1,83 @@ +use super::*; + +#[test] +fn assigned_tool_discovery_repairs_invalid_arguments_through_codex_transport() { + let directory = temporary_directory("assigned-discovery-repair"); + let config = provider_config(&directory, &["--repair-assigned-discovery"]); + let operations = vec![AuthorizedTool { + operation_id: "paperclip_search_assigned_tools".to_owned(), + version: 1, + description: "Search assigned tools.".to_owned(), + input_schema: json!({"type":"object", "required":["query"], + "properties":{"query":{"type":"string"}}, "additionalProperties":false}), + response_schema: json!({"type":"object"}), + }]; + let tool_set = AuthorizedToolSet { + schema: TOOL_SET_SCHEMA.to_owned(), + schema_version: 1, + catalog_digest: authorized_tool_catalog_digest(&operations).unwrap(), + operations, + }; + let mut executor = + CodexCommandExecutor::with_runner_config(&directory, &durable_config(&directory)); + executor + .execute(&command( + "prepare", + 1, + "run.prepare", + json!({ + "provider":config, "authorizedTools":tool_set + }), + )) + .unwrap(); + executor + .execute(&command("open", 2, "session.open", json!({}))) + .unwrap(); + executor + .execute(&command( + "turn", + 3, + "turn.start", + json!({"text":"Discover the assigned GitHub tools."}), + )) + .unwrap(); + + let mut denied = 0; + let mut dispatched = 0; + let mut completed = false; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while !completed && std::time::Instant::now() < deadline { + for event in poll_and_ack(&mut executor).unwrap() { + match event.event_type.as_str() { + "harness.diagnostic" if event.payload["code"] == "semantic_tool_denied" => { + denied += 1; + assert_eq!(event.payload["paperclipExecuted"], false); + assert!(!event.payload.to_string().contains("PRIVATE_QUERY")); + } + "semantic_tool.input" => { + dispatched += 1; + assert_eq!(denied, 1); + assert_eq!( + event.payload["semantic_tool"]["input"], + json!({"query":"github"}) + ); + executor.execute(&command("result", 4, "semantic_tool.result", json!({ + "callId":"assigned-discovery", "operationId":"paperclip_search_assigned_tools", + "result":{"tools":[{"name":"app_github_read_pull_request"}]}, "isError":false + }))).unwrap(); + } + "turn.completed" => completed = true, + "turn.failed" => panic!("provider could not repair discovery: {}", event.payload), + _ => {} + } + } + std::thread::sleep(std::time::Duration::from_millis(1)); + } + executor.shutdown().unwrap(); + assert!(completed); + assert_eq!(denied, 1); + assert_eq!(dispatched, 1); + assert_eq!(call_count(&directory, "discovery:repair-guidance"), 1); + assert_eq!(call_count(&directory, "discovery:success"), 1); + fs::remove_dir_all(directory).unwrap(); +}