From e28b516fb5b3557097a1ec3676c81682c3381196 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Tue, 24 Mar 2026 15:03:40 -0700 Subject: [PATCH] fix(slack): trim DM reply overhead and restore Codex auto transport (#53957) * perf(slack): instrument runtime and trim DM overhead * perf(slack): lazy-init draft previews * perf(slack): add turn summary diagnostics * perf(core): trim repeated runtime setup noise * perf(core): preselect default web search providers * perf(agent): restore OpenAI auto transport defaults * refactor(slack): drop temporary perf wiring * fix(slack): address follow-up review notes * fix(security): tighten slack and runtime defaults * style(web-search): fix import ordering * style(agent): remove useless spread fallback * docs(changelog): note slack runtime hardening --- CHANGELOG.md | 1 + .../dispatch.streaming.test.ts | 84 ++++++++++++- .../src/monitor/message-handler/dispatch.ts | 108 ++++++++++------ extensions/slack/src/send.ts | 46 ++++++- extensions/slack/src/send.upload.test.ts | 42 ++++++- .../pi-embedded-runner-extraparams.test.ts | 73 ++++++++++- src/agents/pi-embedded-runner.ts | 7 +- src/agents/pi-embedded-runner/extra-params.ts | 116 +++++++++++++++--- src/agents/pi-embedded-runner/run/attempt.ts | 15 ++- src/agents/tool-policy-pipeline.test.ts | 74 ++++++++++- src/agents/tool-policy-pipeline.ts | 28 ++++- src/secrets/runtime-web-tools.test.ts | 58 ++++++++- src/secrets/runtime-web-tools.ts | 21 ++-- src/web-search/runtime.test.ts | 90 ++++++++++++++ src/web-search/runtime.ts | 8 +- 15 files changed, 684 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76f1b15121d..54532e5c00b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ Docs: https://docs.openclaw.ai - Feishu/startup: treat unresolved `SecretRef` app credentials as not configured during account resolution so CLI startup and read-only Feishu config surfaces stop crashing before runtime-backed secret resolution is available. (#53675) Thanks @hpt. - WhatsApp/groups: track recent gateway-sent message IDs and suppress only matching group echoes, preserving owner `/status`, `/new`, and `/activation` commands from linked-account `fromMe` traffic. (#53624) Thanks @w-sss. - Runtime/build: stabilize long-lived lazy `dist` runtime entry paths and harden bundled plugin npm staging so local rebuilds stop breaking on missing hashed chunks or broken shell `npm` shims. (#53855) Thanks @vincentkoc. +- Slack/runtime defaults: trim Slack DM reply overhead, restore Codex auto transport, and tighten Slack/web-search runtime defaults around DM preview threading, cache scoping, warning dedupe, and explicit web-search opt-in. (#53957) Thanks @vincentkoc. - Discord/timeouts: send a visible timeout reply when the inbound Discord worker times out before a final reply starts, including created auto-thread targets and queued-run ordering. (#53823) Thanks @Kimbo7870. ## 2026.3.23 diff --git a/extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts b/extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts index dc6eae7a44d..d5588562dee 100644 --- a/extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts +++ b/extensions/slack/src/monitor/message-handler/dispatch.streaming.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from "vitest"; -import { isSlackStreamingEnabled, resolveSlackStreamingThreadHint } from "./dispatch.js"; +import { + isSlackStreamingEnabled, + resolveSlackStreamingThreadHint, + shouldEnableSlackPreviewStreaming, + shouldInitializeSlackDraftStream, +} from "./dispatch.js"; describe("slack native streaming defaults", () => { it("is enabled for partial mode when native streaming is on", () => { @@ -45,3 +50,80 @@ describe("slack native streaming thread hint", () => { ).toBe("2000.1"); }); }); + +describe("slack preview streaming eligibility", () => { + it("stays on for room messages when streaming mode is enabled", () => { + expect( + shouldEnableSlackPreviewStreaming({ + mode: "partial", + isDirectMessage: false, + }), + ).toBe(true); + }); + + it("stays off for top-level DMs without a reply thread", () => { + expect( + shouldEnableSlackPreviewStreaming({ + mode: "partial", + isDirectMessage: true, + }), + ).toBe(false); + }); + + it("allows DM preview when the reply is threaded", () => { + expect( + shouldEnableSlackPreviewStreaming({ + mode: "partial", + isDirectMessage: true, + threadTs: "1000.1", + }), + ).toBe(true); + }); + + it("keeps top-level DMs off even when replyToMode would create a reply thread", () => { + const streamThreadHint = resolveSlackStreamingThreadHint({ + replyToMode: "all", + incomingThreadTs: undefined, + messageTs: "1000.4", + isThreadReply: false, + }); + + expect( + shouldEnableSlackPreviewStreaming({ + mode: "partial", + isDirectMessage: true, + threadTs: undefined, + }), + ).toBe(false); + expect(streamThreadHint).toBe("1000.4"); + }); +}); + +describe("slack draft stream initialization", () => { + it("stays off when preview streaming is disabled", () => { + expect( + shouldInitializeSlackDraftStream({ + previewStreamingEnabled: false, + useStreaming: false, + }), + ).toBe(false); + }); + + it("stays off when native streaming is active", () => { + expect( + shouldInitializeSlackDraftStream({ + previewStreamingEnabled: true, + useStreaming: true, + }), + ).toBe(false); + }); + + it("turns on only for preview-only paths", () => { + expect( + shouldInitializeSlackDraftStream({ + previewStreamingEnabled: true, + useStreaming: false, + }), + ).toBe(true); + }); +}); diff --git a/extensions/slack/src/monitor/message-handler/dispatch.ts b/extensions/slack/src/monitor/message-handler/dispatch.ts index f192b592b59..e09cbf922cd 100644 --- a/extensions/slack/src/monitor/message-handler/dispatch.ts +++ b/extensions/slack/src/monitor/message-handler/dispatch.ts @@ -50,6 +50,27 @@ export function isSlackStreamingEnabled(params: { return params.nativeStreaming; } +export function shouldEnableSlackPreviewStreaming(params: { + mode: "off" | "partial" | "block" | "progress"; + isDirectMessage: boolean; + threadTs?: string; +}): boolean { + if (params.mode === "off") { + return false; + } + if (!params.isDirectMessage) { + return true; + } + return Boolean(params.threadTs); +} + +export function shouldInitializeSlackDraftStream(params: { + previewStreamingEnabled: boolean; + useStreaming: boolean; +}): boolean { + return params.previewStreamingEnabled && !params.useStreaming; +} + export function resolveSlackStreamingThreadHint(params: { replyToMode: "off" | "first" | "all"; incomingThreadTs: string | undefined; @@ -213,21 +234,29 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag streamMode: account.config.streamMode, nativeStreaming: account.config.nativeStreaming, }); - const previewStreamingEnabled = slackStreaming.mode !== "off"; - const streamingEnabled = isSlackStreamingEnabled({ - mode: slackStreaming.mode, - nativeStreaming: slackStreaming.nativeStreaming, - }); const streamThreadHint = resolveSlackStreamingThreadHint({ replyToMode: prepared.replyToMode, incomingThreadTs, messageTs, isThreadReply, }); + const previewStreamingEnabled = shouldEnableSlackPreviewStreaming({ + mode: slackStreaming.mode, + isDirectMessage: prepared.isDirectMessage, + threadTs: streamThreadHint, + }); + const streamingEnabled = isSlackStreamingEnabled({ + mode: slackStreaming.mode, + nativeStreaming: slackStreaming.nativeStreaming, + }); const useStreaming = shouldUseStreaming({ streamingEnabled, threadTs: streamThreadHint, }); + const shouldUseDraftStream = shouldInitializeSlackDraftStream({ + previewStreamingEnabled, + useStreaming, + }); let streamSession: SlackStreamSession | null = null; let streamFailed = false; let usedReplyThreadTs: string | undefined; @@ -372,22 +401,24 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag }, }); - const draftStream = createSlackDraftStream({ - target: prepared.replyTarget, - token: ctx.botToken, - accountId: account.accountId, - maxChars: Math.min(ctx.textLimit, SLACK_TEXT_LIMIT), - resolveThreadTs: () => { - const ts = replyPlan.nextThreadTs(); - if (ts) { - usedReplyThreadTs ??= ts; - } - return ts; - }, - onMessageSent: () => replyPlan.markSent(), - log: logVerbose, - warn: logVerbose, - }); + const draftStream = shouldUseDraftStream + ? createSlackDraftStream({ + target: prepared.replyTarget, + token: ctx.botToken, + accountId: account.accountId, + maxChars: Math.min(ctx.textLimit, SLACK_TEXT_LIMIT), + resolveThreadTs: () => { + const ts = replyPlan.nextThreadTs(); + if (ts) { + usedReplyThreadTs ??= ts; + } + return ts; + }, + onMessageSent: () => replyPlan.markSent(), + log: logVerbose, + warn: logVerbose, + }) + : undefined; let hasStreamedMessage = false; const streamMode = slackStreaming.draftMode; let appendRenderedText = ""; @@ -410,7 +441,7 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag if (!next.changed) { return; } - draftStream.update(next.rendered); + draftStream?.update(next.rendered); hasStreamedMessage = true; return; } @@ -420,26 +451,25 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag if (statusUpdateCount > 1 && statusUpdateCount % 4 !== 0) { return; } - draftStream.update(buildStatusFinalPreviewText(statusUpdateCount)); + draftStream?.update(buildStatusFinalPreviewText(statusUpdateCount)); hasStreamedMessage = true; return; } - draftStream.update(trimmed); + draftStream?.update(trimmed); hasStreamedMessage = true; }; - const onDraftBoundary = - useStreaming || !previewStreamingEnabled - ? undefined - : async () => { - if (hasStreamedMessage) { - draftStream.forceNewMessage(); - hasStreamedMessage = false; - appendRenderedText = ""; - appendSourceText = ""; - statusUpdateCount = 0; - } - }; + const onDraftBoundary = !shouldUseDraftStream + ? undefined + : async () => { + if (hasStreamedMessage) { + draftStream?.forceNewMessage(); + hasStreamedMessage = false; + appendRenderedText = ""; + appendSourceText = ""; + statusUpdateCount = 0; + } + }; const { queuedFinal, counts } = await dispatchInboundMessage({ ctx: prepared.ctxPayload, @@ -466,8 +496,8 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag onReasoningEnd: onDraftBoundary, }, }); - await draftStream.flush(); - draftStream.stop(); + await draftStream?.flush(); + draftStream?.stop(); markDispatchIdle(); // ----------------------------------------------------------------------- @@ -493,7 +523,7 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag } if (!anyReplyDelivered) { - await draftStream.clear(); + await draftStream?.clear(); if (prepared.isRoomish) { clearHistoryEntriesIfEnabled({ historyMap: ctx.channelHistories, diff --git a/extensions/slack/src/send.ts b/extensions/slack/src/send.ts index a33125f558b..d5e82a2aa16 100644 --- a/extensions/slack/src/send.ts +++ b/extensions/slack/src/send.ts @@ -27,6 +27,8 @@ const SLACK_UPLOAD_SSRF_POLICY = { allowedHostnames: ["*.slack.com", "*.slack-edge.com", "*.slack-files.com"], allowRfc2544BenchmarkRange: true, }; +const SLACK_DM_CHANNEL_CACHE_MAX = 1024; +const slackDmChannelCache = new Map(); type SlackRecipient = | { @@ -167,10 +169,31 @@ function parseRecipient(raw: string): SlackRecipient { return { kind: target.kind, id: target.id }; } +function createSlackDmCacheKey(params: { + accountId?: string; + token: string; + recipientId: string; +}): string { + return `${params.accountId ?? "default"}:${params.token}:${params.recipientId}`; +} + +function setSlackDmChannelCache(key: string, channelId: string): void { + if (slackDmChannelCache.has(key)) { + slackDmChannelCache.delete(key); + } else if (slackDmChannelCache.size >= SLACK_DM_CHANNEL_CACHE_MAX) { + const oldest = slackDmChannelCache.keys().next().value; + if (oldest) { + slackDmChannelCache.delete(oldest); + } + } + slackDmChannelCache.set(key, channelId); +} + async function resolveChannelId( client: WebClient, recipient: SlackRecipient, -): Promise<{ channelId: string; isDm?: boolean }> { + params: { accountId?: string; token: string }, +): Promise<{ channelId: string; isDm?: boolean; cacheHit?: boolean }> { // Bare Slack user IDs (U-prefix) may arrive with kind="channel" when the // target string had no explicit prefix (parseSlackTarget defaults bare IDs // to "channel"). chat.postMessage tolerates user IDs directly, but @@ -181,12 +204,26 @@ async function resolveChannelId( if (!isUserId) { return { channelId: recipient.id }; } + const cacheKey = createSlackDmCacheKey({ + accountId: params.accountId, + token: params.token, + recipientId: recipient.id, + }); + const cachedChannelId = slackDmChannelCache.get(cacheKey); + if (cachedChannelId) { + return { channelId: cachedChannelId, isDm: true, cacheHit: true }; + } const response = await client.conversations.open({ users: recipient.id }); const channelId = response.channel?.id; if (!channelId) { throw new Error("Failed to open Slack DM channel"); } - return { channelId, isDm: true }; + setSlackDmChannelCache(cacheKey, channelId); + return { channelId, isDm: true, cacheHit: false }; +} + +export function clearSlackDmChannelCache(): void { + slackDmChannelCache.clear(); } async function uploadSlackFile(params: { @@ -276,7 +313,10 @@ export async function sendMessageSlack( }); const client = opts.client ?? createSlackWebClient(token); const recipient = parseRecipient(to); - const { channelId } = await resolveChannelId(client, recipient); + const { channelId } = await resolveChannelId(client, recipient, { + accountId: account.accountId, + token, + }); if (blocks) { if (opts.mediaUrl) { throw new Error("Slack send does not support blocks with mediaUrl"); diff --git a/extensions/slack/src/send.upload.test.ts b/extensions/slack/src/send.upload.test.ts index 2a208a83836..fd255d687b9 100644 --- a/extensions/slack/src/send.upload.test.ts +++ b/extensions/slack/src/send.upload.test.ts @@ -32,6 +32,7 @@ vi.mock("openclaw/plugin-sdk/web-media", () => ({ })); let sendMessageSlack: typeof import("./send.js").sendMessageSlack; +let clearSlackDmChannelCache: typeof import("./send.js").clearSlackDmChannelCache; type UploadTestClient = WebClient & { conversations: { open: ReturnType }; @@ -66,7 +67,7 @@ describe("sendMessageSlack file upload with user IDs", () => { beforeAll(async () => { vi.resetModules(); - ({ sendMessageSlack } = await import("./send.js")); + ({ sendMessageSlack, clearSlackDmChannelCache } = await import("./send.js")); }); beforeEach(() => { @@ -74,6 +75,7 @@ describe("sendMessageSlack file upload with user IDs", () => { async () => new Response("ok", { status: 200 }), ) as unknown as typeof fetch; fetchWithSsrFGuard.mockClear(); + clearSlackDmChannelCache(); }); afterEach(() => { @@ -121,6 +123,44 @@ describe("sendMessageSlack file upload with user IDs", () => { ); }); + it("caches DM channel resolution per account", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("user:UABC123", "first", { + token: "xoxb-test", + client, + }); + await sendMessageSlack("user:UABC123", "second", { + token: "xoxb-test", + client, + }); + + expect(client.conversations.open).toHaveBeenCalledTimes(1); + expect(client.chat.postMessage).toHaveBeenCalledTimes(2); + expect(client.chat.postMessage).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + channel: "D99RESOLVED", + text: "second", + }), + ); + }); + + it("scopes DM channel resolution cache by token identity", async () => { + const client = createUploadTestClient(); + + await sendMessageSlack("user:UABC123", "first", { + token: "xoxb-test-a", + client, + }); + await sendMessageSlack("user:UABC123", "second", { + token: "xoxb-test-b", + client, + }); + + expect(client.conversations.open).toHaveBeenCalledTimes(2); + }); + it("sends file directly to channel without conversations.open", async () => { const client = createUploadTestClient(); diff --git a/src/agents/pi-embedded-runner-extraparams.test.ts b/src/agents/pi-embedded-runner-extraparams.test.ts index 87bfc7992bd..12fc28bdd44 100644 --- a/src/agents/pi-embedded-runner-extraparams.test.ts +++ b/src/agents/pi-embedded-runner-extraparams.test.ts @@ -30,7 +30,12 @@ const resolveProviderCapabilitiesWithPluginMock = vi.fn( }, ); -import { applyExtraParamsToAgent, resolveExtraParams } from "./pi-embedded-runner.js"; +import { + applyExtraParamsToAgent, + resolveAgentTransportOverride, + resolveExtraParams, + resolvePreparedExtraParams, +} from "./pi-embedded-runner.js"; import { log } from "./pi-embedded-runner/logger.js"; beforeEach(() => { @@ -1531,6 +1536,72 @@ describe("applyExtraParamsToAgent", () => { expect(calls[0]?.transport).toBe("auto"); }); + it("returns prepared Codex transport defaults for runtime sessions", () => { + const effectiveExtraParams = resolvePreparedExtraParams({ + cfg: undefined, + provider: "openai-codex", + modelId: "gpt-5.4", + }); + + expect(effectiveExtraParams.transport).toBe("auto"); + }); + + it("uses prepared transport when session settings did not explicitly set one", () => { + const effectiveExtraParams = resolvePreparedExtraParams({ + cfg: undefined, + provider: "openai-codex", + modelId: "gpt-5.4", + }); + + expect( + resolveAgentTransportOverride({ + settingsManager: { + getGlobalSettings: () => ({}), + getProjectSettings: () => ({}), + }, + effectiveExtraParams, + }), + ).toBe("auto"); + }); + + it("keeps explicit session transport over prepared OpenAI defaults", () => { + const effectiveExtraParams = resolvePreparedExtraParams({ + cfg: undefined, + provider: "openai", + modelId: "gpt-5", + }); + + expect( + resolveAgentTransportOverride({ + settingsManager: { + getGlobalSettings: () => ({ transport: "sse" }), + getProjectSettings: () => ({}), + }, + effectiveExtraParams, + }), + ).toBeUndefined(); + }); + + it("strips prototype pollution keys from extra params overrides", () => { + const effectiveExtraParams = resolvePreparedExtraParams({ + cfg: undefined, + provider: "openai", + modelId: "gpt-5", + extraParamsOverride: { + __proto__: { polluted: true }, + constructor: "blocked", + prototype: "blocked", + temperature: 0.2, + }, + }); + + expect(effectiveExtraParams.temperature).toBe(0.2); + expect(Object.hasOwn(effectiveExtraParams, "__proto__")).toBe(false); + expect(Object.hasOwn(effectiveExtraParams, "constructor")).toBe(false); + expect(Object.hasOwn(effectiveExtraParams, "prototype")).toBe(false); + expect(({} as { polluted?: boolean }).polluted).toBeUndefined(); + }); + it("disables prompt caching for non-Anthropic Bedrock models", () => { const { calls, agent } = createOptionsCaptureAgent(); diff --git a/src/agents/pi-embedded-runner.ts b/src/agents/pi-embedded-runner.ts index 4d968a9c2eb..c53964d51e0 100644 --- a/src/agents/pi-embedded-runner.ts +++ b/src/agents/pi-embedded-runner.ts @@ -1,6 +1,11 @@ export type { MessagingToolSend } from "./pi-embedded-messaging.js"; export { compactEmbeddedPiSession } from "./pi-embedded-runner/compact.js"; -export { applyExtraParamsToAgent, resolveExtraParams } from "./pi-embedded-runner/extra-params.js"; +export { + applyExtraParamsToAgent, + resolveAgentTransportOverride, + resolveExtraParams, + resolvePreparedExtraParams, +} from "./pi-embedded-runner/extra-params.js"; export { applyGoogleTurnOrderingFix } from "./pi-embedded-runner/google.js"; export { diff --git a/src/agents/pi-embedded-runner/extra-params.ts b/src/agents/pi-embedded-runner/extra-params.ts index 27dbeede797..33e19f3d467 100644 --- a/src/agents/pi-embedded-runner/extra-params.ts +++ b/src/agents/pi-embedded-runner/extra-params.ts @@ -1,6 +1,7 @@ import type { StreamFn } from "@mariozechner/pi-agent-core"; import type { SimpleStreamOptions } from "@mariozechner/pi-ai"; import { streamSimple } from "@mariozechner/pi-ai"; +import type { SettingsManager } from "@mariozechner/pi-coding-agent"; import type { ThinkLevel } from "../../auto-reply/thinking.js"; import type { OpenClawConfig } from "../../config/config.js"; import { @@ -105,6 +106,84 @@ type CacheRetentionStreamOptions = Partial & { cacheRetention?: "none" | "short" | "long"; openaiWsWarmup?: boolean; }; +type SupportedTransport = Exclude; + +function resolveSupportedTransport(value: unknown): SupportedTransport | undefined { + return value === "sse" || value === "websocket" || value === "auto" ? value : undefined; +} + +function hasExplicitTransportSetting(settings: { transport?: unknown }): boolean { + return Object.hasOwn(settings, "transport"); +} + +export function resolvePreparedExtraParams(params: { + cfg: OpenClawConfig | undefined; + provider: string; + modelId: string; + extraParamsOverride?: Record; + thinkingLevel?: ThinkLevel; + agentId?: string; + resolvedExtraParams?: Record; +}): Record { + const resolvedExtraParams = + params.resolvedExtraParams ?? + resolveExtraParams({ + cfg: params.cfg, + provider: params.provider, + modelId: params.modelId, + agentId: params.agentId, + }); + const override = + params.extraParamsOverride && Object.keys(params.extraParamsOverride).length > 0 + ? sanitizeExtraParamsRecord( + Object.fromEntries( + Object.entries(params.extraParamsOverride).filter(([, value]) => value !== undefined), + ), + ) + : undefined; + const merged = { + ...sanitizeExtraParamsRecord(resolvedExtraParams), + ...override, + }; + return ( + providerRuntimeDeps.prepareProviderExtraParams({ + provider: params.provider, + config: params.cfg, + context: { + config: params.cfg, + provider: params.provider, + modelId: params.modelId, + extraParams: merged, + thinkingLevel: params.thinkingLevel, + }, + }) ?? merged + ); +} + +function sanitizeExtraParamsRecord( + value: Record | undefined, +): Record | undefined { + if (!value) { + return undefined; + } + return Object.fromEntries( + Object.entries(value).filter( + ([key]) => key !== "__proto__" && key !== "prototype" && key !== "constructor", + ), + ); +} + +export function resolveAgentTransportOverride(params: { + settingsManager: Pick; + effectiveExtraParams: Record | undefined; +}): SupportedTransport | undefined { + const globalSettings = params.settingsManager.getGlobalSettings(); + const projectSettings = params.settingsManager.getProjectSettings(); + if (hasExplicitTransportSetting(globalSettings) || hasExplicitTransportSetting(projectSettings)) { + return undefined; + } + return resolveSupportedTransport(params.effectiveExtraParams?.transport); +} function createStreamFnWithExtraParams( baseStreamFn: StreamFn | undefined, @@ -122,11 +201,14 @@ function createStreamFnWithExtraParams( if (typeof extraParams.maxTokens === "number") { streamParams.maxTokens = extraParams.maxTokens; } - const transport = extraParams.transport; - if (transport === "sse" || transport === "websocket" || transport === "auto") { + const transport = resolveSupportedTransport(extraParams.transport); + if (transport) { streamParams.transport = transport; - } else if (transport != null) { - const transportSummary = typeof transport === "string" ? transport : typeof transport; + } else if (extraParams.transport != null) { + const transportSummary = + typeof extraParams.transport === "string" + ? extraParams.transport + : typeof extraParams.transport; log.warn(`ignoring invalid transport param: ${transportSummary}`); } if (typeof extraParams.openaiWsWarmup === "boolean") { @@ -216,7 +298,7 @@ export function applyExtraParamsToAgent( thinkingLevel?: ThinkLevel, agentId?: string, workspaceDir?: string, -): void { +): { effectiveExtraParams: Record } { const resolvedExtraParams = resolveExtraParams({ cfg, provider, @@ -229,19 +311,15 @@ export function applyExtraParamsToAgent( Object.entries(extraParamsOverride).filter(([, value]) => value !== undefined), ) : undefined; - const merged = Object.assign({}, resolvedExtraParams, override); - const effectiveExtraParams = - providerRuntimeDeps.prepareProviderExtraParams({ - provider, - config: cfg, - context: { - config: cfg, - provider, - modelId, - extraParams: merged, - thinkingLevel, - }, - }) ?? merged; + const effectiveExtraParams = resolvePreparedExtraParams({ + cfg, + provider, + modelId, + extraParamsOverride, + thinkingLevel, + agentId, + resolvedExtraParams, + }); if (provider === "openai" || provider === "openai-codex") { if (provider === "openai") { @@ -371,4 +449,6 @@ export function applyExtraParamsToAgent( log.warn(`ignoring invalid parallel_tool_calls param: ${summary}`); } } + + return { effectiveExtraParams }; } diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index af396207dd9..c99110a4b3c 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -108,7 +108,7 @@ import { buildEmbeddedCompactionRuntimeContext } from "../compaction-runtime-con import { resolveCompactionTimeoutMs } from "../compaction-safety-timeout.js"; import { runContextEngineMaintenance } from "../context-engine-maintenance.js"; import { buildEmbeddedExtensionFactories } from "../extensions.js"; -import { applyExtraParamsToAgent } from "../extra-params.js"; +import { applyExtraParamsToAgent, resolveAgentTransportOverride } from "../extra-params.js"; import { logToolSchemasForGoogle, sanitizeSessionHistory, @@ -2287,7 +2287,7 @@ export async function runEmbeddedAttempt( activeSession.agent.streamFn = wrapOllamaCompatNumCtx(activeSession.agent.streamFn, numCtx); } - applyExtraParamsToAgent( + const { effectiveExtraParams } = applyExtraParamsToAgent( activeSession.agent, params.config, params.provider, @@ -2300,6 +2300,17 @@ export async function runEmbeddedAttempt( sessionAgentId, effectiveWorkspace, ); + const agentTransportOverride = resolveAgentTransportOverride({ + settingsManager, + effectiveExtraParams, + }); + if (agentTransportOverride && activeSession.agent.transport !== agentTransportOverride) { + log.debug( + `embedded agent transport override: ${activeSession.agent.transport} -> ${agentTransportOverride} ` + + `(${params.provider}/${params.modelId})`, + ); + activeSession.agent.setTransport(agentTransportOverride); + } if (cacheTrace) { cacheTrace.recordStage("session:loaded", { diff --git a/src/agents/tool-policy-pipeline.test.ts b/src/agents/tool-policy-pipeline.test.ts index 70d4301d42a..57ccac35bc1 100644 --- a/src/agents/tool-policy-pipeline.test.ts +++ b/src/agents/tool-policy-pipeline.test.ts @@ -1,9 +1,16 @@ -import { describe, expect, test } from "vitest"; -import { applyToolPolicyPipeline } from "./tool-policy-pipeline.js"; +import { beforeEach, describe, expect, test } from "vitest"; +import { + applyToolPolicyPipeline, + resetToolPolicyWarningCacheForTest, +} from "./tool-policy-pipeline.js"; type DummyTool = { name: string }; describe("tool-policy-pipeline", () => { + beforeEach(() => { + resetToolPolicyWarningCacheForTest(); + }); + test("strips allowlists that would otherwise disable core tools", () => { const tools = [{ name: "exec" }, { name: "plugin_tool" }] as unknown as DummyTool[]; const filtered = applyToolPolicyPipeline({ @@ -70,6 +77,69 @@ describe("tool-policy-pipeline", () => { expect(warnings[0]).not.toContain("unless the plugin is enabled"); }); + test("dedupes identical unknown-allowlist warnings across repeated runs", () => { + const warnings: string[] = []; + const tools = [{ name: "exec" }] as unknown as DummyTool[]; + const params = { + // oxlint-disable-next-line typescript/no-explicit-any + tools: tools as any, + // oxlint-disable-next-line typescript/no-explicit-any + toolMeta: () => undefined, + warn: (msg: string) => warnings.push(msg), + steps: [ + { + policy: { allow: ["apply_patch"] }, + label: "tools.profile (coding)", + stripPluginOnlyAllowlist: true, + }, + ], + }; + + applyToolPolicyPipeline(params); + applyToolPolicyPipeline(params); + + expect(warnings).toHaveLength(1); + }); + + test("bounds the warning dedupe cache so new warnings still surface", () => { + const warnings: string[] = []; + const tools = [{ name: "exec" }] as unknown as DummyTool[]; + + for (let i = 0; i < 257; i += 1) { + 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: string) => warnings.push(msg), + steps: [ + { + policy: { allow: [`unknown_${i}`] }, + label: "tools.profile (coding)", + stripPluginOnlyAllowlist: true, + }, + ], + }); + } + + 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: string) => warnings.push(msg), + steps: [ + { + policy: { allow: ["unknown_0"] }, + label: "tools.profile (coding)", + stripPluginOnlyAllowlist: true, + }, + ], + }); + + expect(warnings).toHaveLength(258); + }); + test("applies allowlist filtering when core tools are explicitly listed", () => { const tools = [{ name: "exec" }, { name: "process" }] as unknown as DummyTool[]; const filtered = applyToolPolicyPipeline({ diff --git a/src/agents/tool-policy-pipeline.ts b/src/agents/tool-policy-pipeline.ts index 70a7bddaf29..b384da31b42 100644 --- a/src/agents/tool-policy-pipeline.ts +++ b/src/agents/tool-policy-pipeline.ts @@ -9,6 +9,23 @@ import { type ToolPolicyLike, } from "./tool-policy.js"; +const MAX_TOOL_POLICY_WARNING_CACHE = 256; +const seenToolPolicyWarnings = new Set(); + +function rememberToolPolicyWarning(warning: string): boolean { + if (seenToolPolicyWarnings.has(warning)) { + return false; + } + if (seenToolPolicyWarnings.size >= MAX_TOOL_POLICY_WARNING_CACHE) { + const oldest = seenToolPolicyWarnings.values().next().value; + if (oldest) { + seenToolPolicyWarnings.delete(oldest); + } + } + seenToolPolicyWarnings.add(warning); + return true; +} + export type ToolPolicyPipelineStep = { policy: ToolPolicyLike | undefined; label: string; @@ -101,9 +118,10 @@ export function applyToolPolicyPipeline(params: { hasGatedCoreEntries: gatedCoreEntries.length > 0, hasOtherEntries: otherEntries.length > 0, }); - params.warn( - `tools: ${step.label} allowlist contains unknown entries (${entries}). ${suffix}`, - ); + const warning = `tools: ${step.label} allowlist contains unknown entries (${entries}). ${suffix}`; + if (rememberToolPolicyWarning(warning)) { + params.warn(warning); + } } policy = resolved.policy; } @@ -130,3 +148,7 @@ function describeUnknownAllowlistSuffix(params: { : "These entries won't match any tool unless the plugin is enabled."; return preface ? `${preface} ${detail}` : detail; } + +export function resetToolPolicyWarningCacheForTest(): void { + seenToolPolicyWarnings.clear(); +} diff --git a/src/secrets/runtime-web-tools.test.ts b/src/secrets/runtime-web-tools.test.ts index 0f594f03ed6..d740d4d7c44 100644 --- a/src/secrets/runtime-web-tools.test.ts +++ b/src/secrets/runtime-web-tools.test.ts @@ -225,8 +225,11 @@ describe("runtime web tools resolution", () => { } }); - it("skips loading web search providers when search config is absent", async () => { - const providerSpy = vi.mocked(runtimeWebSearchProviders.resolvePluginWebSearchProviders); + it("keeps web search disabled when search config is absent", async () => { + const bundledProviderSpy = vi.mocked( + bundledWebSearchProviders.resolveBundledPluginWebSearchProviders, + ); + const runtimeProviderSpy = vi.mocked(runtimeWebSearchProviders.resolvePluginWebSearchProviders); const { metadata } = await runRuntimeWebTools({ config: asConfig({ @@ -245,7 +248,9 @@ describe("runtime web tools resolution", () => { }, }); - expect(providerSpy).not.toHaveBeenCalled(); + expect(bundledProviderSpy).not.toHaveBeenCalled(); + expect(runtimeProviderSpy).not.toHaveBeenCalled(); + expect(metadata.search.selectedProvider).toBeUndefined(); expect(metadata.search.providerSource).toBe("none"); expect(metadata.fetch.firecrawl.active).toBe(true); expect(metadata.fetch.firecrawl.apiKeySource).toBe("env"); @@ -681,6 +686,53 @@ describe("runtime web tools resolution", () => { expectInactiveFirecrawlSecretRef({ resolveSpy, metadata, context }); }); + it("keeps configured provider metadata and inactive warnings when search is disabled", async () => { + const { metadata, context } = await runRuntimeWebTools({ + config: asConfig({ + tools: { + web: { + search: { + enabled: false, + provider: "gemini", + }, + }, + }, + plugins: { + entries: { + google: { + enabled: true, + config: { + webSearch: { + apiKey: { source: "env", provider: "default", id: "GEMINI_PROVIDER_REF" }, + }, + }, + }, + }, + }, + }), + }); + + expect(metadata.search.providerConfigured).toBe("gemini"); + expect(metadata.search.providerSource).toBe("configured"); + expect(context.warnings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + code: "SECRETS_REF_IGNORED_INACTIVE_SURFACE", + path: "plugins.entries.google.config.webSearch.apiKey", + }), + ]), + ); + }); + + it("does not auto-enable search when tools.web.search is absent", async () => { + const { metadata } = await runRuntimeWebTools({ + config: asConfig({}), + }); + + expect(metadata.search.providerSource).toBe("none"); + expect(metadata.search.selectedProvider).toBeUndefined(); + }); + it("uses env fallback for unresolved Firecrawl SecretRef when active", async () => { const { metadata, resolvedConfig, context } = await runRuntimeWebTools({ config: asConfig({ diff --git a/src/secrets/runtime-web-tools.ts b/src/secrets/runtime-web-tools.ts index f46667fd6ac..c3b177c19b1 100644 --- a/src/secrets/runtime-web-tools.ts +++ b/src/secrets/runtime-web-tools.ts @@ -298,8 +298,16 @@ export async function resolveRuntimeWebTools(params: { const rawProvider = typeof search?.provider === "string" ? search.provider.trim().toLowerCase() : ""; const configuredBundledPluginId = resolveBundledWebSearchPluginId(rawProvider); + + const searchMetadata: RuntimeWebSearchMetadata = { + providerSource: "none", + diagnostics: [], + }; + + const searchConfigured = Boolean(search); + const searchEnabled = searchConfigured && search?.enabled !== false; const providers = sortWebSearchProvidersForAutoDetect( - search + searchConfigured ? configuredBundledPluginId ? resolveBundledPluginWebSearchProviders({ config: params.sourceConfig, @@ -320,13 +328,6 @@ export async function resolveRuntimeWebTools(params: { }) : [], ); - - const searchMetadata: RuntimeWebSearchMetadata = { - providerSource: "none", - diagnostics: [], - }; - - const searchEnabled = search?.enabled !== false; const configuredProvider = normalizeProvider(rawProvider, providers); if (rawProvider && !configuredProvider) { @@ -349,7 +350,7 @@ export async function resolveRuntimeWebTools(params: { searchMetadata.providerSource = "configured"; } - if (searchEnabled && search) { + if (searchEnabled) { const candidates = configuredProvider ? providers.filter((provider) => provider.id === configuredProvider) : providers; @@ -515,7 +516,7 @@ export async function resolveRuntimeWebTools(params: { } } - if (searchEnabled && search && !configuredProvider && searchMetadata.selectedProvider) { + if (searchEnabled && !configuredProvider && searchMetadata.selectedProvider) { for (const provider of providers) { if (provider.id === searchMetadata.selectedProvider) { continue; diff --git a/src/web-search/runtime.test.ts b/src/web-search/runtime.test.ts index 147c0d48ee5..48aa24104a2 100644 --- a/src/web-search/runtime.test.ts +++ b/src/web-search/runtime.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { createEmptyPluginRegistry } from "../plugins/registry.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; +import { activateSecretsRuntimeSnapshot, clearSecretsRuntimeSnapshot } from "../secrets/runtime.js"; import { runWebSearch } from "./runtime.js"; type TestPluginWebSearchConfig = { @@ -13,6 +14,7 @@ type TestPluginWebSearchConfig = { describe("web search runtime", () => { afterEach(() => { setActivePluginRegistry(createEmptyPluginRegistry()); + clearSecretsRuntimeSnapshot(); }); it("executes searches through the active plugin registry", async () => { @@ -159,4 +161,92 @@ describe("web search runtime", () => { result: { query: "fallback", provider: "duckduckgo" }, }); }); + + it("prefers the active runtime-selected provider when callers omit runtime metadata", async () => { + const registry = createEmptyPluginRegistry(); + registry.webSearchProviders.push({ + pluginId: "alpha-search", + pluginName: "Alpha Search", + provider: { + id: "alpha", + label: "Alpha Search", + hint: "Alpha runtime provider", + envVars: ["ALPHA_SEARCH_API_KEY"], + placeholder: "alpha-...", + signupUrl: "https://example.com/alpha", + credentialPath: "tools.web.search.alpha.apiKey", + autoDetectOrder: 1, + getCredentialValue: () => "alpha-configured", + setCredentialValue: () => {}, + createTool: ({ runtimeMetadata }) => ({ + description: "alpha", + parameters: {}, + execute: async (args) => ({ + ...args, + provider: "alpha", + runtimeSelectedProvider: runtimeMetadata?.selectedProvider, + }), + }), + }, + source: "test", + }); + registry.webSearchProviders.push({ + pluginId: "beta-search", + pluginName: "Beta Search", + provider: { + id: "beta", + label: "Beta Search", + hint: "Beta runtime provider", + envVars: ["BETA_SEARCH_API_KEY"], + placeholder: "beta-...", + signupUrl: "https://example.com/beta", + credentialPath: "tools.web.search.beta.apiKey", + autoDetectOrder: 2, + getCredentialValue: () => "beta-configured", + setCredentialValue: () => {}, + createTool: ({ runtimeMetadata }) => ({ + description: "beta", + parameters: {}, + execute: async (args) => ({ + ...args, + provider: "beta", + runtimeSelectedProvider: runtimeMetadata?.selectedProvider, + }), + }), + }, + source: "test", + }); + setActivePluginRegistry(registry); + activateSecretsRuntimeSnapshot({ + sourceConfig: {}, + config: {}, + authStores: [], + warnings: [], + webTools: { + search: { + providerSource: "auto-detect", + selectedProvider: "beta", + diagnostics: [], + }, + fetch: { + firecrawl: { + active: false, + apiKeySource: "missing", + diagnostics: [], + }, + }, + diagnostics: [], + }, + }); + + await expect( + runWebSearch({ + config: {}, + args: { query: "runtime" }, + }), + ).resolves.toEqual({ + provider: "beta", + result: { query: "runtime", provider: "beta", runtimeSelectedProvider: "beta" }, + }); + }); }); diff --git a/src/web-search/runtime.ts b/src/web-search/runtime.ts index 5fbba005d6a..31040fb3df7 100644 --- a/src/web-search/runtime.ts +++ b/src/web-search/runtime.ts @@ -10,6 +10,7 @@ import { resolvePluginWebSearchProviders } from "../plugins/web-search-providers import { resolveRuntimeWebSearchProviders } from "../plugins/web-search-providers.runtime.js"; import { sortWebSearchProvidersForAutoDetect } from "../plugins/web-search-providers.shared.js"; import type { RuntimeWebSearchMetadata } from "../secrets/runtime-web-tools.types.js"; +import { getActiveRuntimeWebToolsMetadata } from "../secrets/runtime.js"; import { normalizeSecretInput } from "../utils/normalize-secret-input.js"; type WebSearchConfig = NonNullable["web"] extends infer Web @@ -166,6 +167,7 @@ export function resolveWebSearchDefinition( options?: ResolveWebSearchDefinitionParams, ): { provider: PluginWebSearchProviderEntry; definition: WebSearchProviderToolDefinition } | null { const search = resolveSearchConfig(options?.config); + const runtimeWebSearch = options?.runtimeWebSearch ?? getActiveRuntimeWebToolsMetadata()?.search; if (!resolveWebSearchEnabled({ search, sandboxed: options?.sandboxed })) { return null; } @@ -187,8 +189,8 @@ export function resolveWebSearchDefinition( const providerId = options?.providerId ?? - options?.runtimeWebSearch?.selectedProvider ?? - options?.runtimeWebSearch?.providerConfigured ?? + runtimeWebSearch?.selectedProvider ?? + runtimeWebSearch?.providerConfigured ?? resolveWebSearchProviderId({ config: options?.config, search, providers }); const provider = providers.find((entry) => entry.id === providerId) ?? @@ -204,7 +206,7 @@ export function resolveWebSearchDefinition( const definition = provider.createTool({ config: options?.config, searchConfig: search as Record | undefined, - runtimeMetadata: options?.runtimeWebSearch, + runtimeMetadata: runtimeWebSearch, }); if (!definition) { return null;