diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index f25f6a28b94..326593a88f4 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1616,6 +1616,13 @@ does not prove that the provider stopped or that a retry is safe; it remains non-recoverable until the existing ownership and recovery checks establish the next permitted action. +If a failed sidecar control closes an ACPX session before the event pump observes +the failure, its terminal retains the original closed-code diagnostic. Later +polling must not replace it with a generic closed-session message or infer a +provider explanation from that code. Intentional shutdown keeps its ordinary +closed state. A startup failure with a validated provider cause keeps that cause +when cleanup also fails. Neither case authorizes another attempt. + Provider rejections additionally use the versioned `ProviderFailure` cause. The provider adapter extracts and redacts the original explanation before the process boundary. Startup command receipts and failed turns retain the same diff --git a/doc/run-log-events.md b/doc/run-log-events.md index 12bbc26e666..bd935192835 100644 --- a/doc/run-log-events.md +++ b/doc/run-log-events.md @@ -105,6 +105,12 @@ that an ACPX startup rejection keeps Runner's closed rejection text and its classification. Startup receipts retain the cause across controller restart before any provider turn exists. +When a failed ACPX control closes the session before polling, the resulting +`turn.failed` event retains that control's closed-code error. A later closed-session +check does not replace it, and a closed code does not infer a provider explanation. +The terminal still reports `recoverable: false` and no observed provider terminal; +it does not prove termination or authorize replay. + Every ACPX session ensure rejection carries a startup cause. When the provider returned no ACP error, the cause keeps the rejection's own redacted message with category `unknown`. A provider can type the failure with ACP's diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs index d0597a74313..ca505adab0f 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_session.rs @@ -348,6 +348,7 @@ pub struct AcpxProviderSession { catalog_revision: u64, working_directory: PathBuf, closed: bool, + failure: Option, transport_terminated: bool, runtime_retired: bool, } @@ -383,6 +384,7 @@ impl AcpxProviderSession { catalog_revision: config.catalog_revision, working_directory: config.working_directory.clone(), closed: false, + failure: None, transport_terminated: false, runtime_retired: false, }) @@ -1112,7 +1114,10 @@ impl AcpxProviderSession { fn ensure_open(&self) -> Result<(), LocalRunnerError> { if self.closed { - return Err(LocalRunnerError::invalid("ACPX provider session is closed")); + return Err(self + .failure + .clone() + .unwrap_or_else(|| LocalRunnerError::invalid("ACPX provider session is closed"))); } Ok(()) } @@ -1197,7 +1202,12 @@ impl AcpxProviderSession { fn fail_closed(&mut self, error: LocalRunnerError) -> LocalRunnerError { self.closed = true; - with_cleanup_error(error, self.terminate_transport()) + let error = with_cleanup_error( + self.failure.clone().unwrap_or(error), + self.terminate_transport(), + ); + self.failure = Some(error.clone()); + error } } @@ -1701,12 +1711,32 @@ fn with_cleanup_error( Ok(()) => error, Err(cleanup) => LocalRunnerError::invalid(format!( "{error}; ACPX sidecar cleanup also failed: {cleanup}" - )), + )) + .with_provider_failure(error.provider_failure()), } } #[cfg(test)] mod tests { + #[test] + fn startup_cleanup_failure_keeps_the_validated_provider_cause() { + use super::*; + let cause = json!({ + "schema":"paperclip.provider-failure.v1", "provider":"claude", "phase":"startup", + "category":"authentication", "message":"Sign in to the provider.", "retryability":"requires_action" + }); + let original = LocalRunnerError::invalid("ACPX sidecar command session.open was rejected (retryable=false, classification=provider_authentication)") + .with_provider_failure(Some(cause.clone())); + let error = with_cleanup_error( + original.clone(), + Err(LocalRunnerError::invalid("cleanup was not confirmed")), + ); + assert_eq!(error.provider_failure(), Some(cause)); + assert!(error.to_string().starts_with(&original.to_string())); + assert!(error.to_string().contains("cleanup was not confirmed")); + assert!(!error.to_string().contains("Sign in to the provider.")); + } + #[test] fn live_request_snapshot_requires_current_unique_typed_callbacks() { use super::*; diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs index 97c5f60a800..35b3f8b9a1f 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-acpx-sidecar.rs @@ -314,6 +314,7 @@ fn run() -> Result<(), Box> { | "turns-wrong-cancel" | "turns-wrong-scope" | "turns-tool" + | "turns-tool-resolution-rejected" | "turns-tool-terminal" | "turns-reused-tool-id-terminal" | "turns-late-tool-after-suspend" @@ -349,6 +350,18 @@ fn run() -> Result<(), Box> { | "suspend-wrong-ack" | "suspend-wrong-identity" | "suspend-missing-identity" => { + if command == "tool.resolve" && mode == "turns-tool-resolution-rejected" { + write_json( + &mut stdout, + &json!({ + "protocolVersion":GENERATED_ACPX_SIDECAR_PROTOCOL_VERSION, + "id":id,"ok":false,"error":{ + "code":"ACPX_TOOL_CALL_STALE", "message":"private-provider-token-canary", "retryable":false + } + }), + )?; + continue; + } if command == "turn.cancel" && mode == "turns-retired-terminal-first" { let turn_id = request .pointer("/params/turnId") @@ -443,6 +456,7 @@ fn run() -> Result<(), Box> { && matches!( mode, "turns-tool" + | "turns-tool-resolution-rejected" | "turns-tool-terminal" | "turns-reused-tool-id-terminal" | "turns-late-tool-after-suspend" @@ -459,7 +473,7 @@ fn run() -> Result<(), Box> { turn_id, json!({ "callId":tool_call_id.clone(), - "operationId":if matches!(mode, "turns-tool" | "turns-tool-terminal" | "turns-reused-tool-id-terminal" | "turns-late-tool-after-suspend" | "turns-tool-result-terminal" | "turns-tool-error-result-terminal") { "issues.read" } else { "issues.delete" }, + "operationId":if matches!(mode, "turns-tool" | "turns-tool-resolution-rejected" | "turns-tool-terminal" | "turns-reused-tool-id-terminal" | "turns-late-tool-after-suspend" | "turns-tool-result-terminal" | "turns-tool-error-result-terminal") { "issues.read" } else { "issues.delete" }, "input":{"id":"issue-1"}, }), )?; diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_session.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_session.rs index cb28c8c3f72..9478a755975 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_session.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_session.rs @@ -138,6 +138,41 @@ fn bootstraps_a_codex_session_and_confirms_run_identity() { assert_eq!(session.catalog_revision(), 1); session.shutdown("test complete").unwrap(); session.shutdown("already closed").unwrap(); + let closed = session.poll_event(Duration::ZERO).unwrap_err(); + assert_eq!(closed.to_string(), "ACPX provider session is closed"); + assert!(closed.provider_failure().is_none()); +} + +#[test] +fn retains_the_first_failed_control_after_closing_the_session() { + let mut session = + AcpxProviderSession::start(&config("turns-tool-resolution-rejected")).unwrap(); + session + .start_turn("turn-1", "Read the issue", &std::env::temp_dir()) + .unwrap(); + session.poll_event(Duration::from_secs(1)).unwrap().unwrap(); + let result = paperclip_runner_core::provider_bridge::ToolResult { + call_id: "call-1".to_owned(), + operation_id: "issues.read".to_owned(), + result: json!({"id":"issue-1"}), + is_error: false, + }; + let first = session.deliver_tool_result(&result).unwrap_err(); + assert!(first + .to_string() + .contains("classification=provider_tool_call_retired")); + assert!(!first.to_string().contains("private-provider-token-canary")); + assert!(first.provider_failure().is_none()); + assert!(session.state().pending_tool("call-1").is_some()); + assert_eq!(session.poll_event(Duration::ZERO).unwrap_err(), first); + assert_eq!(session.deliver_tool_result(&result).unwrap_err(), first); + assert_eq!( + session + .start_turn("turn-2", "Continue", &std::env::temp_dir()) + .unwrap_err(), + first + ); + session.shutdown("closed failure cleanup").unwrap(); } #[test] diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs index adcda2f58fc..8e74238970e 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/native_provider_backend.rs @@ -1334,6 +1334,71 @@ fn acpx_transport_failure_retains_its_error_without_reopening_the_provider() { fs::remove_dir_all(directory).unwrap(); } +#[test] +#[cfg_attr(windows, ignore = "verified launch is not supported on Windows yet")] +fn acpx_control_failure_keeps_its_classification_in_the_durable_terminal() { + let directory = temporary_directory("acpx-control-failure"); + let mode = "turns-tool-resolution-rejected"; + let config = acpx_config(&directory, mode); + let mut executor = NativeProviderCommandExecutor::with_runner_config(&directory, &config); + let mut prepare = prepare_payload_with_mode(&directory, "codex", mode); + let operations = vec![AuthorizedTool { + operation_id: "issues.read".to_owned(), + version: 1, + description: "Read an issue.".to_owned(), + input_schema: json!({"type":"object"}), + response_schema: json!({"type":"object"}), + }]; + prepare["authorizedTools"]["catalogDigest"] = + json!(authorized_tool_catalog_digest(&operations).unwrap()); + prepare["authorizedTools"]["operations"] = json!(operations); + executor + .execute(&command(1, "run.prepare", prepare)) + .unwrap(); + executor + .execute(&command(2, "session.open", json!({}))) + .unwrap(); + executor + .execute(&command( + 3, + "turn.start", + json!({"text":"Read the issue", "turnId":"turn-1"}), + )) + .unwrap(); + let inputs = executor.poll_events().unwrap(); + assert!(inputs + .iter() + .any(|event| event.event_type == "semantic_tool.input")); + executor.acknowledge_events(inputs.len()).unwrap(); + let rejected = executor.execute(&command(4, "semantic_tool.result", json!({ + "callId":"call-1", "operationId":"issues.read", "result":{"id":"issue-1"}, "isError":false + }))).unwrap_err(); + assert!(rejected + .to_string() + .contains("classification=provider_tool_call_retired")); + let events = executor.poll_events().unwrap(); + let failure = events + .iter() + .find(|event| event.event_type == "turn.failed") + .unwrap(); + assert!(failure.payload["error"]["message"] + .as_str() + .unwrap() + .contains("classification=provider_tool_call_retired")); + assert!(failure.payload["error"]["providerFailure"].is_null()); + assert_eq!(failure.payload["error"]["recoverable"], false); + assert_eq!(failure.payload["providerTerminalObserved"], false); + let retained = executor.retained_events().unwrap(); + assert_eq!(retained, events); + drop(executor); + let mut recovered = NativeProviderCommandExecutor::with_runner_config(&directory, &config); + assert_eq!(recovered.poll_events().unwrap(), retained); + recovered.acknowledge_events(retained.len()).unwrap(); + assert!(recovered.poll_events().unwrap().is_empty()); + shutdown_recovered_acpx_fixture(&mut recovered, &directory); + fs::remove_dir_all(directory).unwrap(); +} + #[test] #[cfg_attr(windows, ignore = "verified launch is not supported on Windows yet")] fn provider_death_expires_input_and_commits_failure_without_querying_the_dead_provider() {