Files
PaperClipAI/packages/adapter-utils/src/provider-stopped-boundary.test.ts
T
DottaandPaperclip cbc5132e6c fix(adapters): expose a verified provider stop before workspace restoration (#14311)
## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - Agent adapters own local or remote provider processes.
> - Run cleanup must know when the final provider process has stopped.
> - A remote timeout or a lost transport does not prove that the
provider stopped.
> - Retried provider invocations also make an earlier stop signal stale.
> - This pull request adds a verified final-invocation stop callback
before workspace restoration.
> - The dependent instruction revision change uses that boundary to
preserve private instruction edits safely.

## Linked Issues or Issue Description

**What happened?**

Adapter completion did not expose a reliable point between provider
shutdown and workspace restoration. Cleanup could lose provider-written
files, or treat a remote timeout as proof that a process stopped.

**Expected behavior**

Cleanup runs once after the final provider invocation has a verified
stop receipt and before workspace restoration. An incomplete remote
command keeps collection pending.

**Steps to reproduce**

1. Run a remote provider that returns a timeout without a numeric exit
code.
2. Let the adapter return or retry the provider.
3. Attempt to collect provider-written files during cleanup. The adapter
has no verified final-process boundary to use.

This is the prerequisite for the stacked canonical instruction revision
pull request. It has no database or UI dependency. Related #13291
verifies remote termination for later recovery; this change exposes the
earlier adapter-owned stop boundary before workspace restoration. The
scopes do not duplicate each other.

## What Changed

- Add a stop callback to adapter execution context and a
final-invocation fence.
- Confirm local child closure and complete remote exit receipts. Reject
remote timeouts, missing exits, transport failures, and SSH exit 255 as
stop proof.
- Invoke collection once before workspace restoration in eight CLI
adapters and at the confirmed ACP stop boundary.
- Preserve the stop observation when a later log flush fails.
- Run bridge and workspace cleanup in `finally` even when collection
rejects. ACP records a safe error without exposing a raw filesystem
path. Grok keeps collection errors separate from workspace restore
failures and preserves completed provider results when both cleanup
steps fail.
- Correct the existing Cursor test shell fixture so bounded remote file
reads run against real fixture files.

## Verification

- Three stop-boundary regressions failed before the callback
implementation and passed after it.
- Independent prerequisite branch: 377 tests passed across 24 adapter,
process-target, and ACP suites. Two added collector-rejection tests
failed before the cleanup fix and passed after it.
- A third regression reproduced Grok misclassifying a collection failure
as failed workspace restoration. Two additional cases covered completed
and failed provider turns when collection and restore both fail. The
Grok and restore-classifier suites passed 48 tests.
- All nine affected package typechecks and affected package builds
passed; Grok checks passed again after its classification fix.
- Integrated instruction branch: native local, legacy local, and native
Daytona each passed three browser tasks with exact persisted bytes,
fresh-task readback, history/restore, and explicit conflict resolution.
Unchanged warm Daytona passed three turns. Legacy Codex passed all five
checks again after the exception-safe cleanup fix.
- Alternate staging passed the same three-task native Daytona flow:
exact stopped-run save, independent downloaded readback, browser
history/restore, and explicit resolution of a real concurrent edit. The
deployed source was `14c3d810c9e05625121b3d27767aea9317b03125`, which
covers the initial adapter callback. Later Grok cleanup failures are
qualified by the adapter tests above.

- The final combined native Daytona flow passed again on deployed
`2bedd0f23bf4698b1f8b818f6796900647030427`: three fresh tasks proved
ordinary instruction edits, independent readback, History/Restore,
concurrent board conflict, and explicit candidate resolution. This
native staging flow does not claim to exercise the Grok adapter.

## Risks

- An unverified remote stop intentionally does not trigger collection. A
later controller with verified stop evidence must recover it or report
the copy unavailable.
- The callback is optional. Callers that do not register it retain their
existing behavior.
- The callback runs before workspace restoration and can delay cleanup
if its caller does not bound its own work. The dependent instruction
collector uses bounded reads and retries. A rejected callback still
permits bridge and workspace cleanup; it cannot claim an instruction
save.

## Model Used

OpenAI Codex, GPT-6, with tool use and code execution. The runtime does
not expose the exact deployment variant or context-window size. Multiple
Codex agents implemented and verified the change.

## 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 <noreply@paperclip.ing>
2026-09-28 08:45:53 -05:00

76 lines
4.3 KiB
TypeScript

import { describe, expect, it, vi, afterEach } from "vitest";
import * as serverUtils from "./server-utils.js";
import { runAdapterExecutionTargetProcess } from "./execution-target.js";
import { createProviderStoppedBoundary } from "./provider-stopped-boundary.js";
const result = { exitCode: 0, signal: null, timedOut: false, stdout: "", stderr: "", pid: 12, startedAt: "now" };
const options = (onProcessStopped: () => void) => ({ cwd: "/tmp", env: {}, timeoutSec: 1, graceSec: 1, onLog: async () => {}, onProcessStopped });
const target = (execute: () => Promise<any>) => ({ kind: "remote", transport: "sandbox", remoteCwd: "/workspace", runner: { execute } }) as any;
afterEach(() => vi.restoreAllMocks());
describe("owned provider process stop boundary", () => {
it.each([0, 1])("notifies after a remote invocation returns an observed exit code %s", async exitCode => {
const stopped = vi.fn();
await runAdapterExecutionTargetProcess("run", target(async () => ({ ...result, exitCode })), "provider", [], options(stopped));
expect(stopped).toHaveBeenCalledOnce();
});
it("notifies after the local child closed on timeout", async () => {
vi.spyOn(serverUtils, "runChildProcess").mockResolvedValue({ ...result, exitCode: null, signal: "SIGTERM", timedOut: true });
const stopped = vi.fn();
await runAdapterExecutionTargetProcess("run", null, "provider", [], options(stopped));
expect(stopped).toHaveBeenCalledOnce();
});
it.each([
{ ...result, exitCode: null, timedOut: true },
{ ...result, exitCode: null, timedOut: false },
])("does not treat an incomplete remote execution as a stop receipt", async incomplete => {
const stopped = vi.fn();
await runAdapterExecutionTargetProcess("run", target(async () => incomplete), "provider", [], options(stopped));
expect(stopped).not.toHaveBeenCalled();
});
it("does not infer remote termination from a transport rejection", async () => {
const stopped = vi.fn();
await expect(runAdapterExecutionTargetProcess("run", target(async () => { throw new Error("transport lost"); }), "provider", [], options(stopped))).rejects.toThrow("transport lost");
expect(stopped).not.toHaveBeenCalled();
});
});
describe("final invocation instruction collection fence", () => {
it("collects once, after the last completed attempt, before restore", async () => {
const order: string[] = [];
const fence = createProviderStoppedBoundary(async () => { order.push("collect"); });
fence.beginInvocation()();
expect(order).toEqual([]);
fence.beginInvocation()();
await fence.collectBeforeRestore();
order.push("restore");
await fence.collectBeforeRestore();
expect(order).toEqual(["collect", "restore"]);
});
it("an uncertain later attempt invalidates an earlier stop, including late callbacks", async () => {
const collect = vi.fn(async () => {});
const fence = createProviderStoppedBoundary(collect);
const old = fence.beginInvocation(); old();
fence.beginInvocation(); old();
await fence.collectBeforeRestore();
expect(collect).not.toHaveBeenCalled();
});
it("does not collect before any provider invocation", async () => {
const collect = vi.fn(async () => {});
await createProviderStoppedBoundary(collect).collectBeforeRestore();
expect(collect).not.toHaveBeenCalled();
});
it("does not infer SSH provider termination from transport exit 255", async () => {
vi.spyOn(serverUtils, "runChildProcess").mockResolvedValue({ ...result, exitCode: 255 });
const stopped = vi.fn();
await runAdapterExecutionTargetProcess("run", { kind: "remote", transport: "ssh", spec: { host: "fixture" }, remoteCwd: "/workspace" } as any, "provider", [], options(stopped));
expect(stopped).not.toHaveBeenCalled();
});
it("retains the stop observation when a subsequent log-tail flush throws", async () => {
const stopped = vi.fn();
await expect(runAdapterExecutionTargetProcess("run", target(async () => result), "provider", [], {
...options(stopped), runLogTail: { create: () => ({ wrapCommand: (command: string, args: string[]) => ({ command, args }), start: () => {}, finish: async () => { throw new Error("log flush failed"); }, abort: async () => {} }) } as any,
})).rejects.toThrow("log flush failed");
expect(stopped).toHaveBeenCalledOnce();
});
});