mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 09:37:19 +01:00
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents for work. > - Local agent heartbeats need durable process identity so the server can supervise them. > - The ACPX runtime owns the child process used by `codex_local` sessions. > - ACPX did not expose the child PID and start time to the Paperclip adapter. > - Warm ACPX runtimes can also serve a later heartbeat without a new spawn event. > - A hot restart could therefore classify a live Codex run as lost because its heartbeat row had no process identity. > - This pull request forwards ACPX spawn identity, reuses it for compatible warm heartbeats, and fails closed when identity cannot be persisted. > - The benefit is reliable hot-restart adoption for eligible local Codex runs. ## Linked Issues or Issue Description No matching public GitHub issue was found. **What happened?** A `codex_local` heartbeat could run through ACPX without a persisted `processPid` or `processStartedAt`. A Paperclip hot restart then had no durable identity for the live ACP child. Recovery could classify the run as `process_lost` even while the child was still alive. **Expected behavior** ACPX reports the real child PID and start time before the first prompt. A compatible warm runtime reports the same known identity to each later heartbeat that reuses the child. ACPX stops the child if the identity is invalid or persistence fails. Hot-restart recovery can then adopt the live run. **Steps to reproduce** 1. Start a `codex_local` heartbeat through the ACPX execution lane. 2. Keep the run active during a Paperclip hot restart. 3. Inspect the heartbeat row before this change. 4. Observe that the process identity can be null and recovery cannot adopt the live child. **Reproduced on** - Paperclip `master` before this change. - Linux source deployment. - `codex_local` with ACPX `0.12.0`. ## What Changed - Add an awaited `onAgentSpawn` lifecycle hook to the patched ACPX runtime. - Forward the ACP child PID and start time through the adapter `onSpawn` callback. - Keep a mutable callback sink for cached runtimes so a later respawn updates the current heartbeat. - Reuse the last known process identity when a compatible warm heartbeat reuses the existing child. - Kill the ACP child and fail session startup when the PID is invalid or identity persistence rejects. - Add ACPX and heartbeat recovery tests for callback ordering, warm reuse, failure cleanup, durable row identity, and hot-restart adoption. - Document the one-time drain required when an installed pre-fix run already lacks process metadata. ## Verification - `PAPERCLIP_HOME="$PAPERCLIP_RUN_SCRATCH_DIR/test-home-execute-escalated" pnpm exec vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts` — 89 passed. - `PAPERCLIP_HOME="$PAPERCLIP_RUN_SCRATCH_DIR/test-home-recovery-escalated" pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts` — 92 passed. - `PAPERCLIP_HOME="$PAPERCLIP_RUN_SCRATCH_DIR/test-home-remote-smoke-escalated" pnpm exec vitest run packages/adapter-utils/src/acpx-engine/remote-spawn-smoke.test.ts` — 3 passed. - `PAPERCLIP_HOME="$PAPERCLIP_RUN_SCRATCH_DIR/test-home-ci-repro-escalated" pnpm exec vitest run server/src/__tests__/heartbeat-dependency-scheduling.test.ts` — 6 passed. - `pnpm --filter @paperclipai/adapter-utils typecheck` — passed. - Reverse and forward dry-run application of `patches/acpx@0.12.0.patch` — passed. - `git diff --check` — passed. - `git diff --exit-code origin/master...HEAD -- pnpm-lock.yaml` — passed. - `git diff --exit-code origin/master...HEAD -- .github/workflows` — passed. ## Risks - Runtime risk is low to moderate. ACPX now awaits process-identity persistence during child startup. - ACPX kills the child when persistence fails. This prevents an unsupervised process, but it makes that heartbeat fail visibly. - A compatible warm heartbeat reuses the identity of the existing ACP child. Regression tests verify that identity is persisted before the next prompt. - The change updates the vendored ACPX patch. Package installation must apply that patch. - There are no schema, migration, public API, UI, workflow, or lockfile changes. > 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 used GPT-5.3-Codex for the earlier implementation. - OpenAI Codex used GPT-5 for the lifecycle-hook revision and the current fail-closed review fix. The runtime did not expose a more specific snapshot ID or context-window size. Both runs used reasoning, repository tools, 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 (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>
174 lines
7.8 KiB
TypeScript
174 lines
7.8 KiB
TypeScript
import fs from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import { fileURLToPath } from "node:url";
|
|
import { afterEach, expect, it } from "vitest";
|
|
import type { AcpRuntimeOptions } from "acpx/runtime";
|
|
import { createAcpRuntime, createAgentRegistry, createRuntimeStore } from "acpx/runtime";
|
|
|
|
// Load-bearing repro for the remote-ACP "process session" lane host-spawn bug.
|
|
//
|
|
// The engine threads ONE cwd (`sessionCwd`) into every cwd-keyed site: the ACP
|
|
// `session/new` cwd, the session fingerprint/compat key, AND the acpx HOST
|
|
// `spawn()` of the host-local relay proxy. On the remote lane that value is the
|
|
// IN-SANDBOX `remoteCwd`, which does not exist on the host, so libuv's pre-`exec`
|
|
// `chdir` fails and acpx raises `AgentSpawnError` at `ensure_session`.
|
|
//
|
|
// A faithful full-engine remote-lane repro is NOT possible without a live
|
|
// sandbox: the only local stand-in for a sandbox runs its commands as host child
|
|
// processes, so every `remoteCwd`-derived operation (workspace staging, the
|
|
// callback bridge, the process-session bridge) executes on the HOST filesystem —
|
|
// either materializing `remoteCwd` on the host (masking the host-spawn ENOENT)
|
|
// or failing earlier at a different phase. So we split the proof at its two real
|
|
// seams: (1) here — the acpx runtime's REAL host `spawn()` honoring
|
|
// `spawnCwd ?? cwd` (real `createAcpRuntime`, real libuv `chdir`); and (2) the
|
|
// engine → `runtimeOptions` threading (`execute.test.ts`, which asserts the
|
|
// engine sets `spawnCwd` host-valid on the remote lane and `undefined`
|
|
// elsewhere, with the advertised `session/new` cwd staying `remoteCwd`).
|
|
// End-to-end validation against a real sandbox is board-run.
|
|
|
|
const repoRoot = fileURLToPath(new URL("../../../..", import.meta.url));
|
|
// A dedicated ACP agent fixture that reports, on stderr, the working directory
|
|
// the host actually spawned it in (`SPAWN_CWD`) and the `cwd` advertised on
|
|
// `session/new` (`SESSION_NEW_CWD`).
|
|
const fixturePath = path.join(repoRoot, "scripts", "mcp-fixtures", "servers", "acp-cwd-report-agent.mjs");
|
|
const tempRoots: string[] = [];
|
|
|
|
type PatchedAcpRuntimeOptions = AcpRuntimeOptions & {
|
|
onAgentSpawn?: (meta: { pid: number; startedAt: string }) => Promise<void>;
|
|
spawnCwd?: string;
|
|
};
|
|
|
|
type PatchedEnsureSessionOptions = Parameters<ReturnType<typeof createAcpRuntime>["ensureSession"]>[0];
|
|
|
|
afterEach(async () => {
|
|
await Promise.all(tempRoots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true })));
|
|
});
|
|
|
|
async function makeTempDir(prefix: string): Promise<string> {
|
|
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
|
|
tempRoots.push(dir);
|
|
return dir;
|
|
}
|
|
|
|
/**
|
|
* Drive a REAL acpx runtime through `ensureSession`, which performs the real
|
|
* host `spawn()` (and its libuv `chdir`). `cwd` is the advertised session cwd;
|
|
* `spawnCwd`, when set, is the host-only spawn cwd the acpx patch consumes as
|
|
* `spawnCwd ?? cwd`.
|
|
*/
|
|
async function ensureRealAcpSession(input: {
|
|
cwd: string;
|
|
spawnCwd?: string;
|
|
onAgentSpawn?: (meta: { pid: number; startedAt: string }) => Promise<void>;
|
|
}) {
|
|
const stateRoot = await makeTempDir("paperclip-acpx-remote-spawn-state-");
|
|
const stderrChunks: string[] = [];
|
|
const agentCommand = `${JSON.stringify(process.execPath.replaceAll("\\", "/"))} ${JSON.stringify(fixturePath.replaceAll("\\", "/"))}`;
|
|
const runtimeOptions: PatchedAcpRuntimeOptions = {
|
|
cwd: input.cwd,
|
|
// `spawnCwd` is the host-only knob added by patches/acpx@0.12.0.patch; when
|
|
// unset acpx falls back to `cwd`, so every non-proxy lane is byte-identical.
|
|
...(input.spawnCwd ? { spawnCwd: input.spawnCwd } : {}),
|
|
sessionStore: createRuntimeStore({ stateDir: path.join(stateRoot, "state") }),
|
|
agentRegistry: createAgentRegistry({ overrides: { custom: agentCommand } }),
|
|
permissionMode: "approve-all",
|
|
nonInteractivePermissions: "deny",
|
|
onAgentStderr: (chunk: string) => stderrChunks.push(chunk),
|
|
onAgentSpawn: input.onAgentSpawn,
|
|
};
|
|
const runtime = createAcpRuntime(runtimeOptions);
|
|
|
|
try {
|
|
const sessionInput: PatchedEnsureSessionOptions = {
|
|
sessionKey: "remote-spawn-smoke",
|
|
agent: "custom",
|
|
mode: "oneshot",
|
|
cwd: input.cwd,
|
|
sessionOptions: { env: {} },
|
|
};
|
|
const handle = await runtime.ensureSession(sessionInput);
|
|
await (runtime as { close: (i: unknown) => Promise<void> }).close({ handle, reason: "done" }).catch(() => {});
|
|
return { resolved: true as const, stderr: stderrChunks.join("") };
|
|
} catch (err) {
|
|
return { resolved: false as const, error: err as NodeJS.ErrnoException & { cause?: NodeJS.ErrnoException }, stderr: stderrChunks.join("") };
|
|
}
|
|
}
|
|
|
|
function isProcessAlive(pid: number): boolean {
|
|
try {
|
|
process.kill(pid, 0);
|
|
return true;
|
|
} catch {
|
|
return false;
|
|
}
|
|
}
|
|
|
|
async function waitForProcessExit(pid: number, timeoutMs = 2_000): Promise<void> {
|
|
const deadline = Date.now() + timeoutMs;
|
|
while (isProcessAlive(pid)) {
|
|
if (Date.now() >= deadline) throw new Error(`Timed out waiting for ACP agent ${pid} to exit`);
|
|
await new Promise((resolve) => setTimeout(resolve, 20));
|
|
}
|
|
}
|
|
|
|
it("reproduces host-spawn ENOENT when the advertised session cwd is host-nonexistent", async () => {
|
|
// The in-sandbox `remoteCwd` that does not exist on the host. Intentionally
|
|
// NOT created: this is what trips the acpx host `spawn()` `chdir`.
|
|
const sandboxParent = await makeTempDir("paperclip-acpx-remote-spawn-sandbox-");
|
|
const remoteCwd = path.join(sandboxParent, "does-not-exist-on-host", "workspace");
|
|
|
|
const outcome = await ensureRealAcpSession({ cwd: remoteCwd });
|
|
|
|
// Before the fix the engine feeds `remoteCwd` as the host spawn cwd, so acpx's
|
|
// real `spawn()` `chdir`s into a host-nonexistent dir and fails ENOENT. This is
|
|
// the exact `ensure_session` failure the remote lane hits in production.
|
|
expect(outcome.resolved, JSON.stringify(outcome)).toBe(false);
|
|
if (outcome.resolved) return;
|
|
expect(outcome.error.name).toBe("AgentSpawnError");
|
|
expect(outcome.error.cause?.code).toBe("ENOENT");
|
|
});
|
|
|
|
it("spawnCwd redirects the host spawn to a host-valid dir while the advertised session cwd stays remoteCwd", async () => {
|
|
const sandboxParent = await makeTempDir("paperclip-acpx-remote-spawn-sandbox-");
|
|
// Host-nonexistent in-sandbox cwd — the advertised `session/new` cwd.
|
|
const remoteCwd = path.join(sandboxParent, "does-not-exist-on-host", "workspace");
|
|
// Host-valid dir the proxy actually spawns in (the engine's host `cwd`).
|
|
const hostSpawnCwd = await makeTempDir("paperclip-acpx-remote-spawn-host-");
|
|
|
|
const outcome = await ensureRealAcpSession({ cwd: remoteCwd, spawnCwd: hostSpawnCwd });
|
|
|
|
// With `spawnCwd` set the host `spawn()` `chdir`s into the host-valid dir, so
|
|
// the session comes up instead of failing at `ensure_session`.
|
|
expect(outcome.resolved, JSON.stringify(outcome)).toBe(true);
|
|
// The host process really ran in `spawnCwd`...
|
|
expect(outcome.stderr).toContain(`SPAWN_CWD=${await fs.realpath(hostSpawnCwd)}`);
|
|
// ...while the in-sandbox data path (the advertised `session/new` cwd) is
|
|
// unchanged — still `remoteCwd`.
|
|
expect(outcome.stderr).toContain(`SESSION_NEW_CWD=${remoteCwd}`);
|
|
});
|
|
|
|
it("kills the ACP agent when process identity persistence rejects", async () => {
|
|
const hostCwd = await makeTempDir("paperclip-acpx-spawn-persistence-");
|
|
let spawnedPid: number | null = null;
|
|
|
|
const outcome = await ensureRealAcpSession({
|
|
cwd: hostCwd,
|
|
onAgentSpawn: async ({ pid }) => {
|
|
spawnedPid = pid;
|
|
throw new Error("process identity persistence failed");
|
|
},
|
|
});
|
|
|
|
expect(outcome.resolved).toBe(false);
|
|
if (outcome.resolved) return;
|
|
expect(outcome.error.message).toContain("process identity persistence failed");
|
|
expect(spawnedPid).not.toBeNull();
|
|
|
|
try {
|
|
await waitForProcessExit(spawnedPid!);
|
|
} finally {
|
|
if (isProcessAlive(spawnedPid!)) process.kill(spawnedPid!, "SIGKILL");
|
|
}
|
|
});
|