mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 12:33:05 +01:00
fix(runner): retain the cause of closed ACPX sessions (#18872)
## Thinking Path > - Paperclip helps people manage AI agents and their work. > - Runner records provider failures so the task can show their cause. > - An ACPX control request can fail and close its session before the event pump runs. > - The next poll then reports only that the session is closed. > - A second cleanup error can also drop a valid startup cause. > - This pull request retains each failure at its existing reporting boundary. > - Operators can inspect the cause while the failed session stays closed. ## Linked Issues or Issue Description Related: Refs #18622, #18847, #18854. Those changes preserve provider causes and handle proven startup failures. This change covers a control failure after provider execution starts. **What happened?** A rejected sidecar control closes the ACPX session. A later poll replaces its error with `ACPX provider session is closed`. The native terminal loses the control classification. Separately, cleanup can discard an existing validated startup cause. **Expected behavior** Retain the first control classification in the failed terminal. Do not invent a provider explanation for that code. Keep a validated startup cause when cleanup also fails. Keep the session closed, uncertain tool outcomes pending, and the existing recovery checks effective. **Steps to reproduce** 1. Start an ACPX turn and receive an authorized tool call. 2. Return its result after the provider has retired that call. The sidecar rejects `tool.resolve`. 3. Poll the native backend. Before this change, the terminal reports only the closed-session guard. 4. For the separate startup case, combine a rejected `session.open` cause with a cleanup error. Before this change, the combined error drops its validated provider cause. **Paperclip version or commit** The two subprocess regressions and the cleanup-composition regression fail against unchanged production code at `683fa4725e`. **Deployment mode** Native Runner with ACPX. The process regressions use the real Rust subprocess transport and durable native backend. Their controlled stale-tool rejection matches the production sidecar response shape. The separate cleanup unit injects a cleanup error at the existing error-composition boundary. ## What Changed - Retain the first failure in the ACPX session and return it from later closed-state checks. - Keep the validated provider cause when cleanup also fails. - Match the stale-tool fixture to production. It carries the closed code and no invented provider cause. - Cover a rejected tool result, normal shutdown, raw sidecar-text exclusion, pending tool preservation, refusal of another turn, and terminal replay after controller restart. - Document the existing failure and recovery boundary. ## Verification - Red: both process regressions fail on the base code. Polling replaces the named control failure with the generic closed-session error. The cleanup unit also fails because the combined error loses its existing startup cause. - Green: all 12 ACPX session tests, all 36 native backend tests, and all four session unit tests pass. The stale-tool cases require the classification and absence of an invented provider cause. - Commands: `cargo test --manifest-path packages/paperclip-runner/runner/Cargo.toml --locked -p paperclip-runner-core --test acpx_provider_session` and the same command with `--test native_provider_backend`. - Map structure and whitespace checks pass. - At head `981faa4`, the full Runner core Rust suite passes: 847 tests passed and three existing tests are ignored. Workspace typecheck and build pass. The existing TypeScript provider-failure and durable-settlement boundary suites pass all 30 tests. - An earlier concurrent local Rust attempt missed an existing Codex diagnostic deadline. That case passes alone on baseline and head, and the final full suite passes with the local socket support its tests require. - Complete current-head CI passes: 55 successful checks, five intentional skips and two neutral checks; no failed or pending checks. Both `ci / verify` and `ci / e2e` pass. - Failed CI attempts were investigated before rerunning affected jobs. Logs showed runner shutdowns/lost communication, an unchanged Windows WebSocket authentication deadline, and an unchanged Cursor Cloud Stop-fixture assertion. The Cursor Cloud case passes on baseline and head. No test assertion or CI guard was disabled. - The local full `pnpm test:run` invocation passed all six chat partitions, covering 1,838 expanded cases. It was stopped as a duplicate while still in general-server after complete sharded CI passed. A completed local full Vitest run is not claimed. - Greptile and Paperclip reviewed the corrected head at 5/5 with no actionable findings. There are no unresolved review threads. - The process test exercises control rejection, process cleanup, durable terminal persistence, and controller restart. The provider boundary is a deterministic sidecar fixture. It is not a live Claude or hosted recovery test. - A genuine provider refusal or uncertain external effect remains unresolved. This change preserves its evidence and does not make it safe to replay. ## Risks - The first control classification becomes visible where a generic closed-session error appeared before. Raw sidecar error text stays excluded. Startup details use the existing validated and redacted `ProviderFailure` contract. - A stored failure must not grant execution authority. The failed session still rejects calls and new turns. Normal shutdown has no fabricated failure cause. - No provider profile, digest, retry budget, process ownership rule, migration, credential, or permission change is required. ## Model Used OpenAI Codex, based on GPT-6, with source inspection, code editing, command execution and regression tests. The exact runtime snapshot and context-window capacity are not exposed in 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 and available 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 described the issue in this PR with the bug template fields - [x] I have not referenced internal or instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal ticket ID - [x] I have run the local Rust, typecheck, build and boundary checks; complete sharded CI passes and the partial local Vitest result is reported above - [x] I have added behavioral regression tests - [x] I have updated the relevant execution and run-log documentation - [x] I have considered and documented the risks - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open findings - [x] I will address all review findings before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
2518ac85ab
commit
c39e076638
6 files changed
+161
-4
No files matched your search
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -348,6 +348,7 @@ pub struct AcpxProviderSession {
|
||||
catalog_revision: u64,
|
||||
working_directory: PathBuf,
|
||||
closed: bool,
|
||||
failure: Option<LocalRunnerError>,
|
||||
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::*;
|
||||
|
||||
@@ -314,6 +314,7 @@ fn run() -> Result<(), Box<dyn std::error::Error>> {
|
||||
| "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<dyn std::error::Error>> {
|
||||
| "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<dyn std::error::Error>> {
|
||||
&& 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<dyn std::error::Error>> {
|
||||
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"},
|
||||
}),
|
||||
)?;
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Reference in new issue
Block a user