From d72cc7a380e53710c9e38f147584da88ccda06fe Mon Sep 17 00:00:00 2001 From: Nyanako <44753291+Nanako0129@users.noreply.github.com> Date: Thu, 26 Mar 2026 14:58:17 +0800 Subject: [PATCH] fix: route codex responses over websocket and preserve tool warnings (#53702) (thanks @Nanako0129) * fix: route codex responses over websocket and suppress gated core tool warnings * fix: rebase codex websocket patch onto main * fix: preserve explicit alsoAllow warnings (#53702) (thanks @Nanako0129) --------- Co-authored-by: Ayaan Zaidi --- .../attempt.spawn-workspace.test-support.ts | 2 +- .../attempt.spawn-workspace.websocket.test.ts | 43 +++++++++++++++ .../run/attempt.thread-helpers.ts | 10 ++++ src/agents/pi-embedded-runner/run/attempt.ts | 8 ++- src/agents/pi-tools.ts | 2 + src/agents/tool-policy-pipeline.test.ts | 52 +++++++++++++++++-- src/agents/tool-policy-pipeline.ts | 47 ++++++++++++++--- src/gateway/tools-invoke-http.ts | 2 + 8 files changed, 153 insertions(+), 13 deletions(-) create mode 100644 src/agents/pi-embedded-runner/run/attempt.spawn-workspace.websocket.test.ts diff --git a/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts b/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts index 82be4d8e648..6748b101859 100644 --- a/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts +++ b/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.test-support.ts @@ -425,7 +425,7 @@ let runEmbeddedAttemptPromise: | Promise | undefined; -export async function loadRunEmbeddedAttempt() { +async function loadRunEmbeddedAttempt() { runEmbeddedAttemptPromise ??= import("./attempt.js").then((mod) => mod.runEmbeddedAttempt); return await runEmbeddedAttemptPromise; } diff --git a/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.websocket.test.ts b/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.websocket.test.ts new file mode 100644 index 00000000000..d8b09eb14bf --- /dev/null +++ b/src/agents/pi-embedded-runner/run/attempt.spawn-workspace.websocket.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; +import { shouldUseOpenAIWebSocketTransport } from "./attempt.thread-helpers.js"; + +describe("openai websocket transport selection", () => { + it("accepts the direct OpenAI responses transport pair", () => { + expect( + shouldUseOpenAIWebSocketTransport({ + provider: "openai", + modelApi: "openai-responses", + }), + ).toBe(true); + }); + + it("accepts the Codex responses transport pair", () => { + expect( + shouldUseOpenAIWebSocketTransport({ + provider: "openai-codex", + modelApi: "openai-codex-responses", + }), + ).toBe(true); + }); + + it("rejects mismatched OpenAI websocket transport pairs", () => { + expect( + shouldUseOpenAIWebSocketTransport({ + provider: "openai", + modelApi: "openai-codex-responses", + }), + ).toBe(false); + expect( + shouldUseOpenAIWebSocketTransport({ + provider: "openai-codex", + modelApi: "openai-responses", + }), + ).toBe(false); + expect( + shouldUseOpenAIWebSocketTransport({ + provider: "anthropic", + modelApi: "openai-responses", + }), + ).toBe(false); + }); +}); diff --git a/src/agents/pi-embedded-runner/run/attempt.thread-helpers.ts b/src/agents/pi-embedded-runner/run/attempt.thread-helpers.ts index 1db0d9ed5c3..c37c78934bc 100644 --- a/src/agents/pi-embedded-runner/run/attempt.thread-helpers.ts +++ b/src/agents/pi-embedded-runner/run/attempt.thread-helpers.ts @@ -31,6 +31,16 @@ export function resolveAttemptSpawnWorkspaceDir(params: { : undefined; } +export function shouldUseOpenAIWebSocketTransport(params: { + provider: string; + modelApi?: string | null; +}): boolean { + return ( + (params.modelApi === "openai-responses" && params.provider === "openai") || + (params.modelApi === "openai-codex-responses" && params.provider === "openai-codex") + ); +} + export function shouldAppendAttemptCacheTtl(params: { timedOutDuringCompaction: boolean; compactionOccurredThisAttempt: boolean; diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index c99110a4b3c..06371413504 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -148,6 +148,7 @@ import { appendAttemptCacheTtlIfNeeded, composeSystemPromptWithHookContext, resolveAttemptSpawnWorkspaceDir, + shouldUseOpenAIWebSocketTransport, } from "./attempt.thread-helpers.js"; import { waitForCompactionRetryWithAggregateTimeout } from "./compaction-retry-aggregate-timeout.js"; import { @@ -2247,7 +2248,12 @@ export async function runEmbeddedAttempt( }); activeSession.agent.streamFn = ollamaStreamFn; ensureCustomApiRegistered(params.model.api, ollamaStreamFn); - } else if (params.model.api === "openai-responses" && params.provider === "openai") { + } else if ( + shouldUseOpenAIWebSocketTransport({ + provider: params.provider, + modelApi: params.model.api, + }) + ) { const wsApiKey = await params.authStorage.getApiKey(params.provider); if (wsApiKey) { activeSession.agent.streamFn = createOpenAIWebSocketStreamFn(wsApiKey, params.sessionId, { diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 33576ea1f3b..8d16675788f 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -589,8 +589,10 @@ export function createOpenClawCodingTools(options?: { ...buildDefaultToolPolicyPipelineSteps({ profilePolicy: profilePolicyWithAlsoAllow, profile, + profileAlsoAllow, providerProfilePolicy: providerProfilePolicyWithAlsoAllow, providerProfile, + providerProfileAlsoAllow, globalPolicy, globalProviderPolicy, agentPolicy, diff --git a/src/agents/tool-policy-pipeline.test.ts b/src/agents/tool-policy-pipeline.test.ts index 57ccac35bc1..94d8d8da1b7 100644 --- a/src/agents/tool-policy-pipeline.test.ts +++ b/src/agents/tool-policy-pipeline.test.ts @@ -52,7 +52,7 @@ describe("tool-policy-pipeline", () => { expect(warnings[0]).toContain("unknown entries (wat)"); }); - test("warns gated core tools as unavailable instead of plugin-only unknowns", () => { + test("suppresses built-in profile warnings for unavailable gated core tools", () => { const warnings: string[] = []; const tools = [{ name: "exec" }] as unknown as DummyTool[]; applyToolPolicyPipeline({ @@ -66,6 +66,52 @@ describe("tool-policy-pipeline", () => { policy: { allow: ["apply_patch"] }, label: "tools.profile (coding)", stripPluginOnlyAllowlist: true, + suppressUnavailableCoreToolWarning: true, + }, + ], + }); + expect(warnings).toEqual([]); + }); + + test("still warns for profile steps when explicit alsoAllow entries are present", () => { + const warnings: string[] = []; + const tools = [{ name: "exec" }] as unknown as DummyTool[]; + applyToolPolicyPipeline({ + // oxlint-disable-next-line typescript/no-explicit-any + tools: tools as any, + // oxlint-disable-next-line typescript/no-explicit-any + toolMeta: () => undefined, + warn: (msg) => warnings.push(msg), + steps: [ + { + policy: { allow: ["apply_patch"] }, + label: "tools.profile (coding)", + stripPluginOnlyAllowlist: true, + suppressUnavailableCoreToolWarning: false, + }, + ], + }); + expect(warnings.length).toBe(1); + expect(warnings[0]).toContain("unknown entries (apply_patch)"); + expect(warnings[0]).toContain( + "shipped core tools but unavailable in the current runtime/provider/model/config", + ); + }); + + test("still warns for explicit allowlists that mention unavailable gated core tools", () => { + const warnings: string[] = []; + const tools = [{ name: "exec" }] as unknown as DummyTool[]; + applyToolPolicyPipeline({ + // oxlint-disable-next-line typescript/no-explicit-any + tools: tools as any, + // oxlint-disable-next-line typescript/no-explicit-any + toolMeta: () => undefined, + warn: (msg) => warnings.push(msg), + steps: [ + { + policy: { allow: ["apply_patch"] }, + label: "tools.allow", + stripPluginOnlyAllowlist: true, }, ], }); @@ -88,8 +134,8 @@ describe("tool-policy-pipeline", () => { warn: (msg: string) => warnings.push(msg), steps: [ { - policy: { allow: ["apply_patch"] }, - label: "tools.profile (coding)", + policy: { allow: ["wat"] }, + label: "tools.allow", stripPluginOnlyAllowlist: true, }, ], diff --git a/src/agents/tool-policy-pipeline.ts b/src/agents/tool-policy-pipeline.ts index b384da31b42..c53d26038b4 100644 --- a/src/agents/tool-policy-pipeline.ts +++ b/src/agents/tool-policy-pipeline.ts @@ -30,13 +30,16 @@ export type ToolPolicyPipelineStep = { policy: ToolPolicyLike | undefined; label: string; stripPluginOnlyAllowlist?: boolean; + suppressUnavailableCoreToolWarning?: boolean; }; export function buildDefaultToolPolicyPipelineSteps(params: { profilePolicy?: ToolPolicyLike; profile?: string; + profileAlsoAllow?: string[]; providerProfilePolicy?: ToolPolicyLike; providerProfile?: string; + providerProfileAlsoAllow?: string[]; globalPolicy?: ToolPolicyLike; globalProviderPolicy?: ToolPolicyLike; agentPolicy?: ToolPolicyLike; @@ -52,6 +55,8 @@ export function buildDefaultToolPolicyPipelineSteps(params: { policy: params.profilePolicy, label: profile ? `tools.profile (${profile})` : "tools.profile", stripPluginOnlyAllowlist: true, + suppressUnavailableCoreToolWarning: + !Array.isArray(params.profileAlsoAllow) || params.profileAlsoAllow.length === 0, }, { policy: params.providerProfilePolicy, @@ -59,6 +64,9 @@ export function buildDefaultToolPolicyPipelineSteps(params: { ? `tools.byProvider.profile (${providerProfile})` : "tools.byProvider.profile", stripPluginOnlyAllowlist: true, + suppressUnavailableCoreToolWarning: + !Array.isArray(params.providerProfileAlsoAllow) || + params.providerProfileAlsoAllow.length === 0, }, { policy: params.globalPolicy, label: "tools.allow", stripPluginOnlyAllowlist: true }, { @@ -113,14 +121,22 @@ export function applyToolPolicyPipeline(params: { isKnownCoreToolId(entry), ); const otherEntries = resolved.unknownAllowlist.filter((entry) => !isKnownCoreToolId(entry)); - const suffix = describeUnknownAllowlistSuffix({ - strippedAllowlist: resolved.strippedAllowlist, - hasGatedCoreEntries: gatedCoreEntries.length > 0, - hasOtherEntries: otherEntries.length > 0, - }); - const warning = `tools: ${step.label} allowlist contains unknown entries (${entries}). ${suffix}`; - if (rememberToolPolicyWarning(warning)) { - params.warn(warning); + if ( + !shouldSuppressUnavailableCoreToolWarning({ + suppressUnavailableCoreToolWarning: step.suppressUnavailableCoreToolWarning === true, + hasGatedCoreEntries: gatedCoreEntries.length > 0, + hasOtherEntries: otherEntries.length > 0, + }) + ) { + const suffix = describeUnknownAllowlistSuffix({ + strippedAllowlist: resolved.strippedAllowlist, + hasGatedCoreEntries: gatedCoreEntries.length > 0, + hasOtherEntries: otherEntries.length > 0, + }); + const warning = `tools: ${step.label} allowlist contains unknown entries (${entries}). ${suffix}`; + if (rememberToolPolicyWarning(warning)) { + params.warn(warning); + } } } policy = resolved.policy; @@ -132,6 +148,21 @@ export function applyToolPolicyPipeline(params: { return filtered; } +function shouldSuppressUnavailableCoreToolWarning(params: { + suppressUnavailableCoreToolWarning: boolean; + hasGatedCoreEntries: boolean; + hasOtherEntries: boolean; +}): boolean { + if ( + !params.suppressUnavailableCoreToolWarning || + !params.hasGatedCoreEntries || + params.hasOtherEntries + ) { + return false; + } + return true; +} + function describeUnknownAllowlistSuffix(params: { strippedAllowlist: boolean; hasGatedCoreEntries: boolean; diff --git a/src/gateway/tools-invoke-http.ts b/src/gateway/tools-invoke-http.ts index a67aa3b38a3..cadf20c9c16 100644 --- a/src/gateway/tools-invoke-http.ts +++ b/src/gateway/tools-invoke-http.ts @@ -278,8 +278,10 @@ export async function handleToolsInvokeHttpRequest( ...buildDefaultToolPolicyPipelineSteps({ profilePolicy: profilePolicyWithAlsoAllow, profile, + profileAlsoAllow, providerProfilePolicy: providerProfilePolicyWithAlsoAllow, providerProfile, + providerProfileAlsoAllow, globalPolicy, globalProviderPolicy, agentPolicy,