Skip to content

Commit 659bcc5

Browse files
steipetesteipete
committed
fix: tighten codex app-server lifecycle
1 parent 485f416 commit 659bcc5

8 files changed

Lines changed: 141 additions & 42 deletions

File tree

‎extensions/codex/provider.test.ts‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
22
import { buildCodexProvider, buildCodexProviderCatalog } from "./provider.js";
33
import { CodexAppServerClient } from "./src/app-server/client.js";
4-
import { resetSharedCodexAppServerClientForTests } from "./src/app-server/shared-client.js";
4+
import {
5+
getSharedCodexAppServerClient,
6+
resetSharedCodexAppServerClientForTests,
7+
} from "./src/app-server/shared-client.js";
58

69
afterEach(() => {
710
resetSharedCodexAppServerClientForTests();
@@ -37,7 +40,7 @@ describe("codex provider", () => {
3740
});
3841

3942
expect(listModels).toHaveBeenCalledWith(
40-
expect.objectContaining({ limit: 100, timeoutMs: 1234 }),
43+
expect.objectContaining({ limit: 100, timeoutMs: 1234, sharedClient: false }),
4144
);
4245
expect(result.provider).toMatchObject({
4346
auth: "token",
@@ -103,6 +106,32 @@ describe("codex provider", () => {
103106
expect(client.close).toHaveBeenCalledTimes(1);
104107
});
105108

109+
it("does not close an active shared app-server client during live discovery", async () => {
110+
const activeClient = {
111+
initialize: vi.fn(async () => undefined),
112+
request: vi.fn(async () => ({ data: [] })),
113+
addCloseHandler: vi.fn(() => () => undefined),
114+
close: vi.fn(),
115+
} as unknown as CodexAppServerClient;
116+
const discoveryClient = {
117+
initialize: vi.fn(async () => undefined),
118+
request: vi.fn(async () => ({ data: [] })),
119+
addCloseHandler: vi.fn(() => () => undefined),
120+
close: vi.fn(),
121+
} as unknown as CodexAppServerClient;
122+
vi.spyOn(CodexAppServerClient, "start")
123+
.mockReturnValueOnce(activeClient)
124+
.mockReturnValueOnce(discoveryClient);
125+
126+
await getSharedCodexAppServerClient({ timeoutMs: 1000 });
127+
await buildCodexProviderCatalog({
128+
env: { OPENCLAW_CODEX_DISCOVERY_LIVE: "1" },
129+
});
130+
131+
expect(activeClient.close).not.toHaveBeenCalled();
132+
expect(discoveryClient.close).toHaveBeenCalledTimes(1);
133+
});
134+
106135
it("resolves arbitrary Codex app-server model ids through the codex provider", () => {
107136
const provider = buildCodexProvider();
108137

‎extensions/codex/provider.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import {
1515
readCodexPluginConfig,
1616
resolveCodexAppServerRuntimeOptions,
1717
} from "./src/app-server/config.js";
18-
import { clearSharedCodexAppServerClient } from "./src/app-server/shared-client.js";
1918

2019
const PROVIDER_ID = "codex";
2120
const CODEX_BASE_URL = "https://chatgpt.com/backend-api";
@@ -28,6 +27,7 @@ type CodexModelLister = (options: {
2827
timeoutMs: number;
2928
limit?: number;
3029
startOptions?: CodexAppServerStartOptions;
30+
sharedClient?: boolean;
3131
}) => Promise<CodexAppServerModelListResult>;
3232

3333
type BuildCodexProviderOptions = {
@@ -102,15 +102,11 @@ export async function buildCodexProviderCatalog(
102102
const timeoutMs = normalizeTimeoutMs(config.discovery?.timeoutMs);
103103
let discovered: CodexAppServerModel[] = [];
104104
if (config.discovery?.enabled !== false && !shouldSkipLiveDiscovery(options.env)) {
105-
try {
106-
discovered = await listModelsBestEffort({
107-
listModels: options.listModels ?? listCodexAppServerModels,
108-
timeoutMs,
109-
startOptions: appServer.start,
110-
});
111-
} finally {
112-
clearSharedCodexAppServerClient();
113-
}
105+
discovered = await listModelsBestEffort({
106+
listModels: options.listModels ?? listCodexAppServerModels,
107+
timeoutMs,
108+
startOptions: appServer.start,
109+
});
114110
}
115111
const models = (discovered.length > 0 ? discovered : FALLBACK_CODEX_MODELS).map(
116112
codexModelToDefinition,
@@ -180,6 +176,7 @@ async function listModelsBestEffort(params: {
180176
timeoutMs: params.timeoutMs,
181177
limit: 100,
182178
startOptions: params.startOptions,
179+
sharedClient: false,
183180
});
184181
return result.models.filter((model) => !model.hidden);
185182
} catch {

‎extensions/codex/src/app-server/models.ts‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import type { CodexAppServerStartOptions } from "./config.js";
22
import { type JsonObject, type JsonValue } from "./protocol.js";
3-
import { getSharedCodexAppServerClient } from "./shared-client.js";
4-
import { withTimeout } from "./timeout.js";
3+
import {
4+
createIsolatedCodexAppServerClient,
5+
getSharedCodexAppServerClient,
6+
} from "./shared-client.js";
57

68
export type CodexAppServerModel = {
79
id: string;
@@ -26,32 +28,39 @@ export type CodexAppServerListModelsOptions = {
2628
includeHidden?: boolean;
2729
timeoutMs?: number;
2830
startOptions?: CodexAppServerStartOptions;
31+
sharedClient?: boolean;
2932
};
3033

3134
export async function listCodexAppServerModels(
3235
options: CodexAppServerListModelsOptions = {},
3336
): Promise<CodexAppServerModelListResult> {
3437
const timeoutMs = options.timeoutMs ?? 2500;
35-
return await withTimeout(
36-
(async () => {
37-
const client = await getSharedCodexAppServerClient({
38+
const useSharedClient = options.sharedClient !== false;
39+
const client = useSharedClient
40+
? await getSharedCodexAppServerClient({
41+
startOptions: options.startOptions,
42+
timeoutMs,
43+
})
44+
: await createIsolatedCodexAppServerClient({
3845
startOptions: options.startOptions,
3946
timeoutMs,
4047
});
41-
const response = await client.request<JsonObject>(
42-
"model/list",
43-
{
44-
limit: options.limit ?? null,
45-
cursor: options.cursor ?? null,
46-
includeHidden: options.includeHidden ?? null,
47-
},
48-
{ timeoutMs },
49-
);
50-
return readModelListResult(response);
51-
})(),
52-
timeoutMs,
53-
"codex app-server model/list timed out",
54-
);
48+
try {
49+
const response = await client.request<JsonObject>(
50+
"model/list",
51+
{
52+
limit: options.limit ?? null,
53+
cursor: options.cursor ?? null,
54+
includeHidden: options.includeHidden ?? null,
55+
},
56+
{ timeoutMs },
57+
);
58+
return readModelListResult(response);
59+
} finally {
60+
if (!useSharedClient) {
61+
client.close();
62+
}
63+
}
5564
}
5665

5766
function readModelListResult(value: JsonValue | undefined): CodexAppServerModelListResult {

‎extensions/codex/src/app-server/run-attempt.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -227,14 +227,10 @@ export async function runCodexAppServerAttempt(
227227
);
228228

229229
const abortListener = () => {
230-
void client
231-
.request("turn/interrupt", {
232-
threadId: thread.threadId,
233-
turnId: activeTurnId,
234-
})
235-
.catch((error: unknown) => {
236-
embeddedAgentLog.debug("codex app-server turn interrupt failed during abort", { error });
237-
});
230+
interruptCodexTurnBestEffort(client, {
231+
threadId: thread.threadId,
232+
turnId: activeTurnId,
233+
});
238234
resolveCompletion?.();
239235
};
240236
runAbortController.signal.addEventListener("abort", abortListener, { once: true });
@@ -268,6 +264,20 @@ export async function runCodexAppServerAttempt(
268264
}
269265
}
270266

267+
function interruptCodexTurnBestEffort(
268+
client: CodexAppServerClient,
269+
params: {
270+
threadId: string;
271+
turnId: string;
272+
},
273+
): void {
274+
void Promise.resolve()
275+
.then(() => client.request("turn/interrupt", params))
276+
.catch((error: unknown) => {
277+
embeddedAgentLog.debug("codex app-server turn interrupt failed during abort", { error });
278+
});
279+
}
280+
271281
type DynamicToolBuildParams = {
272282
params: EmbeddedRunAttemptParams;
273283
resolvedWorkspace: string;

‎extensions/codex/src/app-server/shared-client.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,23 @@ export async function getSharedCodexAppServerClient(options?: {
5959
}
6060
}
6161

62+
export async function createIsolatedCodexAppServerClient(options?: {
63+
startOptions?: CodexAppServerStartOptions;
64+
timeoutMs?: number;
65+
}): Promise<CodexAppServerClient> {
66+
const startOptions = options?.startOptions ?? resolveCodexAppServerRuntimeOptions().start;
67+
const client = CodexAppServerClient.start(startOptions);
68+
const initialize = client.initialize();
69+
try {
70+
await withTimeout(initialize, options?.timeoutMs ?? 0, "codex app-server initialize timed out");
71+
return client;
72+
} catch (error) {
73+
client.close();
74+
await initialize.catch(() => undefined);
75+
throw error;
76+
}
77+
}
78+
6279
export function resetSharedCodexAppServerClientForTests(): void {
6380
const state = getSharedCodexAppServerClientState();
6481
state.client = undefined;

‎src/agents/model-fallback-observation.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ export function logModelFallbackDecision(params: {
5757
const reasonText = params.reason ?? "unknown";
5858
const observedError = buildErrorObservationFields(params.error);
5959
const detailText = observedError.providerErrorMessagePreview ?? observedError.errorPreview;
60+
const providerErrorTypeSuffix = observedError.providerErrorType
61+
? ` providerErrorType=${sanitizeForLog(observedError.providerErrorType)}`
62+
: "";
6063
const detailSuffix = detailText ? ` detail=${sanitizeForLog(detailText)}` : "";
6164
decisionLog.warn("model fallback decision", {
6265
event: "model_fallback_decision",
@@ -90,6 +93,6 @@ export function logModelFallbackDecision(params: {
9093
})),
9194
consoleMessage:
9295
`model fallback decision: decision=${params.decision} requested=${sanitizeForLog(params.requestedProvider)}/${sanitizeForLog(params.requestedModel)} ` +
93-
`candidate=${sanitizeForLog(params.candidate.provider)}/${sanitizeForLog(params.candidate.model)} reason=${reasonText} next=${nextText}${detailSuffix}`,
96+
`candidate=${sanitizeForLog(params.candidate.provider)}/${sanitizeForLog(params.candidate.model)} reason=${reasonText}${providerErrorTypeSuffix} next=${nextText}${detailSuffix}`,
9497
});
9598
}

‎src/agents/skills.loadworkspaceskillentries.test.ts‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import fs from "node:fs/promises";
22
import os from "node:os";
33
import path from "node:path";
4-
import { afterEach, describe, expect, it } from "vitest";
4+
import { afterEach, describe, expect, it, vi } from "vitest";
5+
import { resetLogger, setLoggerOverride } from "../logging/logger.js";
6+
import { loggingState } from "../logging/state.js";
57
import { withPathResolutionEnv } from "../test-utils/env.js";
68
import { writeSkill } from "./skills.e2e-test-helpers.js";
79
import { loadWorkspaceSkillEntries } from "./skills.js";
@@ -21,6 +23,9 @@ function withWorkspaceHome<T>(workspaceDir: string, cb: () => T): T {
2123
}
2224

2325
afterEach(async () => {
26+
setLoggerOverride(null);
27+
loggingState.rawConsole = null;
28+
resetLogger();
2429
await Promise.all(
2530
tempDirs.splice(0, tempDirs.length).map((dir) => fs.rm(dir, { recursive: true, force: true })),
2631
);
@@ -282,14 +287,32 @@ describe("loadWorkspaceSkillEntries", () => {
282287
description: "Outside",
283288
});
284289
await fs.mkdir(path.join(workspaceDir, "skills"), { recursive: true });
285-
await fs.symlink(escapedSkillDir, path.join(workspaceDir, "skills", "escaped-skill"), "dir");
290+
const requestedPath = path.join(workspaceDir, "skills", "escaped-skill");
291+
await fs.symlink(escapedSkillDir, requestedPath, "dir");
292+
setLoggerOverride({ level: "silent", consoleLevel: "warn" });
293+
const warn = vi.fn();
294+
loggingState.rawConsole = {
295+
log: vi.fn(),
296+
info: vi.fn(),
297+
warn,
298+
error: vi.fn(),
299+
};
286300

287301
const entries = loadWorkspaceSkillEntries(workspaceDir, {
288302
managedSkillsDir: path.join(workspaceDir, ".managed"),
289303
bundledSkillsDir: path.join(workspaceDir, ".bundled"),
290304
});
291305

292306
expect(entries.map((entry) => entry.skill.name)).not.toContain("outside-skill");
307+
const [line] = warn.mock.calls[0] ?? [];
308+
const warningLine = String(line);
309+
expect(warningLine).toContain(
310+
"Skipping skill path that resolves outside its configured root:",
311+
);
312+
expect(warningLine).toContain("source=openclaw-workspace");
313+
expect(warningLine).toContain(`root=${path.join(workspaceDir, "skills")}`);
314+
expect(warningLine).toContain(`requested=${requestedPath}`);
315+
expect(warningLine).toContain("resolved=");
293316
},
294317
);
295318

‎src/agents/skills/workspace.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,14 +165,24 @@ function tryRealpath(filePath: string): string | null {
165165
function warnEscapedSkillPath(params: {
166166
source: string;
167167
rootDir: string;
168+
rootRealPath: string;
168169
candidatePath: string;
169170
candidateRealPath: string;
170171
}) {
172+
const rootResolved =
173+
path.resolve(params.rootDir) === params.rootRealPath
174+
? ""
175+
: ` rootResolved=${params.rootRealPath}`;
171176
skillsLogger.warn("Skipping skill path that resolves outside its configured root.", {
172177
source: params.source,
173178
rootDir: params.rootDir,
179+
rootRealPath: params.rootRealPath,
174180
path: params.candidatePath,
175181
realPath: params.candidateRealPath,
182+
consoleMessage:
183+
`Skipping skill path that resolves outside its configured root: ` +
184+
`source=${params.source} root=${params.rootDir}${rootResolved} ` +
185+
`requested=${params.candidatePath} resolved=${params.candidateRealPath}`,
176186
});
177187
}
178188

@@ -192,6 +202,7 @@ function resolveContainedSkillPath(params: {
192202
warnEscapedSkillPath({
193203
source: params.source,
194204
rootDir: params.rootDir,
205+
rootRealPath: params.rootRealPath,
195206
candidatePath: path.resolve(params.candidatePath),
196207
candidateRealPath,
197208
});

0 commit comments

Comments
 (0)