From 9e09d8f51f27f07568eeb05cb6384d26c32ed5c3 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:48:37 -0500 Subject: [PATCH] fix(runner): clarify native approval argument recovery (#18205) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - First-task onboarding requires a saved human proposal decision before child work can start. > - Native proposal tools reject interaction metadata placed outside their advertised schema. > - The existing rejection text focuses on question forms and does not explain the misplaced approval metadata. > - This pull request explains the field placement and keeps approval recovery in its original confirmation or checkbox kind. > - It also retains safe schema guidance in the run log so operators can identify the rejected field. ## Linked Issues or Issue Description Refs: #18025 for native first-task proposal recipes and #16889 for safe native schema guidance. Related open PR #18105 changes separate server API-client guidance. **What happened?** A native first-task confirmation copied `version` and `supersedeOnUserComment` to the tool root. Both belong inside `payload`. The runner correctly rejected the call before execution, but its feedback described question forms. Later calls kept the misplaced field and changed the approval into a question. No proposal card was created. The run log retained only a generic schema error. **Expected behavior** Keep strict validation. Explain the nested field locations. Keep a confirmation or checkbox approval in that kind while correcting its arguments. Save the normal pending card and wait for an actual human decision. Retain bounded schema guidance without storing rejected input. **Steps to reproduce** 1. Send an otherwise valid native confirmation with root-level `version: 1` and `supersedeOnUserComment: false`. 2. Observe the pre-dispatch `invalid_tool_call` rejection. 3. Move both fields into `payload` and keep `interactionKind: "confirmation"`. 4. Verify that the corrected call creates one pending confirmation, exact replay creates no duplicate, and child creation still needs saved human acceptance. **Paperclip version or commit** The observed argument shape and initial regressions came from public source base `c32e1ba6`. The repair is based on maintained master `4a6f6a65`. Public tests use synthetic prompts and identifiers. ## What Changed - Give rejected confirmations and checkboxes approval-specific guidance. It names `payload.version` and `payload.supersedeOnUserComment` without suggesting a question-form recipe. Other human-input errors retain complete question-form guidance and stable question/option ID instructions. - State the allowed native root fields in the first-task skill and put the metadata inside both executable proposal recipes. - Retain the existing safe guidance in `semantic_tool_denied` diagnostics. Keep the original rejection identity and `paperclipExecuted: false`. - Preserve only the exact bounded scalar diagnostic payload after verified event admission. The rest of the envelope and all API request/result bodies keep their existing redaction. - Add a real native Codex transport regression and strengthen advertised-schema, card replay, human-admission, persistence, and privacy controls. Update the execution contract and onboarding maps. ## Verification - Red: the native transport reproduced the malformed confirmation and showed that the old hint omitted the metadata correction. A second red confirmed that approval guidance still included the question-form recipe. The corrected transport case now rejects before execution, retains a safe hint, and dispatches one corrected confirmation that returns a pending card. - Red: both shipped recipe checks lacked explicit nested metadata. All 49 first-task admission tests now pass, including advertised-schema validation, idempotent card replay, actual human acceptance, agent-only rejection, and question-answer controls. - Updated native checks pass on the current repair: 95 native Codex transport tests with 2 existing ignored cases, all 32 provider-bridge tests, and the schema-feedback unit control. - API/case persistence regressions were red before the event-boundary correction. All 71 affected port/redaction tests pass in a clean run at frozen head `3be3a70f`, including the corrected envelope privacy regression. The focused 16-test persistence/privacy group and server typecheck also passed earlier. - Full local `pnpm build` and `pnpm -r typecheck` pass at exact head `3be3a70f` in a clean detached checkout with Node 24.22.0 and pnpm 9.15.4. Exact-head CI passes: [full PR CI](https://github.com/paperclipai/paperclip/actions/runs/40950360142) and [isolated native Runner readiness](https://github.com/paperclipai/paperclip/actions/runs/40950359835). - The full local unit attempt recorded a 15-second timeout in the unrelated Microsoft Teams 100,000-character existing-comment publication case. That failure remains recorded. The exact isolated rerun passed (1 test; 1,814 unselected). The aggregate timeout cause is unproven. That long local run straddled subsequent source edits and also observed the two port regressions during their red phase; it was stopped and is not final-head evidence. The clean affected-suite run above and current-head CI provide the final verification. No timeout or assertion was weakened. - Release-registry CI initially rejected the native test's compile-time schema include outside the release crate's hashed input closure. The guard was reproduced locally. The test now reads the current canonical schema at execution. All 10 crate-cache controls and the real native approval regression pass afterward; the cache guard and workflow keys remain unchanged. - Current-head Greptile review is 5/5 with no unresolved review threads. All required checks pass (56 success, 4 skipped). The advisory Paperclip Review remains neutral/incomplete after one normal re-review request; no completed assessment or score is claimed. - `cargo fmt --check`, map structural checks, and `git diff --check` pass. - Source and scripted-provider checks do not prove that a model will follow the guidance. The original live onboarding attempt remains failed; acceptance at a deployed revision is separate work. ## Risks - This changes repair guidance and shipped instructions. It does not make malformed arguments valid or supply a human decision. - Diagnostic retention adds no rejected arguments, values, or unknown input keys. The API receipt allowlist is unchanged. The validated-event exception restores only the safe scalar payload; optional debug and other envelope metadata still pass through redaction. - The existing 512-byte guidance bound, strict tool schema, company/run binding, idempotency checks, and proposal admission remain effective. ## Model Used OpenAI GPT-6.1 Sol (`gpt-6.1-sol`) through Codex, with tool use and code execution. The context-window size is not exposed to this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/execution-semantics.md | 7 ++ doc/run-log-events.md | 9 ++ maps/agent-access-map/onboarding.md | 5 + maps/feature-map/onboarding.md | 5 + .../src/bin/fake-codex-app-server.rs | 61 ++++++++++ .../runner-core/src/provider_backend.rs | 36 ++++-- .../crates/runner-core/src/provider_bridge.rs | 32 ++++-- .../runner-core/tests/codex_provider.rs | 2 + .../support/codex_assigned_tool_validation.rs | 18 +++ .../support/codex_human_input_validation.rs | 107 ++++++++++++++++++ .../onboarding-first-task-admission.test.ts | 13 ++- .../first-task/skills/first-task/SKILL.md | 6 + .../paperclip-control-plane-port.test.ts | 43 +++++++ .../paperclip-control-plane-port.ts | 4 +- .../runner-api-log-redaction.test.ts | 35 +++++- .../runner-api-log-redaction.ts | 17 +++ 16 files changed, 377 insertions(+), 23 deletions(-) create mode 100644 packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_human_input_validation.rs diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 61ab5136574..6ccaec0bfe4 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -565,6 +565,13 @@ 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. +For native `request_human_input`, `version` and `supersedeOnUserComment` belong +inside `payload`, never at the tool's root. Repair guidance names those paths +and keeps approval requests as `confirmation` or `checkbox`. A question form +collects answers; it cannot replace proposal acceptance. Invalid calls remain +rejected before dispatch, and a corrected call still waits for the normal saved +human decision before it can authorize onboarding child work. + A native provider may omit `continuation` (or send `null`) when it reports `yielded` after saving a monitor, requesting repository preparation, or publishing an authorized current-head GitHub review. The controller reads current authorized task diff --git a/doc/run-log-events.md b/doc/run-log-events.md index 3e2a709551e..1c6d900e926 100644 --- a/doc/run-log-events.md +++ b/doc/run-log-events.md @@ -184,6 +184,15 @@ Paperclip Telemetry export, and legacy adapters do not use this writer. ## Semantic Settlement Diagnostics and Retry Logs +When native tool arguments fail their authorized JSON Schema, the runner records +`harness.diagnostic` with code `semantic_tool_denied` and `paperclipExecuted: false`. +Its optional `inputSchemaGuidance` retains the same safe repair guidance sent to +the provider: at most three authorized schema locations within 512 bytes. +It includes no rejected arguments, submitted values, or unknown input keys. +The rejected call does not create a semantic input event or execution receipt. +Validated runner diagnostics retain this scalar metadata even when the operation +is an API or case tool. API request and response bodies keep their existing redaction. + Before interrupting a Codex turn, the runner durably records a `harness.diagnostic` event with code `provider_interrupt_requested`, the stop reason, provider turn ID, and pending tool call IDs. A harmless identical result diff --git a/maps/agent-access-map/onboarding.md b/maps/agent-access-map/onboarding.md index 4b55082465e..762ac1b5159 100644 --- a/maps/agent-access-map/onboarding.md +++ b/maps/agent-access-map/onboarding.md @@ -194,6 +194,11 @@ conversational resolution with a genuine later same-task user message supplies acceptance; an agent-only resolution does not. A plan approval must name the current same-task revision. Newer pending/rejected proposals stop new creation. Already admitted task replay and ordinary delegation retain their existing rules. +Native proposal metadata `version` and `supersedeOnUserComment` stays inside +`request_human_input.payload`. Invalid root metadata is rejected before dispatch; +repair guidance keeps the approval kind instead of replacing it with questions. +The shipped recipes are checked against the advertised schema and the real +interaction service. Idempotent proposal replay still needs a saved human decision. An accepted onboarding execution cannot create a new task blocked by its authenticated onboarding source. Native and REST creation return a recoverable input error before committing that task or its wake. The same source restriction diff --git a/maps/feature-map/onboarding.md b/maps/feature-map/onboarding.md index fb11e7001e9..e25d45fdc73 100644 --- a/maps/feature-map/onboarding.md +++ b/maps/feature-map/onboarding.md @@ -155,6 +155,11 @@ admit new work. [Admission controls](../../server/src/__tests__/onboarding-first exercise native and REST calls against disposable PostgreSQL; live model and Cloud onboarding remain separate evidence. +For a native proposal, reject root-level `version` or `supersedeOnUserComment` +without creating a card. Correct them inside `payload`, preserve the confirmation +or checkbox approval kind, and verify one pending card after idempotent replay. +An answered question form still cannot authorize the proposed child task. + After acceptance, reject a new task whose blockers include its onboarding source. The rejected command must leave no task, dependency, wake or creation receipt; a corrected retry must still create the ready child. Preserve valid blockers for 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 15dc55908f9..52ef2fac6b4 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 @@ -709,6 +709,9 @@ fn run() -> Result<(), Box> { let repair_assigned_discovery = args .iter() .any(|value| value == "--repair-assigned-discovery"); + let repair_first_task_confirmation = args + .iter() + .any(|value| value == "--repair-first-task-confirmation"); let require_completion_contract = args .iter() .any(|value| value == "--require-completion-contract"); @@ -1007,6 +1010,53 @@ fn run() -> Result<(), Box> { } continue; } + if repair_first_task_confirmation + && message.get("method").is_none() + && message.get("id") == Some(&json!("first-task-confirmation-request")) + { + let text = message + .pointer("/result/contentItems/0/text") + .and_then(Value::as_str) + .ok_or("confirmation 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("confirmation rejection omitted guidance")?; + if !hint.contains("payload.version") + || !hint.contains("payload.supersedeOnUserComment") + || !hint.contains("Keep approval") + || !hint.contains("confirmation or checkbox") + || hint.contains("payload.questionSet") + || !hint.contains("/additionalProperties") + || hint.contains("PRIVATE_CONFIRMATION_SCOPE") + { + return Err("confirmation rejection lacks metadata placement and approval-preserving guidance".into()); + } + log_call(call_log.as_deref(), "first-task:repair-guidance")?; + send( + json!({"id":"first-task-confirmation-request", "method":"item/tool/call", "params":{ + "threadId":state.thread_id, "turnId":state.active_turn_id, + "callId":"first-task-confirmation", "tool":"request_human_input", + "arguments": {"idempotencyKey":"first-task-proposal-confirmation", "interactionKind":"confirmation", + "title":"Approve child task", "prompt":"Approve PRIVATE_CONFIRMATION_SCOPE?", + "continuationPolicy":"wake_assignee", "payload":{"version":1,"supersedeOnUserComment":false, + "acceptLabel":"Approve","rejectLabel":"Revise","rejectRequiresReason":true}} + }}), + )?; + } else { + if message.pointer("/result/success") != Some(&json!(true)) + || result.pointer("/interaction/kind") != Some(&json!("request_confirmation")) + || result.pointer("/interaction/status") != Some(&json!("pending")) + { + return Err("corrected proposal did not return a pending confirmation".into()); + } + log_call(call_log.as_deref(), "first-task:confirmation-pending")?; + finish_turn(&state_path, &mut state, "completed")?; + } + continue; + } if message.get("method").is_none() && message.get("id") == Some(&json!("terminal-result-request")) { @@ -1756,6 +1806,17 @@ fn run() -> Result<(), Box> { "arguments":{"query":{"PRIVATE_QUERY":"private search"}} }}), )?; + } else if repair_first_task_confirmation { + send( + json!({"id":"first-task-confirmation-request", "method":"item/tool/call", "params":{ + "threadId":state.thread_id, "turnId":provider_turn_id, + "callId":"first-task-confirmation", "tool":"request_human_input", + "arguments": {"idempotencyKey":"first-task-proposal-confirmation", "interactionKind":"confirmation", + "title":"Approve child task", "prompt":"Approve PRIVATE_CONFIRMATION_SCOPE?", + "continuationPolicy":"wake_assignee", "payload":{"acceptLabel":"Approve","rejectLabel":"Revise","rejectRequiresReason":true}, + "version":1,"supersedeOnUserComment":false} + }}), + )?; } else if let Some(result) = &terminal_result { send(json!({ "id": "terminal-result-request", "method": "item/tool/call", 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 0587b9537dc..1547ce4a076 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 @@ -3533,17 +3533,21 @@ impl CodexCommandExecutor { .state .as_mut() .expect("Codex state remains available for a rejected tool call"); + let mut payload = json!({ + "provider": state.config.provider, + "code": "semantic_tool_denied", + "operationId": operation_id, + "callId": call_id, + "message": reason, + "paperclipExecuted": false, + }); + if let Some(guidance) = error.safe_provider_message() { + payload["inputSchemaGuidance"] = json!(guidance); + } let event = NormalizedProviderEvent { event_type: "harness.diagnostic".to_owned(), priority: EventPriority::P0, - payload: json!({ - "provider": state.config.provider, - "code": "semantic_tool_denied", - "operationId": operation_id, - "callId": call_id, - "message": reason, - "paperclipExecuted": false, - }), + payload, }; if state.receipt_limit_interrupt_pending { state.push_receipt_limit_cleanup_event(event)?; @@ -5032,15 +5036,29 @@ mod tests { let question_result = schema_rejection( "request_human_input", - json!({"PRIVATE_FIELD": "PRIVATE_VALUE"}), + json!({"interactionKind": "questions", "PRIVATE_FIELD": "PRIVATE_VALUE"}), ); let question_message = question_result.result["error"]["message"].as_str().unwrap(); assert!(question_message.contains("payload.questionSet")); + assert!(question_message.contains("Preserve question and option IDs")); assert!(question_message.contains("/required (missing \"requiredField\")")); assert!(!question_message.contains("PRIVATE_FIELD")); assert!(!question_result.result.to_string().contains("PRIVATE_VALUE")); assert!(question_message.len() <= 512); assert_eq!(question_result.result["error"]["retryable"], false); + for kind in ["confirmation", "checkbox"] { + let proposal = schema_rejection( + "request_human_input", + json!({"interactionKind": kind, "PRIVATE_FIELD": "PRIVATE_VALUE"}), + ); + let guidance = proposal.result["error"]["message"].as_str().unwrap(); + assert!(guidance.contains("payload.version")); + assert!(guidance.contains("payload.supersedeOnUserComment")); + assert!(guidance.contains("confirmation or checkbox")); + assert!(!guidance.contains("payload.questionSet")); + assert!(!proposal.result.to_string().contains("PRIVATE_VALUE")); + assert!(guidance.len() <= 512); + } let monitor_result = schema_rejection( "set_task_monitor", 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 684951199be..aeb60ac744b 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 @@ -40,7 +40,8 @@ const REPLAY_FILTER_WORDS: usize = 32_768; const ACTIVE_TURN_RECEIPT_LIMIT_MESSAGE: &str = "durable provider tool receipt limit reached for the active turn"; const COMPLETION_INPUT_SCHEMA_HINT: &str = "Invalid paperclip_finish arguments. Required fields: reportedWorkDisposition, summary, completionClaim, evidence, and verification. When reportedWorkDisposition is yielded, continuation must include kind=response_wake or monitor, summary, and idempotencyKey."; -const QUESTION_INPUT_SCHEMA_HINT: &str = "Invalid request_human_input arguments. Required fields: idempotencyKey, interactionKind, title, prompt, and continuationPolicy. For questions, use payload.version=1 and a complete payload.questionSet matching the authorized schema. Preserve the supplied question and option IDs."; +const HUMAN_INPUT_SCHEMA_HINT: &str = "Invalid request_human_input arguments. Put version and supersedeOnUserComment at payload.version and payload.supersedeOnUserComment, never at the root. For questions, use payload.version=1 and a complete payload.questionSet. Preserve question and option IDs. Correct the arguments and retry."; +const APPROVAL_INPUT_SCHEMA_HINT: &str = "Invalid request_human_input arguments. Put version and supersedeOnUserComment at payload.version and payload.supersedeOnUserComment, never at the root. Keep approval requests in their confirmation or checkbox interactionKind; do not replace them with questions. Correct the arguments and retry."; 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."; @@ -242,11 +243,16 @@ impl ProviderBridgeError { } } - fn input_schema_validation(operation_id: &str) -> Self { + fn input_schema_validation(operation_id: &str, input: &Value) -> Self { let safe_provider_message = match operation_id { "paperclip_finish" => Some(COMPLETION_INPUT_SCHEMA_HINT), "paperclip_block" => Some(BLOCK_INPUT_SCHEMA_HINT), - "request_human_input" => Some(QUESTION_INPUT_SCHEMA_HINT), + "request_human_input" => { + Some(match input.get("interactionKind").and_then(Value::as_str) { + Some("confirmation" | "checkbox") => APPROVAL_INPUT_SCHEMA_HINT, + _ => HUMAN_INPUT_SCHEMA_HINT, + }) + } "set_task_monitor" => Some(MONITOR_INPUT_SCHEMA_HINT), "call_api" => Some(API_INPUT_SCHEMA_HINT), _ => Some(TOOL_INPUT_SCHEMA_HINT), @@ -667,8 +673,10 @@ impl ProviderToolBridge { )) })?; if !validator.is_valid(&input) { - return Err(ProviderBridgeError::input_schema_validation(&operation_id) - .with_schema_locations(&validator, &input)); + return Err( + ProviderBridgeError::input_schema_validation(&operation_id, &input) + .with_schema_locations(&validator, &input), + ); } if matches!( operation_id.as_str(), @@ -678,7 +686,10 @@ impl ProviderToolBridge { .and_then(Value::as_str) .is_some_and(|summary| summary.chars().count() > MAX_COMPLETION_SUMMARY_CHARS) { - return Err(ProviderBridgeError::input_schema_validation(&operation_id)); + return Err(ProviderBridgeError::input_schema_validation( + &operation_id, + &input, + )); } bounded_json(&input, MAX_TOOL_VALUE_BYTES, "provider tool input")?; let call = PendingToolCall { @@ -1626,7 +1637,7 @@ mod tests { "c": {"type": "string"}, "d": {"type": "string"}, }}); let validator = jsonschema::validator_for(&schema).unwrap(); - let error = ProviderBridgeError::input_schema_validation("paperclip_finish") + let error = ProviderBridgeError::input_schema_validation("paperclip_finish", &json!({})) .with_schema_locations(&validator, &json!({"a": 1, "b": 2, "c": 3, "d": 4})); assert_eq!( error @@ -1636,7 +1647,7 @@ mod tests { .count(), 3 ); - let other = ProviderBridgeError::input_schema_validation("some_other_tool") + let other = ProviderBridgeError::input_schema_validation("some_other_tool", &json!({})) .with_schema_locations(&validator, &json!({"a": 1})); let message = other.safe_provider_message().unwrap(); assert!(message.starts_with(TOOL_INPUT_SCHEMA_HINT)); @@ -1662,8 +1673,9 @@ mod tests { json!({"idempotencyKey": "check-ci", "nextCheckAt": "PRIVATE_TIMESTAMP", "notes": "PRIVATE_VALUE"}), json!({"idempotencyKey": "check-ci", "monitor": {"nextCheckAt": "PRIVATE_TIMESTAMP", "notes": "PRIVATE_VALUE".repeat(100), "PRIVATE_FIELD": "PRIVATE_VALUE"}}), ] { - let error = ProviderBridgeError::input_schema_validation("set_task_monitor") - .with_schema_locations(&validator, &input); + let error = + ProviderBridgeError::input_schema_validation("set_task_monitor", &json!({})) + .with_schema_locations(&validator, &input); let message = error.safe_provider_message().unwrap(); assert!(message.contains("Put all scheduling fields inside monitor")); assert!(message.contains("/required") || message.contains("/properties/monitor/")); 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 5d8ccc10ab7..f4f05edc76d 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 @@ -21,6 +21,8 @@ use serde_json::{json, Value}; #[path = "support/codex_assigned_tool_validation.rs"] mod assigned_tool_validation; +#[path = "support/codex_human_input_validation.rs"] +mod human_input_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 index fe029b8ae2d..9dfc24a3c2c 100644 --- 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 @@ -51,8 +51,26 @@ fn assigned_tool_discovery_repairs_invalid_arguments_through_codex_transport() { match event.event_type.as_str() { "harness.diagnostic" if event.payload["code"] == "semantic_tool_denied" => { denied += 1; + assert_eq!(dispatched, 0); + assert_eq!( + event.payload["operationId"], + "paperclip_search_assigned_tools" + ); + assert_eq!(event.payload["callId"], "assigned-discovery"); + assert_eq!( + event.payload["message"], + "provider arguments for paperclip_search_assigned_tools failed JSON Schema validation" + ); assert_eq!(event.payload["paperclipExecuted"], false); + let guidance = event.payload["inputSchemaGuidance"] + .as_str() + .expect("denied input retains the safe guidance sent to the provider"); + assert!(guidance.contains("/properties/query/type"), "{guidance}"); + assert!(guidance.len() <= 512); + assert!(event.payload.get("input").is_none()); + assert!(event.payload.get("arguments").is_none()); assert!(!event.payload.to_string().contains("PRIVATE_QUERY")); + assert!(!event.payload.to_string().contains("private search")); } "semantic_tool.input" => { dispatched += 1; diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_human_input_validation.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_human_input_validation.rs new file mode 100644 index 00000000000..5a09092158a --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/support/codex_human_input_validation.rs @@ -0,0 +1,107 @@ +use super::*; + +#[test] +fn first_task_confirmation_repairs_misplaced_metadata_without_becoming_a_question() { + let directory = temporary_directory("first-task-confirmation-repair"); + // Read the current generated schema at test execution instead of embedding + // test-only data outside the release crate's hashed build-input closure. + let contracts: Value = serde_json::from_str( + &fs::read_to_string( + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../../generated/capability/semantic-tool-contracts.json"), + ) + .expect("read the generated semantic tool contracts"), + ) + .expect("parse the generated semantic tool contracts"); + let definition = contracts + .as_array() + .unwrap() + .iter() + .find(|tool| tool["name"] == "request_human_input") + .unwrap(); + let operations = vec![AuthorizedTool { + operation_id: "request_human_input".to_owned(), + version: 1, + description: definition["description"].as_str().unwrap().to_owned(), + input_schema: definition["inputSchema"].clone(), + response_schema: json!({"type":"object"}), + }]; + let tools = 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":provider_config(&directory, &["--repair-first-task-confirmation"]), "authorizedTools":tools + }))).unwrap(); + executor + .execute(&command("open", 2, "session.open", json!({}))) + .unwrap(); + executor + .execute(&command( + "turn", + 3, + "turn.start", + json!({"text":"Propose one child task and wait for approval."}), + )) + .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!(dispatched, 0); + assert_eq!(event.payload["paperclipExecuted"], false); + let hint = event.payload["inputSchemaGuidance"].as_str().unwrap(); + assert!(hint.contains("payload.version")); + assert!(hint.contains("payload.supersedeOnUserComment")); + assert!(hint.contains("Keep approval")); + assert!(hint.contains("confirmation or checkbox")); + assert!(!hint.contains("payload.questionSet")); + assert!(hint.contains("/additionalProperties")); + assert!(hint.len() <= 512); + assert!(!event + .payload + .to_string() + .contains("PRIVATE_CONFIRMATION_SCOPE")); + } + "semantic_tool.input" => { + dispatched += 1; + assert_eq!(denied, 1); + let input = &event.payload["semantic_tool"]["input"]; + assert_eq!(input["interactionKind"], "confirmation"); + assert!(input.get("version").is_none()); + assert!(input.get("supersedeOnUserComment").is_none()); + assert_eq!(input["payload"]["version"], 1); + assert_eq!(input["payload"]["supersedeOnUserComment"], false); + executor.execute(&command("result", 4, "semantic_tool.result", json!({ + "callId":"first-task-confirmation", "operationId":"request_human_input", + "result":{"interaction":{"id":"proposal-card","kind":"request_confirmation","status":"pending"}}, + "isError":false + }))).unwrap(); + } + "turn.completed" => completed = true, + "turn.failed" => panic!( + "proposal could not correct its arguments: {}", + 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, "first-task:repair-guidance"), 1); + assert_eq!(call_count(&directory, "first-task:confirmation-pending"), 1); + fs::remove_dir_all(directory).unwrap(); +} diff --git a/server/src/__tests__/onboarding-first-task-admission.test.ts b/server/src/__tests__/onboarding-first-task-admission.test.ts index e91f88b0417..c87ef2c096a 100644 --- a/server/src/__tests__/onboarding-first-task-admission.test.ts +++ b/server/src/__tests__/onboarding-first-task-admission.test.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { readFile } from "node:fs/promises"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import Ajv2020 from "ajv/dist/2020.js"; import { eq } from "drizzle-orm"; import { agentWakeupRequests, authUsers, companyMemberships, heartbeatRuns, issueComments, issueThreadInteractions, issues } from "@paperclipai/db"; import { createIssueThreadInteractionSchema } from "@paperclipai/shared"; @@ -13,6 +14,7 @@ import { initializeRunIdentity } from "../services/run-identity.js"; import { issueService } from "../services/issues.js"; import { recoveryPostgresSupport as support } from "./helpers/recovery-postgres.js"; import { startRunnerApiTestServer } from "./helpers/runner-api-server.js"; +import { CAPABILITY_SEMANTIC_TOOL_CATALOG } from "../vendor/paperclip-runner/index.js"; async function nativeProposalRecipe(kind: "confirmation" | "checkbox", planRevisionId = randomUUID()) { const skill = await readFile(new URL("../onboarding-assets/first-task/skills/first-task/SKILL.md", import.meta.url), "utf8"); @@ -25,6 +27,12 @@ async function nativeProposalRecipe(kind: "confirmation" | "checkbox", planRevis it.each(["confirmation", "checkbox"] as const)("ships a native %s recipe accepted by the production interaction schema", async kind => { const recipe = await nativeProposalRecipe(kind); + const schema = CAPABILITY_SEMANTIC_TOOL_CATALOG.find(tool => tool.operationId === "request_human_input")!.inputSchema; + const validator = new Ajv2020({ allErrors: true, strict: false }); + expect(validator.validate(schema, recipe), JSON.stringify(validator.errors)).toBe(true); + expect(recipe).not.toHaveProperty("version"); + expect(recipe).not.toHaveProperty("supersedeOnUserComment"); + expect(recipe.payload).toMatchObject({ version: 1, supersedeOnUserComment: false }); const parsed = createIssueThreadInteractionSchema.parse({ kind: kind === "confirmation" ? "request_confirmation" : "request_checkbox_confirmation", idempotencyKey: recipe.idempotencyKey, title: recipe.title, continuationPolicy: recipe.continuationPolicy, payload: { ...(recipe.payload as Record), version: 1, prompt: recipe.prompt } }); @@ -427,7 +435,10 @@ describe.skipIf(!support.supported)("onboarding first-task execution admission", title: "Approved child plan", format: "markdown", body: "Create one child for the proposed deliverable.", createdByAgentId: f.agentId, createdByRunId: f.runId })).document : null; const args = await nativeProposalRecipe(kind, plan?.latestRevisionId ?? undefined); - const { interaction } = await f.authority.execute({ tool: "request_human_input", callId: randomUUID(), arguments: args }) as { interaction: { id: string; kind: string } }; + const receipt = await f.authority.execute({ tool: "request_human_input", callId: randomUUID(), arguments: args }) as { interaction: { id: string; kind: string } }; + const { interaction } = receipt; + await expect(f.authority.execute({ tool: "request_human_input", callId: randomUUID(), arguments: args })).resolves.toEqual(receipt); + expect(await server.db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.issueId, f.issueId))).toHaveLength(1); expect(interaction.kind).toBe(kind === "confirmation" ? "request_confirmation" : "request_checkbox_confirmation"); await expect(create(f)).rejects.toThrow(/proposal.*accept/i); const selectedOptionIds = kind === "checkbox" ? ((args.payload as { options: { id: string }[] }).options.map(option => option.id)) : undefined; diff --git a/server/src/onboarding-assets/first-task/skills/first-task/SKILL.md b/server/src/onboarding-assets/first-task/skills/first-task/SKILL.md index e04f30dffff..9afe2d13692 100644 --- a/server/src/onboarding-assets/first-task/skills/first-task/SKILL.md +++ b/server/src/onboarding-assets/first-task/skills/first-task/SKILL.md @@ -40,6 +40,8 @@ Work in this order. For a native single-task confirmation, call `request_human_input` with arguments like these. Replace `` with the proposed title and describe its actual scope in the prompt. Use a new idempotency key for each changed or revised proposal. Reuse a key only for the exact same proposal when reconciling an interrupted call; the example key below is for the first proposal. + The tool's root fields are `idempotencyKey`, `interactionKind`, `title`, `prompt`, `continuationPolicy`, and `payload`, with optional `targetRevisionId` and `resolverPolicy` as advertised. Put `version` and `supersedeOnUserComment` inside `payload`; neither is a root argument. If validation rejects a proposal, correct its argument placement and keep the confirmation or checkbox approval kind. Switching it to a question form cannot authorize the proposed work. Wait for a successful saved card receipt before claiming you requested approval. + ```json { "idempotencyKey": "first-task-proposal-confirmation", @@ -48,6 +50,8 @@ Work in this order. "prompt": "Approve creating exactly one child task named \"\" for the scope described above?", "continuationPolicy": "wake_assignee", "payload": { + "version": 1, + "supersedeOnUserComment": false, "acceptLabel": "Approve", "rejectLabel": "Revise", "rejectRequiresReason": true @@ -65,6 +69,8 @@ Work in this order. "prompt": "Approve the proposed scope in this saved plan?", "continuationPolicy": "wake_assignee", "payload": { + "version": 1, + "supersedeOnUserComment": false, "options": [{ "id": "scope", "label": "Approve the proposed scope for " }], "defaultSelectedOptionIds": ["scope"], "minSelected": 1, diff --git a/server/src/services/native-runtime/paperclip-control-plane-port.test.ts b/server/src/services/native-runtime/paperclip-control-plane-port.test.ts index 7e68810fc6d..57c2d3e6289 100644 --- a/server/src/services/native-runtime/paperclip-control-plane-port.test.ts +++ b/server/src/services/native-runtime/paperclip-control-plane-port.test.ts @@ -486,6 +486,49 @@ describe("PaperclipControlPlanePort conformance", () => { expect(result.data.credentials).toBe("private-history-fixture-value"); }); + it.each(["call_api", "create_case"])("retains bounded %s schema-denial guidance through persistence and replay", async operationId => { + const identity = { ...CONTROL_PLANE_CONFORMANCE_OPEN.identity, runId: randomUUID(), sessionId: randomUUID() }; + const runnerId = randomUUID(); + await db.insert(heartbeatRuns).values({ + id: identity.runId, companyId: identity.companyId, agentId: identity.agentId, + status: "running", runtimeMode: "native", nativeIssueId: identity.issueId, + nativeSessionId: identity.sessionId, runnerInstanceId: runnerId, + completionContractId: contractId, completionContractSha256: contractSha, + contextSnapshot: { issueId: identity.issueId }, + }); + const observed: PrpEvent[] = []; + const port = new PaperclipControlPlanePort(db, { + ...identity, completionContractId: contractId, completionContractSha256: contractSha, + sourceInstanceId: runnerId, controlPlaneSourceInstanceId: `api-${identity.runId}`, + }, { onCommittedEvent: async event => { observed.push(event); } }); + await port.openRun({ identity, backendKind: "mock", sourceInstanceId: runnerId }); + const payload = { + provider: "codex", code: "semantic_tool_denied", operationId, callId: "schema-denied", + message: `provider arguments for ${operationId} failed JSON Schema validation`, paperclipExecuted: false, + inputSchemaGuidance: "Invalid tool arguments. Check these input schema locations: /required (missing title).", + }; + const event: PrpEvent = { + schema: "paperclip.prp.event.v1", sourceEventId: `${runnerId}:1`, sourceSeq: 1, + sourceInstanceId: runnerId, sourceKind: "runner", runId: identity.runId, + normalizedSessionId: identity.sessionId, eventType: "harness.diagnostic", schemaVersion: 1, + priority: 0, emittedAt: new Date().toISOString(), payload, + debug: { operationId, data: { value: "private-diagnostic-debug" } }, + }; + await port.appendEvent(event); + expect(observed[0]?.payload).toEqual(payload); + expect(observed[0]?.debug).toMatchObject({ apiPayloadRedacted: true }); + expect(JSON.stringify(observed[0])).not.toContain("private-diagnostic-debug"); + expect((await port.appendEvent(event)).disposition).toBe("duplicate"); + expect(port.normalizeReplayPayload(payload)).toEqual(port.normalizeReplayPayload(observed[0]!.payload)); + const saved = await db.select().from(heartbeatRunEvents).where(eq(heartbeatRunEvents.runId, identity.runId)); + expect(saved).toHaveLength(1); + expect((saved[0]!.payload?.prpEvent as unknown as PrpEvent).payload).toEqual(payload); + expect(JSON.stringify(saved)).not.toContain("private-diagnostic-debug"); + await expect(port.appendEvent({ ...event, payload: { + ...payload, inputSchemaGuidance: "Invalid tool arguments. Check these input schema locations: /properties/title/type.", + } })).rejects.toThrow(); + }); + it("redacts identity private material before persisting native events and checkpoints", async () => { const identity = { ...CONTROL_PLANE_CONFORMANCE_OPEN.identity, runId: randomUUID(), sessionId: randomUUID() }; const runnerId = randomUUID(); diff --git a/server/src/services/native-runtime/paperclip-control-plane-port.ts b/server/src/services/native-runtime/paperclip-control-plane-port.ts index 47faa2e5231..ef0dfa235cb 100644 --- a/server/src/services/native-runtime/paperclip-control-plane-port.ts +++ b/server/src/services/native-runtime/paperclip-control-plane-port.ts @@ -29,7 +29,7 @@ import { import { appendHeartbeatRunEvent } from "../heartbeat-run-events.js"; import { publishLiveEvent } from "../live-events.js"; import { nativeSha256 } from "./canonical.js"; -import { redactRunnerApiPayloads } from "./runner-api-log-redaction.js"; +import { redactRunnerApiEvent, redactRunnerApiPayloads } from "./runner-api-log-redaction.js"; export interface PaperclipControlPlaneBinding { companyId: string; @@ -262,7 +262,7 @@ export class PaperclipControlPlanePort implements ControlPlanePort { } } } - event = redactRunnerApiPayloads(event); + event = redactRunnerApiEvent(event); const persisted = await this.#db.transaction(async tx => { this.#assertActive(options); const receipt = await appendHeartbeatRunEvent(tx as unknown as Db, { diff --git a/server/src/services/native-runtime/runner-api-log-redaction.test.ts b/server/src/services/native-runtime/runner-api-log-redaction.test.ts index 95b3d403d63..3862c53f0a1 100644 --- a/server/src/services/native-runtime/runner-api-log-redaction.test.ts +++ b/server/src/services/native-runtime/runner-api-log-redaction.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { redactRunnerApiPayloads } from "./runner-api-log-redaction.js"; +import { redactRunnerApiEvent, redactRunnerApiPayloads } from "./runner-api-log-redaction.js"; describe("API payload persistence redaction", () => { it.each(["POST /api/agents/me/secrets/{key}/value", "POST /api/companies/{companyId}/exports", "GET /api/projects/{id}"])("preserves private HTTP results on the wire, but omits %s from history", operationId => { @@ -26,6 +26,39 @@ describe("API payload persistence redaction", () => { const value = { result: { operationId: "get_task_context", data: { title: "Work" } }, text: '{ "ordinary": "output" }' }; expect(redactRunnerApiPayloads(value)).toEqual(value); }); + it.each(["call_api", "create_case"])("keeps %s denial metadata out of the API-body exception", operationId => { + const payload = { + provider: "codex", operationId, callId: "schema-denied", code: "semantic_tool_denied", + paperclipExecuted: false, message: "Provider arguments failed JSON Schema validation.", + inputSchemaGuidance: "Invalid tool arguments. Check these input schema locations: /properties/title/type.", + }; + const event = { sourceKind: "runner", eventType: "harness.diagnostic", payload }; + expect(redactRunnerApiEvent(event)).toEqual(event); + const debugEvent = { ...event, debug: { operationId, data: { value: "private-debug-value" } } }; + const savedDebug = redactRunnerApiEvent(debugEvent); + expect(savedDebug.payload).toEqual(payload); + expect(savedDebug.debug).toMatchObject({ apiPayloadRedacted: true }); + expect(JSON.stringify(savedDebug)).not.toContain("private-debug-value"); + expect(redactRunnerApiEvent(savedDebug)).toEqual(savedDebug); + for (const value of [ + { ...event, eventType: "item.completed" }, + { ...event, sourceKind: "control_plane" }, + { ...event, payload: { ...payload, arguments: { value: "private-fixture-value" } } }, + { ...event, payload: { ...payload, inputSchemaGuidance: "x".repeat(513) } }, + { ...event, payload: { ...payload, inputSchemaGuidance: "unsafe\nmetadata" } }, + { ...event, payload: { ...payload, inputSchemaGuidance: { value: "private-fixture-value" } } }, + ]) { + const saved = redactRunnerApiEvent(value); + expect(saved.payload).toMatchObject({ apiPayloadRedacted: true }); + expect(saved.payload).not.toHaveProperty("inputSchemaGuidance"); + expect(JSON.stringify(saved)).not.toContain("private-fixture-value"); + } + const nested = { sourceKind: "runner", eventType: "item.completed", payload: { + operationId, data: { ...event, payload: { ...payload, inputSchemaGuidance: "private-fixture-value" } }, + } }; + expect(JSON.stringify(redactRunnerApiEvent(nested))).not.toContain("private-fixture-value"); + expect(redactRunnerApiPayloads(payload)).not.toHaveProperty("inputSchemaGuidance"); + }); it("omits malformed API arguments and semantic result envelopes", () => { const value = { item: { type: "tool_use", name: "call_api", input: { operationId: "invalid", body: { value: "private-fixture-value" } } }, result: { operationId: "call_api", result: { data: "private-fixture-value" } } }; diff --git a/server/src/services/native-runtime/runner-api-log-redaction.ts b/server/src/services/native-runtime/runner-api-log-redaction.ts index 9be08bb9661..58091b32f18 100644 --- a/server/src/services/native-runtime/runner-api-log-redaction.ts +++ b/server/src/services/native-runtime/runner-api-log-redaction.ts @@ -4,6 +4,23 @@ import { nativeSha256 } from "./canonical.js"; const RECEIPT_FIELDS = new Set(["operationId", "apiOperationId", "callId", "ok", "status", "contentType"]); const REDACTED_FIELDS = new Set([...RECEIPT_FIELDS, "data", "apiPayloadRedacted", "apiPayloadSha256"]); const CASE_OPERATIONS = new Set(["create_case", "read_case", "update_case", "read_case_document", "write_case_document", "list_workflows", "read_workflow", "create_workflow", "update_workflow"]); +const SCHEMA_DENIAL_STRINGS = ["provider", "operationId", "callId", "message", "inputSchemaGuidance"]; +const SCHEMA_DENIAL_FIELDS = new Set([...SCHEMA_DENIAL_STRINGS, "code", "paperclipExecuted"]); + +/** Only use after the PRP event's run and source bindings have been checked. */ +export function redactRunnerApiEvent(event: T): T { + const payload = event.payload && typeof event.payload === "object" ? event.payload as Record : {}; + // These scalar fields are authored by the runner's validation boundary, not + // an API request or result. Never apply this exception inside arbitrary data. + if (event.sourceKind === "runner" && event.eventType === "harness.diagnostic" + && payload.code === "semantic_tool_denied" && payload.paperclipExecuted === false + && Object.keys(payload).every(key => SCHEMA_DENIAL_FIELDS.has(key)) + && SCHEMA_DENIAL_STRINGS.every(key => typeof payload[key] === "string" + && payload[key].length > 0 && payload[key].length <= 512 && /^[\x20-\x7e]+$/.test(payload[key]))) { + return { ...redactRunnerApiPayloads(event), payload: event.payload }; + } + return redactRunnerApiPayloads(event); +} function redactedPayload(value: unknown) { const entry = value && typeof value === "object" ? value as Record : {};