From 659bcc5e5b59b43cbb8a64d5f26dfc839b7ae610 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 12 Apr 2026 16:13:49 +0100 Subject: [PATCH] fix: tighten codex app-server lifecycle --- extensions/codex/provider.test.ts | 33 ++++++++++++- extensions/codex/provider.ts | 17 +++---- extensions/codex/src/app-server/models.ts | 47 +++++++++++-------- .../codex/src/app-server/run-attempt.ts | 26 ++++++---- .../codex/src/app-server/shared-client.ts | 17 +++++++ src/agents/model-fallback-observation.ts | 5 +- .../skills.loadworkspaceskillentries.test.ts | 27 ++++++++++- src/agents/skills/workspace.ts | 11 +++++ 8 files changed, 141 insertions(+), 42 deletions(-) diff --git a/extensions/codex/provider.test.ts b/extensions/codex/provider.test.ts index 3d1c26b0266..fea2ed4ff58 100644 --- a/extensions/codex/provider.test.ts +++ b/extensions/codex/provider.test.ts @@ -1,7 +1,10 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { buildCodexProvider, buildCodexProviderCatalog } from "./provider.js"; import { CodexAppServerClient } from "./src/app-server/client.js"; -import { resetSharedCodexAppServerClientForTests } from "./src/app-server/shared-client.js"; +import { + getSharedCodexAppServerClient, + resetSharedCodexAppServerClientForTests, +} from "./src/app-server/shared-client.js"; afterEach(() => { resetSharedCodexAppServerClientForTests(); @@ -37,7 +40,7 @@ describe("codex provider", () => { }); expect(listModels).toHaveBeenCalledWith( - expect.objectContaining({ limit: 100, timeoutMs: 1234 }), + expect.objectContaining({ limit: 100, timeoutMs: 1234, sharedClient: false }), ); expect(result.provider).toMatchObject({ auth: "token", @@ -103,6 +106,32 @@ describe("codex provider", () => { expect(client.close).toHaveBeenCalledTimes(1); }); + it("does not close an active shared app-server client during live discovery", async () => { + const activeClient = { + initialize: vi.fn(async () => undefined), + request: vi.fn(async () => ({ data: [] })), + addCloseHandler: vi.fn(() => () => undefined), + close: vi.fn(), + } as unknown as CodexAppServerClient; + const discoveryClient = { + initialize: vi.fn(async () => undefined), + request: vi.fn(async () => ({ data: [] })), + addCloseHandler: vi.fn(() => () => undefined), + close: vi.fn(), + } as unknown as CodexAppServerClient; + vi.spyOn(CodexAppServerClient, "start") + .mockReturnValueOnce(activeClient) + .mockReturnValueOnce(discoveryClient); + + await getSharedCodexAppServerClient({ timeoutMs: 1000 }); + await buildCodexProviderCatalog({ + env: { OPENCLAW_CODEX_DISCOVERY_LIVE: "1" }, + }); + + expect(activeClient.close).not.toHaveBeenCalled(); + expect(discoveryClient.close).toHaveBeenCalledTimes(1); + }); + it("resolves arbitrary Codex app-server model ids through the codex provider", () => { const provider = buildCodexProvider(); diff --git a/extensions/codex/provider.ts b/extensions/codex/provider.ts index c737a24ad58..2cbcc6f076d 100644 --- a/extensions/codex/provider.ts +++ b/extensions/codex/provider.ts @@ -15,7 +15,6 @@ import { readCodexPluginConfig, resolveCodexAppServerRuntimeOptions, } from "./src/app-server/config.js"; -import { clearSharedCodexAppServerClient } from "./src/app-server/shared-client.js"; const PROVIDER_ID = "codex"; const CODEX_BASE_URL = "https://chatgpt.com/backend-api"; @@ -28,6 +27,7 @@ type CodexModelLister = (options: { timeoutMs: number; limit?: number; startOptions?: CodexAppServerStartOptions; + sharedClient?: boolean; }) => Promise; type BuildCodexProviderOptions = { @@ -102,15 +102,11 @@ export async function buildCodexProviderCatalog( const timeoutMs = normalizeTimeoutMs(config.discovery?.timeoutMs); let discovered: CodexAppServerModel[] = []; if (config.discovery?.enabled !== false && !shouldSkipLiveDiscovery(options.env)) { - try { - discovered = await listModelsBestEffort({ - listModels: options.listModels ?? listCodexAppServerModels, - timeoutMs, - startOptions: appServer.start, - }); - } finally { - clearSharedCodexAppServerClient(); - } + discovered = await listModelsBestEffort({ + listModels: options.listModels ?? listCodexAppServerModels, + timeoutMs, + startOptions: appServer.start, + }); } const models = (discovered.length > 0 ? discovered : FALLBACK_CODEX_MODELS).map( codexModelToDefinition, @@ -180,6 +176,7 @@ async function listModelsBestEffort(params: { timeoutMs: params.timeoutMs, limit: 100, startOptions: params.startOptions, + sharedClient: false, }); return result.models.filter((model) => !model.hidden); } catch { diff --git a/extensions/codex/src/app-server/models.ts b/extensions/codex/src/app-server/models.ts index ea130aba9ab..98e504754f8 100644 --- a/extensions/codex/src/app-server/models.ts +++ b/extensions/codex/src/app-server/models.ts @@ -1,7 +1,9 @@ import type { CodexAppServerStartOptions } from "./config.js"; import { type JsonObject, type JsonValue } from "./protocol.js"; -import { getSharedCodexAppServerClient } from "./shared-client.js"; -import { withTimeout } from "./timeout.js"; +import { + createIsolatedCodexAppServerClient, + getSharedCodexAppServerClient, +} from "./shared-client.js"; export type CodexAppServerModel = { id: string; @@ -26,32 +28,39 @@ export type CodexAppServerListModelsOptions = { includeHidden?: boolean; timeoutMs?: number; startOptions?: CodexAppServerStartOptions; + sharedClient?: boolean; }; export async function listCodexAppServerModels( options: CodexAppServerListModelsOptions = {}, ): Promise { const timeoutMs = options.timeoutMs ?? 2500; - return await withTimeout( - (async () => { - const client = await getSharedCodexAppServerClient({ + const useSharedClient = options.sharedClient !== false; + const client = useSharedClient + ? await getSharedCodexAppServerClient({ + startOptions: options.startOptions, + timeoutMs, + }) + : await createIsolatedCodexAppServerClient({ startOptions: options.startOptions, timeoutMs, }); - const response = await client.request( - "model/list", - { - limit: options.limit ?? null, - cursor: options.cursor ?? null, - includeHidden: options.includeHidden ?? null, - }, - { timeoutMs }, - ); - return readModelListResult(response); - })(), - timeoutMs, - "codex app-server model/list timed out", - ); + try { + const response = await client.request( + "model/list", + { + limit: options.limit ?? null, + cursor: options.cursor ?? null, + includeHidden: options.includeHidden ?? null, + }, + { timeoutMs }, + ); + return readModelListResult(response); + } finally { + if (!useSharedClient) { + client.close(); + } + } } function readModelListResult(value: JsonValue | undefined): CodexAppServerModelListResult { diff --git a/extensions/codex/src/app-server/run-attempt.ts b/extensions/codex/src/app-server/run-attempt.ts index 4a86686f4ad..241fdd426ff 100644 --- a/extensions/codex/src/app-server/run-attempt.ts +++ b/extensions/codex/src/app-server/run-attempt.ts @@ -227,14 +227,10 @@ export async function runCodexAppServerAttempt( ); const abortListener = () => { - void client - .request("turn/interrupt", { - threadId: thread.threadId, - turnId: activeTurnId, - }) - .catch((error: unknown) => { - embeddedAgentLog.debug("codex app-server turn interrupt failed during abort", { error }); - }); + interruptCodexTurnBestEffort(client, { + threadId: thread.threadId, + turnId: activeTurnId, + }); resolveCompletion?.(); }; runAbortController.signal.addEventListener("abort", abortListener, { once: true }); @@ -268,6 +264,20 @@ export async function runCodexAppServerAttempt( } } +function interruptCodexTurnBestEffort( + client: CodexAppServerClient, + params: { + threadId: string; + turnId: string; + }, +): void { + void Promise.resolve() + .then(() => client.request("turn/interrupt", params)) + .catch((error: unknown) => { + embeddedAgentLog.debug("codex app-server turn interrupt failed during abort", { error }); + }); +} + type DynamicToolBuildParams = { params: EmbeddedRunAttemptParams; resolvedWorkspace: string; diff --git a/extensions/codex/src/app-server/shared-client.ts b/extensions/codex/src/app-server/shared-client.ts index 6921ec5b739..1fd51ee95c3 100644 --- a/extensions/codex/src/app-server/shared-client.ts +++ b/extensions/codex/src/app-server/shared-client.ts @@ -59,6 +59,23 @@ export async function getSharedCodexAppServerClient(options?: { } } +export async function createIsolatedCodexAppServerClient(options?: { + startOptions?: CodexAppServerStartOptions; + timeoutMs?: number; +}): Promise { + const startOptions = options?.startOptions ?? resolveCodexAppServerRuntimeOptions().start; + const client = CodexAppServerClient.start(startOptions); + const initialize = client.initialize(); + try { + await withTimeout(initialize, options?.timeoutMs ?? 0, "codex app-server initialize timed out"); + return client; + } catch (error) { + client.close(); + await initialize.catch(() => undefined); + throw error; + } +} + export function resetSharedCodexAppServerClientForTests(): void { const state = getSharedCodexAppServerClientState(); state.client = undefined; diff --git a/src/agents/model-fallback-observation.ts b/src/agents/model-fallback-observation.ts index c769e361458..0a2c2eeef39 100644 --- a/src/agents/model-fallback-observation.ts +++ b/src/agents/model-fallback-observation.ts @@ -57,6 +57,9 @@ export function logModelFallbackDecision(params: { const reasonText = params.reason ?? "unknown"; const observedError = buildErrorObservationFields(params.error); const detailText = observedError.providerErrorMessagePreview ?? observedError.errorPreview; + const providerErrorTypeSuffix = observedError.providerErrorType + ? ` providerErrorType=${sanitizeForLog(observedError.providerErrorType)}` + : ""; const detailSuffix = detailText ? ` detail=${sanitizeForLog(detailText)}` : ""; decisionLog.warn("model fallback decision", { event: "model_fallback_decision", @@ -90,6 +93,6 @@ export function logModelFallbackDecision(params: { })), consoleMessage: `model fallback decision: decision=${params.decision} requested=${sanitizeForLog(params.requestedProvider)}/${sanitizeForLog(params.requestedModel)} ` + - `candidate=${sanitizeForLog(params.candidate.provider)}/${sanitizeForLog(params.candidate.model)} reason=${reasonText} next=${nextText}${detailSuffix}`, + `candidate=${sanitizeForLog(params.candidate.provider)}/${sanitizeForLog(params.candidate.model)} reason=${reasonText}${providerErrorTypeSuffix} next=${nextText}${detailSuffix}`, }); } diff --git a/src/agents/skills.loadworkspaceskillentries.test.ts b/src/agents/skills.loadworkspaceskillentries.test.ts index 70d35df5f19..ed4d37aa5a6 100644 --- a/src/agents/skills.loadworkspaceskillentries.test.ts +++ b/src/agents/skills.loadworkspaceskillentries.test.ts @@ -1,7 +1,9 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { resetLogger, setLoggerOverride } from "../logging/logger.js"; +import { loggingState } from "../logging/state.js"; import { withPathResolutionEnv } from "../test-utils/env.js"; import { writeSkill } from "./skills.e2e-test-helpers.js"; import { loadWorkspaceSkillEntries } from "./skills.js"; @@ -21,6 +23,9 @@ function withWorkspaceHome(workspaceDir: string, cb: () => T): T { } afterEach(async () => { + setLoggerOverride(null); + loggingState.rawConsole = null; + resetLogger(); await Promise.all( tempDirs.splice(0, tempDirs.length).map((dir) => fs.rm(dir, { recursive: true, force: true })), ); @@ -282,7 +287,16 @@ describe("loadWorkspaceSkillEntries", () => { description: "Outside", }); await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true }); - await fs.symlink(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill"), "dir"); + const requestedPath = path.join(workspaceDir, "skills", "escaped-skill"); + await fs.symlink(escapedSkillDir, requestedPath, "dir"); + setLoggerOverride({ level: "silent", consoleLevel: "warn" }); + const warn = vi.fn(); + loggingState.rawConsole = { + log: vi.fn(), + info: vi.fn(), + warn, + error: vi.fn(), + }; const entries = loadWorkspaceSkillEntries(workspaceDir, { managedSkillsDir: path.join(workspaceDir, ".managed"), @@ -290,6 +304,15 @@ describe("loadWorkspaceSkillEntries", () => { }); expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-skill"); + const [line] = warn.mock.calls[0] ?? []; + const warningLine = String(line); + expect(warningLine).toContain( + "Skipping skill path that resolves outside its configured root:", + ); + expect(warningLine).toContain("source=openclaw-workspace"); + expect(warningLine).toContain(`root=${path.join(workspaceDir, "skills")}`); + expect(warningLine).toContain(`requested=${requestedPath}`); + expect(warningLine).toContain("resolved="); }, ); diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index 383f560a801..4cdd76e406e 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -165,14 +165,24 @@ function tryRealpath(filePath: string): string | null { function warnEscapedSkillPath(params: { source: string; rootDir: string; + rootRealPath: string; candidatePath: string; candidateRealPath: string; }) { + const rootResolved = + path.resolve(params.rootDir) === params.rootRealPath + ? "" + : ` rootResolved=${params.rootRealPath}`; skillsLogger.warn("Skipping skill path that resolves outside its configured root.", { source: params.source, rootDir: params.rootDir, + rootRealPath: params.rootRealPath, path: params.candidatePath, realPath: params.candidateRealPath, + consoleMessage: + `Skipping skill path that resolves outside its configured root: ` + + `source=${params.source} root=${params.rootDir}${rootResolved} ` + + `requested=${params.candidatePath} resolved=${params.candidateRealPath}`, }); } @@ -192,6 +202,7 @@ function resolveContainedSkillPath(params: { warnEscapedSkillPath({ source: params.source, rootDir: params.rootDir, + rootRealPath: params.rootRealPath, candidatePath: path.resolve(params.candidatePath), candidateRealPath, });