From cc42a67e7e9e8eb183097afc8ff4ebfa694fb3e0 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Sat, 22 Aug 2026 11:12:51 -0700 Subject: [PATCH] fix(adapter-utils): extend the duplex fail-closed run disposition to the CLI lane (#11966) 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 > - Paperclip runs agents through adapter execution lanes > - Duplex adapters can lose their control channel before a process completes > - The ACP lane already fails closed, but the CLI lane can report false success > - This pull request applies the same completion rule to the CLI lane and shares the loss code > - The benefit is consistent failure reporting when a duplex channel closes during a run ## Linked Issues or Issue Description **What happened?** A CLI-lane duplex run can lose its control channel before clean process completion. The run can then report `succeeded` with exit code 0 and no error code. **Expected behavior** The execution target must fail closed when the channel dies before clean completion. It must return exit code 1, the typed `duplex_channel_lost` error code, and a short stderr note. **Steps to reproduce** 1. Start a duplex adapter run through the CLI execution lane. 2. Close the duplex control channel before the process completes cleanly. 3. Inspect the run result and error code. **Paperclip version or commit** Commit `5e01523d4eb6df4a20a0bddd05374c9c42225203`. **Deployment mode** Built from source. **Installation method** Built from source with pnpm. **Agent adapter(s) involved** Claude Code, Codex, Cursor, Gemini, Kimi, OpenCode, and Pi local adapters. **Database mode** Not database-related. ## What Changed - Add an optional `errorCode` field to `RunProcessResult`. - Add a one-read completion seam to the execution target process options. - Fail closed when a duplex channel dies before clean process completion. - Add `settleRunDisposition()` to atomically read and mark orderly completion. - Share the typed duplex loss error code across the ACP and CLI lanes. - Mark non-success terminal results as orderly completion before teardown. - Wire the seam through the seven duplex adapters. - Add regression tests for channel loss, clean completion, and non-clean terminal results. ## Verification - `npx vitest run packages/adapter-utils/src/execution-target-sandbox.test.ts` — 118 passed. - `npx vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts -t "sandbox duplex run-disposition seam"` — 4 passed. - The author confirmed a clean type-check for `@paperclipai/adapter-utils` and the seven duplex adapter packages. - Pre-existing environment failures remain outside this change. They include `EACCES mkdir '/srv/paperclip'` and remote file-size setup failures. ## Risks The change alters terminal status for CLI duplex runs that lose control before clean completion. The typed error code and stderr note keep the failure visible. The broker marks failed, cancelled, and timed-out results as orderly completion to prevent false loss events during teardown. ## Model Used OpenAI Codex, GPT-5, tool use and code execution, with the standard GPT-5 context window. The model assisted with the implementation and test work. ## 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 --- .../src/acpx-engine/execute.test.ts | 63 ++++- .../adapter-utils/src/acpx-engine/execute.ts | 20 +- .../adapter-utils/src/duplex-bridge-broker.ts | 25 ++ .../src/execution-target-sandbox.test.ts | 215 +++++++++++++++++- .../adapter-utils/src/execution-target.ts | 64 +++++- packages/adapter-utils/src/server-utils.ts | 7 + .../src/server/execute.remote.test.ts | 67 +++++- .../claude-local/src/server/execute.ts | 16 +- .../codex-local/src/server/execute.ts | 10 +- .../cursor-local/src/server/execute.ts | 6 + .../gemini-local/src/server/execute.ts | 9 +- .../adapters/kimi-local/src/server/execute.ts | 9 +- .../opencode-local/src/server/execute.ts | 7 +- .../adapters/pi-local/src/server/execute.ts | 7 +- 14 files changed, 503 insertions(+), 22 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index b3bcd72acd..6f5ae7dff6 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -5770,6 +5770,7 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => }); broker.start(); const markOrderlyCompletion = vi.fn(() => broker.markOrderlyCompletion()); + const settleRunDisposition = vi.fn(() => broker.settleRunDisposition()); const stop = vi.fn(async () => {}); const handle = { env: { @@ -5778,10 +5779,11 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => PAPERCLIP_API_BRIDGE_MODE: "duplex_v1", }, readRunDisposition: () => broker.runDisposition, + settleRunDisposition, markOrderlyCompletion, stop, }; - return { broker, handle, markOrderlyCompletion }; + return { broker, handle, markOrderlyCompletion, settleRunDisposition }; } // A runtime whose one turn completes cleanly. The `beforeResult` hook runs at @@ -5809,6 +5811,31 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => }; } + // A runtime whose one turn fails. The `beforeResult` hook runs at the exact + // point the ACP terminal resolves, so the test orders channel activity before + // the failed finalization when it needs to. + function runtimeWithFailedResult(beforeResult?: () => void) { + return { + ensureSession: async () => ({ + backendSessionId: "backend-session", + agentSessionId: "agent-session", + runtimeSessionName: "runtime-session", + }), + startTurn: () => ({ + events: (async function* () { + yield { type: "done", stopReason: "end_turn" }; + })(), + result: (async () => { + beforeResult?.(); + return { status: "failed" as const, error: new Error("agent failed") }; + })(), + cancel: async () => {}, + }), + setConfigOption: async () => {}, + close: async () => {}, + }; + } + async function setupRemoteSandbox() { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); @@ -5864,7 +5891,7 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => it("fails a completed run when the duplex channel was lost before the completion", async () => { const sandbox = await setupRemoteSandbox(); const fake = createFakeDuplexChannel(); - const { handle, markOrderlyCompletion } = bridgeOverBroker(fake); + const { broker, handle, settleRunDisposition } = bridgeOverBroker(fake); // Latch the loss before the ACP terminal resolves. const runtime = runtimeWithControlledResult(() => fake.emitExit({ exitCode: 1 })); @@ -5876,14 +5903,16 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => // The message carries only the typed loss reason, not raw provider text. expect(result.errorMessage).toContain("provider_exit"); expect(result.resultJson).toMatchObject({ status: "failed" }); - // The seam did not mark an orderly completion for a lost channel. - expect(markOrderlyCompletion).not.toHaveBeenCalled(); + // The seam read the disposition through the atomic settle step, and the + // latched loss kept the failure, so no orderly completion ordered. + expect(settleRunDisposition).toHaveBeenCalledTimes(1); + expect(broker.runDisposition.failed).toBe(true); }); it("keeps a completed run a success when the channel stays live, and a later teardown loss is benign", async () => { const sandbox = await setupRemoteSandbox(); const fake = createFakeDuplexChannel(); - const { broker, handle, markOrderlyCompletion } = bridgeOverBroker(fake); + const { broker, handle, settleRunDisposition } = bridgeOverBroker(fake); // No loss before the completion. const runtime = runtimeWithControlledResult(); @@ -5891,8 +5920,9 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => expect(result.exitCode).toBe(0); expect(result.errorCode ?? null).toBeNull(); - // The seam marked the orderly completion for the success-eligible terminal. - expect(markOrderlyCompletion).toHaveBeenCalledTimes(1); + // The atomic settle step marked the orderly completion for the + // success-eligible terminal. + expect(settleRunDisposition).toHaveBeenCalledTimes(1); // A teardown loss ordered after the orderly completion is a normal teardown, // so the run disposition stays a success. fake.emitExit({ exitCode: 0 }); @@ -5917,4 +5947,23 @@ describe("ACPX engine sandbox duplex run-disposition seam (fail-closed)", () => expect(broker.runDisposition.failed).toBe(true); expect(broker.runDisposition.lossReason).toBe("provider_exit"); }); + + it("marks an orderly completion on a failed terminal so the teardown loss emits no false loss", async () => { + const sandbox = await setupRemoteSandbox(); + const fake = createFakeDuplexChannel(); + const { broker, handle, markOrderlyCompletion } = bridgeOverBroker(fake); + // The turn fails, and no channel loss ordered before the finalization. + const runtime = runtimeWithFailedResult(); + + const result = await runRemote(handle, runtime, sandbox); + + // The failed terminal stays a failure, but not a duplex loss. + expect(result.exitCode).not.toBe(0); + expect(result.errorCode).not.toBe("duplex_channel_lost"); + // The non-success-eligible terminal marked the orderly completion, so the + // teardown channel_exit orders after the mark and does not latch a loss. + expect(markOrderlyCompletion).toHaveBeenCalledTimes(1); + fake.emitExit({ exitCode: 0 }); + expect(broker.runDisposition.failed).toBe(false); + }); }); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index 08ec83295b..45a73f34b9 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -34,6 +34,7 @@ import { type SandboxAdditionalSource, } from "@paperclipai/adapter-utils/execution-target"; import type { DuplexLossReason } from "../duplex-telemetry.js"; +import { DUPLEX_CHANNEL_LOST_ERROR_CODE } from "../duplex-bridge-broker.js"; import { DEFAULT_PAPERCLIP_AGENT_PROMPT_TEMPLATE, applyPaperclipWorkspaceEnv, @@ -4019,12 +4020,23 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { // never sets these methods, so the optional calls no-op there. let duplexLossReason: DuplexLossReason | null = null; if (terminal.status === "completed" && !timedOut) { - const disposition = prepared.paperclipBridge?.readRunDisposition?.() ?? null; + // Success-eligible terminal. Atomically read the disposition and mark + // the orderly completion in one broker step. No `await` separates the + // read from the mark, so a teardown loss cannot slip in between them. A + // latched loss fails the run closed; a healthy channel marks its + // orderly completion, so a later teardown loss stays a normal teardown. + const disposition = prepared.paperclipBridge?.settleRunDisposition?.() ?? null; if (disposition?.failed) { duplexLossReason = disposition.lossReason ?? "other"; - } else { - prepared.paperclipBridge?.markOrderlyCompletion?.(); } + } else { + // Non-success-eligible terminal (failed, cancelled, or timed out). A + // deliberate host teardown follows, so mark the orderly completion now. + // This stops the teardown `channel_exit` from latching `lossSeq`, from + // emitting a false loss event, and from incrementing the loss counters. + // The mark no-ops once a loss latched, so a real mid-run loss still + // fails the run. + prepared.paperclipBridge?.markOrderlyCompletion?.(); } // A terminal that reports "completed" but whose duplex control channel // died before the completion is not a success. The seam fails it closed. @@ -4098,7 +4110,7 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { : timedOut ? "acpx_timeout" : channelLost - ? "duplex_channel_lost" + ? DUPLEX_CHANNEL_LOST_ERROR_CODE : null, sessionId: sessionHandle.backendSessionId ?? sessionHandle.runtimeSessionName, sessionParams: buildSessionParams({ prepared, handle: sessionHandle }), diff --git a/packages/adapter-utils/src/duplex-bridge-broker.ts b/packages/adapter-utils/src/duplex-bridge-broker.ts index dbd1734d6b..2cba409766 100644 --- a/packages/adapter-utils/src/duplex-bridge-broker.ts +++ b/packages/adapter-utils/src/duplex-bridge-broker.ts @@ -96,6 +96,13 @@ export function typedDuplexLossReason(reason: DuplexBrokerLossReason): DuplexLos return BROKER_LOSS_REASON_TO_TYPED[reason] ?? "other"; } +/** + * The typed error code the host reports when the duplex control channel died + * before an orderly completion. Both the ACP lane and the CLI lane report this + * one code, so the run disposition is identical across the two lanes. + */ +export const DUPLEX_CHANNEL_LOST_ERROR_CODE = "duplex_channel_lost"; + /** * The terminal run disposition the broker computes from its ordered lifecycle. A * `failed` disposition means a terminal loss ordered before an orderly completion, @@ -259,6 +266,15 @@ export interface DuplexBridgeBroker { * and on a host-initiated orderly close. Safe to call more than one time. */ markOrderlyCompletion(): void; + /** + * Atomically read the run disposition and mark the host-observed orderly + * completion in one synchronous step. The host calls it at the run-disposition + * seam for a success-eligible terminal. The broker marks the orderly completion + * only while no loss ordered, then returns the disposition, so no caller can + * insert an `await` between the read and the mark. A loss that already latched + * keeps the failure, because the mark no-ops after a latched loss. + */ + settleRunDisposition(): DuplexBrokerRunDisposition; /** Start the broker. It wires the channel listeners and moves to `open`. */ start(): void; /** Close the channel cleanly. It moves through `closing` to `closed`. */ @@ -396,6 +412,14 @@ export function createDuplexBridgeBroker(options: DuplexBrokerOptions): DuplexBr if (orderlyCompletionSeq !== null || lossSeq !== null) return; orderlyCompletionSeq = nextLifecycleSeq(); }; + // Atomically read the run disposition and mark the host-observed orderly + // completion. The mark and the read run in one synchronous step, so no caller + // can insert an `await` between them and no teardown loss can slip in. The mark + // no-ops once a loss latched, so a real mid-run loss keeps the failure. + const settleRunDisposition = (): DuplexBrokerRunDisposition => { + markOrderlyCompletion(); + return { failed: lossSeq !== null, lossReason: typedLossReason }; + }; // The ids the broker already dispatched. The broker forwards one id one time, // so a repeated frame never reaches the API twice. const seenRequestIds = new Set(); @@ -873,6 +897,7 @@ export function createDuplexBridgeBroker(options: DuplexBrokerOptions): DuplexBr return { failed: lossSeq !== null, lossReason: typedLossReason }; }, markOrderlyCompletion, + settleRunDisposition, start, close, stop, diff --git a/packages/adapter-utils/src/execution-target-sandbox.test.ts b/packages/adapter-utils/src/execution-target-sandbox.test.ts index 3f23cc3871..0fd7cb426e 100644 --- a/packages/adapter-utils/src/execution-target-sandbox.test.ts +++ b/packages/adapter-utils/src/execution-target-sandbox.test.ts @@ -43,7 +43,7 @@ import { type StartupTraceContext, type StartupTracer, } from "./acpx-engine/startup-timing.js"; -import { createSandboxRunLogTailFactory } from "./sandbox-run-log-stream.js"; +import { createSandboxRunLogTailFactory, type SandboxRunLogTailFactory } from "./sandbox-run-log-stream.js"; import { runChildProcess } from "./server-utils.js"; import { shellQuote } from "./ssh.js"; import type { CommandManagedDuplexChannel } from "./command-managed-runtime.js"; @@ -58,6 +58,7 @@ import { import { assertNestedDuplexBrokerBudgets, createDuplexBridgeBroker, + DUPLEX_CHANNEL_LOST_ERROR_CODE, type DuplexBrokerForwardResult, type DuplexBrokerLossRecord, type DuplexBrokerRequestRecord, @@ -5865,3 +5866,215 @@ describe("sandbox target spec parse: enableSandboxDuplexBridge", () => { expect(adapterExecutionTargetEnablesSandboxDuplexBridge(null)).toBe(false); }); }); + +describe("settleRunDisposition atomic read and mark", () => { + it("marks the orderly completion and reports a success for a healthy channel", async () => { + const fake = createFakeDuplexChannel(); + const broker = createDuplexBridgeBroker({ + channel: fake.channel, + forwardRequest: async () => ({ status: 200 }), + }); + broker.start(); + + // The one atomic step marks the orderly completion and reads the success. + expect(broker.settleRunDisposition()).toEqual({ failed: false, lossReason: null }); + // A later teardown loss orders after the mark, so it stays a normal teardown. + fake.emitExit({ exitCode: 0 }); + await flushMacrotasks(); + expect(broker.runDisposition).toEqual({ failed: false, lossReason: null }); + }); + + it("reports the failure and does not mark for a latched loss", async () => { + const fake = createFakeDuplexChannel(); + const broker = createDuplexBridgeBroker({ + channel: fake.channel, + forwardRequest: async () => ({ status: 200 }), + }); + broker.start(); + + // A loss ordered before any orderly completion latches the failure. + fake.emitExit({ exitCode: 1 }); + await flushMacrotasks(); + // The atomic step reads the failure and no-ops the mark, so a later + // completion cannot clear the latch. + expect(broker.settleRunDisposition()).toEqual({ failed: true, lossReason: "provider_exit" }); + broker.markOrderlyCompletion(); + expect(broker.runDisposition).toEqual({ failed: true, lossReason: "provider_exit" }); + }); +}); + +describe("CLI-lane run-disposition seam", () => { + const CLEAN_RESULT = { + exitCode: 0, + signal: null, + timedOut: false, + stdout: "ok\n", + stderr: "", + pid: null, + startedAt: "2026-08-22T00:00:00.000Z", + } as const; + + function mockRunner(result: Record) { + return { execute: vi.fn(async () => result) }; + } + + function sandboxTarget(runner: unknown): AdapterSandboxExecutionTarget { + return { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: "/workspace", + timeoutMs: 30_000, + runner, + } as AdapterSandboxExecutionTarget; + } + + function startBroker(fake: ReturnType) { + const broker = createDuplexBridgeBroker({ + channel: fake.channel, + forwardRequest: async () => ({ status: 200 }), + }); + broker.start(); + return broker; + } + + it("fails a clean CLI completion closed when the duplex channel was lost mid-turn", async () => { + const fake = createFakeDuplexChannel(); + const broker = startBroker(fake); + // The control channel dies mid-turn, before the CLI process exits. + fake.emitExit({ exitCode: 1 }); + await flushMacrotasks(); + + const runner = mockRunner({ ...CLEAN_RESULT }); + const result = await runAdapterExecutionTargetProcess("run-cli-lost", sandboxTarget(runner), "agent-cli", [], { + cwd: "/local", + env: {}, + timeoutSec: 5, + graceSec: 1, + onLog: async () => {}, + settleRunDisposition: () => broker.settleRunDisposition(), + }); + + // The lost channel overrides the clean exit to a failure with the typed code. + expect(result.exitCode).toBe(1); + expect(result.errorCode).toBe(DUPLEX_CHANNEL_LOST_ERROR_CODE); + // The note names only the typed loss reason, not raw provider text. + expect(result.stderr).toContain("provider_exit"); + }); + + it("keeps a clean CLI completion a success when the channel stays healthy, and a teardown loss stays benign", async () => { + const fake = createFakeDuplexChannel(); + const broker = startBroker(fake); + + const runner = mockRunner({ ...CLEAN_RESULT }); + const result = await runAdapterExecutionTargetProcess("run-cli-ok", sandboxTarget(runner), "agent-cli", [], { + cwd: "/local", + env: {}, + timeoutSec: 5, + graceSec: 1, + onLog: async () => {}, + settleRunDisposition: () => broker.settleRunDisposition(), + }); + + expect(result.exitCode).toBe(0); + expect(result.errorCode ?? null).toBeNull(); + // The seam's atomic settle marked the orderly completion at agent + // completion. A teardown loss ordered after it is a normal teardown, so the + // run stays a success without any manual mark here. + fake.emitExit({ exitCode: 0 }); + await flushMacrotasks(); + expect(broker.runDisposition.failed).toBe(false); + }); + + it("keeps a clean CLI completion a success when the gateway exits during the run-log tail finish", async () => { + const fake = createFakeDuplexChannel(); + const broker = startBroker(fake); + + // A run-log tail whose finish emits a gateway exit. This reproduces the + // race where the duplex gateway dies after the clean process completion but + // before the host reads the disposition. The seam settles the disposition + // synchronously before this finish await, so the mark orders first and the + // teardown exit stays benign. + const runLogTail: SandboxRunLogTailFactory = { + create: () => ({ + wrapCommand: (command, args) => ({ command, args }), + start: () => {}, + finish: async () => { + fake.emitExit({ exitCode: 1 }); + await flushMacrotasks(); + }, + abort: async () => {}, + }), + }; + + const runner = mockRunner({ ...CLEAN_RESULT }); + const result = await runAdapterExecutionTargetProcess("run-cli-race", sandboxTarget(runner), "agent-cli", [], { + cwd: "/local", + env: {}, + timeoutSec: 5, + graceSec: 1, + onLog: async () => {}, + runLogTail, + settleRunDisposition: () => broker.settleRunDisposition(), + }); + + // The atomic settle at the completion boundary marked the orderly + // completion before the finish await, so the gateway exit never latches a + // false loss and the run stays a clean success. + expect(result.exitCode).toBe(0); + expect(result.errorCode ?? null).toBeNull(); + expect(broker.runDisposition.failed).toBe(false); + }); + + it("cannot clear the loss latch with a later completion", async () => { + const fake = createFakeDuplexChannel(); + const broker = startBroker(fake); + // The loss latches before the CLI process exits. + fake.emitExit({ exitCode: 1 }); + await flushMacrotasks(); + // A later orderly completion cannot clear the latch. + broker.markOrderlyCompletion(); + + const runner = mockRunner({ ...CLEAN_RESULT }); + const result = await runAdapterExecutionTargetProcess("run-cli-latch", sandboxTarget(runner), "agent-cli", [], { + cwd: "/local", + env: {}, + timeoutSec: 5, + graceSec: 1, + onLog: async () => {}, + settleRunDisposition: () => broker.settleRunDisposition(), + }); + + expect(result.exitCode).toBe(1); + expect(result.errorCode).toBe(DUPLEX_CHANNEL_LOST_ERROR_CODE); + }); + + it("leaves an already-failed CLI result unchanged and never settles the disposition", async () => { + const fake = createFakeDuplexChannel(); + const broker = startBroker(fake); + // The control channel is lost, but the process itself also exited non-zero. + fake.emitExit({ exitCode: 1 }); + await flushMacrotasks(); + + let settleCalls = 0; + const runner = mockRunner({ ...CLEAN_RESULT, exitCode: 2, stderr: "boom\n" }); + const result = await runAdapterExecutionTargetProcess("run-cli-failed", sandboxTarget(runner), "agent-cli", [], { + cwd: "/local", + env: {}, + timeoutSec: 5, + graceSec: 1, + onLog: async () => {}, + settleRunDisposition: () => { + settleCalls += 1; + return broker.settleRunDisposition(); + }, + }); + + // A non-zero exit is already a failure, so the seam leaves it unchanged and + // reports no transport-level code. This is the same success-eligibility rule + // the ACP lane applies. + expect(result.exitCode).toBe(2); + expect(result.errorCode ?? null).toBeNull(); + expect(settleCalls).toBe(0); + }); +}); diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 300053ccea..faece1d9bf 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -45,6 +45,7 @@ import { import { createDuplexBridgeBroker, DEFAULT_DUPLEX_BROKER_BUDGETS, + DUPLEX_CHANNEL_LOST_ERROR_CODE, isSafeBridgeMethod, typedDuplexLossReason, type DuplexBridgeBroker, @@ -235,6 +236,18 @@ export interface AdapterExecutionTargetProcessOptions { * onLog is suppressed and incremental chunks flow through `onLog` instead. */ runLogTail?: SandboxRunLogTailFactory | null; + /** + * Sandbox-only: the atomic run-disposition settle from the Paperclip bridge + * handle. When provided, `runAdapterExecutionTargetProcess` calls it once at + * the clean-completion boundary of the process, synchronously and before the + * run-log tail finishes. The call reads the disposition and marks the + * host-observed orderly completion in one broker step, so a gateway exit + * after the clean process completion cannot latch a false mid-run loss. A + * control channel that died before the clean completion still fails the run + * closed with the typed `duplex_channel_lost` code. The file bridge path + * never sets it. + */ + settleRunDisposition?: (() => DuplexBrokerRunDisposition) | null; localProcessSandbox?: LocalProcessSandboxOptions | null; } @@ -263,6 +276,15 @@ export interface AdapterExecutionTargetPaperclipBridgeHandle { * whose control channel died mid-turn. */ readRunDisposition?(): DuplexBrokerRunDisposition; + /** + * Atomically read the run disposition and mark the host-observed orderly + * completion in one broker step. The ACP lane calls it at the terminal + * finalization boundary for a success-eligible completion, so no `await` can + * separate the read from the mark and a teardown loss cannot slip in between. + * A loss that already latched keeps the failure, because the broker no-ops the + * mark after a latched loss. The file bridge path never sets it. + */ + settleRunDisposition?(): DuplexBrokerRunDisposition; /** * Mark the host-observed orderly completion of the agent turn on the broker's * ordered lifecycle. The caller marks it at the ACP terminal-finalization @@ -720,6 +742,35 @@ export async function resolveAdapterExecutionTargetCommandForLogs( }); } +// Apply the run-disposition seam to one clean process result. Only a clean +// completion is success-eligible: a timed-out, signalled, or non-zero-exit +// result is already a failure, so the seam leaves it unchanged. This is the same +// success-eligibility rule the ACP lane applies. For a success-eligible result +// the seam settles the disposition in one atomic broker step: it reads the +// disposition and marks the host-observed orderly completion together, so a +// gateway exit after the clean completion cannot latch a false mid-run loss. A +// duplex control channel that died before the clean completion fails the run +// closed: the seam sets a non-zero exit code, the typed `duplex_channel_lost` +// error code, and a stderr note that names only the typed loss reason. +function applyRunDispositionSeam( + result: RunProcessResult, + settleRunDisposition: (() => DuplexBrokerRunDisposition) | null | undefined, +): RunProcessResult { + const successEligible = result.exitCode === 0 && !result.timedOut && result.signal === null; + if (!successEligible || !settleRunDisposition) return result; + const disposition = settleRunDisposition(); + if (!disposition.failed) return result; + const lossReason = disposition.lossReason ?? "other"; + const note = `[paperclip] The sandbox duplex control channel was lost (${lossReason}) before the run completed.\n`; + const separator = result.stderr.length > 0 && !result.stderr.endsWith("\n") ? "\n" : ""; + return { + ...result, + exitCode: 1, + errorCode: DUPLEX_CHANNEL_LOST_ERROR_CODE, + stderr: `${result.stderr}${separator}${note}`, + }; +} + export async function runAdapterExecutionTargetProcess( runId: string, target: AdapterExecutionTarget | null | undefined, @@ -756,10 +807,17 @@ export async function runAdapterExecutionTargetProcess( ? async (meta) => options.onSpawn?.({ ...meta, processGroupId: null }) : undefined, }); + // Settle the duplex run disposition synchronously at the clean-completion + // boundary, before the run-log tail finishes. The atomic settle marks the + // host-observed orderly completion in one broker step, so a gateway exit + // after the clean process completion cannot latch a false mid-run loss. A + // control channel that died before this clean completion still fails the + // run closed. + const settled = applyRunDispositionSeam(result, options.settleRunDisposition); if (runLogTail) { await runLogTail.finish({ stdout: result.stdout, stderr: result.stderr }); } - return result; + return settled; } catch (error) { if (runLogTail) { await runLogTail.abort(); @@ -3144,6 +3202,10 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { // seam. A loss ordered before an orderly completion reports a failure // with the typed loss reason; every other state reports a success. readRunDisposition: (): DuplexBrokerRunDisposition => activeBroker.runDisposition, + // Atomically read the latch and mark the orderly completion for the + // ACP success-eligible terminal, so no await separates the read from + // the mark and a teardown loss cannot slip in between. + settleRunDisposition: (): DuplexBrokerRunDisposition => activeBroker.settleRunDisposition(), // Surface the broker's orderly-completion mark to the run-disposition // seam. The seam marks the completion for a success-eligible terminal, // so a teardown loss after the completion stays a normal teardown. diff --git a/packages/adapter-utils/src/server-utils.ts b/packages/adapter-utils/src/server-utils.ts index 23a73e09b9..c9f291387e 100644 --- a/packages/adapter-utils/src/server-utils.ts +++ b/packages/adapter-utils/src/server-utils.ts @@ -29,6 +29,13 @@ export interface RunProcessResult { // The sandbox runner sets them, so the exec span records a true wall time. finishedAt?: string | null; durationMs?: number | null; + // The typed error code of a transport-level failure, or absent when the + // process result carries no such code. It follows the same additive-optional + // convention as the timing fields: a producer that names no code leaves it + // absent, so the existing `RunProcessResult` producers stay unchanged. The + // run-disposition seam sets it to `duplex_channel_lost` when the sandbox + // duplex control channel died before a clean completion. + errorCode?: string | null; terminalResultCleanup?: TerminalResultCleanupEvidence | null; } diff --git a/packages/adapters/claude-local/src/server/execute.remote.test.ts b/packages/adapters/claude-local/src/server/execute.remote.test.ts index ab27ab03f6..c0cd542205 100644 --- a/packages/adapters/claude-local/src/server/execute.remote.test.ts +++ b/packages/adapters/claude-local/src/server/execute.remote.test.ts @@ -2,6 +2,7 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import type { RunProcessResult } from "@paperclipai/adapter-utils/server-utils"; const { runChildProcess, @@ -12,7 +13,7 @@ const { syncDirectoryToSsh, startAdapterExecutionTargetPaperclipBridge, } = vi.hoisted(() => ({ - runChildProcess: vi.fn(async () => ({ + runChildProcess: vi.fn(async (): Promise => ({ exitCode: 0, signal: null, timedOut: false, @@ -338,4 +339,68 @@ describe("claude remote execution", () => { expect(call?.[2]).toContain("12345678-1234-4abc-9def-123456789012"); }); + it("forwards the duplex_channel_lost transport code on the unparsed Claude result path", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-claude-remote-duplex-")); + cleanupDirs.push(rootDir); + const workspaceDir = path.join(rootDir, "workspace"); + await mkdir(workspaceDir, { recursive: true }); + + // The run-disposition seam sets `errorCode: "duplex_channel_lost"` on the + // process result, and the CLI stdout has no parsed Claude result. This + // drives `toAdapterResult` into the unparsed branch, which must forward the + // transport code rather than drop it to a provider classification. + runChildProcess.mockResolvedValueOnce({ + exitCode: 1, + signal: null, + timedOut: false, + stdout: "not a Claude JSON result\n", + stderr: + "[paperclip] The sandbox duplex control channel was lost (provider_exit) before the run completed.\n", + pid: 123, + startedAt: new Date().toISOString(), + errorCode: "duplex_channel_lost", + }); + + const result = await execute({ + runId: "run-ssh-duplex-lost", + agent: { + id: "agent-1", + companyId: "company-1", + name: "Claude Coder", + adapterType: "claude_local", + adapterConfig: {}, + }, + runtime: { + sessionId: null, + sessionParams: null, + sessionDisplayId: null, + taskKey: null, + }, + config: { + command: "claude", + }, + context: { + paperclipWorkspace: { + cwd: workspaceDir, + source: "project_primary", + }, + }, + executionTransport: { + remoteExecution: { + host: "127.0.0.1", + port: 2222, + username: "fixture", + remoteWorkspacePath: "/remote/workspace", + remoteCwd: "/remote/workspace", + privateKey: "PRIVATE KEY", + knownHosts: "[127.0.0.1]:2222 ssh-ed25519 AAAA", + strictHostKeyChecking: true, + }, + }, + onLog: async () => {}, + }); + + expect(result.errorCode).toBe("duplex_channel_lost"); + }); + }); diff --git a/packages/adapters/claude-local/src/server/execute.ts b/packages/adapters/claude-local/src/server/execute.ts index 6e927cff21..f8532e8a50 100644 --- a/packages/adapters/claude-local/src/server/execute.ts +++ b/packages/adapters/claude-local/src/server/execute.ts @@ -931,6 +931,7 @@ export async function execute(ctx: AdapterExecutionContext): Promise parseClaudeStreamJson(stdout).resultJson !== null, @@ -1005,7 +1006,13 @@ export async function execute(ctx: AdapterExecutionContext): Promise; monitor?: @@ -1478,7 +1479,12 @@ export async function execute(ctx: AdapterExecutionContext): Promise; }, @@ -715,6 +717,10 @@ export async function execute(ctx: AdapterExecutionContext): Promise; }, @@ -713,7 +715,12 @@ export async function execute(ctx: AdapterExecutionContext): Promise; }, @@ -681,7 +683,12 @@ export async function execute(ctx: AdapterExecutionContext): Promise; }, @@ -681,6 +682,10 @@ export async function execute(ctx: AdapterExecutionContext): Promise; }, @@ -777,6 +778,10 @@ export async function execute(ctx: AdapterExecutionContext): Promise