diff --git a/src/acp/client.test.ts b/src/acp/client.test.ts index fb806eed943..96822423c94 100644 --- a/src/acp/client.test.ts +++ b/src/acp/client.test.ts @@ -1,8 +1,8 @@ -import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { mkdir, writeFile } from "node:fs/promises"; import path from "node:path"; import type { RequestPermissionRequest } from "@agentclientprotocol/sdk"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { resolveAcpClientSpawnEnv, resolveAcpClientSpawnInvocation, @@ -35,22 +35,11 @@ function makePermissionRequest( }; } -const tempDirs: string[] = []; - -async function createTempDir(): Promise { - const dir = await mkdtemp(path.join(tmpdir(), "openclaw-acp-client-test-")); - tempDirs.push(dir); - return dir; -} +const tempDirs = createTrackedTempDirs(); +const createTempDir = () => tempDirs.make("openclaw-acp-client-test-"); afterEach(async () => { - while (tempDirs.length > 0) { - const dir = tempDirs.pop(); - if (!dir) { - continue; - } - await rm(dir, { recursive: true, force: true }); - } + await tempDirs.cleanup(); }); describe("resolveAcpClientSpawnEnv", () => { diff --git a/src/acp/translator.ts b/src/acp/translator.ts index bc51509e776..5039cb15504 100644 --- a/src/acp/translator.ts +++ b/src/acp/translator.ts @@ -150,17 +150,9 @@ export class AcpGatewayAgent implements Agent { const sessionId = randomUUID(); const meta = parseSessionMeta(params._meta); - const sessionKey = await resolveSessionKey({ + const sessionKey = await this.resolveSessionKeyFromMeta({ meta, fallbackKey: `acp:${sessionId}`, - gateway: this.gateway, - opts: this.opts, - }); - await resetSessionIfNeeded({ - meta, - sessionKey, - gateway: this.gateway, - opts: this.opts, }); const session = this.sessionStore.createSession({ @@ -182,17 +174,9 @@ export class AcpGatewayAgent implements Agent { } const meta = parseSessionMeta(params._meta); - const sessionKey = await resolveSessionKey({ + const sessionKey = await this.resolveSessionKeyFromMeta({ meta, fallbackKey: params.sessionId, - gateway: this.gateway, - opts: this.opts, - }); - await resetSessionIfNeeded({ - meta, - sessionKey, - gateway: this.gateway, - opts: this.opts, }); const session = this.sessionStore.createSession({ @@ -328,6 +312,25 @@ export class AcpGatewayAgent implements Agent { } } + private async resolveSessionKeyFromMeta(params: { + meta: ReturnType; + fallbackKey: string; + }): Promise { + const sessionKey = await resolveSessionKey({ + meta: params.meta, + fallbackKey: params.fallbackKey, + gateway: this.gateway, + opts: this.opts, + }); + await resetSessionIfNeeded({ + meta: params.meta, + sessionKey, + gateway: this.gateway, + opts: this.opts, + }); + return sessionKey; + } + private async handleAgentEvent(evt: EventFrame): Promise { const payload = evt.payload as Record | undefined; if (!payload) { diff --git a/src/agents/auth-profiles/usage.ts b/src/agents/auth-profiles/usage.ts index 60c43c9c3c8..92c22ac14b2 100644 --- a/src/agents/auth-profiles/usage.ts +++ b/src/agents/auth-profiles/usage.ts @@ -241,16 +241,9 @@ export async function markAuthProfileUsed(params: { if (!freshStore.profiles[profileId]) { return false; } - freshStore.usageStats = freshStore.usageStats ?? {}; - freshStore.usageStats[profileId] = { - ...freshStore.usageStats[profileId], - lastUsed: Date.now(), - errorCount: 0, - cooldownUntil: undefined, - disabledUntil: undefined, - disabledReason: undefined, - failureCounts: undefined, - }; + updateUsageStatsEntry(freshStore, profileId, (existing) => + resetUsageStats(existing, { lastUsed: Date.now() }), + ); return true; }, }); @@ -262,16 +255,9 @@ export async function markAuthProfileUsed(params: { return; } - store.usageStats = store.usageStats ?? {}; - store.usageStats[profileId] = { - ...store.usageStats[profileId], - lastUsed: Date.now(), - errorCount: 0, - cooldownUntil: undefined, - disabledUntil: undefined, - disabledReason: undefined, - failureCounts: undefined, - }; + updateUsageStatsEntry(store, profileId, (existing) => + resetUsageStats(existing, { lastUsed: Date.now() }), + ); saveAuthProfileStore(store, agentDir); } @@ -360,6 +346,30 @@ export function resolveProfileUnusableUntilForDisplay( return resolveProfileUnusableUntil(stats); } +function resetUsageStats( + existing: ProfileUsageStats | undefined, + overrides?: Partial, +): ProfileUsageStats { + return { + ...existing, + errorCount: 0, + cooldownUntil: undefined, + disabledUntil: undefined, + disabledReason: undefined, + failureCounts: undefined, + ...overrides, + }; +} + +function updateUsageStatsEntry( + store: AuthProfileStore, + profileId: string, + updater: (existing: ProfileUsageStats | undefined) => ProfileUsageStats, +): void { + store.usageStats = store.usageStats ?? {}; + store.usageStats[profileId] = updater(store.usageStats[profileId]); +} + function keepActiveWindowOrRecompute(params: { existingUntil: number | undefined; now: number; @@ -448,9 +458,6 @@ export async function markAuthProfileFailure(params: { if (!profile || isAuthCooldownBypassedForProvider(profile.provider)) { return false; } - freshStore.usageStats = freshStore.usageStats ?? {}; - const existing = freshStore.usageStats[profileId] ?? {}; - const now = Date.now(); const providerKey = normalizeProviderId(profile.provider); const cfgResolved = resolveAuthCooldownConfig({ @@ -458,12 +465,14 @@ export async function markAuthProfileFailure(params: { providerId: providerKey, }); - freshStore.usageStats[profileId] = computeNextProfileUsageStats({ - existing, - now, - reason, - cfgResolved, - }); + updateUsageStatsEntry(freshStore, profileId, (existing) => + computeNextProfileUsageStats({ + existing: existing ?? {}, + now, + reason, + cfgResolved, + }), + ); return true; }, }); @@ -475,8 +484,6 @@ export async function markAuthProfileFailure(params: { return; } - store.usageStats = store.usageStats ?? {}; - const existing = store.usageStats[profileId] ?? {}; const now = Date.now(); const providerKey = normalizeProviderId(store.profiles[profileId]?.provider ?? ""); const cfgResolved = resolveAuthCooldownConfig({ @@ -484,12 +491,14 @@ export async function markAuthProfileFailure(params: { providerId: providerKey, }); - store.usageStats[profileId] = computeNextProfileUsageStats({ - existing, - now, - reason, - cfgResolved, - }); + updateUsageStatsEntry(store, profileId, (existing) => + computeNextProfileUsageStats({ + existing: existing ?? {}, + now, + reason, + cfgResolved, + }), + ); saveAuthProfileStore(store, agentDir); } @@ -528,14 +537,7 @@ export async function clearAuthProfileCooldown(params: { return false; } - freshStore.usageStats[profileId] = { - ...freshStore.usageStats[profileId], - errorCount: 0, - cooldownUntil: undefined, - disabledUntil: undefined, - disabledReason: undefined, - failureCounts: undefined, - }; + updateUsageStatsEntry(freshStore, profileId, (existing) => resetUsageStats(existing)); return true; }, }); @@ -547,13 +549,6 @@ export async function clearAuthProfileCooldown(params: { return; } - store.usageStats[profileId] = { - ...store.usageStats[profileId], - errorCount: 0, - cooldownUntil: undefined, - disabledUntil: undefined, - disabledReason: undefined, - failureCounts: undefined, - }; + updateUsageStatsEntry(store, profileId, (existing) => resetUsageStats(existing)); saveAuthProfileStore(store, agentDir); } diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index d2a0ad7259f..04f88497843 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -6,12 +6,9 @@ import { type ExecSecurity, buildEnforcedShellCommand, evaluateShellAllowlist, - maxAsk, - minSecurity, recordAllowlistUse, requiresExecApproval, resolveAllowAlwaysPatterns, - resolveExecApprovals, } from "../infra/exec-approvals.js"; import { detectCommandObfuscation } from "../infra/exec-obfuscation-detect.js"; import type { SafeBinProfile } from "../infra/exec-safe-bin-policy.js"; @@ -19,10 +16,13 @@ import { logInfo } from "../logger.js"; import { markBackgrounded, tail } from "./bash-process-registry.js"; import { buildExecApprovalRequesterContext, - resolveRegisteredExecApprovalDecision, buildExecApprovalTurnSourceContext, registerExecApprovalRequestForHostOrThrow, } from "./bash-tools.exec-approval-request.js"; +import { + resolveApprovalDecisionOrUndefined, + resolveExecHostApprovalContext, +} from "./bash-tools.exec-host-shared.js"; import { DEFAULT_APPROVAL_TIMEOUT_MS, DEFAULT_NOTIFY_TAIL_CHARS, @@ -67,16 +67,12 @@ export type ProcessGatewayAllowlistResult = { export async function processGatewayAllowlist( params: ProcessGatewayAllowlistParams, ): Promise { - const approvals = resolveExecApprovals(params.agentId, { + const { approvals, hostSecurity, hostAsk, askFallback } = resolveExecHostApprovalContext({ + agentId: params.agentId, security: params.security, ask: params.ask, + host: "gateway", }); - const hostSecurity = minSecurity(params.security, approvals.agent.security); - const hostAsk = maxAsk(params.ask, approvals.agent.ask); - const askFallback = approvals.agent.askFallback; - if (hostSecurity === "deny") { - throw new Error("exec denied: host=gateway security=deny"); - } const allowlistEval = evaluateShellAllowlist({ command: params.command, allowlist: approvals.allowlist, @@ -172,20 +168,19 @@ export async function processGatewayAllowlist( preResolvedDecision = registration.finalDecision; void (async () => { - let decision: string | null = null; - try { - decision = await resolveRegisteredExecApprovalDecision({ - approvalId, - preResolvedDecision, - }); - } catch { - emitExecSystemEvent( - `Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`, - { - sessionKey: params.notifySessionKey, - contextKey, - }, - ); + const decision = await resolveApprovalDecisionOrUndefined({ + approvalId, + preResolvedDecision, + onFailure: () => + emitExecSystemEvent( + `Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`, + { + sessionKey: params.notifySessionKey, + contextKey, + }, + ), + }); + if (decision === undefined) { return; } diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index c9a85566c05..74c740cc1da 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -5,10 +5,7 @@ import { type ExecAsk, type ExecSecurity, evaluateShellAllowlist, - maxAsk, - minSecurity, requiresExecApproval, - resolveExecApprovals, resolveExecApprovalsFromFile, } from "../infra/exec-approvals.js"; import { detectCommandObfuscation } from "../infra/exec-obfuscation-detect.js"; @@ -17,10 +14,13 @@ import { parsePreparedSystemRunPayload } from "../infra/system-run-approval-cont import { logInfo } from "../logger.js"; import { buildExecApprovalRequesterContext, - resolveRegisteredExecApprovalDecision, buildExecApprovalTurnSourceContext, registerExecApprovalRequestForHostOrThrow, } from "./bash-tools.exec-approval-request.js"; +import { + resolveApprovalDecisionOrUndefined, + resolveExecHostApprovalContext, +} from "./bash-tools.exec-host-shared.js"; import { DEFAULT_APPROVAL_TIMEOUT_MS, createApprovalSlug, @@ -56,16 +56,12 @@ export type ExecuteNodeHostCommandParams = { export async function executeNodeHostCommand( params: ExecuteNodeHostCommandParams, ): Promise> { - const approvals = resolveExecApprovals(params.agentId, { + const { hostSecurity, hostAsk, askFallback } = resolveExecHostApprovalContext({ + agentId: params.agentId, security: params.security, ask: params.ask, + host: "node", }); - const hostSecurity = minSecurity(params.security, approvals.agent.security); - const hostAsk = maxAsk(params.ask, approvals.agent.ask); - const askFallback = approvals.agent.askFallback; - if (hostSecurity === "deny") { - throw new Error("exec denied: host=node security=deny"); - } if (params.boundNode && params.requestedNode && params.boundNode !== params.requestedNode) { throw new Error(`exec node not allowed (bound to ${params.boundNode})`); } @@ -243,17 +239,16 @@ export async function executeNodeHostCommand( preResolvedDecision = registration.finalDecision; void (async () => { - let decision: string | null = null; - try { - decision = await resolveRegisteredExecApprovalDecision({ - approvalId, - preResolvedDecision, - }); - } catch { - emitExecSystemEvent( - `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${params.command}`, - { sessionKey: params.notifySessionKey, contextKey }, - ); + const decision = await resolveApprovalDecisionOrUndefined({ + approvalId, + preResolvedDecision, + onFailure: () => + emitExecSystemEvent( + `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${params.command}`, + { sessionKey: params.notifySessionKey, contextKey }, + ), + }); + if (decision === undefined) { return; } diff --git a/src/agents/bash-tools.exec-host-shared.ts b/src/agents/bash-tools.exec-host-shared.ts new file mode 100644 index 00000000000..37ee0320c3f --- /dev/null +++ b/src/agents/bash-tools.exec-host-shared.ts @@ -0,0 +1,52 @@ +import { + maxAsk, + minSecurity, + resolveExecApprovals, + type ExecAsk, + type ExecSecurity, +} from "../infra/exec-approvals.js"; +import { resolveRegisteredExecApprovalDecision } from "./bash-tools.exec-approval-request.js"; + +type ResolvedExecApprovals = ReturnType; + +export type ExecHostApprovalContext = { + approvals: ResolvedExecApprovals; + hostSecurity: ExecSecurity; + hostAsk: ExecAsk; + askFallback: ResolvedExecApprovals["agent"]["askFallback"]; +}; + +export function resolveExecHostApprovalContext(params: { + agentId?: string; + security: ExecSecurity; + ask: ExecAsk; + host: "gateway" | "node"; +}): ExecHostApprovalContext { + const approvals = resolveExecApprovals(params.agentId, { + security: params.security, + ask: params.ask, + }); + const hostSecurity = minSecurity(params.security, approvals.agent.security); + const hostAsk = maxAsk(params.ask, approvals.agent.ask); + const askFallback = approvals.agent.askFallback; + if (hostSecurity === "deny") { + throw new Error(`exec denied: host=${params.host} security=deny`); + } + return { approvals, hostSecurity, hostAsk, askFallback }; +} + +export async function resolveApprovalDecisionOrUndefined(params: { + approvalId: string; + preResolvedDecision: string | null | undefined; + onFailure: () => void; +}): Promise { + try { + return await resolveRegisteredExecApprovalDecision({ + approvalId: params.approvalId, + preResolvedDecision: params.preResolvedDecision, + }); + } catch { + params.onFailure(); + return undefined; + } +} diff --git a/src/agents/failover-error.ts b/src/agents/failover-error.ts index 97cdd3ae6e0..7b9f5194fd3 100644 --- a/src/agents/failover-error.ts +++ b/src/agents/failover-error.ts @@ -1,3 +1,4 @@ +import { readErrorName } from "../infra/errors.js"; import { classifyFailoverReason, isAuthPermanentErrorMessage, @@ -82,13 +83,6 @@ function getStatusCode(err: unknown): number | undefined { return undefined; } -function getErrorName(err: unknown): string { - if (!err || typeof err !== "object") { - return ""; - } - return "name" in err ? String(err.name) : ""; -} - function getErrorCode(err: unknown): string | undefined { if (!err || typeof err !== "object") { return undefined; @@ -127,7 +121,7 @@ function hasTimeoutHint(err: unknown): boolean { if (!err) { return false; } - if (getErrorName(err) === "TimeoutError") { + if (readErrorName(err) === "TimeoutError") { return true; } const message = getErrorMessage(err); @@ -141,7 +135,7 @@ export function isTimeoutError(err: unknown): boolean { if (!err || typeof err !== "object") { return false; } - if (getErrorName(err) !== "AbortError") { + if (readErrorName(err) !== "AbortError") { return false; } const message = getErrorMessage(err); diff --git a/src/agents/google-gemini-switch.live.test.ts b/src/agents/google-gemini-switch.live.test.ts index 80973455dab..38303602ce4 100644 --- a/src/agents/google-gemini-switch.live.test.ts +++ b/src/agents/google-gemini-switch.live.test.ts @@ -2,6 +2,7 @@ import { completeSimple, getModel } from "@mariozechner/pi-ai"; import { Type } from "@sinclair/typebox"; import { describe, expect, it } from "vitest"; import { isTruthyEnvValue } from "../infra/env.js"; +import { makeZeroUsageSnapshot } from "./usage.js"; const GEMINI_KEY = process.env.GEMINI_API_KEY ?? ""; const LIVE = isTruthyEnvValue(process.env.GEMINI_LIVE_TEST) || isTruthyEnvValue(process.env.LIVE); @@ -39,20 +40,7 @@ describeLive("gemini live switch", () => { api: "google-gemini-cli", provider: "google-antigravity", model: "claude-sonnet-4-20250514", - usage: { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - totalTokens: 0, - cost: { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - total: 0, - }, - }, + usage: makeZeroUsageSnapshot(), stopReason: "stop", timestamp: now, }, diff --git a/src/agents/openclaw-tools.agents.test.ts b/src/agents/openclaw-tools.agents.test.ts index 3ff997300ce..6cf8afa93fc 100644 --- a/src/agents/openclaw-tools.agents.test.ts +++ b/src/agents/openclaw-tools.agents.test.ts @@ -1,10 +1,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; let configOverride: ReturnType<(typeof import("../config/config.js"))["loadConfig"]> = { - session: { - mainKey: "main", - scope: "per-sender", - }, + session: createPerSenderSessionConfig(), }; vi.mock("../config/config.js", async (importOriginal) => { @@ -24,10 +22,7 @@ describe("agents_list", () => { function setConfigWithAgentList(agentList: AgentConfig[]) { configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - }, + session: createPerSenderSessionConfig(), agents: { list: agentList, }, @@ -51,10 +46,7 @@ describe("agents_list", () => { beforeEach(() => { configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - }, + session: createPerSenderSessionConfig(), }; }); diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts index b764189c149..7a5b93d7ae1 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn-depth-limits.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { addSubagentRunForTests, resetSubagentRegistryForTests } from "./subagent-registry.js"; +import { createPerSenderSessionConfig } from "./test-helpers/session-config.js"; import { createSessionsSpawnTool } from "./tools/sessions-spawn-tool.js"; const callGatewayMock = vi.fn(); @@ -13,10 +14,7 @@ vi.mock("../gateway/call.js", () => ({ let storeTemplatePath = ""; let configOverride: Record = { - session: { - mainKey: "main", - scope: "per-sender", - }, + session: createPerSenderSessionConfig(), }; vi.mock("../config/config.js", async (importOriginal) => { @@ -35,11 +33,7 @@ function writeStore(agentId: string, store: Record) { function setSubagentLimits(subagents: Record) { configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - store: storeTemplatePath, - }, + session: createPerSenderSessionConfig({ store: storeTemplatePath }), agents: { defaults: { subagents, @@ -75,11 +69,7 @@ describe("sessions_spawn depth + child limits", () => { `openclaw-subagent-depth-${Date.now()}-${Math.random().toString(16).slice(2)}-{agentId}.json`, ); configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - store: storeTemplatePath, - }, + session: createPerSenderSessionConfig({ store: storeTemplatePath }), }; callGatewayMock.mockImplementation(async (opts: unknown) => { @@ -177,11 +167,7 @@ describe("sessions_spawn depth + child limits", () => { it("rejects when active children for requester session reached maxChildrenPerAgent", async () => { configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - store: storeTemplatePath, - }, + session: createPerSenderSessionConfig({ store: storeTemplatePath }), agents: { defaults: { subagents: { @@ -214,11 +200,7 @@ describe("sessions_spawn depth + child limits", () => { it("does not use subagent maxConcurrent as a per-parent spawn gate", async () => { configOverride = { - session: { - mainKey: "main", - scope: "per-sender", - store: storeTemplatePath, - }, + session: createPerSenderSessionConfig({ store: storeTemplatePath }), agents: { defaults: { subagents: { diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts index d66046268bc..49698bc6395 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts @@ -55,6 +55,40 @@ describe("openclaw-tools: subagents (sessions_spawn allowlist)", () => { return tool.execute(callId, { task: "do thing", agentId, sandbox }); } + function setResearchUnsandboxedConfig(params?: { includeSandboxedDefault?: boolean }) { + setSessionsSpawnConfigOverride({ + session: { + mainKey: "main", + scope: "per-sender", + }, + agents: { + ...(params?.includeSandboxedDefault + ? { + defaults: { + sandbox: { + mode: "all", + }, + }, + } + : {}), + list: [ + { + id: "main", + subagents: { + allowAgents: ["research"], + }, + }, + { + id: "research", + sandbox: { + mode: "off", + }, + }, + ], + }, + }); + } + async function expectAllowedSpawn(params: { allowAgents: string[]; agentId: string; @@ -156,33 +190,7 @@ describe("openclaw-tools: subagents (sessions_spawn allowlist)", () => { }); it("forbids sandboxed cross-agent spawns that would unsandbox the child", async () => { - setSessionsSpawnConfigOverride({ - session: { - mainKey: "main", - scope: "per-sender", - }, - agents: { - defaults: { - sandbox: { - mode: "all", - }, - }, - list: [ - { - id: "main", - subagents: { - allowAgents: ["research"], - }, - }, - { - id: "research", - sandbox: { - mode: "off", - }, - }, - ], - }, - }); + setResearchUnsandboxedConfig({ includeSandboxedDefault: true }); const result = await executeSpawn("call11", "research"); const details = result.details as { status?: string; error?: string }; @@ -193,28 +201,7 @@ describe("openclaw-tools: subagents (sessions_spawn allowlist)", () => { }); it('forbids sandbox="require" when target runtime is unsandboxed', async () => { - setSessionsSpawnConfigOverride({ - session: { - mainKey: "main", - scope: "per-sender", - }, - agents: { - list: [ - { - id: "main", - subagents: { - allowAgents: ["research"], - }, - }, - { - id: "research", - sandbox: { - mode: "off", - }, - }, - ], - }, - }); + setResearchUnsandboxedConfig(); const result = await executeSpawn("call12", "research", "require"); const details = result.details as { status?: string; error?: string }; diff --git a/src/agents/pi-embedded-runner.splitsdktools.test.ts b/src/agents/pi-embedded-runner.splitsdktools.test.ts index 9a376ebf6f0..fb212ca1dc2 100644 --- a/src/agents/pi-embedded-runner.splitsdktools.test.ts +++ b/src/agents/pi-embedded-runner.splitsdktools.test.ts @@ -1,17 +1,6 @@ -import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core"; -import { Type } from "@sinclair/typebox"; import { describe, expect, it } from "vitest"; import { splitSdkTools } from "./pi-embedded-runner.js"; - -function createStubTool(name: string): AgentTool { - return { - name, - label: name, - description: "", - parameters: Type.Object({}), - execute: async () => ({}) as AgentToolResult, - }; -} +import { createStubTool } from "./test-helpers/pi-tool-stubs.js"; describe("splitSdkTools", () => { const tools = [ diff --git a/src/agents/pi-embedded-subscribe.e2e-harness.ts b/src/agents/pi-embedded-subscribe.e2e-harness.ts index 0c9a9240df0..53fc38233f4 100644 --- a/src/agents/pi-embedded-subscribe.e2e-harness.ts +++ b/src/agents/pi-embedded-subscribe.e2e-harness.ts @@ -182,6 +182,16 @@ export function emitAssistantLifecycleErrorAndEnd(params: { params.emit({ type: "agent_end" }); } +export function createReasoningFinalAnswerMessage(): AssistantMessage { + return { + role: "assistant", + content: [ + { type: "thinking", thinking: "Because it helps" }, + { type: "text", text: "Final answer" }, + ], + } as AssistantMessage; +} + type LifecycleErrorAgentEvent = { stream?: unknown; data?: { diff --git a/src/agents/pi-embedded-subscribe.handlers.messages.ts b/src/agents/pi-embedded-subscribe.handlers.messages.ts index a32c9fdf219..a988ebcc653 100644 --- a/src/agents/pi-embedded-subscribe.handlers.messages.ts +++ b/src/agents/pi-embedded-subscribe.handlers.messages.ts @@ -346,6 +346,33 @@ export function handleMessageEnd( maybeEmitReasoning(); } + const emitSplitResultAsBlockReply = ( + splitResult: ReturnType | null | undefined, + ) => { + if (!splitResult || !onBlockReply) { + return; + } + const { + text: cleanedText, + mediaUrls, + audioAsVoice, + replyToId, + replyToTag, + replyToCurrent, + } = splitResult; + // Emit if there's content OR audioAsVoice flag (to propagate the flag). + if (cleanedText || (mediaUrls && mediaUrls.length > 0) || audioAsVoice) { + void onBlockReply({ + text: cleanedText, + mediaUrls: mediaUrls?.length ? mediaUrls : undefined, + audioAsVoice, + replyToId, + replyToTag, + replyToCurrent, + }); + } + }; + if ( (ctx.state.blockReplyBreak === "message_end" || (ctx.blockChunker ? ctx.blockChunker.hasBuffered() : ctx.state.blockBuffer.length > 0)) && @@ -369,28 +396,7 @@ export function handleMessageEnd( ); } else { ctx.state.lastBlockReplyText = text; - const splitResult = ctx.consumeReplyDirectives(text, { final: true }); - if (splitResult) { - const { - text: cleanedText, - mediaUrls, - audioAsVoice, - replyToId, - replyToTag, - replyToCurrent, - } = splitResult; - // Emit if there's content OR audioAsVoice flag (to propagate the flag). - if (cleanedText || (mediaUrls && mediaUrls.length > 0) || audioAsVoice) { - void onBlockReply({ - text: cleanedText, - mediaUrls: mediaUrls?.length ? mediaUrls : undefined, - audioAsVoice, - replyToId, - replyToTag, - replyToCurrent, - }); - } - } + emitSplitResultAsBlockReply(ctx.consumeReplyDirectives(text, { final: true })); } } } @@ -403,27 +409,7 @@ export function handleMessageEnd( } if (ctx.state.blockReplyBreak === "text_end" && onBlockReply) { - const tailResult = ctx.consumeReplyDirectives("", { final: true }); - if (tailResult) { - const { - text: cleanedText, - mediaUrls, - audioAsVoice, - replyToId, - replyToTag, - replyToCurrent, - } = tailResult; - if (cleanedText || (mediaUrls && mediaUrls.length > 0) || audioAsVoice) { - void onBlockReply({ - text: cleanedText, - mediaUrls: mediaUrls?.length ? mediaUrls : undefined, - audioAsVoice, - replyToId, - replyToTag, - replyToCurrent, - }); - } - } + emitSplitResultAsBlockReply(ctx.consumeReplyDirectives("", { final: true })); } ctx.state.deltaBuffer = ""; diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.emits-reasoning-as-separate-message-enabled.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.emits-reasoning-as-separate-message-enabled.test.ts index 98b4ce09237..515bfd4e3b1 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.emits-reasoning-as-separate-message-enabled.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.emits-reasoning-as-separate-message-enabled.test.ts @@ -2,6 +2,7 @@ import type { AssistantMessage } from "@mariozechner/pi-ai"; import { describe, expect, it, vi } from "vitest"; import { THINKING_TAG_CASES, + createReasoningFinalAnswerMessage, createStubSessionHarness, } from "./pi-embedded-subscribe.e2e-harness.js"; import { subscribeEmbeddedPiSession } from "./pi-embedded-subscribe.js"; @@ -31,13 +32,7 @@ describe("subscribeEmbeddedPiSession", () => { it("emits reasoning as a separate message when enabled", () => { const { emit, onBlockReply } = createReasoningBlockReplyHarness(); - const assistantMessage = { - role: "assistant", - content: [ - { type: "thinking", thinking: "Because it helps" }, - { type: "text", text: "Final answer" }, - ], - } as AssistantMessage; + const assistantMessage = createReasoningFinalAnswerMessage(); emit({ type: "message_end", message: assistantMessage }); diff --git a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.keeps-assistanttexts-final-answer-block-replies-are.test.ts b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.keeps-assistanttexts-final-answer-block-replies-are.test.ts index 710b1f280fa..87f824473d7 100644 --- a/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.keeps-assistanttexts-final-answer-block-replies-are.test.ts +++ b/src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.keeps-assistanttexts-final-answer-block-replies-are.test.ts @@ -1,6 +1,6 @@ -import type { AssistantMessage } from "@mariozechner/pi-ai"; import { describe, expect, it, vi } from "vitest"; import { + createReasoningFinalAnswerMessage, createStubSessionHarness, emitAssistantTextDelta, emitAssistantTextEnd, @@ -22,13 +22,7 @@ describe("subscribeEmbeddedPiSession", () => { emitAssistantTextDelta({ emit, delta: "answer" }); emitAssistantTextEnd({ emit }); - const assistantMessage = { - role: "assistant", - content: [ - { type: "thinking", thinking: "Because it helps" }, - { type: "text", text: "Final answer" }, - ], - } as AssistantMessage; + const assistantMessage = createReasoningFinalAnswerMessage(); emit({ type: "message_end", message: assistantMessage }); @@ -52,13 +46,7 @@ describe("subscribeEmbeddedPiSession", () => { expect(onPartialReply).not.toHaveBeenCalled(); - const assistantMessage = { - role: "assistant", - content: [ - { type: "thinking", thinking: "Because it helps" }, - { type: "text", text: "Final answer" }, - ], - } as AssistantMessage; + const assistantMessage = createReasoningFinalAnswerMessage(); emit({ type: "message_end", message: assistantMessage }); emitAssistantTextEnd({ emit, content: "Draft reply" }); diff --git a/src/agents/pi-model-discovery.auth.test.ts b/src/agents/pi-model-discovery.auth.test.ts index 0804ed42312..c4d4bae25d5 100644 --- a/src/agents/pi-model-discovery.auth.test.ts +++ b/src/agents/pi-model-discovery.auth.test.ts @@ -9,6 +9,15 @@ async function createAgentDir(): Promise { return await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pi-auth-storage-")); } +async function withAgentDir(run: (agentDir: string) => Promise): Promise { + const agentDir = await createAgentDir(); + try { + await run(agentDir); + } finally { + await fs.rm(agentDir, { recursive: true, force: true }); + } +} + async function pathExists(pathname: string): Promise { try { await fs.stat(pathname); @@ -18,10 +27,25 @@ async function pathExists(pathname: string): Promise { } } +function writeRuntimeOpenRouterProfile(agentDir: string): void { + saveAuthProfileStore( + { + version: 1, + profiles: { + "openrouter:default": { + type: "api_key", + provider: "openrouter", + key: "sk-or-v1-runtime", + }, + }, + }, + agentDir, + ); +} + describe("discoverAuthStorage", () => { it("loads runtime credentials from auth-profiles without writing auth.json", async () => { - const agentDir = await createAgentDir(); - try { + await withAgentDir(async (agentDir) => { saveAuthProfileStore( { version: 1, @@ -61,27 +85,12 @@ describe("discoverAuthStorage", () => { }); expect(await pathExists(path.join(agentDir, "auth.json"))).toBe(false); - } finally { - await fs.rm(agentDir, { recursive: true, force: true }); - } + }); }); it("scrubs static api_key entries from legacy auth.json and keeps oauth entries", async () => { - const agentDir = await createAgentDir(); - try { - saveAuthProfileStore( - { - version: 1, - profiles: { - "openrouter:default": { - type: "api_key", - provider: "openrouter", - key: "sk-or-v1-runtime", - }, - }, - }, - agentDir, - ); + await withAgentDir(async (agentDir) => { + writeRuntimeOpenRouterProfile(agentDir); await fs.writeFile( path.join(agentDir, "auth.json"), JSON.stringify( @@ -109,53 +118,39 @@ describe("discoverAuthStorage", () => { type: "oauth", access: "oauth-access", }); - } finally { - await fs.rm(agentDir, { recursive: true, force: true }); - } + }); }); it("preserves legacy auth.json when auth store is forced read-only", async () => { - const agentDir = await createAgentDir(); - const previous = process.env.OPENCLAW_AUTH_STORE_READONLY; - process.env.OPENCLAW_AUTH_STORE_READONLY = "1"; - try { - saveAuthProfileStore( - { - version: 1, - profiles: { - "openrouter:default": { - type: "api_key", - provider: "openrouter", - key: "sk-or-v1-runtime", + await withAgentDir(async (agentDir) => { + const previous = process.env.OPENCLAW_AUTH_STORE_READONLY; + process.env.OPENCLAW_AUTH_STORE_READONLY = "1"; + try { + writeRuntimeOpenRouterProfile(agentDir); + await fs.writeFile( + path.join(agentDir, "auth.json"), + JSON.stringify( + { + openrouter: { type: "api_key", key: "legacy-static-key" }, }, - }, - }, - agentDir, - ); - await fs.writeFile( - path.join(agentDir, "auth.json"), - JSON.stringify( - { - openrouter: { type: "api_key", key: "legacy-static-key" }, - }, - null, - 2, - ), - ); + null, + 2, + ), + ); - discoverAuthStorage(agentDir); + discoverAuthStorage(agentDir); - const parsed = JSON.parse(await fs.readFile(path.join(agentDir, "auth.json"), "utf8")) as { - [key: string]: unknown; - }; - expect(parsed.openrouter).toMatchObject({ type: "api_key", key: "legacy-static-key" }); - } finally { - if (previous === undefined) { - delete process.env.OPENCLAW_AUTH_STORE_READONLY; - } else { - process.env.OPENCLAW_AUTH_STORE_READONLY = previous; + const parsed = JSON.parse(await fs.readFile(path.join(agentDir, "auth.json"), "utf8")) as { + [key: string]: unknown; + }; + expect(parsed.openrouter).toMatchObject({ type: "api_key", key: "legacy-static-key" }); + } finally { + if (previous === undefined) { + delete process.env.OPENCLAW_AUTH_STORE_READONLY; + } else { + process.env.OPENCLAW_AUTH_STORE_READONLY = previous; + } } - await fs.rm(agentDir, { recursive: true, force: true }); - } + }); }); }); diff --git a/src/agents/pi-tools.policy.test.ts b/src/agents/pi-tools.policy.test.ts index 77bc99dc92c..4b7a16b4d92 100644 --- a/src/agents/pi-tools.policy.test.ts +++ b/src/agents/pi-tools.policy.test.ts @@ -1,5 +1,3 @@ -import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core"; -import { Type } from "@sinclair/typebox"; import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { @@ -7,16 +5,7 @@ import { isToolAllowedByPolicyName, resolveSubagentToolPolicy, } from "./pi-tools.policy.js"; - -function createStubTool(name: string): AgentTool { - return { - name, - label: name, - description: "", - parameters: Type.Object({}), - execute: async () => ({}) as AgentToolResult, - }; -} +import { createStubTool } from "./test-helpers/pi-tool-stubs.js"; describe("pi-tools.policy", () => { it("treats * in allow as allow-all", () => { diff --git a/src/agents/pi-tools.sandbox-mounted-paths.workspace-only.test.ts b/src/agents/pi-tools.sandbox-mounted-paths.workspace-only.test.ts index 6e0563d7540..1e02c2be160 100644 --- a/src/agents/pi-tools.sandbox-mounted-paths.workspace-only.test.ts +++ b/src/agents/pi-tools.sandbox-mounted-paths.workspace-only.test.ts @@ -18,6 +18,30 @@ vi.mock("../infra/shell-env.js", async (importOriginal) => { type ToolWithExecute = { execute: (toolCallId: string, args: unknown, signal?: AbortSignal) => Promise; }; +type CodingToolsInput = NonNullable[0]>; + +const APPLY_PATCH_PAYLOAD = `*** Begin Patch +*** Add File: /agent/pwned.txt ++owned-by-apply-patch +*** End Patch`; + +function resolveApplyPatchTool( + params: Pick & { config: OpenClawConfig }, +): ToolWithExecute { + const tools = createOpenClawCodingTools({ + sandbox: params.sandbox, + workspaceDir: params.workspaceDir, + config: params.config, + modelProvider: "openai", + modelId: "gpt-5.2", + }); + const applyPatchTool = tools.find((t) => t.name === "apply_patch") as ToolWithExecute | undefined; + if (!applyPatchTool) { + throw new Error("apply_patch tool missing"); + } + return applyPatchTool; +} + describe("tools.fs.workspaceOnly", () => { it("defaults to allowing sandbox mounts outside the workspace root", async () => { await withUnsafeMountedSandboxHarness(async ({ sandboxRoot, agentRoot, sandbox }) => { @@ -62,32 +86,18 @@ describe("tools.fs.workspaceOnly", () => { it("enforces apply_patch workspace-only in sandbox mounts by default", async () => { await withUnsafeMountedSandboxHarness(async ({ sandboxRoot, agentRoot, sandbox }) => { - const cfg: OpenClawConfig = { - tools: { - allow: ["read", "exec"], - exec: { applyPatch: { enabled: true } }, - }, - }; - const tools = createOpenClawCodingTools({ + const applyPatchTool = resolveApplyPatchTool({ sandbox, workspaceDir: sandboxRoot, - config: cfg, - modelProvider: "openai", - modelId: "gpt-5.2", + config: { + tools: { + allow: ["read", "exec"], + exec: { applyPatch: { enabled: true } }, + }, + } as OpenClawConfig, }); - const applyPatchTool = tools.find((t) => t.name === "apply_patch") as - | ToolWithExecute - | undefined; - if (!applyPatchTool) { - throw new Error("apply_patch tool missing"); - } - const patch = `*** Begin Patch -*** Add File: /agent/pwned.txt -+owned-by-apply-patch -*** End Patch`; - - await expect(applyPatchTool.execute("t1", { input: patch })).rejects.toThrow( + await expect(applyPatchTool.execute("t1", { input: APPLY_PATCH_PAYLOAD })).rejects.toThrow( /Path escapes sandbox root/i, ); await expect(fs.stat(path.join(agentRoot, "pwned.txt"))).rejects.toMatchObject({ @@ -98,32 +108,18 @@ describe("tools.fs.workspaceOnly", () => { it("allows apply_patch outside workspace root when explicitly disabled", async () => { await withUnsafeMountedSandboxHarness(async ({ sandboxRoot, agentRoot, sandbox }) => { - const cfg: OpenClawConfig = { - tools: { - allow: ["read", "exec"], - exec: { applyPatch: { enabled: true, workspaceOnly: false } }, - }, - }; - const tools = createOpenClawCodingTools({ + const applyPatchTool = resolveApplyPatchTool({ sandbox, workspaceDir: sandboxRoot, - config: cfg, - modelProvider: "openai", - modelId: "gpt-5.2", + config: { + tools: { + allow: ["read", "exec"], + exec: { applyPatch: { enabled: true, workspaceOnly: false } }, + }, + } as OpenClawConfig, }); - const applyPatchTool = tools.find((t) => t.name === "apply_patch") as - | ToolWithExecute - | undefined; - if (!applyPatchTool) { - throw new Error("apply_patch tool missing"); - } - const patch = `*** Begin Patch -*** Add File: /agent/pwned.txt -+owned-by-apply-patch -*** End Patch`; - - await applyPatchTool.execute("t2", { input: patch }); + await applyPatchTool.execute("t2", { input: APPLY_PATCH_PAYLOAD }); expect(await fs.readFile(path.join(agentRoot, "pwned.txt"), "utf8")).toBe( "owned-by-apply-patch\n", ); diff --git a/src/agents/sandbox/docker.windows.test.ts b/src/agents/sandbox/docker.windows.test.ts index d9fe1d1f567..3dd294e8360 100644 --- a/src/agents/sandbox/docker.windows.test.ts +++ b/src/agents/sandbox/docker.windows.test.ts @@ -1,25 +1,14 @@ -import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { mkdir, writeFile } from "node:fs/promises"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { createTrackedTempDirs } from "../../test-utils/tracked-temp-dirs.js"; import { resolveDockerSpawnInvocation } from "./docker.js"; -const tempDirs: string[] = []; - -async function createTempDir(): Promise { - const dir = await mkdtemp(path.join(tmpdir(), "openclaw-docker-spawn-test-")); - tempDirs.push(dir); - return dir; -} +const tempDirs = createTrackedTempDirs(); +const createTempDir = () => tempDirs.make("openclaw-docker-spawn-test-"); afterEach(async () => { - while (tempDirs.length > 0) { - const dir = tempDirs.pop(); - if (!dir) { - continue; - } - await rm(dir, { recursive: true, force: true }); - } + await tempDirs.cleanup(); }); describe("resolveDockerSpawnInvocation", () => { diff --git a/src/agents/subagent-registry-queries.ts b/src/agents/subagent-registry-queries.ts index 62fd743998b..2407acb8c5b 100644 --- a/src/agents/subagent-registry-queries.ts +++ b/src/agents/subagent-registry-queries.ts @@ -79,53 +79,17 @@ export function countActiveRunsForSessionFromRuns( return count; } -export function countActiveDescendantRunsFromRuns( +function forEachDescendantRun( runs: Map, rootSessionKey: string, -): number { + visitor: (runId: string, entry: SubagentRunRecord) => void, +): boolean { const root = rootSessionKey.trim(); if (!root) { - return 0; + return false; } const pending = [root]; const visited = new Set([root]); - let count = 0; - while (pending.length > 0) { - const requester = pending.shift(); - if (!requester) { - continue; - } - for (const entry of runs.values()) { - if (entry.requesterSessionKey !== requester) { - continue; - } - if (typeof entry.endedAt !== "number") { - count += 1; - } - const childKey = entry.childSessionKey.trim(); - if (!childKey || visited.has(childKey)) { - continue; - } - visited.add(childKey); - pending.push(childKey); - } - } - return count; -} - -function countPendingDescendantRunsInternal( - runs: Map, - rootSessionKey: string, - excludeRunId?: string, -): number { - const root = rootSessionKey.trim(); - if (!root) { - return 0; - } - const excludedRunId = excludeRunId?.trim(); - const pending = [root]; - const visited = new Set([root]); - let count = 0; for (let index = 0; index < pending.length; index += 1) { const requester = pending[index]; if (!requester) { @@ -135,11 +99,7 @@ function countPendingDescendantRunsInternal( if (entry.requesterSessionKey !== requester) { continue; } - const runEnded = typeof entry.endedAt === "number"; - const cleanupCompleted = typeof entry.cleanupCompletedAt === "number"; - if ((!runEnded || !cleanupCompleted) && runId !== excludedRunId) { - count += 1; - } + visitor(runId, entry); const childKey = entry.childSessionKey.trim(); if (!childKey || visited.has(childKey)) { continue; @@ -148,6 +108,44 @@ function countPendingDescendantRunsInternal( pending.push(childKey); } } + return true; +} + +export function countActiveDescendantRunsFromRuns( + runs: Map, + rootSessionKey: string, +): number { + let count = 0; + if ( + !forEachDescendantRun(runs, rootSessionKey, (_runId, entry) => { + if (typeof entry.endedAt !== "number") { + count += 1; + } + }) + ) { + return 0; + } + return count; +} + +function countPendingDescendantRunsInternal( + runs: Map, + rootSessionKey: string, + excludeRunId?: string, +): number { + const excludedRunId = excludeRunId?.trim(); + let count = 0; + if ( + !forEachDescendantRun(runs, rootSessionKey, (runId, entry) => { + const runEnded = typeof entry.endedAt === "number"; + const cleanupCompleted = typeof entry.cleanupCompletedAt === "number"; + if ((!runEnded || !cleanupCompleted) && runId !== excludedRunId) { + count += 1; + } + }) + ) { + return 0; + } return count; } @@ -170,30 +168,13 @@ export function listDescendantRunsForRequesterFromRuns( runs: Map, rootSessionKey: string, ): SubagentRunRecord[] { - const root = rootSessionKey.trim(); - if (!root) { - return []; - } - const pending = [root]; - const visited = new Set([root]); const descendants: SubagentRunRecord[] = []; - while (pending.length > 0) { - const requester = pending.shift(); - if (!requester) { - continue; - } - for (const entry of runs.values()) { - if (entry.requesterSessionKey !== requester) { - continue; - } + if ( + !forEachDescendantRun(runs, rootSessionKey, (_runId, entry) => { descendants.push(entry); - const childKey = entry.childSessionKey.trim(); - if (!childKey || visited.has(childKey)) { - continue; - } - visited.add(childKey); - pending.push(childKey); - } + }) + ) { + return []; } return descendants; } diff --git a/src/agents/test-helpers/pi-tool-stubs.ts b/src/agents/test-helpers/pi-tool-stubs.ts new file mode 100644 index 00000000000..71fe740234f --- /dev/null +++ b/src/agents/test-helpers/pi-tool-stubs.ts @@ -0,0 +1,12 @@ +import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core"; +import { Type } from "@sinclair/typebox"; + +export function createStubTool(name: string): AgentTool { + return { + name, + label: name, + description: "", + parameters: Type.Object({}), + execute: async () => ({}) as AgentToolResult, + }; +} diff --git a/src/agents/test-helpers/session-config.ts b/src/agents/test-helpers/session-config.ts new file mode 100644 index 00000000000..6017e01d0e0 --- /dev/null +++ b/src/agents/test-helpers/session-config.ts @@ -0,0 +1,11 @@ +import type { OpenClawConfig } from "../../config/config.js"; + +export function createPerSenderSessionConfig( + overrides: Partial> = {}, +): NonNullable { + return { + mainKey: "main", + scope: "per-sender", + ...overrides, + }; +} diff --git a/src/agents/tool-display-common.ts b/src/agents/tool-display-common.ts index 7d098297198..a7564c98052 100644 --- a/src/agents/tool-display-common.ts +++ b/src/agents/tool-display-common.ts @@ -63,6 +63,31 @@ export function resolveActionArg(args: unknown): string | undefined { return action || undefined; } +export function resolveToolVerbAndDetailForArgs(params: { + toolKey: string; + args?: unknown; + meta?: string; + spec?: ToolDisplaySpec; + fallbackDetailKeys?: string[]; + detailMode: "first" | "summary"; + detailCoerce?: CoerceDisplayValueOptions; + detailMaxEntries?: number; + detailFormatKey?: (raw: string) => string; +}): { verb?: string; detail?: string } { + return resolveToolVerbAndDetail({ + toolKey: params.toolKey, + args: params.args, + meta: params.meta, + action: resolveActionArg(params.args), + spec: params.spec, + fallbackDetailKeys: params.fallbackDetailKeys, + detailMode: params.detailMode, + detailCoerce: params.detailCoerce, + detailMaxEntries: params.detailMaxEntries, + detailFormatKey: params.detailFormatKey, + }); +} + export function coerceDisplayValue( value: unknown, opts: CoerceDisplayValueOptions = {}, diff --git a/src/agents/tool-display.ts b/src/agents/tool-display.ts index 17183d6fe1d..1285b4dc52f 100644 --- a/src/agents/tool-display.ts +++ b/src/agents/tool-display.ts @@ -6,8 +6,7 @@ import { formatToolDetailText, formatDetailKey, normalizeToolName, - resolveActionArg, - resolveToolVerbAndDetail, + resolveToolVerbAndDetailForArgs, type ToolDisplaySpec as ToolDisplaySpecBase, } from "./tool-display-common.js"; import TOOL_DISPLAY_OVERRIDES_JSON from "./tool-display-overrides.json" with { type: "json" }; @@ -67,12 +66,10 @@ export function resolveToolDisplay(params: { const emoji = spec?.emoji ?? FALLBACK.emoji ?? "🧩"; const title = spec?.title ?? defaultTitle(name); const label = spec?.label ?? title; - const action = resolveActionArg(params.args); - let { verb, detail } = resolveToolVerbAndDetail({ + let { verb, detail } = resolveToolVerbAndDetailForArgs({ toolKey: key, args: params.args, meta: params.meta, - action, spec, fallbackDetailKeys: FALLBACK.detailKeys, detailMode: "summary", @@ -96,7 +93,7 @@ export function resolveToolDisplay(params: { export function formatToolDetail(display: ToolDisplay): string | undefined { const detailRaw = display.detail ? redactToolDetail(display.detail) : undefined; - return formatToolDetailText(detailRaw, { prefixWithWith: true }); + return formatToolDetailText(detailRaw); } export function formatToolSummary(display: ToolDisplay): string { diff --git a/src/agents/tools/discord-actions-guild.ts b/src/agents/tools/discord-actions-guild.ts index 630c6e9acf1..93d81f5724e 100644 --- a/src/agents/tools/discord-actions-guild.ts +++ b/src/agents/tools/discord-actions-guild.ts @@ -29,16 +29,7 @@ import { readStringArrayParam, readStringParam, } from "./common.js"; - -function readParentIdParam(params: Record): string | null | undefined { - if (params.clearParent === true) { - return null; - } - if (params.parentId === null) { - return null; - } - return readStringParam(params, "parentId"); -} +import { readDiscordParentIdParam } from "./discord-actions-shared.js"; type DiscordRoleMutation = (params: { guildId: string; @@ -287,7 +278,7 @@ export async function handleDiscordGuildAction( const guildId = readStringParam(params, "guildId", { required: true }); const name = readStringParam(params, "name", { required: true }); const type = readNumberParam(params, "type", { integer: true }); - const parentId = readParentIdParam(params); + const parentId = readDiscordParentIdParam(params); const topic = readStringParam(params, "topic"); const position = readNumberParam(params, "position", { integer: true }); const nsfw = params.nsfw as boolean | undefined; @@ -325,7 +316,7 @@ export async function handleDiscordGuildAction( const name = readStringParam(params, "name"); const topic = readStringParam(params, "topic"); const position = readNumberParam(params, "position", { integer: true }); - const parentId = readParentIdParam(params); + const parentId = readDiscordParentIdParam(params); const nsfw = params.nsfw as boolean | undefined; const rateLimitPerUser = readNumberParam(params, "rateLimitPerUser", { integer: true, @@ -388,7 +379,7 @@ export async function handleDiscordGuildAction( const channelId = readStringParam(params, "channelId", { required: true, }); - const parentId = readParentIdParam(params); + const parentId = readDiscordParentIdParam(params); const position = readNumberParam(params, "position", { integer: true }); if (accountId) { await moveChannelDiscord( diff --git a/src/agents/tools/discord-actions-shared.ts b/src/agents/tools/discord-actions-shared.ts new file mode 100644 index 00000000000..6f8283b5240 --- /dev/null +++ b/src/agents/tools/discord-actions-shared.ts @@ -0,0 +1,13 @@ +import { readStringParam } from "./common.js"; + +export function readDiscordParentIdParam( + params: Record, +): string | null | undefined { + if (params.clearParent === true) { + return null; + } + if (params.parentId === null) { + return null; + } + return readStringParam(params, "parentId"); +} diff --git a/src/agents/tools/image-tool.test.ts b/src/agents/tools/image-tool.test.ts index c11799660ff..a2771fb4215 100644 --- a/src/agents/tools/image-tool.test.ts +++ b/src/agents/tools/image-tool.test.ts @@ -8,6 +8,7 @@ import { withFetchPreconnect } from "../../test-utils/fetch-mock.js"; import { createOpenClawCodingTools } from "../pi-tools.js"; import { createHostSandboxFsBridge } from "../test-helpers/host-sandbox-fs-bridge.js"; import { createUnsafeMountedSandbox } from "../test-helpers/unsafe-mounted-sandbox.js"; +import { makeZeroUsageSnapshot } from "../usage.js"; import { __testing, createImageTool, resolveImageModelConfigForTool } from "./image-tool.js"; async function writeAuthProfiles(agentDir: string, profiles: unknown) { @@ -766,23 +767,6 @@ describe("image tool MiniMax VLM routing", () => { }); describe("image tool response validation", () => { - function zeroUsage() { - return { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - totalTokens: 0, - cost: { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - total: 0, - }, - }; - } - function createAssistantMessage( overrides: Partial<{ api: string; @@ -800,7 +784,7 @@ describe("image tool response validation", () => { model: "gpt-5-mini", stopReason: "stop", timestamp: Date.now(), - usage: zeroUsage(), + usage: makeZeroUsageSnapshot(), content: [] as unknown[], ...overrides, }; diff --git a/src/agents/tools/image-tool.ts b/src/agents/tools/image-tool.ts index 55352eb373f..89a0da6d070 100644 --- a/src/agents/tools/image-tool.ts +++ b/src/agents/tools/image-tool.ts @@ -1,8 +1,8 @@ -import { type Api, type Context, complete, type Model } from "@mariozechner/pi-ai"; +import { type Context, complete } from "@mariozechner/pi-ai"; import { Type } from "@sinclair/typebox"; import type { OpenClawConfig } from "../../config/config.js"; import { resolveUserPath } from "../../utils.js"; -import { getDefaultLocalRoots, loadWebMedia } from "../../web/media.js"; +import { loadWebMedia } from "../../web/media.js"; import { minimaxUnderstandImage } from "../minimax-vlm.js"; import { coerceImageAssistantText, @@ -11,15 +11,20 @@ import { type ImageModelConfig, resolveProviderVisionModelFromConfig, } from "./image-tool.helpers.js"; +import { + applyImageModelConfigDefaults, + buildTextToolResult, + resolveMediaToolLocalRoots, + resolveModelFromRegistry, + resolveModelRuntimeApiKey, + resolvePromptAndModelOverride, +} from "./media-tool-shared.js"; import { hasAuthForProvider, resolveDefaultModelRef } from "./model-config.helpers.js"; import { createSandboxBridgeReadFile, discoverAuthStorage, discoverModels, ensureOpenClawModelsJson, - getApiKeyForModel, - normalizeWorkspaceDir, - requireApiKey, resolveSandboxedBridgeMediaPath, runWithImageModelFallback, type AnyAgentTool, @@ -202,18 +207,7 @@ async function runImagePrompt(params: { model: string; attempts: Array<{ provider: string; model: string; error: string }>; }> { - const effectiveCfg: OpenClawConfig | undefined = params.cfg - ? { - ...params.cfg, - agents: { - ...params.cfg.agents, - defaults: { - ...params.cfg.agents?.defaults, - imageModel: params.imageModelConfig, - }, - }, - } - : undefined; + const effectiveCfg = applyImageModelConfigDefaults(params.cfg, params.imageModelConfig); await ensureOpenClawModelsJson(effectiveCfg, params.agentDir); const authStorage = discoverAuthStorage(params.agentDir); @@ -223,20 +217,16 @@ async function runImagePrompt(params: { cfg: effectiveCfg, modelOverride: params.modelOverride, run: async (provider, modelId) => { - const model = modelRegistry.find(provider, modelId) as Model | null; - if (!model) { - throw new Error(`Unknown model: ${provider}/${modelId}`); - } + const model = resolveModelFromRegistry({ modelRegistry, provider, modelId }); if (!model.input?.includes("image")) { throw new Error(`Model does not support images: ${provider}/${modelId}`); } - const apiKeyInfo = await getApiKeyForModel({ + const apiKey = await resolveModelRuntimeApiKey({ model, cfg: effectiveCfg, agentDir: params.agentDir, + authStorage, }); - const apiKey = requireApiKey(apiKeyInfo, model.provider); - authStorage.setRuntimeApiKey(model.provider, apiKey); // MiniMax VLM only supports a single image; use the first one. if (model.provider === "minimax") { @@ -308,6 +298,7 @@ export function createImageTool(options?: { ? "Analyze one or more images with a vision model. Use image for a single path/URL, or images for multiple (up to 20). Only use this tool when images were NOT already provided in the user's message. Images mentioned in the prompt are automatically visible to you." : "Analyze one or more images with the configured image model (agents.defaults.imageModel). Use image for a single path/URL, or images for multiple (up to 20). Provide a prompt describing what to analyze."; +<<<<<<< HEAD const localRoots = (() => { const workspaceDir = normalizeWorkspaceDir(options?.workspaceDir); if (options?.fsPolicy?.workspaceOnly) { @@ -319,6 +310,18 @@ export function createImageTool(options?: { } return Array.from(new Set([...roots, workspaceDir])); })(); +||||||| parent of 4a741746c (refactor: dedupe agent and reply runtimes) + const localRoots = (() => { + const roots = getDefaultLocalRoots(); + const workspaceDir = normalizeWorkspaceDir(options?.workspaceDir); + if (!workspaceDir) { + return roots; + } + return Array.from(new Set([...roots, workspaceDir])); + })(); +======= + const localRoots = resolveMediaToolLocalRoots(options?.workspaceDir); +>>>>>>> 4a741746c (refactor: dedupe agent and reply runtimes) return { label: "Image", @@ -383,12 +386,10 @@ export function createImageTool(options?: { }; } - const promptRaw = - typeof record.prompt === "string" && record.prompt.trim() - ? record.prompt.trim() - : DEFAULT_PROMPT; - const modelOverride = - typeof record.model === "string" && record.model.trim() ? record.model.trim() : undefined; + const { prompt: promptRaw, modelOverride } = resolvePromptAndModelOverride( + record, + DEFAULT_PROMPT, + ); const maxBytesMb = typeof record.maxBytesMb === "number" ? record.maxBytesMb : undefined; const maxBytes = pickMaxBytes(options?.config, maxBytesMb); @@ -525,14 +526,7 @@ export function createImageTool(options?: { })), }; - return { - content: [{ type: "text", text: result.text }], - details: { - model: `${result.provider}/${result.model}`, - ...imageDetails, - attempts: result.attempts, - }, - }; + return buildTextToolResult(result, imageDetails); }, }; } diff --git a/src/agents/tools/media-tool-shared.ts b/src/agents/tools/media-tool-shared.ts new file mode 100644 index 00000000000..a869a9df9cb --- /dev/null +++ b/src/agents/tools/media-tool-shared.ts @@ -0,0 +1,107 @@ +import { type Api, type Model } from "@mariozechner/pi-ai"; +import type { OpenClawConfig } from "../../config/config.js"; +import { getDefaultLocalRoots } from "../../web/media.js"; +import type { ImageModelConfig } from "./image-tool.helpers.js"; +import { getApiKeyForModel, normalizeWorkspaceDir, requireApiKey } from "./tool-runtime.helpers.js"; + +type TextToolAttempt = { + provider: string; + model: string; + error: string; +}; + +type TextToolResult = { + text: string; + provider: string; + model: string; + attempts: TextToolAttempt[]; +}; + +export function applyImageModelConfigDefaults( + cfg: OpenClawConfig | undefined, + imageModelConfig: ImageModelConfig, +): OpenClawConfig | undefined { + if (!cfg) { + return undefined; + } + return { + ...cfg, + agents: { + ...cfg.agents, + defaults: { + ...cfg.agents?.defaults, + imageModel: imageModelConfig, + }, + }, + }; +} + +export function resolveMediaToolLocalRoots(workspaceDirRaw: string | undefined): string[] { + const roots = getDefaultLocalRoots(); + const workspaceDir = normalizeWorkspaceDir(workspaceDirRaw); + if (!workspaceDir) { + return [...roots]; + } + return Array.from(new Set([...roots, workspaceDir])); +} + +export function resolvePromptAndModelOverride( + args: Record, + defaultPrompt: string, +): { + prompt: string; + modelOverride?: string; +} { + const prompt = + typeof args.prompt === "string" && args.prompt.trim() ? args.prompt.trim() : defaultPrompt; + const modelOverride = + typeof args.model === "string" && args.model.trim() ? args.model.trim() : undefined; + return { prompt, modelOverride }; +} + +export function buildTextToolResult( + result: TextToolResult, + extraDetails: Record, +): { + content: Array<{ type: "text"; text: string }>; + details: Record; +} { + return { + content: [{ type: "text", text: result.text }], + details: { + model: `${result.provider}/${result.model}`, + ...extraDetails, + attempts: result.attempts, + }, + }; +} + +export function resolveModelFromRegistry(params: { + modelRegistry: { find: (provider: string, modelId: string) => unknown }; + provider: string; + modelId: string; +}): Model { + const model = params.modelRegistry.find(params.provider, params.modelId) as Model | null; + if (!model) { + throw new Error(`Unknown model: ${params.provider}/${params.modelId}`); + } + return model; +} + +export async function resolveModelRuntimeApiKey(params: { + model: Model; + cfg: OpenClawConfig | undefined; + agentDir: string; + authStorage: { + setRuntimeApiKey: (provider: string, apiKey: string) => void; + }; +}): Promise { + const apiKeyInfo = await getApiKeyForModel({ + model: params.model, + cfg: params.cfg, + agentDir: params.agentDir, + }); + const apiKey = requireApiKey(apiKeyInfo, params.model.provider); + params.authStorage.setRuntimeApiKey(params.model.provider, apiKey); + return apiKey; +} diff --git a/src/agents/tools/pdf-tool.ts b/src/agents/tools/pdf-tool.ts index 5df6a95cae1..34d1a548219 100644 --- a/src/agents/tools/pdf-tool.ts +++ b/src/agents/tools/pdf-tool.ts @@ -1,14 +1,22 @@ -import { type Api, type Context, complete, type Model } from "@mariozechner/pi-ai"; +import { type Context, complete } from "@mariozechner/pi-ai"; import { Type } from "@sinclair/typebox"; import type { OpenClawConfig } from "../../config/config.js"; import { extractPdfContent, type PdfExtractedContent } from "../../media/pdf-extract.js"; import { resolveUserPath } from "../../utils.js"; -import { getDefaultLocalRoots, loadWebMediaRaw } from "../../web/media.js"; +import { loadWebMediaRaw } from "../../web/media.js"; import { coerceImageModelConfig, type ImageModelConfig, resolveProviderVisionModelFromConfig, } from "./image-tool.helpers.js"; +import { + applyImageModelConfigDefaults, + buildTextToolResult, + resolveMediaToolLocalRoots, + resolveModelFromRegistry, + resolveModelRuntimeApiKey, + resolvePromptAndModelOverride, +} from "./media-tool-shared.js"; import { hasAuthForProvider, resolveDefaultModelRef } from "./model-config.helpers.js"; import { anthropicAnalyzePdf, geminiAnalyzePdf } from "./pdf-native-providers.js"; import { @@ -23,9 +31,6 @@ import { discoverAuthStorage, discoverModels, ensureOpenClawModelsJson, - getApiKeyForModel, - normalizeWorkspaceDir, - requireApiKey, resolveSandboxedBridgeMediaPath, runWithImageModelFallback, type AnyAgentTool, @@ -176,18 +181,7 @@ async function runPdfPrompt(params: { native: boolean; attempts: Array<{ provider: string; model: string; error: string }>; }> { - const effectiveCfg: OpenClawConfig | undefined = params.cfg - ? { - ...params.cfg, - agents: { - ...params.cfg.agents, - defaults: { - ...params.cfg.agents?.defaults, - imageModel: params.pdfModelConfig, - }, - }, - } - : undefined; + const effectiveCfg = applyImageModelConfigDefaults(params.cfg, params.pdfModelConfig); await ensureOpenClawModelsJson(effectiveCfg, params.agentDir); const authStorage = discoverAuthStorage(params.agentDir); @@ -205,18 +199,13 @@ async function runPdfPrompt(params: { cfg: effectiveCfg, modelOverride: params.modelOverride, run: async (provider, modelId) => { - const model = modelRegistry.find(provider, modelId) as Model | null; - if (!model) { - throw new Error(`Unknown model: ${provider}/${modelId}`); - } - - const apiKeyInfo = await getApiKeyForModel({ + const model = resolveModelFromRegistry({ modelRegistry, provider, modelId }); + const apiKey = await resolveModelRuntimeApiKey({ model, cfg: effectiveCfg, agentDir: params.agentDir, + authStorage, }); - const apiKey = requireApiKey(apiKeyInfo, model.provider); - authStorage.setRuntimeApiKey(model.provider, apiKey); if (providerSupportsNativePdf(provider)) { if (params.pageNumbers && params.pageNumbers.length > 0) { @@ -338,6 +327,7 @@ export function createPdfTool(options?: { ? Math.floor(maxPagesDefault) : DEFAULT_MAX_PAGES; +<<<<<<< HEAD const localRoots = (() => { const workspaceDir = normalizeWorkspaceDir(options?.workspaceDir); if (options?.fsPolicy?.workspaceOnly) { @@ -349,6 +339,18 @@ export function createPdfTool(options?: { } return Array.from(new Set([...roots, workspaceDir])); })(); +||||||| parent of 4a741746c (refactor: dedupe agent and reply runtimes) + const localRoots = (() => { + const roots = getDefaultLocalRoots(); + const workspaceDir = normalizeWorkspaceDir(options?.workspaceDir); + if (!workspaceDir) { + return roots; + } + return Array.from(new Set([...roots, workspaceDir])); + })(); +======= + const localRoots = resolveMediaToolLocalRoots(options?.workspaceDir); +>>>>>>> 4a741746c (refactor: dedupe agent and reply runtimes) const description = "Analyze one or more PDF documents with a model. Supports native PDF analysis for Anthropic and Google models, with text/image extraction fallback for other providers. Use pdf for a single path/URL, or pdfs for multiple (up to 10). Provide a prompt describing what to analyze."; @@ -412,12 +414,10 @@ export function createPdfTool(options?: { }; } - const promptRaw = - typeof record.prompt === "string" && record.prompt.trim() - ? record.prompt.trim() - : DEFAULT_PROMPT; - const modelOverride = - typeof record.model === "string" && record.model.trim() ? record.model.trim() : undefined; + const { prompt: promptRaw, modelOverride } = resolvePromptAndModelOverride( + record, + DEFAULT_PROMPT, + ); const maxBytesMbRaw = typeof record.maxBytesMb === "number" ? record.maxBytesMb : undefined; const maxBytesMb = typeof maxBytesMbRaw === "number" && Number.isFinite(maxBytesMbRaw) && maxBytesMbRaw > 0 @@ -573,15 +573,7 @@ export function createPdfTool(options?: { })), }; - return { - content: [{ type: "text", text: result.text }], - details: { - model: `${result.provider}/${result.model}`, - native: result.native, - ...pdfDetails, - attempts: result.attempts, - }, - }; + return buildTextToolResult(result, { native: result.native, ...pdfDetails }); }, }; } diff --git a/src/agents/tools/sessions-helpers.ts b/src/agents/tools/sessions-helpers.ts index 6573b1e9cb5..7a244e32de0 100644 --- a/src/agents/tools/sessions-helpers.ts +++ b/src/agents/tools/sessions-helpers.ts @@ -23,6 +23,7 @@ export { resolveInternalSessionKey, resolveMainSessionAlias, resolveSessionReference, + resolveVisibleSessionReference, shouldResolveSessionIdInput, shouldVerifyRequesterSpawnedSessionVisibility, } from "./sessions-resolution.js"; diff --git a/src/agents/tools/sessions-history-tool.ts b/src/agents/tools/sessions-history-tool.ts index 18d9576f0b2..3d5deeadcdb 100644 --- a/src/agents/tools/sessions-history-tool.ts +++ b/src/agents/tools/sessions-history-tool.ts @@ -10,10 +10,10 @@ import { jsonResult, readStringParam } from "./common.js"; import { createSessionVisibilityGuard, createAgentToAgentPolicy, - isResolvedSessionVisibleToRequester, resolveEffectiveSessionToolsVisibility, resolveSessionReference, resolveSandboxedSessionToolContext, + resolveVisibleSessionReference, stripToolMessages, } from "./sessions-helpers.js"; @@ -197,23 +197,21 @@ export function createSessionsHistoryTool(opts?: { if (!resolvedSession.ok) { return jsonResult({ status: resolvedSession.status, error: resolvedSession.error }); } - // From here on, use the canonical key (sessionId inputs already resolved). - const resolvedKey = resolvedSession.key; - const displayKey = resolvedSession.displayKey; - const resolvedViaSessionId = resolvedSession.resolvedViaSessionId; - - const visible = await isResolvedSessionVisibleToRequester({ + const visibleSession = await resolveVisibleSessionReference({ + resolvedSession, requesterSessionKey: effectiveRequesterKey, - targetSessionKey: resolvedKey, restrictToSpawned, - resolvedViaSessionId, + visibilitySessionKey: sessionKeyParam, }); - if (!visible) { + if (!visibleSession.ok) { return jsonResult({ - status: "forbidden", - error: `Session not visible from this sandboxed agent session: ${sessionKeyParam}`, + status: visibleSession.status, + error: visibleSession.error, }); } + // From here on, use the canonical key (sessionId inputs already resolved). + const resolvedKey = visibleSession.key; + const displayKey = visibleSession.displayKey; const a2aPolicy = createAgentToAgentPolicy(cfg); const visibility = resolveEffectiveSessionToolsVisibility({ diff --git a/src/agents/tools/sessions-resolution.ts b/src/agents/tools/sessions-resolution.ts index f350adb1830..7eb730da09c 100644 --- a/src/agents/tools/sessions-resolution.ts +++ b/src/agents/tools/sessions-resolution.ts @@ -159,6 +159,19 @@ export type SessionReferenceResolution = } | { ok: false; status: "error" | "forbidden"; error: string }; +export type VisibleSessionReferenceResolution = + | { + ok: true; + key: string; + displayKey: string; + } + | { + ok: false; + status: "forbidden"; + error: string; + displayKey: string; + }; + async function resolveSessionKeyFromSessionId(params: { sessionId: string; alias: string; @@ -289,6 +302,31 @@ export async function resolveSessionReference(params: { return { ok: true, key: resolvedKey, displayKey, resolvedViaSessionId: false }; } +export async function resolveVisibleSessionReference(params: { + resolvedSession: Extract; + requesterSessionKey: string; + restrictToSpawned: boolean; + visibilitySessionKey: string; +}): Promise { + const resolvedKey = params.resolvedSession.key; + const displayKey = params.resolvedSession.displayKey; + const visible = await isResolvedSessionVisibleToRequester({ + requesterSessionKey: params.requesterSessionKey, + targetSessionKey: resolvedKey, + restrictToSpawned: params.restrictToSpawned, + resolvedViaSessionId: params.resolvedSession.resolvedViaSessionId, + }); + if (!visible) { + return { + ok: false, + status: "forbidden", + error: `Session not visible from this sandboxed agent session: ${params.visibilitySessionKey}`, + displayKey, + }; + } + return { ok: true, key: resolvedKey, displayKey }; +} + export function normalizeOptionalKey(value?: string) { return normalizeKey(value); } diff --git a/src/agents/tools/sessions-send-tool.ts b/src/agents/tools/sessions-send-tool.ts index bb1693c8469..82eff0adf7a 100644 --- a/src/agents/tools/sessions-send-tool.ts +++ b/src/agents/tools/sessions-send-tool.ts @@ -15,10 +15,10 @@ import { createSessionVisibilityGuard, createAgentToAgentPolicy, extractAssistantText, - isResolvedSessionVisibleToRequester, resolveEffectiveSessionToolsVisibility, resolveSessionReference, resolveSandboxedSessionToolContext, + resolveVisibleSessionReference, stripToolMessages, } from "./sessions-helpers.js"; import { buildAgentToAgentMessageContext, resolvePingPongTurns } from "./sessions-send-helpers.js"; @@ -171,25 +171,23 @@ export function createSessionsSendTool(opts?: { error: resolvedSession.error, }); } - // Normalize sessionKey/sessionId input into a canonical session key. - const resolvedKey = resolvedSession.key; - const displayKey = resolvedSession.displayKey; - const resolvedViaSessionId = resolvedSession.resolvedViaSessionId; - - const visible = await isResolvedSessionVisibleToRequester({ + const visibleSession = await resolveVisibleSessionReference({ + resolvedSession, requesterSessionKey: effectiveRequesterKey, - targetSessionKey: resolvedKey, restrictToSpawned, - resolvedViaSessionId, + visibilitySessionKey: sessionKey, }); - if (!visible) { + if (!visibleSession.ok) { return jsonResult({ runId: crypto.randomUUID(), - status: "forbidden", - error: `Session not visible from this sandboxed agent session: ${sessionKey}`, - sessionKey: displayKey, + status: visibleSession.status, + error: visibleSession.error, + sessionKey: visibleSession.displayKey, }); } + // Normalize sessionKey/sessionId input into a canonical session key. + const resolvedKey = visibleSession.key; + const displayKey = visibleSession.displayKey; const timeoutSeconds = typeof params.timeoutSeconds === "number" && Number.isFinite(params.timeoutSeconds) ? Math.max(0, Math.floor(params.timeoutSeconds)) diff --git a/src/auto-reply/reply/acp-projector.test.ts b/src/auto-reply/reply/acp-projector.test.ts index 57882b3b755..c3dc0c259ff 100644 --- a/src/auto-reply/reply/acp-projector.test.ts +++ b/src/auto-reply/reply/acp-projector.test.ts @@ -33,6 +33,107 @@ function expectToolCallSummary(delivery: Delivery | undefined) { expect(delivery?.text).toContain("Tool Call"); } +function createFinalOnlyStatusToolHarness() { + return createProjectorHarness({ + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 512, + deliveryMode: "final_only", + tagVisibility: { + available_commands_update: true, + tool_call: true, + }, + }, + }, + }); +} + +function createLiveToolLifecycleHarness(params?: { + coalesceIdleMs?: number; + maxChunkChars?: number; + maxSessionUpdateChars?: number; + repeatSuppression?: boolean; +}) { + return createProjectorHarness({ + acp: { + enabled: true, + stream: { + deliveryMode: "live", + ...params, + tagVisibility: { + tool_call: true, + tool_call_update: true, + }, + }, + }, + }); +} + +function createLiveStatusAndToolLifecycleHarness(params?: { + coalesceIdleMs?: number; + maxChunkChars?: number; + repeatSuppression?: boolean; +}) { + return createProjectorHarness({ + acp: { + enabled: true, + stream: { + deliveryMode: "live", + ...params, + tagVisibility: { + available_commands_update: true, + tool_call: true, + tool_call_update: true, + }, + }, + }, + }); +} + +async function runHiddenBoundaryCase(params: { + cfgOverrides?: Parameters[0]; + toolCallId: string; + includeNonTerminalUpdate?: boolean; + firstText?: string; + secondText?: string; + expectedText: string; +}) { + const { deliveries, projector } = createProjectorHarness(params.cfgOverrides); + await projector.onEvent({ + type: "text_delta", + text: params.firstText ?? "fallback.", + tag: "agent_message_chunk", + }); + await projector.onEvent({ + type: "tool_call", + tag: "tool_call", + toolCallId: params.toolCallId, + status: "in_progress", + title: "Run test", + text: "Run test (in_progress)", + }); + if (params.includeNonTerminalUpdate) { + await projector.onEvent({ + type: "tool_call", + tag: "tool_call_update", + toolCallId: params.toolCallId, + status: "in_progress", + title: "Run test", + text: "Run test (in_progress)", + }); + } + await projector.onEvent({ + type: "text_delta", + text: params.secondText ?? "I don't", + tag: "agent_message_chunk", + }); + await projector.flush(true); + + expect(combinedBlockText(deliveries)).toBe(params.expectedText); +} + describe("createAcpReplyProjector", () => { it("coalesces text deltas into bounded block chunks", async () => { const { deliveries, projector } = createProjectorHarness(); @@ -174,20 +275,7 @@ describe("createAcpReplyProjector", () => { }); it("supports deliveryMode=final_only by buffering all projected output until done", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 512, - deliveryMode: "final_only", - tagVisibility: { - available_commands_update: true, - tool_call: true, - }, - }, - }, - }); + const { deliveries, projector } = createFinalOnlyStatusToolHarness(); await projector.onEvent({ type: "text_delta", @@ -225,20 +313,7 @@ describe("createAcpReplyProjector", () => { }); it("flushes buffered status/tool output on error in deliveryMode=final_only", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 512, - deliveryMode: "final_only", - tagVisibility: { - available_commands_update: true, - tool_call: true, - }, - }, - }, - }); + const { deliveries, projector } = createFinalOnlyStatusToolHarness(); await projector.onEvent({ type: "status", @@ -329,18 +404,7 @@ describe("createAcpReplyProjector", () => { }); it("dedupes repeated tool lifecycle updates when repeatSuppression is enabled", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - deliveryMode: "live", - tagVisibility: { - tool_call: true, - tool_call_update: true, - }, - }, - }, - }); + const { deliveries, projector } = createLiveToolLifecycleHarness(); await projector.onEvent({ type: "tool_call", @@ -381,18 +445,8 @@ describe("createAcpReplyProjector", () => { }); it("keeps terminal tool updates even when rendered summaries are truncated", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - deliveryMode: "live", - maxSessionUpdateChars: 48, - tagVisibility: { - tool_call: true, - tool_call_update: true, - }, - }, - }, + const { deliveries, projector } = createLiveToolLifecycleHarness({ + maxSessionUpdateChars: 48, }); const longTitle = @@ -420,18 +474,7 @@ describe("createAcpReplyProjector", () => { }); it("renders fallback tool labels without leaking call ids as primary label", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - deliveryMode: "live", - tagVisibility: { - tool_call: true, - tool_call_update: true, - }, - }, - }, - }); + const { deliveries, projector } = createLiveToolLifecycleHarness(); await projector.onEvent({ type: "tool_call", @@ -446,21 +489,10 @@ describe("createAcpReplyProjector", () => { }); it("allows repeated status/tool summaries when repeatSuppression is disabled", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", - repeatSuppression: false, - tagVisibility: { - available_commands_update: true, - tool_call: true, - tool_call_update: true, - }, - }, - }, + const { deliveries, projector } = createLiveStatusAndToolLifecycleHarness({ + coalesceIdleMs: 0, + maxChunkChars: 256, + repeatSuppression: false, }); await projector.onEvent({ @@ -616,156 +648,96 @@ describe("createAcpReplyProjector", () => { }); it("inserts a space boundary before visible text after hidden tool updates by default", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", - }, - }, - }); - - await projector.onEvent({ type: "text_delta", text: "fallback.", tag: "agent_message_chunk" }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call", - toolCallId: "call_hidden_1", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", - }); - await projector.onEvent({ type: "text_delta", text: "I don't", tag: "agent_message_chunk" }); - await projector.flush(true); - - expect(combinedBlockText(deliveries)).toBe("fallback. I don't"); - }); - - it("preserves hidden boundary across nonterminal hidden tool updates", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", - tagVisibility: { - tool_call: false, - tool_call_update: false, + await runHiddenBoundaryCase({ + cfgOverrides: { + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 256, + deliveryMode: "live", }, }, }, + toolCallId: "call_hidden_1", + expectedText: "fallback. I don't", }); + }); - await projector.onEvent({ type: "text_delta", text: "fallback.", tag: "agent_message_chunk" }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call", + it("preserves hidden boundary across nonterminal hidden tool updates", async () => { + await runHiddenBoundaryCase({ + cfgOverrides: { + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 256, + deliveryMode: "live", + tagVisibility: { + tool_call: false, + tool_call_update: false, + }, + }, + }, + }, toolCallId: "hidden_boundary_1", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", + includeNonTerminalUpdate: true, + expectedText: "fallback. I don't", }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call_update", - toolCallId: "hidden_boundary_1", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", - }); - await projector.onEvent({ type: "text_delta", text: "I don't", tag: "agent_message_chunk" }); - await projector.flush(true); - - expect(combinedBlockText(deliveries)).toBe("fallback. I don't"); }); it("supports hiddenBoundarySeparator=space", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", - hiddenBoundarySeparator: "space", + await runHiddenBoundaryCase({ + cfgOverrides: { + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 256, + deliveryMode: "live", + hiddenBoundarySeparator: "space", + }, }, }, - }); - - await projector.onEvent({ type: "text_delta", text: "fallback.", tag: "agent_message_chunk" }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call", toolCallId: "call_hidden_2", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", + expectedText: "fallback. I don't", }); - await projector.onEvent({ type: "text_delta", text: "I don't", tag: "agent_message_chunk" }); - await projector.flush(true); - - expect(combinedBlockText(deliveries)).toBe("fallback. I don't"); }); it("supports hiddenBoundarySeparator=none", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", - hiddenBoundarySeparator: "none", + await runHiddenBoundaryCase({ + cfgOverrides: { + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 256, + deliveryMode: "live", + hiddenBoundarySeparator: "none", + }, }, }, - }); - - await projector.onEvent({ type: "text_delta", text: "fallback.", tag: "agent_message_chunk" }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call", toolCallId: "call_hidden_3", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", + expectedText: "fallback.I don't", }); - await projector.onEvent({ type: "text_delta", text: "I don't", tag: "agent_message_chunk" }); - await projector.flush(true); - - expect(combinedBlockText(deliveries)).toBe("fallback.I don't"); }); it("does not duplicate newlines when previous visible text already ends with newline", async () => { - const { deliveries, projector } = createProjectorHarness({ - acp: { - enabled: true, - stream: { - coalesceIdleMs: 0, - maxChunkChars: 256, - deliveryMode: "live", + await runHiddenBoundaryCase({ + cfgOverrides: { + acp: { + enabled: true, + stream: { + coalesceIdleMs: 0, + maxChunkChars: 256, + deliveryMode: "live", + }, }, }, - }); - - await projector.onEvent({ - type: "text_delta", - text: "fallback.\n", - tag: "agent_message_chunk", - }); - await projector.onEvent({ - type: "tool_call", - tag: "tool_call", toolCallId: "call_hidden_4", - status: "in_progress", - title: "Run test", - text: "Run test (in_progress)", + firstText: "fallback.\n", + expectedText: "fallback.\nI don't", }); - await projector.onEvent({ type: "text_delta", text: "I don't", tag: "agent_message_chunk" }); - await projector.flush(true); - - expect(combinedBlockText(deliveries)).toBe("fallback.\nI don't"); }); it("does not insert boundary separator for hidden non-tool status updates", async () => { diff --git a/src/auto-reply/reply/commands-context-report.ts b/src/auto-reply/reply/commands-context-report.ts index bf8b5f694b9..fd6df7d70a1 100644 --- a/src/auto-reply/reply/commands-context-report.ts +++ b/src/auto-reply/reply/commands-context-report.ts @@ -181,6 +181,20 @@ export async function buildContextReply(params: HandleCommandsParams): Promise[0]): boolean { - const channel = - params.ctx.OriginatingChannel ?? - params.command.channel ?? - params.ctx.Surface ?? - params.ctx.Provider; - return ( - String(channel ?? "") - .trim() - .toLowerCase() === "discord" - ); -} - -function resolveDiscordAccountId(params: Parameters[0]): string { - const accountId = typeof params.ctx.AccountId === "string" ? params.ctx.AccountId.trim() : ""; - return accountId || "default"; -} - function resolveSessionCommandUsage() { return "Usage: /session idle | /session max-age (example: /session idle 24h)"; } diff --git a/src/auto-reply/reply/commands-subagents/shared.ts b/src/auto-reply/reply/commands-subagents/shared.ts index 0d2b23a19b6..65149c0e55e 100644 --- a/src/auto-reply/reply/commands-subagents/shared.ts +++ b/src/auto-reply/reply/commands-subagents/shared.ts @@ -22,6 +22,7 @@ import { truncateLine, } from "../../../shared/subagents-format.js"; import type { CommandHandler, CommandHandlerResult } from "../commands-types.js"; +import { isDiscordSurface, resolveDiscordAccountId } from "../discord-context.js"; import { formatRunLabel, formatRunStatus, @@ -30,6 +31,7 @@ import { } from "../subagents-utils.js"; export { extractAssistantText, stripToolMessages }; +export { isDiscordSurface, resolveDiscordAccountId }; export const COMMAND = "/subagents"; export const COMMAND_KILL = "/kill"; @@ -267,24 +269,6 @@ export type FocusTargetResolution = { label?: string; }; -export function isDiscordSurface(params: SubagentsCommandParams): boolean { - const channel = - params.ctx.OriginatingChannel ?? - params.command.channel ?? - params.ctx.Surface ?? - params.ctx.Provider; - return ( - String(channel ?? "") - .trim() - .toLowerCase() === "discord" - ); -} - -export function resolveDiscordAccountId(params: SubagentsCommandParams): string { - const accountId = typeof params.ctx.AccountId === "string" ? params.ctx.AccountId.trim() : ""; - return accountId || "default"; -} - export function resolveDiscordChannelIdForFocus( params: SubagentsCommandParams, ): string | undefined { diff --git a/src/auto-reply/reply/discord-context.ts b/src/auto-reply/reply/discord-context.ts new file mode 100644 index 00000000000..2eb810d5e1d --- /dev/null +++ b/src/auto-reply/reply/discord-context.ts @@ -0,0 +1,35 @@ +type DiscordSurfaceParams = { + ctx: { + OriginatingChannel?: string; + Surface?: string; + Provider?: string; + AccountId?: string; + }; + command: { + channel?: string; + }; +}; + +type DiscordAccountParams = { + ctx: { + AccountId?: string; + }; +}; + +export function isDiscordSurface(params: DiscordSurfaceParams): boolean { + const channel = + params.ctx.OriginatingChannel ?? + params.command.channel ?? + params.ctx.Surface ?? + params.ctx.Provider; + return ( + String(channel ?? "") + .trim() + .toLowerCase() === "discord" + ); +} + +export function resolveDiscordAccountId(params: DiscordAccountParams): string { + const accountId = typeof params.ctx.AccountId === "string" ? params.ctx.AccountId.trim() : ""; + return accountId || "default"; +} diff --git a/src/auto-reply/reply/model-selection.test.ts b/src/auto-reply/reply/model-selection.test.ts index 493adec0515..5b90b34d4d5 100644 --- a/src/auto-reply/reply/model-selection.test.ts +++ b/src/auto-reply/reply/model-selection.test.ts @@ -68,6 +68,28 @@ describe("createModelSelectionState parent inheritance", () => { }); } + async function resolveStateWithParent(params: { + cfg: OpenClawConfig; + parentKey: string; + sessionKey: string; + parentEntry: ReturnType; + sessionEntry?: ReturnType; + parentSessionKey?: string; + }) { + const sessionEntry = params.sessionEntry ?? makeEntry(); + const sessionStore = { + [params.parentKey]: params.parentEntry, + [params.sessionKey]: sessionEntry, + }; + return resolveState({ + cfg: params.cfg, + sessionEntry, + sessionStore, + sessionKey: params.sessionKey, + parentSessionKey: params.parentSessionKey, + }); + } + it("inherits parent override from explicit parentSessionKey", async () => { const cfg = {} as OpenClawConfig; const parentKey = "agent:main:discord:channel:c1"; @@ -76,17 +98,11 @@ describe("createModelSelectionState parent inheritance", () => { providerOverride: "openai", modelOverride: "gpt-4o", }); - const sessionEntry = makeEntry(); - const sessionStore = { - [parentKey]: parentEntry, - [sessionKey]: sessionEntry, - }; - - const state = await resolveState({ + const state = await resolveStateWithParent({ cfg, - sessionEntry, - sessionStore, + parentKey, sessionKey, + parentEntry, parentSessionKey: parentKey, }); @@ -102,17 +118,11 @@ describe("createModelSelectionState parent inheritance", () => { providerOverride: "openai", modelOverride: "gpt-4o", }); - const sessionEntry = makeEntry(); - const sessionStore = { - [parentKey]: parentEntry, - [sessionKey]: sessionEntry, - }; - - const state = await resolveState({ + const state = await resolveStateWithParent({ cfg, - sessionEntry, - sessionStore, + parentKey, sessionKey, + parentEntry, }); expect(state.provider).toBe("openai"); @@ -131,15 +141,11 @@ describe("createModelSelectionState parent inheritance", () => { providerOverride: "anthropic", modelOverride: "claude-opus-4-5", }); - const sessionStore = { - [parentKey]: parentEntry, - [sessionKey]: sessionEntry, - }; - - const state = await resolveState({ + const state = await resolveStateWithParent({ cfg, + parentKey, + parentEntry, sessionEntry, - sessionStore, sessionKey, }); @@ -163,17 +169,11 @@ describe("createModelSelectionState parent inheritance", () => { providerOverride: "anthropic", modelOverride: "claude-opus-4-5", }); - const sessionEntry = makeEntry(); - const sessionStore = { - [parentKey]: parentEntry, - [sessionKey]: sessionEntry, - }; - - const state = await resolveState({ + const state = await resolveStateWithParent({ cfg, - sessionEntry, - sessionStore, + parentKey, sessionKey, + parentEntry, }); expect(state.provider).toBe(defaultProvider); diff --git a/src/browser/cdp-proxy-bypass.test.ts b/src/browser/cdp-proxy-bypass.test.ts index 1840005392e..138853eb0d5 100644 --- a/src/browser/cdp-proxy-bypass.test.ts +++ b/src/browser/cdp-proxy-bypass.test.ts @@ -8,6 +8,37 @@ import { withNoProxyForLocalhost, } from "./cdp-proxy-bypass.js"; +const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +async function withIsolatedNoProxyEnv(fn: () => Promise) { + const origNoProxy = process.env.NO_PROXY; + const origNoProxyLower = process.env.no_proxy; + const origHttpProxy = process.env.HTTP_PROXY; + delete process.env.NO_PROXY; + delete process.env.no_proxy; + process.env.HTTP_PROXY = "http://proxy:8080"; + + try { + await fn(); + } finally { + if (origHttpProxy !== undefined) { + process.env.HTTP_PROXY = origHttpProxy; + } else { + delete process.env.HTTP_PROXY; + } + if (origNoProxy !== undefined) { + process.env.NO_PROXY = origNoProxy; + } else { + delete process.env.NO_PROXY; + } + if (origNoProxyLower !== undefined) { + process.env.no_proxy = origNoProxyLower; + } else { + delete process.env.no_proxy; + } + } +} + describe("cdp-proxy-bypass", () => { describe("getDirectAgentForCdp", () => { it("returns http.Agent for http://localhost URLs", () => { @@ -170,17 +201,10 @@ describe("cdp-proxy-bypass", () => { describe("withNoProxyForLocalhost concurrency", () => { it("does not leak NO_PROXY when called concurrently", async () => { - const origNoProxy = process.env.NO_PROXY; - const origNoProxyLower = process.env.no_proxy; - delete process.env.NO_PROXY; - delete process.env.no_proxy; - process.env.HTTP_PROXY = "http://proxy:8080"; - - try { + await withIsolatedNoProxyEnv(async () => { const { withNoProxyForLocalhost } = await import("./cdp-proxy-bypass.js"); // Simulate concurrent calls - const delay = (ms: number) => new Promise((r) => setTimeout(r, ms)); const callA = withNoProxyForLocalhost(async () => { // While A is running, NO_PROXY should be set expect(process.env.NO_PROXY).toContain("localhost"); @@ -198,35 +222,15 @@ describe("withNoProxyForLocalhost concurrency", () => { // After both complete, NO_PROXY should be restored (deleted) expect(process.env.NO_PROXY).toBeUndefined(); expect(process.env.no_proxy).toBeUndefined(); - } finally { - delete process.env.HTTP_PROXY; - if (origNoProxy !== undefined) { - process.env.NO_PROXY = origNoProxy; - } else { - delete process.env.NO_PROXY; - } - if (origNoProxyLower !== undefined) { - process.env.no_proxy = origNoProxyLower; - } else { - delete process.env.no_proxy; - } - } + }); }); }); describe("withNoProxyForLocalhost reverse exit order", () => { it("restores NO_PROXY when first caller exits before second", async () => { - const origNoProxy = process.env.NO_PROXY; - const origNoProxyLower = process.env.no_proxy; - delete process.env.NO_PROXY; - delete process.env.no_proxy; - process.env.HTTP_PROXY = "http://proxy:8080"; - - try { + await withIsolatedNoProxyEnv(async () => { const { withNoProxyForLocalhost } = await import("./cdp-proxy-bypass.js"); - const delay = (ms: number) => new Promise((r) => setTimeout(r, ms)); - // Call A enters first, exits first (short task) // Call B enters second, exits last (long task) const callA = withNoProxyForLocalhost(async () => { @@ -243,19 +247,7 @@ describe("withNoProxyForLocalhost reverse exit order", () => { // After both complete, NO_PROXY must be cleaned up expect(process.env.NO_PROXY).toBeUndefined(); expect(process.env.no_proxy).toBeUndefined(); - } finally { - delete process.env.HTTP_PROXY; - if (origNoProxy !== undefined) { - process.env.NO_PROXY = origNoProxy; - } else { - delete process.env.NO_PROXY; - } - if (origNoProxyLower !== undefined) { - process.env.no_proxy = origNoProxyLower; - } else { - delete process.env.no_proxy; - } - } + }); }); }); diff --git a/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts b/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts index eb93eb00d64..f475d84d00b 100644 --- a/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts +++ b/src/browser/server-context.ensure-browser-available.waits-for-cdp-ready.test.ts @@ -13,6 +13,7 @@ vi.mock("./chrome.js", () => ({ })); import * as chromeModule from "./chrome.js"; +import type { RunningChrome } from "./chrome.js"; import type { BrowserServerState } from "./server-context.js"; import { createBrowserRouteContext } from "./server-context.js"; @@ -47,6 +48,21 @@ function makeBrowserState(): BrowserServerState { }; } +function mockLaunchedChrome( + launchOpenClawChrome: { mockResolvedValue: (value: RunningChrome) => unknown }, + pid: number, +) { + const proc = new EventEmitter() as unknown as ChildProcessWithoutNullStreams; + launchOpenClawChrome.mockResolvedValue({ + pid, + exe: { kind: "chromium", path: "/usr/bin/chromium" }, + userDataDir: "/tmp/openclaw-test", + cdpPort: 18800, + startedAt: Date.now(), + proc, + }); +} + afterEach(() => { vi.useRealTimers(); vi.clearAllMocks(); @@ -64,16 +80,7 @@ describe("browser server-context ensureBrowserAvailable", () => { isChromeReachable.mockResolvedValue(false); isChromeCdpReady.mockResolvedValueOnce(false).mockResolvedValue(true); - - const proc = new EventEmitter() as unknown as ChildProcessWithoutNullStreams; - launchOpenClawChrome.mockResolvedValue({ - pid: 123, - exe: { kind: "chromium", path: "/usr/bin/chromium" }, - userDataDir: "/tmp/openclaw-test", - cdpPort: 18800, - startedAt: Date.now(), - proc, - }); + mockLaunchedChrome(launchOpenClawChrome, 123); const state = makeBrowserState(); const ctx = createBrowserRouteContext({ getState: () => state }); @@ -98,16 +105,7 @@ describe("browser server-context ensureBrowserAvailable", () => { isChromeReachable.mockResolvedValue(false); isChromeCdpReady.mockResolvedValue(false); - - const proc = new EventEmitter() as unknown as ChildProcessWithoutNullStreams; - launchOpenClawChrome.mockResolvedValue({ - pid: 321, - exe: { kind: "chromium", path: "/usr/bin/chromium" }, - userDataDir: "/tmp/openclaw-test", - cdpPort: 18800, - startedAt: Date.now(), - proc, - }); + mockLaunchedChrome(launchOpenClawChrome, 321); const state = makeBrowserState(); const ctx = createBrowserRouteContext({ getState: () => state }); diff --git a/src/browser/server-context.remote-tab-ops.test.ts b/src/browser/server-context.remote-tab-ops.test.ts new file mode 100644 index 00000000000..b6127478390 --- /dev/null +++ b/src/browser/server-context.remote-tab-ops.test.ts @@ -0,0 +1,623 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { withFetchPreconnect } from "../test-utils/fetch-mock.js"; +import "./server-context.chrome-test-harness.js"; +import * as cdpModule from "./cdp.js"; +import * as chromeModule from "./chrome.js"; +import { InvalidBrowserNavigationUrlError } from "./navigation-guard.js"; +import * as pwAiModule from "./pw-ai-module.js"; +import type { BrowserServerState } from "./server-context.js"; +import { createBrowserRouteContext } from "./server-context.js"; + +const originalFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = originalFetch; + vi.restoreAllMocks(); +}); + +function makeState( + profile: "remote" | "openclaw", +): BrowserServerState & { profiles: Map } { + return { + // oxlint-disable-next-line typescript/no-explicit-any + server: null as any, + port: 0, + resolved: { + enabled: true, + controlPort: 18791, + cdpPortRangeStart: 18800, + cdpPortRangeEnd: 18899, + cdpProtocol: profile === "remote" ? "https" : "http", + cdpHost: profile === "remote" ? "browserless.example" : "127.0.0.1", + cdpIsLoopback: profile !== "remote", + remoteCdpTimeoutMs: 1500, + remoteCdpHandshakeTimeoutMs: 3000, + evaluateEnabled: false, + extraArgs: [], + color: "#FF4500", + headless: true, + noSandbox: false, + attachOnly: false, + ssrfPolicy: { allowPrivateNetwork: true }, + defaultProfile: profile, + profiles: { + remote: { + cdpUrl: "https://browserless.example/chrome?token=abc", + cdpPort: 443, + color: "#00AA00", + }, + openclaw: { cdpPort: 18800, color: "#FF4500" }, + }, + }, + profiles: new Map(), + }; +} + +function makeUnexpectedFetchMock() { + return vi.fn(async () => { + throw new Error("unexpected fetch"); + }); +} + +function createRemoteRouteHarness(fetchMock?: ReturnType) { + const activeFetchMock = fetchMock ?? makeUnexpectedFetchMock(); + global.fetch = withFetchPreconnect(activeFetchMock); + const state = makeState("remote"); + const ctx = createBrowserRouteContext({ getState: () => state }); + return { state, remote: ctx.forProfile("remote"), fetchMock: activeFetchMock }; +} + +function createSequentialPageLister(responses: T[]) { + return vi.fn(async () => { + const next = responses.shift(); + if (!next) { + throw new Error("no more responses"); + } + return next; + }); +} + +type JsonListEntry = { + id: string; + title: string; + url: string; + webSocketDebuggerUrl: string; + type: "page"; +}; + +function createJsonListFetchMock(entries: JsonListEntry[]) { + return vi.fn(async (url: unknown) => { + const u = String(url); + if (!u.includes("/json/list")) { + throw new Error(`unexpected fetch: ${u}`); + } + return { + ok: true, + json: async () => entries, + } as unknown as Response; + }); +} + +function createOpenclawManagedTab(id: string, index: number): JsonListEntry { + return { + id, + title: String(index), + url: `http://127.0.0.1:300${index}`, + webSocketDebuggerUrl: `ws://127.0.0.1/devtools/page/${id}`, + type: "page", + }; +} + +function createOpenclawManagedTabs(params?: { + includeNew?: boolean; + newFirst?: boolean; +}): JsonListEntry[] { + const oldTabs = Array.from({ length: 8 }, (_, idx) => + createOpenclawManagedTab(`OLD${idx + 1}`, idx + 1), + ); + if (params?.includeNew === false) { + return oldTabs; + } + const newTab = createOpenclawManagedTab("NEW", 9); + return params?.newFirst ? [newTab, ...oldTabs] : [...oldTabs, newTab]; +} + +function createOpenclawRouteHarness( + fetchMock: ReturnType, + params?: { attachOnly?: boolean; seedRunningProfile?: boolean }, +) { + global.fetch = withFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + if (params?.attachOnly) { + state.resolved.attachOnly = true; + } + if (params?.seedRunningProfile ?? true) { + (state.profiles as Map).set("openclaw", { + profile: { name: "openclaw" }, + running: { pid: 1234, proc: { on: vi.fn() } }, + lastTargetId: null, + }); + } + const ctx = createBrowserRouteContext({ getState: () => state }); + return { state, openclaw: ctx.forProfile("openclaw") }; +} + +function createJsonOkResponse(payload: unknown): Response { + return { + ok: true, + json: async () => payload, + } as unknown as Response; +} + +function createManagedTabsFetchMock(params: { + existingTabs: JsonListEntry[]; + onClose?: (url: string) => Promise | Response; +}) { + return vi.fn(async (url: unknown) => { + const value = String(url); + if (value.includes("/json/list")) { + return createJsonOkResponse(params.existingTabs); + } + if (value.includes("/json/close/")) { + if (params.onClose) { + return params.onClose(value); + } + throw new Error(`unexpected fetch: ${value}`); + } + throw new Error(`unexpected fetch: ${value}`); + }); +} + +async function expectManagedOldestTabClose(fetchMock: ReturnType) { + await vi.waitFor(() => { + expect(fetchMock).toHaveBeenCalledWith( + expect.stringContaining("/json/close/OLD1"), + expect.any(Object), + ); + }); +} + +describe("browser server-context remote profile tab operations", () => { + it("uses profile-level attachOnly when global attachOnly is false", async () => { + const state = makeState("openclaw"); + state.resolved.attachOnly = false; + state.resolved.profiles.openclaw = { + cdpPort: 18800, + attachOnly: true, + color: "#FF4500", + }; + + const reachableMock = vi.mocked(chromeModule.isChromeReachable).mockResolvedValueOnce(false); + const launchMock = vi.mocked(chromeModule.launchOpenClawChrome); + const ctx = createBrowserRouteContext({ getState: () => state }); + + await expect(ctx.forProfile("openclaw").ensureBrowserAvailable()).rejects.toThrow( + /attachOnly is enabled/i, + ); + expect(reachableMock).toHaveBeenCalled(); + expect(launchMock).not.toHaveBeenCalled(); + }); + + it("keeps attachOnly websocket failures off the loopback ownership error path", async () => { + const state = makeState("openclaw"); + state.resolved.attachOnly = false; + state.resolved.profiles.openclaw = { + cdpPort: 18800, + attachOnly: true, + color: "#FF4500", + }; + + const httpReachableMock = vi.mocked(chromeModule.isChromeReachable).mockResolvedValueOnce(true); + const wsReachableMock = vi.mocked(chromeModule.isChromeCdpReady).mockResolvedValueOnce(false); + const launchMock = vi.mocked(chromeModule.launchOpenClawChrome); + const ctx = createBrowserRouteContext({ getState: () => state }); + + await expect(ctx.forProfile("openclaw").ensureBrowserAvailable()).rejects.toThrow( + /attachOnly is enabled and CDP websocket/i, + ); + expect(httpReachableMock).toHaveBeenCalled(); + expect(wsReachableMock).toHaveBeenCalled(); + expect(launchMock).not.toHaveBeenCalled(); + }); + + it("uses Playwright tab operations when available", async () => { + const listPagesViaPlaywright = vi.fn(async () => [ + { targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }, + ]); + const createPageViaPlaywright = vi.fn(async () => ({ + targetId: "T2", + title: "Tab 2", + url: "http://127.0.0.1:3000", + type: "page", + })); + const closePageByTargetIdViaPlaywright = vi.fn(async () => {}); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + createPageViaPlaywright, + closePageByTargetIdViaPlaywright, + } as unknown as Awaited>); + + const { state, remote, fetchMock } = createRemoteRouteHarness(); + + const tabs = await remote.listTabs(); + expect(tabs.map((t) => t.targetId)).toEqual(["T1"]); + + const opened = await remote.openTab("http://127.0.0.1:3000"); + expect(opened.targetId).toBe("T2"); + expect(state.profiles.get("remote")?.lastTargetId).toBe("T2"); + expect(createPageViaPlaywright).toHaveBeenCalledWith({ + cdpUrl: "https://browserless.example/chrome?token=abc", + url: "http://127.0.0.1:3000", + ssrfPolicy: { allowPrivateNetwork: true }, + }); + + await remote.closeTab("T1"); + expect(closePageByTargetIdViaPlaywright).toHaveBeenCalledWith({ + cdpUrl: "https://browserless.example/chrome?token=abc", + targetId: "T1", + }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("prefers lastTargetId for remote profiles when targetId is omitted", async () => { + const responses = [ + // ensureTabAvailable() calls listTabs twice + [ + { targetId: "A", title: "A", url: "https://example.com", type: "page" }, + { targetId: "B", title: "B", url: "https://www.example.com", type: "page" }, + ], + [ + { targetId: "A", title: "A", url: "https://example.com", type: "page" }, + { targetId: "B", title: "B", url: "https://www.example.com", type: "page" }, + ], + // second ensureTabAvailable() calls listTabs twice, order flips + [ + { targetId: "B", title: "B", url: "https://www.example.com", type: "page" }, + { targetId: "A", title: "A", url: "https://example.com", type: "page" }, + ], + [ + { targetId: "B", title: "B", url: "https://www.example.com", type: "page" }, + { targetId: "A", title: "A", url: "https://example.com", type: "page" }, + ], + ]; + + const listPagesViaPlaywright = vi.fn(async () => { + const next = responses.shift(); + if (!next) { + throw new Error("no more responses"); + } + return next; + }); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + createPageViaPlaywright: vi.fn(async () => { + throw new Error("unexpected create"); + }), + closePageByTargetIdViaPlaywright: vi.fn(async () => { + throw new Error("unexpected close"); + }), + } as unknown as Awaited>); + + const { remote } = createRemoteRouteHarness(); + + const first = await remote.ensureTabAvailable(); + expect(first.targetId).toBe("A"); + const second = await remote.ensureTabAvailable(); + expect(second.targetId).toBe("A"); + }); + + it("falls back to the only tab for remote profiles when targetId is stale", async () => { + const responses = [ + [{ targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }], + [{ targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }], + ]; + const listPagesViaPlaywright = createSequentialPageLister(responses); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + } as unknown as Awaited>); + + const { remote } = createRemoteRouteHarness(); + const chosen = await remote.ensureTabAvailable("STALE_TARGET"); + expect(chosen.targetId).toBe("T1"); + }); + + it("keeps rejecting stale targetId for remote profiles when multiple tabs exist", async () => { + const responses = [ + [ + { targetId: "A", title: "A", url: "https://a.example", type: "page" }, + { targetId: "B", title: "B", url: "https://b.example", type: "page" }, + ], + [ + { targetId: "A", title: "A", url: "https://a.example", type: "page" }, + { targetId: "B", title: "B", url: "https://b.example", type: "page" }, + ], + ]; + const listPagesViaPlaywright = createSequentialPageLister(responses); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + } as unknown as Awaited>); + + const { remote } = createRemoteRouteHarness(); + await expect(remote.ensureTabAvailable("STALE_TARGET")).rejects.toThrow(/tab not found/i); + }); + + it("uses Playwright focus for remote profiles when available", async () => { + const listPagesViaPlaywright = vi.fn(async () => [ + { targetId: "T1", title: "Tab 1", url: "https://example.com", type: "page" }, + ]); + const focusPageByTargetIdViaPlaywright = vi.fn(async () => {}); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + focusPageByTargetIdViaPlaywright, + } as unknown as Awaited>); + + const { state, remote, fetchMock } = createRemoteRouteHarness(); + + await remote.focusTab("T1"); + expect(focusPageByTargetIdViaPlaywright).toHaveBeenCalledWith({ + cdpUrl: "https://browserless.example/chrome?token=abc", + targetId: "T1", + }); + expect(fetchMock).not.toHaveBeenCalled(); + expect(state.profiles.get("remote")?.lastTargetId).toBe("T1"); + }); + + it("does not swallow Playwright runtime errors for remote profiles", async () => { + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright: vi.fn(async () => { + throw new Error("boom"); + }), + } as unknown as Awaited>); + + const { remote, fetchMock } = createRemoteRouteHarness(); + + await expect(remote.listTabs()).rejects.toThrow(/boom/); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("falls back to /json/list when Playwright is not available", async () => { + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue(null); + + const fetchMock = createJsonListFetchMock([ + { + id: "T1", + title: "Tab 1", + url: "https://example.com", + webSocketDebuggerUrl: "wss://browserless.example/devtools/page/T1", + type: "page", + }, + ]); + + const { remote } = createRemoteRouteHarness(fetchMock); + + const tabs = await remote.listTabs(); + expect(tabs.map((t) => t.targetId)).toEqual(["T1"]); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not enforce managed tab cap for remote openclaw profiles", async () => { + const listPagesViaPlaywright = vi + .fn() + .mockResolvedValueOnce([ + { targetId: "T1", title: "1", url: "https://1.example", type: "page" }, + ]) + .mockResolvedValueOnce([ + { targetId: "T1", title: "1", url: "https://1.example", type: "page" }, + { targetId: "T2", title: "2", url: "https://2.example", type: "page" }, + { targetId: "T3", title: "3", url: "https://3.example", type: "page" }, + { targetId: "T4", title: "4", url: "https://4.example", type: "page" }, + { targetId: "T5", title: "5", url: "https://5.example", type: "page" }, + { targetId: "T6", title: "6", url: "https://6.example", type: "page" }, + { targetId: "T7", title: "7", url: "https://7.example", type: "page" }, + { targetId: "T8", title: "8", url: "https://8.example", type: "page" }, + { targetId: "T9", title: "9", url: "https://9.example", type: "page" }, + ]); + + const createPageViaPlaywright = vi.fn(async () => ({ + targetId: "T1", + title: "Tab 1", + url: "https://1.example", + type: "page", + })); + + vi.spyOn(pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + createPageViaPlaywright, + } as unknown as Awaited>); + + const fetchMock = vi.fn(async (url: unknown) => { + throw new Error(`unexpected fetch: ${String(url)}`); + }); + + const { remote } = createRemoteRouteHarness(fetchMock); + const opened = await remote.openTab("https://1.example"); + expect(opened.targetId).toBe("T1"); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); + +describe("browser server-context tab selection state", () => { + it("updates lastTargetId when openTab is created via CDP", async () => { + const createTargetViaCdp = vi + .spyOn(cdpModule, "createTargetViaCdp") + .mockResolvedValue({ targetId: "CREATED" }); + + const fetchMock = createJsonListFetchMock([ + { + id: "CREATED", + title: "New Tab", + url: "http://127.0.0.1:8080", + webSocketDebuggerUrl: "ws://127.0.0.1/devtools/page/CREATED", + type: "page", + }, + ]); + + global.fetch = withFetchPreconnect(fetchMock); + + const state = makeState("openclaw"); + const ctx = createBrowserRouteContext({ getState: () => state }); + const openclaw = ctx.forProfile("openclaw"); + + const opened = await openclaw.openTab("http://127.0.0.1:8080"); + expect(opened.targetId).toBe("CREATED"); + expect(state.profiles.get("openclaw")?.lastTargetId).toBe("CREATED"); + expect(createTargetViaCdp).toHaveBeenCalledWith({ + cdpUrl: "http://127.0.0.1:18800", + url: "http://127.0.0.1:8080", + ssrfPolicy: { allowPrivateNetwork: true }, + }); + }); + + it("closes excess managed tabs after opening a new tab", async () => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "NEW" }); + const existingTabs = createOpenclawManagedTabs(); + + const fetchMock = createManagedTabsFetchMock({ + existingTabs, + onClose: (value) => { + if (value.includes("/json/close/OLD1")) { + return createJsonOkResponse({}); + } + throw new Error(`unexpected fetch: ${value}`); + }, + }); + + const { openclaw } = createOpenclawRouteHarness(fetchMock); + + const opened = await openclaw.openTab("http://127.0.0.1:3009"); + expect(opened.targetId).toBe("NEW"); + await expectManagedOldestTabClose(fetchMock); + }); + + it("never closes the just-opened managed tab during cap cleanup", async () => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "NEW" }); + const existingTabs = createOpenclawManagedTabs({ newFirst: true }); + + const fetchMock = createManagedTabsFetchMock({ + existingTabs, + onClose: (value) => { + if (value.includes("/json/close/OLD1")) { + return createJsonOkResponse({}); + } + if (value.includes("/json/close/NEW")) { + throw new Error("cleanup must not close NEW"); + } + throw new Error(`unexpected fetch: ${value}`); + }, + }); + + const { openclaw } = createOpenclawRouteHarness(fetchMock); + + const opened = await openclaw.openTab("http://127.0.0.1:3009"); + expect(opened.targetId).toBe("NEW"); + await expectManagedOldestTabClose(fetchMock); + expect(fetchMock).not.toHaveBeenCalledWith( + expect.stringContaining("/json/close/NEW"), + expect.anything(), + ); + }); + + it("does not fail tab open when managed-tab cleanup list fails", async () => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "NEW" }); + + let listCount = 0; + const fetchMock = vi.fn(async (url: unknown) => { + const value = String(url); + if (value.includes("/json/list")) { + listCount += 1; + if (listCount === 1) { + return { + ok: true, + json: async () => [ + { + id: "NEW", + title: "New Tab", + url: "http://127.0.0.1:3009", + webSocketDebuggerUrl: "ws://127.0.0.1/devtools/page/NEW", + type: "page", + }, + ], + } as unknown as Response; + } + throw new Error("/json/list timeout"); + } + throw new Error(`unexpected fetch: ${value}`); + }); + + const { openclaw } = createOpenclawRouteHarness(fetchMock); + + const opened = await openclaw.openTab("http://127.0.0.1:3009"); + expect(opened.targetId).toBe("NEW"); + }); + + it("does not run managed tab cleanup in attachOnly mode", async () => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "NEW" }); + const existingTabs = createOpenclawManagedTabs(); + + const fetchMock = createManagedTabsFetchMock({ + existingTabs, + onClose: (_value) => { + throw new Error("should not close tabs in attachOnly mode"); + }, + }); + + const { openclaw } = createOpenclawRouteHarness(fetchMock, { + attachOnly: true, + seedRunningProfile: false, + }); + + const opened = await openclaw.openTab("http://127.0.0.1:3009"); + expect(opened.targetId).toBe("NEW"); + expect(fetchMock).not.toHaveBeenCalledWith( + expect.stringContaining("/json/close/"), + expect.anything(), + ); + }); + + it("does not block openTab on slow best-effort cleanup closes", async () => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ targetId: "NEW" }); + const existingTabs = createOpenclawManagedTabs(); + + const fetchMock = createManagedTabsFetchMock({ + existingTabs, + onClose: (value) => { + if (value.includes("/json/close/OLD1")) { + return new Promise(() => {}); + } + throw new Error(`unexpected fetch: ${value}`); + }, + }); + + const { openclaw } = createOpenclawRouteHarness(fetchMock); + + const opened = await Promise.race([ + openclaw.openTab("http://127.0.0.1:3009"), + new Promise((_, reject) => + setTimeout(() => reject(new Error("openTab timed out waiting for cleanup")), 300), + ), + ]); + + expect(opened.targetId).toBe("NEW"); + }); + + it("blocks unsupported non-network URLs before any HTTP tab-open fallback", async () => { + const fetchMock = vi.fn(async () => { + throw new Error("unexpected fetch"); + }); + + global.fetch = withFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + const ctx = createBrowserRouteContext({ getState: () => state }); + const openclaw = ctx.forProfile("openclaw"); + + await expect(openclaw.openTab("file:///etc/passwd")).rejects.toBeInstanceOf( + InvalidBrowserNavigationUrlError, + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/logger.ts b/src/logger.ts index 4ae1cb20d53..f8b94b0764f 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -14,44 +14,68 @@ function splitSubsystem(message: string) { return { subsystem, rest }; } -export function logInfo(message: string, runtime: RuntimeEnv = defaultRuntime) { - const parsed = runtime === defaultRuntime ? splitSubsystem(message) : null; +type LogMethod = "info" | "warn" | "error"; +type RuntimeMethod = "log" | "error"; + +function logWithSubsystem(params: { + message: string; + runtime: RuntimeEnv; + runtimeMethod: RuntimeMethod; + runtimeFormatter: (value: string) => string; + loggerMethod: LogMethod; + subsystemMethod: LogMethod; +}) { + const parsed = params.runtime === defaultRuntime ? splitSubsystem(params.message) : null; if (parsed) { - createSubsystemLogger(parsed.subsystem).info(parsed.rest); + createSubsystemLogger(parsed.subsystem)[params.subsystemMethod](parsed.rest); return; } - runtime.log(info(message)); - getLogger().info(message); + params.runtime[params.runtimeMethod](params.runtimeFormatter(params.message)); + getLogger()[params.loggerMethod](params.message); +} + +export function logInfo(message: string, runtime: RuntimeEnv = defaultRuntime) { + logWithSubsystem({ + message, + runtime, + runtimeMethod: "log", + runtimeFormatter: info, + loggerMethod: "info", + subsystemMethod: "info", + }); } export function logWarn(message: string, runtime: RuntimeEnv = defaultRuntime) { - const parsed = runtime === defaultRuntime ? splitSubsystem(message) : null; - if (parsed) { - createSubsystemLogger(parsed.subsystem).warn(parsed.rest); - return; - } - runtime.log(warn(message)); - getLogger().warn(message); + logWithSubsystem({ + message, + runtime, + runtimeMethod: "log", + runtimeFormatter: warn, + loggerMethod: "warn", + subsystemMethod: "warn", + }); } export function logSuccess(message: string, runtime: RuntimeEnv = defaultRuntime) { - const parsed = runtime === defaultRuntime ? splitSubsystem(message) : null; - if (parsed) { - createSubsystemLogger(parsed.subsystem).info(parsed.rest); - return; - } - runtime.log(success(message)); - getLogger().info(message); + logWithSubsystem({ + message, + runtime, + runtimeMethod: "log", + runtimeFormatter: success, + loggerMethod: "info", + subsystemMethod: "info", + }); } export function logError(message: string, runtime: RuntimeEnv = defaultRuntime) { - const parsed = runtime === defaultRuntime ? splitSubsystem(message) : null; - if (parsed) { - createSubsystemLogger(parsed.subsystem).error(parsed.rest); - return; - } - runtime.error(danger(message)); - getLogger().error(message); + logWithSubsystem({ + message, + runtime, + runtimeMethod: "error", + runtimeFormatter: danger, + loggerMethod: "error", + subsystemMethod: "error", + }); } export function logDebug(message: string) { diff --git a/src/logging/console-capture.test.ts b/src/logging/console-capture.test.ts index 42339c195bf..87827c23927 100644 --- a/src/logging/console-capture.test.ts +++ b/src/logging/console-capture.test.ts @@ -10,27 +10,16 @@ import { setLoggerOverride, } from "../logging.js"; import { loggingState } from "./state.js"; - -type ConsoleSnapshot = { - log: typeof console.log; - info: typeof console.info; - warn: typeof console.warn; - error: typeof console.error; - debug: typeof console.debug; - trace: typeof console.trace; -}; +import { + captureConsoleSnapshot, + type ConsoleSnapshot, + restoreConsoleSnapshot, +} from "./test-helpers/console-snapshot.js"; let snapshot: ConsoleSnapshot; beforeEach(() => { - snapshot = { - log: console.log, - info: console.info, - warn: console.warn, - error: console.error, - debug: console.debug, - trace: console.trace, - }; + snapshot = captureConsoleSnapshot(); loggingState.consolePatched = false; loggingState.forceConsoleToStderr = false; loggingState.consoleTimestampPrefix = false; @@ -39,12 +28,7 @@ beforeEach(() => { }); afterEach(() => { - console.log = snapshot.log; - console.info = snapshot.info; - console.warn = snapshot.warn; - console.error = snapshot.error; - console.debug = snapshot.debug; - console.trace = snapshot.trace; + restoreConsoleSnapshot(snapshot); loggingState.consolePatched = false; loggingState.forceConsoleToStderr = false; loggingState.consoleTimestampPrefix = false; diff --git a/src/logging/console-settings.test.ts b/src/logging/console-settings.test.ts index 6431b67c8e9..7beeee111c9 100644 --- a/src/logging/console-settings.test.ts +++ b/src/logging/console-settings.test.ts @@ -1,4 +1,9 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { + captureConsoleSnapshot, + type ConsoleSnapshot, + restoreConsoleSnapshot, +} from "./test-helpers/console-snapshot.js"; vi.mock("./config.js", () => ({ readLoggingConfig: () => undefined, @@ -16,15 +21,6 @@ vi.mock("./logger.js", () => ({ })); let loadConfigCalls = 0; -type ConsoleSnapshot = { - log: typeof console.log; - info: typeof console.info; - warn: typeof console.warn; - error: typeof console.error; - debug: typeof console.debug; - trace: typeof console.trace; -}; - let originalIsTty: boolean | undefined; let originalOpenClawTestConsole: string | undefined; let snapshot: ConsoleSnapshot; @@ -38,14 +34,7 @@ beforeAll(async () => { beforeEach(() => { loadConfigCalls = 0; - snapshot = { - log: console.log, - info: console.info, - warn: console.warn, - error: console.error, - debug: console.debug, - trace: console.trace, - }; + snapshot = captureConsoleSnapshot(); originalIsTty = process.stdout.isTTY; originalOpenClawTestConsole = process.env.OPENCLAW_TEST_CONSOLE; process.env.OPENCLAW_TEST_CONSOLE = "1"; @@ -53,6 +42,7 @@ beforeEach(() => { }); afterEach(() => { +<<<<<<< HEAD console.log = snapshot.log; console.info = snapshot.info; console.warn = snapshot.warn; @@ -64,6 +54,16 @@ afterEach(() => { } else { process.env.OPENCLAW_TEST_CONSOLE = originalOpenClawTestConsole; } +||||||| parent of 4a741746c (refactor: dedupe agent and reply runtimes) + console.log = snapshot.log; + console.info = snapshot.info; + console.warn = snapshot.warn; + console.error = snapshot.error; + console.debug = snapshot.debug; + console.trace = snapshot.trace; +======= + restoreConsoleSnapshot(snapshot); +>>>>>>> 4a741746c (refactor: dedupe agent and reply runtimes) Object.defineProperty(process.stdout, "isTTY", { value: originalIsTty, configurable: true }); logging.setConsoleConfigLoaderForTests(); vi.restoreAllMocks(); diff --git a/src/logging/test-helpers/console-snapshot.ts b/src/logging/test-helpers/console-snapshot.ts new file mode 100644 index 00000000000..d6b1f1ee36f --- /dev/null +++ b/src/logging/test-helpers/console-snapshot.ts @@ -0,0 +1,28 @@ +export type ConsoleSnapshot = { + log: typeof console.log; + info: typeof console.info; + warn: typeof console.warn; + error: typeof console.error; + debug: typeof console.debug; + trace: typeof console.trace; +}; + +export function captureConsoleSnapshot(): ConsoleSnapshot { + return { + log: console.log, + info: console.info, + warn: console.warn, + error: console.error, + debug: console.debug, + trace: console.trace, + }; +} + +export function restoreConsoleSnapshot(snapshot: ConsoleSnapshot): void { + console.log = snapshot.log; + console.info = snapshot.info; + console.warn = snapshot.warn; + console.error = snapshot.error; + console.debug = snapshot.debug; + console.trace = snapshot.trace; +} diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 6296ae6f101..c7f63d453df 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -87,6 +87,48 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { return [...Array(params.depth).fill("/usr/bin/env"), "/bin/sh", "-c", params.payload]; } + function createMacExecHostSuccess(stdout = "app-ok"): ExecHostResponse { + return { + ok: true, + payload: { + success: true, + stdout, + stderr: "", + timedOut: false, + exitCode: 0, + error: null, + }, + }; + } + + function createAllowlistOnMissApprovals(params?: { + autoAllowSkills?: boolean; + agents?: Parameters[0]["agents"]; + }): Parameters[0] { + return { + version: 1, + defaults: { + security: "allowlist", + ask: "on-miss", + askFallback: "deny", + ...(params?.autoAllowSkills ? { autoAllowSkills: true } : {}), + }, + agents: params?.agents ?? {}, + }; + } + + function createInvokeSpies(params?: { runCommand?: MockedRunCommand }): { + runCommand: MockedRunCommand; + sendInvokeResult: MockedSendInvokeResult; + sendNodeEvent: MockedSendNodeEvent; + } { + return { + runCommand: params?.runCommand ?? vi.fn(async () => createLocalRunResult()), + sendInvokeResult: vi.fn(async () => {}), + sendNodeEvent: vi.fn(async () => {}), + }; + } + async function withTempApprovalsHome(params: { approvals: Parameters[0]; run: (ctx: { tempHome: string }) => Promise; @@ -246,17 +288,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { it("uses mac app exec host when explicitly preferred", async () => { const { runCommand, runViaMacAppExecHost, sendInvokeResult } = await runSystemInvoke({ preferMacAppExecHost: true, - runViaResponse: { - ok: true, - payload: { - success: true, - stdout: "app-ok", - stderr: "", - timedOut: false, - exitCode: 0, - error: null, - }, - }, + runViaResponse: createMacExecHostSuccess(), }); expect(runViaMacAppExecHost).toHaveBeenCalledWith({ @@ -278,17 +310,7 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { const { runViaMacAppExecHost } = await runSystemInvoke({ preferMacAppExecHost: true, command: ["/bin/sh", "-lc", '$0 "$1"', "/usr/bin/touch", "/tmp/marker"], - runViaResponse: { - ok: true, - payload: { - success: true, - stdout: "app-ok", - stderr: "", - timedOut: false, - exitCode: 0, - error: null, - }, - }, + runViaResponse: createMacExecHostSuccess(), }); expect(runViaMacAppExecHost).toHaveBeenCalledWith({ @@ -584,21 +606,10 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { }); it("denies ./skill-bin even when autoAllowSkills trust entry exists", async () => { - const runCommand = vi.fn(async () => createLocalRunResult()); - const sendInvokeResult = vi.fn(async () => {}); - const sendNodeEvent = vi.fn(async () => {}); + const { runCommand, sendInvokeResult, sendNodeEvent } = createInvokeSpies(); await withTempApprovalsHome({ - approvals: { - version: 1, - defaults: { - security: "allowlist", - ask: "on-miss", - askFallback: "deny", - autoAllowSkills: true, - }, - agents: {}, - }, + approvals: createAllowlistOnMissApprovals({ autoAllowSkills: true }), run: async ({ tempHome }) => { const skillBinPath = path.join(tempHome, "skill-bin"); fs.writeFileSync(skillBinPath, "#!/bin/sh\necho should-not-run\n", { mode: 0o755 }); @@ -656,26 +667,20 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { if (process.platform === "win32") { return; } - const runCommand = vi.fn(async () => { - throw new Error("runCommand should not be called for nested env depth overflow"); + const { runCommand, sendInvokeResult, sendNodeEvent } = createInvokeSpies({ + runCommand: vi.fn(async () => { + throw new Error("runCommand should not be called for nested env depth overflow"); + }), }); - const sendInvokeResult = vi.fn(async () => {}); - const sendNodeEvent = vi.fn(async () => {}); await withTempApprovalsHome({ - approvals: { - version: 1, - defaults: { - security: "allowlist", - ask: "on-miss", - askFallback: "deny", - }, + approvals: createAllowlistOnMissApprovals({ agents: { main: { allowlist: [{ pattern: "/usr/bin/env" }], }, }, - }, + }), run: async ({ tempHome }) => { const marker = path.join(tempHome, "pwned.txt"); await runSystemInvoke({ diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index 94173ec005f..78e5d589f8e 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -26,6 +26,7 @@ import { import { hardenApprovedExecutionPaths } from "./invoke-system-run-plan.js"; import type { ExecEventPayload, + ExecFinishedEventParams, RunResult, SkillBinsProvider, SystemRunParams, @@ -129,19 +130,7 @@ export type HandleSystemRunInvokeOptions = { sendNodeEvent: (client: GatewayClient, event: string, payload: unknown) => Promise; buildExecEventPayload: (payload: ExecEventPayload) => ExecEventPayload; sendInvokeResult: (result: SystemRunInvokeResult) => Promise; - sendExecFinishedEvent: (params: { - sessionKey: string; - runId: string; - cmdText: string; - result: { - stdout?: string; - stderr?: string; - error?: string | null; - exitCode?: number | null; - timedOut?: boolean; - success?: boolean; - }; - }) => Promise; + sendExecFinishedEvent: (params: ExecFinishedEventParams) => Promise; preferMacAppExecHost: boolean; }; diff --git a/src/node-host/invoke-types.ts b/src/node-host/invoke-types.ts index 7246ba2925f..72ffe75c2d7 100644 --- a/src/node-host/invoke-types.ts +++ b/src/node-host/invoke-types.ts @@ -36,6 +36,22 @@ export type ExecEventPayload = { reason?: string; }; +export type ExecFinishedResult = { + stdout?: string; + stderr?: string; + error?: string | null; + exitCode?: number | null; + timedOut?: boolean; + success?: boolean; +}; + +export type ExecFinishedEventParams = { + sessionKey: string; + runId: string; + cmdText: string; + result: ExecFinishedResult; +}; + export type SkillBinsProvider = { current(force?: boolean): Promise; }; diff --git a/src/node-host/invoke.ts b/src/node-host/invoke.ts index 5d2fdd3d15c..bd570201eca 100644 --- a/src/node-host/invoke.ts +++ b/src/node-host/invoke.ts @@ -23,6 +23,7 @@ import { runBrowserProxyCommand } from "./invoke-browser.js"; import { buildSystemRunApprovalPlan, handleSystemRunInvoke } from "./invoke-system-run.js"; import type { ExecEventPayload, + ExecFinishedEventParams, RunResult, SkillBinsProvider, SystemRunParams, @@ -334,20 +335,11 @@ function buildExecEventPayload(payload: ExecEventPayload): ExecEventPayload { return { ...payload, output: text }; } -async function sendExecFinishedEvent(params: { - client: GatewayClient; - sessionKey: string; - runId: string; - cmdText: string; - result: { - stdout?: string; - stderr?: string; - error?: string | null; - exitCode?: number | null; - timedOut?: boolean; - success?: boolean; - }; -}) { +async function sendExecFinishedEvent( + params: ExecFinishedEventParams & { + client: GatewayClient; + }, +) { const combined = [params.result.stdout, params.result.stderr, params.result.error] .filter(Boolean) .join("\n"); diff --git a/src/providers/google-shared.test-helpers.ts b/src/providers/google-shared.test-helpers.ts index c98fad72af1..80839ae6085 100644 --- a/src/providers/google-shared.test-helpers.ts +++ b/src/providers/google-shared.test-helpers.ts @@ -1,5 +1,6 @@ import type { Model } from "@mariozechner/pi-ai/dist/types.js"; import { expect } from "vitest"; +import { makeZeroUsageSnapshot } from "../agents/usage.js"; export const asRecord = (value: unknown): Record => { expect(value).toBeTruthy(); @@ -48,23 +49,6 @@ export const makeGeminiCliModel = (id: string): Model<"google-gemini-cli"> => maxTokens: 1, }) as Model<"google-gemini-cli">; -function makeZeroUsage() { - return { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - totalTokens: 0, - cost: { - input: 0, - output: 0, - cacheRead: 0, - cacheWrite: 0, - total: 0, - }, - }; -} - export function makeGoogleAssistantMessage(model: string, content: unknown) { return { role: "assistant", @@ -72,7 +56,7 @@ export function makeGoogleAssistantMessage(model: string, content: unknown) { api: "google-generative-ai", provider: "google", model, - usage: makeZeroUsage(), + usage: makeZeroUsageSnapshot(), stopReason: "stop", timestamp: 0, }; @@ -85,7 +69,7 @@ export function makeGeminiCliAssistantMessage(model: string, content: unknown) { api: "google-gemini-cli", provider: "google-gemini-cli", model, - usage: makeZeroUsage(), + usage: makeZeroUsageSnapshot(), stopReason: "stop", timestamp: 0, };