From 106256d896af6dfd30ebda5ffbcd456026b7b536 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 10 Apr 2026 17:17:19 +0100 Subject: [PATCH] fix: address Codex harness review regressions --- extensions/codex/app-server/client.ts | 7 ++ .../codex/app-server/run-attempt.test.ts | 76 +++++++++++++++++++ extensions/codex/app-server/run-attempt.ts | 69 +++++++++++++++-- extensions/codex/openclaw.plugin.json | 4 - src/gateway/server-startup-post-attach.ts | 3 +- src/gateway/server-startup.test.ts | 25 ++++++ 6 files changed, 172 insertions(+), 12 deletions(-) diff --git a/extensions/codex/app-server/client.ts b/extensions/codex/app-server/client.ts index 5982116d3bb..449ddefb14a 100644 --- a/extensions/codex/app-server/client.ts +++ b/extensions/codex/app-server/client.ts @@ -282,6 +282,13 @@ export function resetSharedCodexAppServerClientForTests(): void { sharedClientPromise = undefined; } +export function clearSharedCodexAppServerClient(): void { + const client = sharedClient; + sharedClient = undefined; + sharedClientPromise = undefined; + client?.close(); +} + function clearSharedClientIfCurrent(client: CodexAppServerClient): void { if (sharedClient !== client) { return; diff --git a/extensions/codex/app-server/run-attempt.test.ts b/extensions/codex/app-server/run-attempt.test.ts index a4168d8aaa9..b64a5c89d8f 100644 --- a/extensions/codex/app-server/run-attempt.test.ts +++ b/extensions/codex/app-server/run-attempt.test.ts @@ -10,6 +10,7 @@ import { import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { CodexServerNotification } from "./protocol.js"; import { runCodexAppServerAttempt, __testing } from "./run-attempt.js"; +import { writeCodexAppServerBinding } from "./session-binding.js"; let tempDir: string; @@ -226,4 +227,79 @@ describe("runCodexAppServerAttempt", () => { timedOut: false, }); }); + + it("times out app-server startup before thread setup can hang forever", async () => { + __testing.setCodexAppServerClientFactoryForTests(() => new Promise(() => undefined)); + const params = createParams( + path.join(tempDir, "session.jsonl"), + path.join(tempDir, "workspace"), + ); + params.timeoutMs = 1; + + await expect(runCodexAppServerAttempt(params)).rejects.toThrow( + "codex app-server startup timed out", + ); + expect(queueAgentHarnessMessage("session-1", "after timeout")).toBe(false); + }); + + it("keeps extended history enabled when resuming a bound Codex thread", async () => { + const sessionFile = path.join(tempDir, "session.jsonl"); + const workspaceDir = path.join(tempDir, "workspace"); + await writeCodexAppServerBinding(sessionFile, { + threadId: "thread-existing", + cwd: workspaceDir, + model: "gpt-5.4-codex", + modelProvider: "openai", + dynamicToolsFingerprint: "[]", + }); + const requests: Array<{ method: string; params: unknown }> = []; + let notify: (notification: CodexServerNotification) => Promise = async () => undefined; + const request = vi.fn(async (method: string, params?: unknown) => { + requests.push({ method, params }); + if (method === "thread/resume") { + return { thread: { id: "thread-existing" }, modelProvider: "openai" }; + } + if (method === "turn/start") { + return { turn: { id: "turn-1", status: "inProgress" } }; + } + return {}; + }); + __testing.setCodexAppServerClientFactoryForTests( + async () => + ({ + request, + addNotificationHandler: (handler: typeof notify) => { + notify = handler; + return () => undefined; + }, + addRequestHandler: () => () => undefined, + }) as never, + ); + + const run = runCodexAppServerAttempt(createParams(sessionFile, workspaceDir)); + await vi.waitFor(() => + expect(requests.some((entry) => entry.method === "turn/start")).toBe(true), + ); + await notify({ + method: "turn/completed", + params: { + threadId: "thread-existing", + turnId: "turn-1", + turn: { id: "turn-1", status: "completed" }, + }, + }); + await run; + + expect(requests).toEqual( + expect.arrayContaining([ + { + method: "thread/resume", + params: { + threadId: "thread-existing", + persistExtendedHistory: true, + }, + }, + ]), + ); + }); }); diff --git a/extensions/codex/app-server/run-attempt.ts b/extensions/codex/app-server/run-attempt.ts index 7fdf2601e3b..215afa3d505 100644 --- a/extensions/codex/app-server/run-attempt.ts +++ b/extensions/codex/app-server/run-attempt.ts @@ -19,6 +19,7 @@ import { } from "openclaw/plugin-sdk/agent-harness"; import { handleCodexAppServerApprovalRequest } from "./approval-bridge.js"; import { + clearSharedCodexAppServerClient, getSharedCodexAppServerClient, isCodexAppServerApprovalRequest, type CodexAppServerClient, @@ -98,13 +99,28 @@ export async function runCodexAppServerAttempt( tools, signal: runAbortController.signal, }); - const client = await clientFactory(); - const thread = await startOrResumeThread({ - client, - params, - cwd: effectiveWorkspace, - dynamicTools: toolBridge.specs, - }); + let client: CodexAppServerClient; + let thread: CodexAppServerThreadBinding; + try { + ({ client, thread } = await withCodexStartupTimeout({ + timeoutMs: params.timeoutMs, + signal: runAbortController.signal, + operation: async () => { + const startupClient = await clientFactory(); + const startupThread = await startOrResumeThread({ + client: startupClient, + params, + cwd: effectiveWorkspace, + dynamicTools: toolBridge.specs, + }); + return { client: startupClient, thread: startupThread }; + }, + })); + } catch (error) { + clearSharedCodexAppServerClient(); + params.abortSignal?.removeEventListener("abort", abortFromUpstream); + throw error; + } let projector: CodexAppServerEventProjector | undefined; let turnId: string | undefined; @@ -335,6 +351,44 @@ async function buildDynamicTools(input: DynamicToolBuildParams) { }); } +async function withCodexStartupTimeout(params: { + timeoutMs: number; + signal: AbortSignal; + operation: () => Promise; +}): Promise { + if (params.signal.aborted) { + throw new Error("codex app-server startup aborted"); + } + let timeout: NodeJS.Timeout | undefined; + let abortCleanup: (() => void) | undefined; + try { + return await Promise.race([ + params.operation(), + new Promise((_, reject) => { + const rejectOnce = (error: Error) => { + if (timeout) { + clearTimeout(timeout); + timeout = undefined; + } + reject(error); + }; + const timeoutMs = Math.max(100, params.timeoutMs); + timeout = setTimeout(() => { + rejectOnce(new Error("codex app-server startup timed out")); + }, timeoutMs); + const abortListener = () => rejectOnce(new Error("codex app-server startup aborted")); + params.signal.addEventListener("abort", abortListener, { once: true }); + abortCleanup = () => params.signal.removeEventListener("abort", abortListener); + }), + ]); + } finally { + if (timeout) { + clearTimeout(timeout); + } + abortCleanup?.(); + } +} + async function startOrResumeThread(params: { client: CodexAppServerClient; params: EmbeddedRunAttemptParams; @@ -356,6 +410,7 @@ async function startOrResumeThread(params: { try { const response = await params.client.request("thread/resume", { threadId: binding.threadId, + persistExtendedHistory: true, }); await writeCodexAppServerBinding(params.params.sessionFile, { threadId: response.thread.id, diff --git a/extensions/codex/openclaw.plugin.json b/extensions/codex/openclaw.plugin.json index 7c52147d19e..09329e3e3c2 100644 --- a/extensions/codex/openclaw.plugin.json +++ b/extensions/codex/openclaw.plugin.json @@ -4,10 +4,6 @@ "name": "Codex", "description": "Codex app-server harness and Codex-managed GPT model catalog.", "providers": ["codex"], - "modelSupport": { - "providerIds": ["codex"], - "modelPrefixes": ["gpt-", "o1", "o3", "o4", "arcanine"] - }, "configSchema": { "type": "object", "additionalProperties": false, diff --git a/src/gateway/server-startup-post-attach.ts b/src/gateway/server-startup-post-attach.ts index c1a6ff3d95b..22190a2c755 100644 --- a/src/gateway/server-startup-post-attach.ts +++ b/src/gateway/server-startup-post-attach.ts @@ -63,7 +63,8 @@ async function prewarmConfiguredPrimaryModel(params: { if (isCliProvider(provider, params.cfg)) { return; } - if (resolveEmbeddedAgentRuntime() !== "auto") { + const runtime = resolveEmbeddedAgentRuntime(); + if (runtime !== "auto" && runtime !== "pi") { return; } if (selectAgentHarness({ provider, modelId: model }).id !== "pi") { diff --git a/src/gateway/server-startup.test.ts b/src/gateway/server-startup.test.ts index c3474e3aa8c..fe2e3905610 100644 --- a/src/gateway/server-startup.test.ts +++ b/src/gateway/server-startup.test.ts @@ -167,4 +167,29 @@ describe("gateway startup primary model warmup", () => { expect(ensureOpenClawModelsJsonMock).not.toHaveBeenCalled(); expect(resolveModelMock).not.toHaveBeenCalled(); }); + + it("keeps PI static warmup when the PI agent runtime is forced", async () => { + resolveEmbeddedAgentRuntimeMock.mockReturnValue("pi"); + const cfg = { + agents: { + defaults: { + model: { + primary: "openai-codex/gpt-5.4", + }, + }, + }, + } as OpenClawConfig; + + await prewarmConfiguredPrimaryModel({ + cfg, + log: { warn: vi.fn() }, + }); + + expect(selectAgentHarnessMock).toHaveBeenCalledWith({ + provider: "openai-codex", + modelId: "gpt-5.4", + }); + expect(ensureOpenClawModelsJsonMock).toHaveBeenCalledWith(cfg, "/tmp/agent"); + expect(resolveModelMock).toHaveBeenCalled(); + }); });