From 3286791316a04c45dd1b0b95c3469ec716007e6b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 17:54:42 +0000 Subject: [PATCH] refactor(agents): dedupe config and truncation guards --- .../tool-result-truncation.test.ts | 51 +++++++++ .../tool-result-truncation.ts | 31 ++++-- src/agents/sandbox/types.docker.ts | 35 +++--- src/agents/session-tool-result-guard.ts | 61 ++-------- src/agents/skills/config.ts | 20 +--- src/commands/agent.test.ts | 58 +++++----- src/config/types.whatsapp.ts | 92 ++++------------ src/hooks/config.ts | 54 +++++---- src/infra/shell-env.test.ts | 49 ++++----- src/infra/update-startup.test.ts | 104 ++++++------------ src/shared/config-eval.test.ts | 53 +++++++++ src/shared/config-eval.ts | 35 +++++- 12 files changed, 325 insertions(+), 318 deletions(-) create mode 100644 src/shared/config-eval.test.ts diff --git a/src/agents/pi-embedded-runner/tool-result-truncation.test.ts b/src/agents/pi-embedded-runner/tool-result-truncation.test.ts index 6b7bbcf4517..27483469748 100644 --- a/src/agents/pi-embedded-runner/tool-result-truncation.test.ts +++ b/src/agents/pi-embedded-runner/tool-result-truncation.test.ts @@ -2,7 +2,9 @@ import type { AgentMessage } from "@mariozechner/pi-agent-core"; import { describe, expect, it } from "vitest"; import { truncateToolResultText, + truncateToolResultMessage, calculateMaxToolResultChars, + getToolResultTextLength, truncateOversizedToolResultsInMessages, isOversizedToolResult, sessionLikelyHasOversizedToolResults, @@ -82,6 +84,55 @@ describe("truncateToolResultText", () => { expect(lastNewline).toBeGreaterThan(keptContent.length - 100); } }); + + it("supports custom suffix and min keep chars", () => { + const text = "x".repeat(5_000); + const result = truncateToolResultText(text, 300, { + suffix: "\n\n[custom-truncated]", + minKeepChars: 250, + }); + expect(result).toContain("[custom-truncated]"); + expect(result.length).toBeGreaterThan(250); + }); +}); + +describe("getToolResultTextLength", () => { + it("sums all text blocks in tool results", () => { + const msg = { + role: "toolResult", + content: [ + { type: "text", text: "abc" }, + { type: "image", source: { type: "base64", mediaType: "image/png", data: "x" } }, + { type: "text", text: "12345" }, + ], + } as unknown as AgentMessage; + + expect(getToolResultTextLength(msg)).toBe(8); + }); + + it("returns zero for non-toolResult messages", () => { + expect(getToolResultTextLength(makeAssistantMessage("hello"))).toBe(0); + }); +}); + +describe("truncateToolResultMessage", () => { + it("truncates with a custom suffix", () => { + const msg = { + role: "toolResult", + toolCallId: "call_1", + toolName: "read", + content: [{ type: "text", text: "x".repeat(50_000) }], + isError: false, + timestamp: Date.now(), + } as unknown as AgentMessage; + + const result = truncateToolResultMessage(msg, 10_000, { + suffix: "\n\n[persist-truncated]", + minKeepChars: 2_000, + }) as { content: Array<{ type: string; text: string }> }; + + expect(result.content[0]?.text).toContain("[persist-truncated]"); + }); }); describe("calculateMaxToolResultChars", () => { diff --git a/src/agents/pi-embedded-runner/tool-result-truncation.ts b/src/agents/pi-embedded-runner/tool-result-truncation.ts index 5d54cbf888d..05bce138868 100644 --- a/src/agents/pi-embedded-runner/tool-result-truncation.ts +++ b/src/agents/pi-embedded-runner/tool-result-truncation.ts @@ -33,21 +33,32 @@ const TRUNCATION_SUFFIX = "The content above is a partial view. If you need more, request specific sections or use " + "offset/limit parameters to read smaller chunks.]"; +type ToolResultTruncationOptions = { + suffix?: string; + minKeepChars?: number; +}; + /** * Truncate a single text string to fit within maxChars, preserving the beginning. */ -export function truncateToolResultText(text: string, maxChars: number): string { +export function truncateToolResultText( + text: string, + maxChars: number, + options: ToolResultTruncationOptions = {}, +): string { + const suffix = options.suffix ?? TRUNCATION_SUFFIX; + const minKeepChars = options.minKeepChars ?? MIN_KEEP_CHARS; if (text.length <= maxChars) { return text; } - const keepChars = Math.max(MIN_KEEP_CHARS, maxChars - TRUNCATION_SUFFIX.length); + const keepChars = Math.max(minKeepChars, maxChars - suffix.length); // Try to break at a newline boundary to avoid cutting mid-line let cutPoint = keepChars; const lastNewline = text.lastIndexOf("\n", keepChars); if (lastNewline > keepChars * 0.8) { cutPoint = lastNewline; } - return text.slice(0, cutPoint) + TRUNCATION_SUFFIX; + return text.slice(0, cutPoint) + suffix; } /** @@ -67,7 +78,7 @@ export function calculateMaxToolResultChars(contextWindowTokens: number): number /** * Get the total character count of text content blocks in a tool result message. */ -function getToolResultTextLength(msg: AgentMessage): number { +export function getToolResultTextLength(msg: AgentMessage): number { if (!msg || (msg as { role?: string }).role !== "toolResult") { return 0; } @@ -91,7 +102,13 @@ function getToolResultTextLength(msg: AgentMessage): number { * Truncate a tool result message's text content blocks to fit within maxChars. * Returns a new message (does not mutate the original). */ -function truncateToolResultMessage(msg: AgentMessage, maxChars: number): AgentMessage { +export function truncateToolResultMessage( + msg: AgentMessage, + maxChars: number, + options: ToolResultTruncationOptions = {}, +): AgentMessage { + const suffix = options.suffix ?? TRUNCATION_SUFFIX; + const minKeepChars = options.minKeepChars ?? MIN_KEEP_CHARS; const content = (msg as { content?: unknown }).content; if (!Array.isArray(content)) { return msg; @@ -114,10 +131,10 @@ function truncateToolResultMessage(msg: AgentMessage, maxChars: number): AgentMe } // Proportional budget for this block const blockShare = textBlock.text.length / totalTextChars; - const blockBudget = Math.max(MIN_KEEP_CHARS, Math.floor(maxChars * blockShare)); + const blockBudget = Math.max(minKeepChars + suffix.length, Math.floor(maxChars * blockShare)); return { ...textBlock, - text: truncateToolResultText(textBlock.text, blockBudget), + text: truncateToolResultText(textBlock.text, blockBudget, { suffix, minKeepChars }), }; }); diff --git a/src/agents/sandbox/types.docker.ts b/src/agents/sandbox/types.docker.ts index 51e1a6b8cd6..a594c0e7dbc 100644 --- a/src/agents/sandbox/types.docker.ts +++ b/src/agents/sandbox/types.docker.ts @@ -1,22 +1,13 @@ -export type SandboxDockerConfig = { - image: string; - containerPrefix: string; - workdir: string; - readOnlyRoot: boolean; - tmpfs: string[]; - network: string; - user?: string; - capDrop: string[]; - env?: Record; - setupCommand?: string; - pidsLimit?: number; - memory?: string | number; - memorySwap?: string | number; - cpus?: number; - ulimits?: Record; - seccompProfile?: string; - apparmorProfile?: string; - dns?: string[]; - extraHosts?: string[]; - binds?: string[]; -}; +import type { SandboxDockerSettings } from "../../config/types.sandbox.js"; + +type RequiredDockerConfigKeys = + | "image" + | "containerPrefix" + | "workdir" + | "readOnlyRoot" + | "tmpfs" + | "network" + | "capDrop"; + +export type SandboxDockerConfig = Omit & + Required>; diff --git a/src/agents/session-tool-result-guard.ts b/src/agents/session-tool-result-guard.ts index 01619917863..689bb816c1e 100644 --- a/src/agents/session-tool-result-guard.ts +++ b/src/agents/session-tool-result-guard.ts @@ -1,12 +1,14 @@ import type { AgentMessage } from "@mariozechner/pi-agent-core"; -import type { TextContent } from "@mariozechner/pi-ai"; import type { SessionManager } from "@mariozechner/pi-coding-agent"; import type { PluginHookBeforeMessageWriteEvent, PluginHookBeforeMessageWriteResult, } from "../plugins/types.js"; import { emitSessionTranscriptUpdate } from "../sessions/transcript-events.js"; -import { HARD_MAX_TOOL_RESULT_CHARS } from "./pi-embedded-runner/tool-result-truncation.js"; +import { + HARD_MAX_TOOL_RESULT_CHARS, + truncateToolResultMessage, +} from "./pi-embedded-runner/tool-result-truncation.js"; import { makeMissingToolResult, sanitizeToolCallInputs } from "./session-transcript-repair.js"; import { extractToolCallsFromAssistant, extractToolResultId } from "./tool-call-id.js"; @@ -20,60 +22,13 @@ const GUARD_TRUNCATION_SUFFIX = * truncated text blocks otherwise. */ function capToolResultSize(msg: AgentMessage): AgentMessage { - const role = (msg as { role?: string }).role; - if (role !== "toolResult") { + if ((msg as { role?: string }).role !== "toolResult") { return msg; } - const content = (msg as { content?: unknown }).content; - if (!Array.isArray(content)) { - return msg; - } - - // Calculate total text size - let totalTextChars = 0; - for (const block of content) { - if (block && typeof block === "object" && (block as { type?: string }).type === "text") { - const text = (block as TextContent).text; - if (typeof text === "string") { - totalTextChars += text.length; - } - } - } - - if (totalTextChars <= HARD_MAX_TOOL_RESULT_CHARS) { - return msg; - } - - // Truncate proportionally - const newContent = content.map((block: unknown) => { - if (!block || typeof block !== "object" || (block as { type?: string }).type !== "text") { - return block; - } - const textBlock = block as TextContent; - if (typeof textBlock.text !== "string") { - return block; - } - const blockShare = textBlock.text.length / totalTextChars; - const blockBudget = Math.max( - 2_000, - Math.floor(HARD_MAX_TOOL_RESULT_CHARS * blockShare) - GUARD_TRUNCATION_SUFFIX.length, - ); - if (textBlock.text.length <= blockBudget) { - return block; - } - // Try to cut at a newline boundary - let cutPoint = blockBudget; - const lastNewline = textBlock.text.lastIndexOf("\n", blockBudget); - if (lastNewline > blockBudget * 0.8) { - cutPoint = lastNewline; - } - return { - ...textBlock, - text: textBlock.text.slice(0, cutPoint) + GUARD_TRUNCATION_SUFFIX, - }; + return truncateToolResultMessage(msg, HARD_MAX_TOOL_RESULT_CHARS, { + suffix: GUARD_TRUNCATION_SUFFIX, + minKeepChars: 2_000, }); - - return { ...msg, content: newContent } as AgentMessage; } export function installSessionToolResultGuard( diff --git a/src/agents/skills/config.ts b/src/agents/skills/config.ts index 212dc9907cd..b210efc9eaf 100644 --- a/src/agents/skills/config.ts +++ b/src/agents/skills/config.ts @@ -1,6 +1,6 @@ import type { OpenClawConfig, SkillConfig } from "../../config/config.js"; import { - evaluateRuntimeRequires, + evaluateRuntimeEligibility, hasBinary, isConfigPathTruthyWithDefaults, resolveConfigPath, @@ -76,8 +76,6 @@ export function shouldIncludeSkill(params: { const skillKey = resolveSkillKey(entry.skill, entry); const skillConfig = resolveSkillConfig(config, skillKey); const allowBundled = normalizeAllowlist(config?.skills?.allowBundled); - const osList = entry.metadata?.os ?? []; - const remotePlatforms = eligibility?.remote?.platforms ?? []; if (skillConfig?.enabled === false) { return false; @@ -85,18 +83,10 @@ export function shouldIncludeSkill(params: { if (!isBundledSkillAllowed(entry, allowBundled)) { return false; } - if ( - osList.length > 0 && - !osList.includes(resolveRuntimePlatform()) && - !remotePlatforms.some((platform) => osList.includes(platform)) - ) { - return false; - } - if (entry.metadata?.always === true) { - return true; - } - - return evaluateRuntimeRequires({ + return evaluateRuntimeEligibility({ + os: entry.metadata?.os, + remotePlatforms: eligibility?.remote?.platforms, + always: entry.metadata?.always, requires: entry.metadata?.requires, hasBin: hasBinary, hasRemoteBin: eligibility?.remote?.hasBin, diff --git a/src/commands/agent.test.ts b/src/commands/agent.test.ts index a30b67bdc98..91b4fd77979 100644 --- a/src/commands/agent.test.ts +++ b/src/commands/agent.test.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { beforeEach, describe, expect, it, type MockInstance, vi } from "vitest"; import { withTempHome as withTempHomeBase } from "../../test/helpers/temp-home.js"; +import "../cron/isolated-agent.mocks.js"; import * as cliRunnerModule from "../agents/cli-runner.js"; import { loadModelCatalog } from "../agents/model-catalog.js"; import { runEmbeddedPiAgent } from "../agents/pi-embedded.js"; @@ -14,22 +15,6 @@ import type { RuntimeEnv } from "../runtime.js"; import { createOutboundTestPlugin, createTestRegistry } from "../test-utils/channel-plugins.js"; import { agentCommand } from "./agent.js"; -vi.mock("../agents/pi-embedded.js", () => ({ - runEmbeddedPiAgent: vi.fn(), -})); - -vi.mock("../agents/model-catalog.js", () => ({ - loadModelCatalog: vi.fn(), -})); - -vi.mock("../agents/model-selection.js", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - isCliProvider: vi.fn(() => false), - }; -}); - vi.mock("../agents/auth-profiles.js", async (importOriginal) => { const actual = await importOriginal(); return { @@ -38,9 +23,13 @@ vi.mock("../agents/auth-profiles.js", async (importOriginal) => { }; }); -vi.mock("../agents/workspace.js", () => ({ - ensureAgentWorkspace: vi.fn(async ({ dir }: { dir: string }) => ({ dir })), -})); +vi.mock("../agents/workspace.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + ensureAgentWorkspace: vi.fn(async ({ dir }: { dir: string }) => ({ dir })), + }; +}); vi.mock("../agents/skills.js", () => ({ buildWorkspaceSkillSnapshot: vi.fn(() => undefined), @@ -89,6 +78,17 @@ function mockConfig( }); } +async function runWithDefaultAgentConfig(params: { + home: string; + args: Parameters[0]; + agentsList?: Array<{ id: string; default?: boolean }>; +}) { + const store = path.join(params.home, "sessions.json"); + mockConfig(params.home, store, undefined, undefined, params.agentsList); + await agentCommand(params.args, runtime); + return vi.mocked(runEmbeddedPiAgent).mock.calls.at(-1)?.[0]; +} + function writeSessionStoreSeed( storePath: string, sessions: Record>, @@ -441,12 +441,11 @@ describe("agentCommand", () => { it("derives session key from --agent when no routing target is provided", async () => { await withTempHome(async (home) => { - const store = path.join(home, "sessions.json"); - mockConfig(home, store, undefined, undefined, [{ id: "ops" }]); - - await agentCommand({ message: "hi", agentId: "ops" }, runtime); - - const callArgs = vi.mocked(runEmbeddedPiAgent).mock.calls.at(-1)?.[0]; + const callArgs = await runWithDefaultAgentConfig({ + home, + args: { message: "hi", agentId: "ops" }, + agentsList: [{ id: "ops" }], + }); expect(callArgs?.sessionKey).toBe("agent:ops:main"); expect(callArgs?.sessionFile).toContain(`${path.sep}agents${path.sep}ops${path.sep}sessions`); }); @@ -614,10 +613,11 @@ describe("agentCommand", () => { it("logs output when delivery is disabled", async () => { await withTempHome(async (home) => { - const store = path.join(home, "sessions.json"); - mockConfig(home, store, undefined, undefined, [{ id: "ops" }]); - - await agentCommand({ message: "hi", agentId: "ops" }, runtime); + await runWithDefaultAgentConfig({ + home, + args: { message: "hi", agentId: "ops" }, + agentsList: [{ id: "ops" }], + }); expect(runtime.log).toHaveBeenCalledWith("ok"); }); diff --git a/src/config/types.whatsapp.ts b/src/config/types.whatsapp.ts index 72890d0b31b..6fa99ea7b84 100644 --- a/src/config/types.whatsapp.ts +++ b/src/config/types.whatsapp.ts @@ -35,35 +35,10 @@ export type WhatsAppAckReactionConfig = { group?: "always" | "mentions" | "never"; }; -export type WhatsAppConfig = { - /** Optional per-account WhatsApp configuration (multi-account). */ - accounts?: Record; - /** Optional provider capability tags used for agent/runtime guidance. */ - capabilities?: string[]; - /** Markdown formatting overrides (tables). */ - markdown?: MarkdownConfig; - /** Allow channel-initiated config writes (default: true). */ - configWrites?: boolean; - /** Send read receipts for incoming messages (default true). */ - sendReadReceipts?: boolean; - /** - * Inbound message prefix (WhatsApp only). - * Default: `[{agents.list[].identity.name}]` (or `[openclaw]`) when allowFrom is empty, else `""`. - */ - messagePrefix?: string; - /** - * Per-channel outbound response prefix override. - * - * When set, this takes precedence over the global `messages.responsePrefix`. - * Use `""` to explicitly disable a global prefix for this channel. - * Use `"auto"` to derive `[{identity.name}]` from the routed agent. - */ - responsePrefix?: string; +type WhatsAppSharedConfig = { /** Direct message access policy (default: pairing). */ dmPolicy?: DmPolicy; - /** - * Same-phone setup (bot uses your personal WhatsApp number). - */ + /** Same-phone setup (bot uses your personal WhatsApp number). */ selfChatMode?: boolean; /** Optional allowlist for WhatsApp direct chats (E.164). */ allowFrom?: string[]; @@ -94,63 +69,44 @@ export type WhatsAppConfig = { blockStreaming?: boolean; /** Merge streamed block replies before sending. */ blockStreamingCoalesce?: BlockStreamingCoalesceConfig; - /** Per-action tool gating (default: true for all). */ - actions?: WhatsAppActionConfig; groups?: Record; /** Acknowledgment reaction sent immediately upon message receipt. */ ackReaction?: WhatsAppAckReactionConfig; /** Debounce window (ms) for batching rapid consecutive messages from the same sender (0 to disable). */ debounceMs?: number; - /** Heartbeat visibility settings for this channel. */ + /** Heartbeat visibility settings. */ heartbeat?: ChannelHeartbeatVisibilityConfig; }; -export type WhatsAppAccountConfig = { - /** Optional display name for this account (used in CLI/UI lists). */ - name?: string; +type WhatsAppConfigCore = { /** Optional provider capability tags used for agent/runtime guidance. */ capabilities?: string[]; /** Markdown formatting overrides (tables). */ markdown?: MarkdownConfig; /** Allow channel-initiated config writes (default: true). */ configWrites?: boolean; - /** If false, do not start this WhatsApp account provider. Default: true. */ - enabled?: boolean; /** Send read receipts for incoming messages (default true). */ sendReadReceipts?: boolean; - /** Inbound message prefix override for this account (WhatsApp only). */ + /** Inbound message prefix override (WhatsApp only). */ messagePrefix?: string; - /** Per-account outbound response prefix override (takes precedence over channel and global). */ + /** Outbound response prefix override. */ responsePrefix?: string; - /** Override auth directory (Baileys multi-file auth state). */ - authDir?: string; - /** Direct message access policy (default: pairing). */ - dmPolicy?: DmPolicy; - /** Same-phone setup for this account (bot uses your personal WhatsApp number). */ - selfChatMode?: boolean; - allowFrom?: string[]; - /** Default delivery target for CLI `--deliver` when no explicit `--reply-to` is provided (E.164 or group JID). */ - defaultTo?: string; - groupAllowFrom?: string[]; - groupPolicy?: GroupPolicy; - /** Max group messages to keep as history context (0 disables). */ - historyLimit?: number; - /** Max DM turns to keep as history context. */ - dmHistoryLimit?: number; - /** Per-DM config overrides keyed by user ID. */ - dms?: Record; - textChunkLimit?: number; - /** Chunking mode: "length" (default) splits by size; "newline" splits on every newline. */ - chunkMode?: "length" | "newline"; - mediaMaxMb?: number; - blockStreaming?: boolean; - /** Merge streamed block replies before sending. */ - blockStreamingCoalesce?: BlockStreamingCoalesceConfig; - groups?: Record; - /** Acknowledgment reaction sent immediately upon message receipt. */ - ackReaction?: WhatsAppAckReactionConfig; - /** Debounce window (ms) for batching rapid consecutive messages from the same sender (0 to disable). */ - debounceMs?: number; - /** Heartbeat visibility settings for this account. */ - heartbeat?: ChannelHeartbeatVisibilityConfig; }; + +export type WhatsAppConfig = WhatsAppConfigCore & + WhatsAppSharedConfig & { + /** Optional per-account WhatsApp configuration (multi-account). */ + accounts?: Record; + /** Per-action tool gating (default: true for all). */ + actions?: WhatsAppActionConfig; + }; + +export type WhatsAppAccountConfig = WhatsAppConfigCore & + WhatsAppSharedConfig & { + /** Optional display name for this account (used in CLI/UI lists). */ + name?: string; + /** If false, do not start this WhatsApp account provider. Default: true. */ + enabled?: boolean; + /** Override auth directory (Baileys multi-file auth state). */ + authDir?: string; + }; diff --git a/src/hooks/config.ts b/src/hooks/config.ts index 0a7aa89fef9..24e3c17271f 100644 --- a/src/hooks/config.ts +++ b/src/hooks/config.ts @@ -1,6 +1,6 @@ import type { OpenClawConfig, HookConfig } from "../config/config.js"; import { - evaluateRuntimeRequires, + evaluateRuntimeEligibility, hasBinary, isConfigPathTruthyWithDefaults, resolveConfigPath, @@ -36,6 +36,30 @@ export function resolveHookConfig( return entry; } +function evaluateHookRuntimeEligibility(params: { + entry: HookEntry; + config?: OpenClawConfig; + hookConfig?: HookConfig; + eligibility?: HookEligibilityContext; +}): boolean { + const { entry, config, hookConfig, eligibility } = params; + const remote = eligibility?.remote; + const base = { + os: entry.metadata?.os, + remotePlatforms: remote?.platforms, + always: entry.metadata?.always, + requires: entry.metadata?.requires, + hasRemoteBin: remote?.hasBin, + hasAnyRemoteBin: remote?.hasAnyBin, + }; + return evaluateRuntimeEligibility({ + ...base, + hasBin: hasBinary, + hasEnv: (envName) => Boolean(process.env[envName] || hookConfig?.env?.[envName]), + isConfigPathTruthy: (configPath) => isConfigPathTruthy(config, configPath), + }); +} + export function shouldIncludeHook(params: { entry: HookEntry; config?: OpenClawConfig; @@ -45,34 +69,16 @@ export function shouldIncludeHook(params: { const hookKey = resolveHookKey(entry.hook.name, entry); const hookConfig = resolveHookConfig(config, hookKey); const pluginManaged = entry.hook.source === "openclaw-plugin"; - const osList = entry.metadata?.os ?? []; - const remotePlatforms = eligibility?.remote?.platforms ?? []; // Check if explicitly disabled if (!pluginManaged && hookConfig?.enabled === false) { return false; } - // Check OS requirement - if ( - osList.length > 0 && - !osList.includes(resolveRuntimePlatform()) && - !remotePlatforms.some((platform) => osList.includes(platform)) - ) { - return false; - } - - // If marked as 'always', bypass all other checks - if (entry.metadata?.always === true) { - return true; - } - - return evaluateRuntimeRequires({ - requires: entry.metadata?.requires, - hasBin: hasBinary, - hasRemoteBin: eligibility?.remote?.hasBin, - hasAnyRemoteBin: eligibility?.remote?.hasAnyBin, - hasEnv: (envName) => Boolean(process.env[envName] || hookConfig?.env?.[envName]), - isConfigPathTruthy: (configPath) => isConfigPathTruthy(config, configPath), + return evaluateHookRuntimeEligibility({ + entry, + config, + hookConfig, + eligibility, }); } diff --git a/src/infra/shell-env.test.ts b/src/infra/shell-env.test.ts index 41958cc4017..80eda1da580 100644 --- a/src/infra/shell-env.test.ts +++ b/src/infra/shell-env.test.ts @@ -39,6 +39,25 @@ describe("shell env fallback", () => { return { res, exec }; } + function makeUnsafeStartupEnv(): NodeJS.ProcessEnv { + return { + SHELL: "/bin/bash", + HOME: "/tmp/evil-home", + ZDOTDIR: "/tmp/evil-zdotdir", + BASH_ENV: "/tmp/evil-bash-env", + PS4: "$(touch /tmp/pwned)", + }; + } + + function expectSanitizedStartupEnv(receivedEnv: NodeJS.ProcessEnv | undefined) { + expect(receivedEnv).toBeDefined(); + expect(receivedEnv?.BASH_ENV).toBeUndefined(); + expect(receivedEnv?.PS4).toBeUndefined(); + expect(receivedEnv?.ZDOTDIR).toBeUndefined(); + expect(receivedEnv?.SHELL).toBeUndefined(); + expect(receivedEnv?.HOME).toBe(os.homedir()); + } + it("is disabled by default", () => { expect(shouldEnableShellEnvFallback({} as NodeJS.ProcessEnv)).toBe(false); expect(shouldEnableShellEnvFallback({ OPENCLAW_LOAD_SHELL_ENV: "0" })).toBe(false); @@ -167,13 +186,7 @@ describe("shell env fallback", () => { }); it("sanitizes startup-related env vars before shell fallback exec", () => { - const env: NodeJS.ProcessEnv = { - SHELL: "/bin/bash", - HOME: "/tmp/evil-home", - ZDOTDIR: "/tmp/evil-zdotdir", - BASH_ENV: "/tmp/evil-bash-env", - PS4: "$(touch /tmp/pwned)", - }; + const env = makeUnsafeStartupEnv(); let receivedEnv: NodeJS.ProcessEnv | undefined; const exec = vi.fn((_shell: string, _args: string[], options: { env: NodeJS.ProcessEnv }) => { receivedEnv = options.env; @@ -189,23 +202,12 @@ describe("shell env fallback", () => { expect(res.ok).toBe(true); expect(exec).toHaveBeenCalledTimes(1); - expect(receivedEnv).toBeDefined(); - expect(receivedEnv?.BASH_ENV).toBeUndefined(); - expect(receivedEnv?.PS4).toBeUndefined(); - expect(receivedEnv?.ZDOTDIR).toBeUndefined(); - expect(receivedEnv?.SHELL).toBeUndefined(); - expect(receivedEnv?.HOME).toBe(os.homedir()); + expectSanitizedStartupEnv(receivedEnv); }); it("sanitizes startup-related env vars before login-shell PATH probe", () => { resetShellPathCacheForTests(); - const env: NodeJS.ProcessEnv = { - SHELL: "/bin/bash", - HOME: "/tmp/evil-home", - ZDOTDIR: "/tmp/evil-zdotdir", - BASH_ENV: "/tmp/evil-bash-env", - PS4: "$(touch /tmp/pwned)", - }; + const env = makeUnsafeStartupEnv(); let receivedEnv: NodeJS.ProcessEnv | undefined; const exec = vi.fn((_shell: string, _args: string[], options: { env: NodeJS.ProcessEnv }) => { receivedEnv = options.env; @@ -220,12 +222,7 @@ describe("shell env fallback", () => { expect(result).toBe("/usr/local/bin:/usr/bin"); expect(exec).toHaveBeenCalledTimes(1); - expect(receivedEnv).toBeDefined(); - expect(receivedEnv?.BASH_ENV).toBeUndefined(); - expect(receivedEnv?.PS4).toBeUndefined(); - expect(receivedEnv?.ZDOTDIR).toBeUndefined(); - expect(receivedEnv?.SHELL).toBeUndefined(); - expect(receivedEnv?.HOME).toBe(os.homedir()); + expectSanitizedStartupEnv(receivedEnv); }); it("returns null without invoking shell on win32", () => { diff --git a/src/infra/update-startup.test.ts b/src/infra/update-startup.test.ts index c9c03812cc4..845b8f0f2e4 100644 --- a/src/infra/update-startup.test.ts +++ b/src/infra/update-startup.test.ts @@ -106,7 +106,7 @@ describe("update-startup", () => { suiteCase = 0; }); - async function runUpdateCheckAndReadState(channel: "stable" | "beta") { + function mockPackageUpdateStatus(tag = "latest", version = "2.0.0") { vi.mocked(resolveOpenClawPackageRoot).mockResolvedValue("/opt/openclaw"); vi.mocked(checkUpdateStatus).mockResolvedValue({ root: "/opt/openclaw", @@ -114,9 +114,13 @@ describe("update-startup", () => { packageManager: "npm", } satisfies UpdateCheckResult); vi.mocked(resolveNpmChannelTag).mockResolvedValue({ - tag: "latest", - version: "2.0.0", + tag, + version, }); + } + + async function runUpdateCheckAndReadState(channel: "stable" | "beta") { + mockPackageUpdateStatus("latest", "2.0.0"); const log = { info: vi.fn() }; await runGatewayUpdateCheck({ @@ -136,6 +140,13 @@ describe("update-startup", () => { return { log, parsed }; } + function createAutoUpdateSuccessMock() { + return vi.fn().mockResolvedValue({ + ok: true, + code: 0, + }); + } + it.each([ { name: "stable channel", @@ -252,33 +263,25 @@ describe("update-startup", () => { }); it("defers stable auto-update until rollout window is due", async () => { - vi.mocked(resolveOpenClawPackageRoot).mockResolvedValue("/opt/openclaw"); - vi.mocked(checkUpdateStatus).mockResolvedValue({ - root: "/opt/openclaw", - installKind: "package", - packageManager: "npm", - } satisfies UpdateCheckResult); - vi.mocked(resolveNpmChannelTag).mockResolvedValue({ - tag: "latest", - version: "2.0.0", - }); + mockPackageUpdateStatus("latest", "2.0.0"); const runAutoUpdate = vi.fn().mockResolvedValue({ ok: true, code: 0, }); - - await runGatewayUpdateCheck({ - cfg: { - update: { - channel: "stable", - auto: { - enabled: true, - stableDelayHours: 6, - stableJitterHours: 12, - }, + const stableAutoConfig = { + update: { + channel: "stable" as const, + auto: { + enabled: true, + stableDelayHours: 6, + stableJitterHours: 12, }, }, + }; + + await runGatewayUpdateCheck({ + cfg: stableAutoConfig, log: { info: vi.fn() }, isNixMode: false, allowInTests: true, @@ -288,16 +291,7 @@ describe("update-startup", () => { vi.setSystemTime(new Date("2026-01-18T07:00:00Z")); await runGatewayUpdateCheck({ - cfg: { - update: { - channel: "stable", - auto: { - enabled: true, - stableDelayHours: 6, - stableJitterHours: 12, - }, - }, - }, + cfg: stableAutoConfig, log: { info: vi.fn() }, isNixMode: false, allowInTests: true, @@ -313,21 +307,8 @@ describe("update-startup", () => { }); it("runs beta auto-update checks hourly when enabled", async () => { - vi.mocked(resolveOpenClawPackageRoot).mockResolvedValue("/opt/openclaw"); - vi.mocked(checkUpdateStatus).mockResolvedValue({ - root: "/opt/openclaw", - installKind: "package", - packageManager: "npm", - } satisfies UpdateCheckResult); - vi.mocked(resolveNpmChannelTag).mockResolvedValue({ - tag: "beta", - version: "2.0.0-beta.1", - }); - - const runAutoUpdate = vi.fn().mockResolvedValue({ - ok: true, - code: 0, - }); + mockPackageUpdateStatus("beta", "2.0.0-beta.1"); + const runAutoUpdate = createAutoUpdateSuccessMock(); await runGatewayUpdateCheck({ cfg: { @@ -354,20 +335,8 @@ describe("update-startup", () => { }); it("runs auto-update when checkOnStart is false but auto-update is enabled", async () => { - vi.mocked(resolveOpenClawPackageRoot).mockResolvedValue("/opt/openclaw"); - vi.mocked(checkUpdateStatus).mockResolvedValue({ - root: "/opt/openclaw", - installKind: "package", - packageManager: "npm", - } satisfies UpdateCheckResult); - vi.mocked(resolveNpmChannelTag).mockResolvedValue({ - tag: "beta", - version: "2.0.0-beta.1", - }); - const runAutoUpdate = vi.fn().mockResolvedValue({ - ok: true, - code: 0, - }); + mockPackageUpdateStatus("beta", "2.0.0-beta.1"); + const runAutoUpdate = createAutoUpdateSuccessMock(); await runGatewayUpdateCheck({ cfg: { @@ -450,16 +419,7 @@ describe("update-startup", () => { }); it("scheduleGatewayUpdateCheck returns a cleanup function", async () => { - vi.mocked(resolveOpenClawPackageRoot).mockResolvedValue("/opt/openclaw"); - vi.mocked(checkUpdateStatus).mockResolvedValue({ - root: "/opt/openclaw", - installKind: "package", - packageManager: "npm", - } satisfies UpdateCheckResult); - vi.mocked(resolveNpmChannelTag).mockResolvedValue({ - tag: "latest", - version: "2.0.0", - }); + mockPackageUpdateStatus("latest", "2.0.0"); const stop = scheduleGatewayUpdateCheck({ cfg: { update: { channel: "stable" } }, diff --git a/src/shared/config-eval.test.ts b/src/shared/config-eval.test.ts new file mode 100644 index 00000000000..2ef18d1bef6 --- /dev/null +++ b/src/shared/config-eval.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest"; +import { evaluateRuntimeEligibility } from "./config-eval.js"; + +describe("evaluateRuntimeEligibility", () => { + it("rejects entries when required OS does not match local or remote", () => { + const result = evaluateRuntimeEligibility({ + os: ["definitely-not-a-runtime-platform"], + remotePlatforms: [], + hasBin: () => true, + hasEnv: () => true, + isConfigPathTruthy: () => true, + }); + expect(result).toBe(false); + }); + + it("accepts entries when remote platform satisfies OS requirements", () => { + const result = evaluateRuntimeEligibility({ + os: ["linux"], + remotePlatforms: ["linux"], + hasBin: () => true, + hasEnv: () => true, + isConfigPathTruthy: () => true, + }); + expect(result).toBe(true); + }); + + it("bypasses runtime requirements when always=true", () => { + const result = evaluateRuntimeEligibility({ + always: true, + requires: { env: ["OPENAI_API_KEY"] }, + hasBin: () => false, + hasEnv: () => false, + isConfigPathTruthy: () => false, + }); + expect(result).toBe(true); + }); + + it("evaluates runtime requirements when always is false", () => { + const result = evaluateRuntimeEligibility({ + requires: { + bins: ["node"], + anyBins: ["bun", "node"], + env: ["OPENAI_API_KEY"], + config: ["browser.enabled"], + }, + hasBin: (bin) => bin === "node", + hasAnyRemoteBin: () => false, + hasEnv: (name) => name === "OPENAI_API_KEY", + isConfigPathTruthy: (path) => path === "browser.enabled", + }); + expect(result).toBe(true); + }); +}); diff --git a/src/shared/config-eval.ts b/src/shared/config-eval.ts index d11fccf7fd1..5d1fb10807b 100644 --- a/src/shared/config-eval.ts +++ b/src/shared/config-eval.ts @@ -48,14 +48,16 @@ export type RuntimeRequires = { config?: string[]; }; -export function evaluateRuntimeRequires(params: { +type RuntimeRequirementEvalParams = { requires?: RuntimeRequires; hasBin: (bin: string) => boolean; hasAnyRemoteBin?: (bins: string[]) => boolean; hasRemoteBin?: (bin: string) => boolean; hasEnv: (envName: string) => boolean; isConfigPathTruthy: (pathStr: string) => boolean; -}): boolean { +}; + +export function evaluateRuntimeRequires(params: RuntimeRequirementEvalParams): boolean { const requires = params.requires; if (!requires) { return true; @@ -103,6 +105,35 @@ export function evaluateRuntimeRequires(params: { return true; } +export function evaluateRuntimeEligibility( + params: { + os?: string[]; + remotePlatforms?: string[]; + always?: boolean; + } & RuntimeRequirementEvalParams, +): boolean { + const osList = params.os ?? []; + const remotePlatforms = params.remotePlatforms ?? []; + if ( + osList.length > 0 && + !osList.includes(resolveRuntimePlatform()) && + !remotePlatforms.some((platform) => osList.includes(platform)) + ) { + return false; + } + if (params.always === true) { + return true; + } + return evaluateRuntimeRequires({ + requires: params.requires, + hasBin: params.hasBin, + hasRemoteBin: params.hasRemoteBin, + hasAnyRemoteBin: params.hasAnyRemoteBin, + hasEnv: params.hasEnv, + isConfigPathTruthy: params.isConfigPathTruthy, + }); +} + export function resolveRuntimePlatform(): string { return process.platform; }