From 80639f4f69c8938eb74bdc0833df93e0ed91dab3 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 31 Aug 2026 02:52:19 -0500 Subject: [PATCH] feat(runner): suspend safe ACPX sessions (#12422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Rust runner now owns exact ACPX request resolution and fail-closed session state > - A recoverable provider session needs an explicit suspension boundary before runnerd can safely hand work across process lifetimes > - Suspension is unsafe while a turn or provider request is active because remote effects may still be in flight > - A successful acknowledgement must preserve the exact immutable session identity > - This pull request adds only that guarded package-local lifecycle operation without selecting ACPX in runnerd > - The benefit is a small recovery primitive whose safety rules are independently reviewable ## Linked Issues or Issue Description Refs #12421 ## What Changed - Add a provider-state query for active pending tool, input, or permission requests. - Permit session suspension only when no turn or provider request is active. - Send a bounded `session.suspend` command with an operator-safe reason. - Require an affirmative suspension acknowledgement and the exact existing session identity. - Treat transport failure, rejection, omitted or malformed identity, and identity drift as fail-closed terminal errors. - Mark a successfully suspended session closed and terminate the local sidecar process while retaining cleanup ownership if termination must be retried. - Preserve a valid session after purely local unsafe-state rejection so it can settle before retry. - Extend the fake sidecar with deterministic suspension success, acknowledgement mismatch, identity mismatch, and missing-identity modes. - Add integration coverage for safe suspension, active-turn rejection, fail-closed remote mismatches, and retained cleanup. - Document the package-local suspension boundary. - Do not change dependencies, lockfiles, workflows, runnerd selection, server behavior, UI, or migrations. ## Verification - Replay base: `91d861ff69d415a3b105ae2eaad9cc56c66a9231` (`master` after #12421 merged). - Exact replay head: `085667e10c51c6c0360732f63c8fef83e806dd88`. - Stable patch ID: `b18d6b7efd1de569d3068b0a782f8aac2fbc9322`; this is the prepared suspension delta plus the focused fake-sidecar fix that consolidates mismatch modes into the existing command arm. - The exact delta is 5 files, 136 additions, and 5 deletions, all in `packages/paperclip-runner`; it contains no lockfile, workflow, server, UI, or migration change. - Exact-head GitHub Actions run `33369571343` (attempt 1): **PASSED** with 23 jobs passed and zero failures. - Greptile reviewed exact head `085667e10c51c6c0360732f63c8fef83e806dd88`: **5/5**, with zero unresolved review threads. - Superagent, contributor trust, Socket, and Snyk security checks: **PASSED**. - No local test result is claimed. GitHub Actions is the authoritative verification environment for this replayed revision. ## Risks - The sidecar may apply suspension before a transport failure is observed. The local session closes rather than retrying an ambiguous effect. - Local active-work rejection happens before transport and leaves the valid session open so the caller can settle it safely. - Identity equality is checked across provider, driver, session, thread, run, and company fields before accepting suspension. - No production path invokes suspension in this pull request. Runnerd execution and durable recovery wiring remain later slices. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - OpenAI Codex with GPT-5.6, agentic reasoning, tool use, and code execution. ## 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 linked existing issues with `Refs #` or described the issue in-PR - [x] I have not referenced internal or instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal Paperclip ticket id - [ ] 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 --- packages/paperclip-runner/README.md | 3 + .../runner-core/src/acpx_provider_session.rs | 44 +++++++++++ .../runner-core/src/acpx_provider_state.rs | 6 ++ .../runner-core/src/bin/fake-acpx-sidecar.rs | 14 ++-- .../tests/acpx_provider_suspend.rs | 74 +++++++++++++++++++ 5 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_suspend.rs diff --git a/packages/paperclip-runner/README.md b/packages/paperclip-runner/README.md index fedf71d0eb..447a4892ba 100644 --- a/packages/paperclip-runner/README.md +++ b/packages/paperclip-runner/README.md @@ -111,6 +111,9 @@ validate retained identity and schema, require the exact sidecar acknowledgement, and only then clear pending local state. Codex permission requests violate its pinned sidecar policy and terminate the session fail closed. +Safe suspension is available only with no active turn or pending request. The +sidecar must return the exact persistent session identity before runnerd +terminates the local process. Run the complete contract gate with: 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 34330e5149..486a7121ec 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 @@ -592,6 +592,50 @@ impl AcpxProviderSession { Ok(()) } + pub fn suspend( + &mut self, + reason: &str, + ) -> Result { + self.ensure_open()?; + if self.state.active_turn_id().is_some() || self.state.has_pending_requests() { + return Err(LocalRunnerError::invalid( + "ACPX provider session is not at a safe suspension point", + )); + } + let response = match self.transport.request( + GeneratedAcpxSidecarCommand::SessionSuspend, + json!({"reason":bounded_reason(reason)}), + ) { + Ok(response) => response, + Err(error) => return Err(self.fail_closed(error)), + }; + let identity = response + .get("identity") + .cloned() + .ok_or_else(|| LocalRunnerError::invalid("ACPX suspension omitted its identity")) + .and_then(|value| { + serde_json::from_value::(value).map_err(|error| { + LocalRunnerError::invalid(format!( + "ACPX suspension identity is invalid: {error}" + )) + }) + }); + let identity = match identity { + Ok(identity) => identity, + Err(error) => return Err(self.fail_closed(error)), + }; + if response.get("suspended").and_then(Value::as_bool) != Some(true) + || identity != self.identity + { + return Err(self.fail_closed(LocalRunnerError::invalid( + "ACPX sidecar did not confirm the exact suspended session", + ))); + } + self.closed = true; + self.terminate_transport()?; + Ok(identity) + } + pub fn shutdown(&mut self, reason: &str) -> Result<(), LocalRunnerError> { if self.closed { return self.terminate_transport(); diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_state.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_state.rs index 78b547f049..8ad2058d7f 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_state.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_provider_state.rs @@ -150,6 +150,12 @@ impl AcpxProviderState { .rotate_settled_turn_identities_after_provider_restart() } + pub fn has_pending_requests(&self) -> bool { + !self.pending_tools.is_empty() + || !self.pending_permissions.is_empty() + || !self.pending_inputs.is_empty() + } + pub fn begin_turn(&mut self, turn_id: impl Into) -> Result<(), LocalRunnerError> { if self.scope.active_turn_id().is_some() || !self.pending_tools.is_empty() 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 0d4feec4a4..f9e7632a32 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 @@ -110,7 +110,11 @@ fn run() -> Result<(), Box> { | "turns-permission" | "resolutions" | "resolutions-error-redaction" - | "resolutions-wrong-ack" => { + | "resolutions-wrong-ack" + | "suspend" + | "suspend-wrong-ack" + | "suspend-wrong-identity" + | "suspend-missing-identity" => { write_json(&mut stdout, &bootstrap_success(id, command, &request, mode))?; let params = request.get("params").unwrap_or(&Value::Null); let turn_id = params @@ -571,10 +575,10 @@ fn bootstrap_success(id: u64, command: &str, request: &Value, mode: &str) -> Val }), "turn.cancel" => json!({"cancelled":mode != "turns-wrong-cancel"}), "session.suspend" => json!({ - "suspended":true, - "identity": { + "suspended":mode != "suspend-wrong-ack", + "identity": if mode == "suspend-missing-identity" { Value::Null } else { json!({ "kind": "acpx", - "normalizedSessionId": "session-1", + "normalizedSessionId": if mode == "suspend-wrong-identity" { "another-session" } else { "session-1" }, "acpxRecordId": "record-1", "backendSessionId": "backend-1", "agentSessionId": "agent-1", @@ -583,7 +587,7 @@ fn bootstrap_success(id: u64, command: &str, request: &Value, mode: &str) -> Val "requestedModel": "gpt-5.6-sol", "effectiveModel": "gpt-5.6-sol", "permissionMode": "approve-reads", - }, + })}, }), "tool.resolve" => json!({ "resolved":if mode == "resolutions-error-redaction" { diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_suspend.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_suspend.rs new file mode 100644 index 0000000000..444ee6ddd9 --- /dev/null +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_provider_suspend.rs @@ -0,0 +1,74 @@ +use std::path::PathBuf; +use std::time::Duration; + +use paperclip_runner_core::acpx_provider_session::{ + AcpxPermissionMode, AcpxProviderSession, AcpxProviderSessionConfig, +}; +use paperclip_runner_core::acpx_sidecar_transport::AcpxSidecarTransportConfig; +use paperclip_runner_core::provider_bridge::{authorized_tool_catalog_digest, AuthorizedToolSet}; + +fn config(mode: &str) -> AcpxProviderSessionConfig { + let operations = Vec::new(); + AcpxProviderSessionConfig { + transport: AcpxSidecarTransportConfig { + command: PathBuf::from(env!("CARGO_BIN_EXE_fake-acpx-sidecar")), + args: vec!["--mode".to_owned(), mode.to_owned()], + request_timeout: Duration::from_secs(1), + shutdown_grace: Duration::from_millis(100), + }, + agent: "codex".to_owned(), + model: "gpt-5.6-sol".to_owned(), + run_id: "run-1".to_owned(), + catalog_revision: 1, + runtime_directory: std::env::temp_dir(), + normalized_session_id: "session-1".to_owned(), + working_directory: std::env::temp_dir(), + permission_mode: AcpxPermissionMode::ApproveReads, + permission_mode_pinned: true, + system_instructions: "Complete the supplied task.".to_owned(), + tool_set: AuthorizedToolSet { + schema: "paperclip.runner.authorized-tools.v1".to_owned(), + schema_version: 1, + catalog_digest: authorized_tool_catalog_digest(&operations).unwrap(), + operations, + }, + expected_identity: None, + } +} + +#[test] +fn suspends_only_after_the_exact_persistent_identity_is_confirmed() { + let mut session = AcpxProviderSession::start(&config("suspend")).unwrap(); + let expected = session.identity().clone(); + + assert_eq!(session.suspend("worker restart").unwrap(), expected); + assert!(session.shutdown("already suspended").is_ok()); +} + +#[test] +fn rejects_suspension_during_an_active_turn_without_closing_the_session() { + let mut session = AcpxProviderSession::start(&config("suspend")).unwrap(); + session + .start_turn("turn-1", "Please help", &std::env::temp_dir()) + .unwrap(); + + let error = session.suspend("too early").unwrap_err().to_string(); + assert!(error.contains("safe suspension point"), "{error}"); + assert_eq!(session.state().active_turn_id(), Some("turn-1")); + session.shutdown("test complete").unwrap(); +} + +#[test] +fn fails_closed_when_the_suspension_acknowledgement_does_not_match() { + for mode in ["suspend-wrong-ack", "suspend-wrong-identity"] { + let mut session = AcpxProviderSession::start(&config(mode)).unwrap(); + let error = session.suspend("worker restart").unwrap_err().to_string(); + assert!(error.contains("exact suspended session"), "{mode}: {error}"); + assert!(session.shutdown("already closed").is_ok()); + } + + let mut session = AcpxProviderSession::start(&config("suspend-missing-identity")).unwrap(); + let error = session.suspend("worker restart").unwrap_err().to_string(); + assert!(error.contains("identity is invalid"), "{error}"); + assert!(session.shutdown("already closed").is_ok()); +}