From 71ab478278867667da0cb28215d00f3c8dac0556 Mon Sep 17 00:00:00 2001 From: Kritarth-Dandapat Date: Wed, 12 Aug 2026 07:04:19 +0530 Subject: [PATCH] fix(acp): prove signed-in Codex session cleanup --- core/acp/README.md | 26 ++++ core/acp/smoke.ts | 15 +- core/acp/stdio-client.test.ts | 70 ++++++++- core/acp/stdio-client.ts | 98 ++++++++++-- docs/research/codex-acp-current-contract.md | 164 ++++++++++++++++++++ docs/verification/codex-acp-2026-08-12.md | 86 ++++++++++ 6 files changed, 446 insertions(+), 13 deletions(-) create mode 100644 docs/research/codex-acp-current-contract.md create mode 100644 docs/verification/codex-acp-2026-08-12.md diff --git a/core/acp/README.md b/core/acp/README.md index fe44c1a..e15231e 100644 --- a/core/acp/README.md +++ b/core/acp/README.md @@ -12,6 +12,13 @@ for the implementation ([P1-01] — done). Adding a backend later = spawn a different ACP server. No new client code. +Install the maintained Codex adapter as +`@agentclientprotocol/codex-acp`. The older +`@zed-industries/codex-acp` package is deprecated and embeds an older Codex +runtime even when a newer standalone `codex` CLI is on `PATH`. Record the +adapter package and version during live verification; executable name alone +does not distinguish them. + ## Dependency `@agentclientprotocol/sdk` (npm, verified on the registry 2026-07-21). This is @@ -64,6 +71,13 @@ child adapter process is killed and the caller gets a thrown error hanging forever — this is what protects against a stalled adapter (e.g. an errored codex-acp turn, or any other hang mid-session). +On POSIX, adapters start in their own process group. Session disposal and +handshake/turn failures terminate the complete adapter process tree, wait up +to one second, then escalate to `SIGKILL` if required. This matters for npm +launcher scripts that spawn a native adapter child: killing only the launcher +can otherwise orphan the native process. Windows terminates the direct child, +waits with the same bound, and escalates before reporting cleanup failure. + The handshake default is deliberately generous, not tight: measured against a real cold-start `claude-code-acp` (`CLAUDECODE` stripped), `initialize` returns in ~226ms but `session/new` alone legitimately takes ~16.2s — it @@ -83,6 +97,18 @@ Requires the adapter CLI(s) actually installed and signed in to your Claude Pro/Max or ChatGPT plan. This is **not** part of `npm test` / CI — it's a separate, honest check of the real sanctioned-plan path. See `smoke.ts`. +Smoke output is sanitized and compact: UTC start/end times, streamed text, +update kinds, and terminal stop reason. Adapter diagnostics remain on stderr. + +If a service rollout selects a model newer than the installed adapter runtime, +set `VELLUM_CODEX_MODEL` to a model supported by that runtime while upgrading +the adapter. The value goes directly to the first-party adapter as +`-c model=` without a shell or alternate authentication path: + +```bash +VELLUM_CODEX_MODEL=gpt-5.5 npm run smoke:acp -- codex +``` + Runs fine from inside a Claude Code terminal/agent now (the `CLAUDECODE` stripping above handles it automatically) and no longer hangs forever on a stalled adapter (the turn/handshake timeouts above make it fail loud diff --git a/core/acp/smoke.ts b/core/acp/smoke.ts index e7cc8d9..ecfaf36 100644 --- a/core/acp/smoke.ts +++ b/core/acp/smoke.ts @@ -40,6 +40,7 @@ const PROMPT = 'Reply with exactly the word "pong" and nothing else.' async function smoke(backend: AcpBackend): Promise { console.log(`\n=== ${backend} (${backend === 'claude' ? 'claude-code-acp' : 'codex-acp'}) ===`) + console.log(`startedAt=${new Date().toISOString()}`) const client = new StdioAcpClient() let session try { @@ -49,22 +50,34 @@ async function smoke(backend: AcpBackend): Promise { console.log( ` -> install/sign in the adapter, confirm it's on PATH, then re-run: npm run smoke:acp -- ${backend}`, ) + process.exitCode = 1 return } try { let sawDone = false + const updateKinds: string[] = [] for await (const update of session.prompt({ text: PROMPT })) { - console.log(JSON.stringify(update)) + updateKinds.push(update.kind) + if (update.kind === 'text' && typeof update.data === 'object' && update.data !== null) { + const text = 'text' in update.data ? update.data.text : undefined + if (typeof text === 'string') console.log(`text=${JSON.stringify(text)}`) + } + if (update.kind === 'done') console.log(`done=${JSON.stringify(update.data)}`) if (update.kind === 'done') sawDone = true if (update.kind === 'error') { + console.log(`error=${JSON.stringify(update.data)}`) console.log(`UNVERIFIED — adapter reported an error mid-turn.`) + process.exitCode = 1 return } } + console.log(`updateKinds=${updateKinds.join(',')}`) console.log(sawDone ? 'VERIFIED — stream ended in a done update.' : 'UNVERIFIED — stream ended without done.') + if (!sawDone) process.exitCode = 1 } finally { await session.dispose() + console.log(`endedAt=${new Date().toISOString()}`) } } diff --git a/core/acp/stdio-client.test.ts b/core/acp/stdio-client.test.ts index d0a59cd..04acce6 100644 --- a/core/acp/stdio-client.test.ts +++ b/core/acp/stdio-client.test.ts @@ -13,7 +13,13 @@ import type { Readable, Writable } from 'node:stream' import { describe, expect, it } from 'vitest' import type { AcpBackend, AcpUpdate } from './client.js' -import { StdioAcpClient, buildAdapterEnv, mapSessionUpdate, type SpawnAdapter } from './stdio-client.js' +import { + StdioAcpClient, + buildAdapterArgs, + buildAdapterEnv, + mapSessionUpdate, + type SpawnAdapter, +} from './stdio-client.js' type AdapterChildProcess = ChildProcessByStdio @@ -22,6 +28,7 @@ type AdapterChildProcess = ChildProcessByStdio // Behavior is selected via argv so one script covers every test scenario. const FAKE_AGENT_SCRIPT = String.raw` const readline = require('node:readline') +const { spawn } = require('node:child_process') // argv[0] is the node binary; with \`node -e