diff --git a/CHANGELOG.md b/CHANGELOG.md index fec9157e06d..dc26802bb6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Agents/subagents: enforce `subagents.allowAgents` for explicit same-agent `sessions_spawn(agentId=...)` calls instead of auto-allowing requester self-targets. Fixes #72827. Thanks @oiGaDio. - ACP/sessions_spawn: let explicit `sessions_spawn(runtime="acp")` bootstrap turns run while `acp.dispatch.enabled=false` still blocks automatic ACP thread dispatch. Fixes #63591. Thanks @moeedahmed. - Google Meet: route local Chrome joins through OpenClaw browser control instead of raw default Chrome, so agents use the configured OpenClaw browser profile when opening Meet. - Plugins/discovery: follow symlinked plugin directories in global and workspace plugin roots while keeping broken links ignored and existing package safety checks in place. Fixes #36754; carries forward #72695 and #63206. Thanks @Quackstro, @ming1523, and @xsfX20. diff --git a/docs/gateway/config-agents.md b/docs/gateway/config-agents.md index 46258d396db..51628a5092a 100644 --- a/docs/gateway/config-agents.md +++ b/docs/gateway/config-agents.md @@ -979,7 +979,7 @@ for provider examples and precedence. - `runtime`: optional per-agent runtime descriptor. Use `type: "acp"` with `runtime.acp` defaults (`agent`, `backend`, `mode`, `cwd`) when the agent should default to ACP harness sessions. - `identity.avatar`: workspace-relative path, `http(s)` URL, or `data:` URI. - `identity` derives defaults: `ackReaction` from `emoji`, `mentionPatterns` from `name`/`emoji`. -- `subagents.allowAgents`: allowlist of agent ids for `sessions_spawn` (`["*"]` = any; default: same agent only). +- `subagents.allowAgents`: allowlist of agent ids for explicit `sessions_spawn.agentId` targets (`["*"]` = any; default: same agent only). Include the requester id when self-targeted `agentId` calls should be allowed. - Sandbox inheritance guard: if the requester session is sandboxed, `sessions_spawn` rejects targets that would run unsandboxed. - `subagents.requireAgentId`: when true, block `sessions_spawn` calls that omit `agentId` (forces explicit profile selection; default: false). diff --git a/docs/tools/subagents.md b/docs/tools/subagents.md index f694e657715..f00f879553d 100644 --- a/docs/tools/subagents.md +++ b/docs/tools/subagents.md @@ -244,7 +244,7 @@ See [Configuration reference](/gateway/configuration-reference) and ### Allowlist - List of agent ids that can be targeted via `agentId` (`["*"]` allows any). Default: only the requester agent. + List of agent ids that can be targeted via explicit `agentId` (`["*"]` allows any). Default: only the requester agent. If you set a list and still want the requester to spawn itself with `agentId`, include the requester id in the list. Default target-agent allowlist used when the requester agent does not set its own `subagents.allowAgents`. diff --git a/src/agents/acp-spawn.test.ts b/src/agents/acp-spawn.test.ts index 28d2ce19e71..d581fcf8a9a 100644 --- a/src/agents/acp-spawn.test.ts +++ b/src/agents/acp-spawn.test.ts @@ -1100,6 +1100,38 @@ describe("spawnAcpDirect", () => { expect(failed.error).toContain("agentId is not allowed"); }); + it("rejects explicit ACP self-targets when the subagent allowlist excludes the requester", async () => { + replaceSpawnConfig({ + ...hoisted.state.cfg, + acp: { + ...hoisted.state.cfg.acp, + allowedAgents: ["codex", "writer"], + }, + agents: { + defaults: { + subagents: { + allowAgents: ["writer"], + maxSpawnDepth: 2, + }, + }, + }, + }); + + const result = await spawnAcpDirect( + createSpawnRequest({ + agentId: "codex", + }), + { + ...createRequesterContext(), + agentSessionKey: "agent:codex:subagent:parent", + }, + ); + + const failed = expectFailedSpawn(result, "forbidden"); + expect(failed.errorCode).toBe("subagent_policy"); + expect(failed.error).toContain("agentId is not allowed"); + }); + it("spawns Matrix thread-bound ACP sessions from top-level room targets", async () => { enableMatrixAcpThreadBindings(); hoisted.sessionBindingBindMock.mockImplementationOnce( diff --git a/src/agents/acp-spawn.ts b/src/agents/acp-spawn.ts index 6f60a288937..b58e18786e6 100644 --- a/src/agents/acp-spawn.ts +++ b/src/agents/acp-spawn.ts @@ -82,6 +82,7 @@ import { } from "./subagent-capabilities.js"; import { getSubagentDepthFromSessionStore } from "./subagent-depth.js"; import { countActiveRunsForSession, getSubagentRunByChildSessionKey } from "./subagent-registry.js"; +import { resolveSubagentTargetPolicy } from "./subagent-target-policy.js"; import { resolveInternalSessionKey, resolveMainSessionAlias } from "./tools/sessions-helpers.js"; const log = createSubsystemLogger("agents/acp-spawn"); @@ -786,24 +787,18 @@ function resolveAcpSubagentEnvelopeState(params: { }; } - if (params.targetAgentId !== requesterAgentId) { - const allowAgents = + const targetPolicy = resolveSubagentTargetPolicy({ + requesterAgentId, + targetAgentId: params.targetAgentId, + requestedAgentId: params.requestedAgentId, + allowAgents: resolveAgentConfig(params.cfg, requesterAgentId)?.subagents?.allowAgents ?? - params.cfg.agents?.defaults?.subagents?.allowAgents ?? - []; - const allowAny = allowAgents.some((value) => value.trim() === "*"); - const normalizedTargetId = normalizeOptionalLowercaseString(params.targetAgentId) ?? ""; - const allowSet = new Set( - allowAgents - .filter((value) => value.trim() && value.trim() !== "*") - .map((value) => normalizeOptionalLowercaseString(normalizeAgentId(value)) ?? ""), - ); - if (!allowAny && !allowSet.has(normalizedTargetId)) { - const allowedText = allowSet.size > 0 ? Array.from(allowSet).join(", ") : "none"; - return { - error: `agentId is not allowed for sessions_spawn (allowed: ${allowedText})`, - }; - } + params.cfg.agents?.defaults?.subagents?.allowAgents, + }); + if (!targetPolicy.ok) { + return { + error: targetPolicy.error, + }; } const childCapabilities = resolveSubagentCapabilities({ diff --git a/src/agents/subagent-spawn.test.ts b/src/agents/subagent-spawn.test.ts index 169d232e693..9e68678af15 100644 --- a/src/agents/subagent-spawn.test.ts +++ b/src/agents/subagent-spawn.test.ts @@ -14,6 +14,7 @@ const hoisted = vi.hoisted(() => ({ pruneLegacyStoreKeysMock: vi.fn(), registerSubagentRunMock: vi.fn(), emitSessionLifecycleEventMock: vi.fn(), + resolveAgentConfigMock: vi.fn(), configOverride: {} as Record, })); @@ -46,7 +47,7 @@ describe("spawnSubagentDirect seam flow", () => { pruneLegacyStoreKeysMock: hoisted.pruneLegacyStoreKeysMock, registerSubagentRunMock: hoisted.registerSubagentRunMock, emitSessionLifecycleEventMock: hoisted.emitSessionLifecycleEventMock, - resolveAgentConfig: () => undefined, + resolveAgentConfig: hoisted.resolveAgentConfigMock, resolveSubagentSpawnModelSelection: () => "openai-codex/gpt-5.4", resolveSandboxRuntimeStatus: () => ({ sandboxed: false }), sessionStorePath: "/tmp/subagent-spawn-session-store.json", @@ -61,6 +62,11 @@ describe("spawnSubagentDirect seam flow", () => { hoisted.pruneLegacyStoreKeysMock.mockReset(); hoisted.registerSubagentRunMock.mockReset(); hoisted.emitSessionLifecycleEventMock.mockReset(); + hoisted.resolveAgentConfigMock.mockReset(); + hoisted.resolveAgentConfigMock.mockImplementation( + (cfg: { agents?: { list?: Array<{ id?: string }> } }, agentId: string) => + cfg.agents?.list?.find((agent) => agent.id === agentId), + ); hoisted.configOverride = createConfigOverride(); installAcceptedSubagentGatewayMock(hoisted.callGatewayMock); @@ -76,6 +82,84 @@ describe("spawnSubagentDirect seam flow", () => { ); }); + it("rejects explicit same-agent targets when allowAgents excludes the requester", async () => { + hoisted.configOverride = createConfigOverride({ + agents: { + defaults: { + workspace: os.tmpdir(), + }, + list: [ + { + id: "task-manager", + workspace: "/tmp/workspace-task-manager", + subagents: { + allowAgents: ["planner"], + }, + }, + { + id: "planner", + workspace: "/tmp/workspace-planner", + }, + ], + }, + }); + + const result = await spawnSubagentDirect( + { + task: "spawn myself explicitly", + agentId: "task-manager", + }, + { + agentSessionKey: "agent:task-manager:main", + }, + ); + + expect(result).toMatchObject({ + status: "forbidden", + error: "agentId is not allowed for sessions_spawn (allowed: planner)", + }); + expect(hoisted.callGatewayMock).not.toHaveBeenCalledWith( + expect.objectContaining({ method: "agent" }), + ); + }); + + it("allows omitted agentId to default to requester even when allowAgents excludes requester", async () => { + hoisted.configOverride = createConfigOverride({ + agents: { + defaults: { + workspace: os.tmpdir(), + }, + list: [ + { + id: "task-manager", + workspace: "/tmp/workspace-task-manager", + subagents: { + allowAgents: ["planner"], + }, + }, + { + id: "planner", + workspace: "/tmp/workspace-planner", + }, + ], + }, + }); + + const result = await spawnSubagentDirect( + { + task: "spawn default target", + }, + { + agentSessionKey: "agent:task-manager:main", + }, + ); + + expect(result).toMatchObject({ + status: "accepted", + childSessionKey: expect.stringMatching(/^agent:task-manager:subagent:/), + }); + }); + it("accepts a spawned run across session patching, runtime-model persistence, registry registration, and lifecycle emission", async () => { const operations: string[] = []; let persistedStore: Record> | undefined; diff --git a/src/agents/subagent-spawn.ts b/src/agents/subagent-spawn.ts index 762353f2395..644631b0d48 100644 --- a/src/agents/subagent-spawn.ts +++ b/src/agents/subagent-spawn.ts @@ -8,10 +8,7 @@ import type { SubagentSpawnPreparation } from "../context-engine/types.js"; import { listRegisteredPluginAgentPromptGuidance } from "../plugins/command-registry-state.js"; import type { SubagentLifecycleHookRunner } from "../plugins/hooks.js"; import { isValidAgentId, normalizeAgentId, parseAgentSessionKey } from "../routing/session-key.js"; -import { - normalizeLowercaseStringOrEmpty, - normalizeOptionalString, -} from "../shared/string-coerce.js"; +import { normalizeOptionalString } from "../shared/string-coerce.js"; import type { DeliveryContext } from "../utils/delivery-context.types.js"; import type { BootstrapContextMode } from "./bootstrap-files.js"; import { @@ -29,6 +26,7 @@ import { getSubagentDepthFromSessionStore } from "./subagent-depth.js"; import { buildSubagentInitialUserMessage } from "./subagent-initial-user-message.js"; import { countActiveRunsForSession, registerSubagentRun } from "./subagent-registry.js"; import { resolveSubagentSpawnAcceptedNote } from "./subagent-spawn-accepted-note.js"; +import { resolveSubagentTargetPolicy } from "./subagent-target-policy.js"; export { SUBAGENT_SPAWN_ACCEPTED_NOTE, SUBAGENT_SPAWN_SESSION_ACCEPTED_NOTE, @@ -744,25 +742,19 @@ export async function spawnSubagentDirect( requesterGroupSpace: ctx.agentGroupSpace, requesterMemberRoleIds: ctx.agentMemberRoleIds, }); - if (targetAgentId !== requesterAgentId) { - const allowAgents = + const targetPolicy = resolveSubagentTargetPolicy({ + requesterAgentId, + targetAgentId, + requestedAgentId, + allowAgents: resolveAgentConfig(cfg, requesterAgentId)?.subagents?.allowAgents ?? - cfg?.agents?.defaults?.subagents?.allowAgents ?? - []; - const allowAny = allowAgents.some((value) => value.trim() === "*"); - const normalizedTargetId = normalizeLowercaseStringOrEmpty(targetAgentId); - const allowSet = new Set( - allowAgents - .filter((value) => value.trim() && value.trim() !== "*") - .map((value) => normalizeLowercaseStringOrEmpty(normalizeAgentId(value))), - ); - if (!allowAny && !allowSet.has(normalizedTargetId)) { - const allowedText = allowSet.size > 0 ? Array.from(allowSet).join(", ") : "none"; - return { - status: "forbidden", - error: `agentId is not allowed for sessions_spawn (allowed: ${allowedText})`, - }; - } + cfg?.agents?.defaults?.subagents?.allowAgents, + }); + if (!targetPolicy.ok) { + return { + status: "forbidden", + error: targetPolicy.error, + }; } const childSessionKey = `agent:${targetAgentId}:subagent:${crypto.randomUUID()}`; const requesterRuntime = resolveSandboxRuntimeStatus({ diff --git a/src/agents/subagent-target-policy.test.ts b/src/agents/subagent-target-policy.test.ts new file mode 100644 index 00000000000..096a9bb7ead --- /dev/null +++ b/src/agents/subagent-target-policy.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "vitest"; +import { + resolveSubagentAllowedTargetIds, + resolveSubagentTargetPolicy, +} from "./subagent-target-policy.js"; + +describe("subagent target policy", () => { + it("defaults to requester-only when no allowlist is configured", () => { + expect( + resolveSubagentTargetPolicy({ + requesterAgentId: "main", + targetAgentId: "main", + requestedAgentId: "main", + }), + ).toEqual({ ok: true }); + expect( + resolveSubagentTargetPolicy({ + requesterAgentId: "main", + targetAgentId: "other", + requestedAgentId: "other", + }), + ).toMatchObject({ ok: false, allowedText: "main" }); + }); + + it("keeps omitted agentId self-spawns allowed even when an allowlist is configured", () => { + expect( + resolveSubagentTargetPolicy({ + requesterAgentId: "task-manager", + targetAgentId: "task-manager", + allowAgents: ["planner"], + }), + ).toEqual({ ok: true }); + }); + + it("rejects explicit self-targets when the configured allowlist excludes the requester", () => { + expect( + resolveSubagentTargetPolicy({ + requesterAgentId: "task-manager", + targetAgentId: "task-manager", + requestedAgentId: "task-manager", + allowAgents: ["planner", "checker"], + }), + ).toMatchObject({ + ok: false, + allowedText: "checker, planner", + error: "agentId is not allowed for sessions_spawn (allowed: checker, planner)", + }); + }); + + it("resolves allowed target ids without auto-adding requester for explicit allowlists", () => { + expect( + resolveSubagentAllowedTargetIds({ + requesterAgentId: "main", + allowAgents: ["planner"], + configuredAgentIds: ["main", "planner"], + }), + ).toEqual({ + allowAny: false, + allowedIds: ["planner"], + }); + }); +}); diff --git a/src/agents/subagent-target-policy.ts b/src/agents/subagent-target-policy.ts new file mode 100644 index 00000000000..2a5e0255fd6 --- /dev/null +++ b/src/agents/subagent-target-policy.ts @@ -0,0 +1,84 @@ +import { normalizeAgentId } from "../routing/session-key.js"; + +export type SubagentTargetPolicyResult = + | { ok: true } + | { ok: false; allowedText: string; error: string }; + +function normalizeAllowAgents(allowAgents: readonly string[] | undefined): { + configured: boolean; + allowAny: boolean; + allowedIds: string[]; +} { + if (!Array.isArray(allowAgents)) { + return { + configured: false, + allowAny: false, + allowedIds: [], + }; + } + const allowedIds = allowAgents + .map((value) => value.trim()) + .filter((value) => value && value !== "*") + .map((value) => normalizeAgentId(value)) + .filter(Boolean); + return { + configured: true, + allowAny: allowAgents.some((value) => value.trim() === "*"), + allowedIds: Array.from(new Set(allowedIds)).toSorted((a, b) => a.localeCompare(b)), + }; +} + +export function resolveSubagentAllowedTargetIds(params: { + requesterAgentId: string; + allowAgents?: readonly string[]; + configuredAgentIds?: readonly string[]; +}): { allowAny: boolean; allowedIds: string[] } { + const requesterAgentId = normalizeAgentId(params.requesterAgentId); + const policy = normalizeAllowAgents(params.allowAgents); + if (!policy.configured) { + return { + allowAny: false, + allowedIds: requesterAgentId ? [requesterAgentId] : [], + }; + } + if (policy.allowAny) { + const configuredIds = (params.configuredAgentIds ?? []) + .map((id) => normalizeAgentId(id)) + .filter(Boolean); + return { + allowAny: true, + allowedIds: Array.from(new Set(configuredIds)).toSorted((a, b) => a.localeCompare(b)), + }; + } + return { + allowAny: false, + allowedIds: policy.allowedIds, + }; +} + +export function resolveSubagentTargetPolicy(params: { + requesterAgentId: string; + targetAgentId: string; + requestedAgentId?: string; + allowAgents?: readonly string[]; +}): SubagentTargetPolicyResult { + const requesterAgentId = normalizeAgentId(params.requesterAgentId); + const targetAgentId = normalizeAgentId(params.targetAgentId); + if (!params.requestedAgentId?.trim() && targetAgentId === requesterAgentId) { + return { ok: true }; + } + + const allowed = resolveSubagentAllowedTargetIds({ + requesterAgentId, + allowAgents: params.allowAgents, + }); + if (allowed.allowAny || allowed.allowedIds.includes(targetAgentId)) { + return { ok: true }; + } + const allowedText = allowed.allowedIds.length > 0 ? allowed.allowedIds.join(", ") : "none"; + return { + ok: false, + allowedText, + error: `agentId is not allowed for sessions_spawn (allowed: ${allowedText})`, + }; +} diff --git a/src/agents/tools/agents-list-tool.test.ts b/src/agents/tools/agents-list-tool.test.ts index 86d1e907c34..8aff1ecd48c 100644 --- a/src/agents/tools/agents-list-tool.test.ts +++ b/src/agents/tools/agents-list-tool.test.ts @@ -47,12 +47,6 @@ describe("agents_list tool", () => { expect(result.details).toMatchObject({ requester: "main", agents: [ - { - id: "main", - configured: true, - model: "anthropic/claude-opus-4.5", - agentRuntime: { id: "pi", source: "defaults" }, - }, { id: "codex", name: "Codex", @@ -64,6 +58,31 @@ describe("agents_list tool", () => { }); }); + it("returns requester as the only target when no subagent allowlist is configured", async () => { + loadConfigMock.mockReturnValue({ + agents: { + list: [{ id: "main", default: true }, { id: "codex" }], + }, + } satisfies OpenClawConfig); + + const { createAgentsListTool } = await import("./agents-list-tool.js"); + const result = await createAgentsListTool({ agentSessionKey: "agent:main:main" }).execute( + "call", + {}, + ); + + expect(result.details).toMatchObject({ + requester: "main", + allowAny: false, + agents: [ + { + id: "main", + configured: true, + }, + ], + }); + }); + it("marks OPENCLAW_AGENT_RUNTIME as the effective runtime source", async () => { vi.stubEnv("OPENCLAW_AGENT_RUNTIME", "codex"); loadConfigMock.mockReturnValue({ diff --git a/src/agents/tools/agents-list-tool.ts b/src/agents/tools/agents-list-tool.ts index e7cea9e614b..86a975eff2b 100644 --- a/src/agents/tools/agents-list-tool.ts +++ b/src/agents/tools/agents-list-tool.ts @@ -11,6 +11,7 @@ import { resolveAgentConfig, resolveAgentEffectiveModelPrimary, } from "../agent-scope.js"; +import { resolveSubagentAllowedTargetIds } from "../subagent-target-policy.js"; import type { AnyAgentTool } from "./common.js"; import { jsonResult } from "./common.js"; import { resolveInternalSessionKey, resolveMainSessionAlias } from "./sessions-helpers.js"; @@ -102,14 +103,7 @@ export function createAgentsListTool(opts?: { const allowAgents = resolveAgentConfig(cfg, requesterAgentId)?.subagents?.allowAgents ?? - cfg?.agents?.defaults?.subagents?.allowAgents ?? - []; - const allowAny = allowAgents.some((value) => value.trim() === "*"); - const allowSet = new Set( - allowAgents - .filter((value) => value.trim() && value.trim() !== "*") - .map((value) => normalizeAgentId(value)), - ); + cfg?.agents?.defaults?.subagents?.allowAgents; const configuredAgents = Array.isArray(cfg.agents?.list) ? cfg.agents?.list : []; const configuredIds = configuredAgents.map((entry) => normalizeAgentId(entry.id)); @@ -122,23 +116,16 @@ export function createAgentsListTool(opts?: { configuredNameMap.set(normalizeAgentId(entry.id), name); } - const allowed = new Set(); - allowed.add(requesterAgentId); - if (allowAny) { - for (const id of configuredIds) { - allowed.add(id); - } - } else { - for (const id of allowSet) { - allowed.add(id); - } - } - - const all = Array.from(allowed); + const allowed = resolveSubagentAllowedTargetIds({ + requesterAgentId, + allowAgents, + configuredAgentIds: configuredIds, + }); + const all = allowed.allowedIds; const rest = all .filter((id) => id !== requesterAgentId) .toSorted((a, b) => a.localeCompare(b)); - const ordered = [requesterAgentId, ...rest]; + const ordered = all.includes(requesterAgentId) ? [requesterAgentId, ...rest] : rest; const agents: AgentListEntry[] = ordered.map((id) => { const agentRuntime = resolveAgentRuntimeMetadata(cfg, id); return { @@ -152,7 +139,7 @@ export function createAgentsListTool(opts?: { return jsonResult({ requester: requesterAgentId, - allowAny, + allowAny: allowed.allowAny, agents, }); },