From 577e64db63b4640bb07fc794dde7108e9620dd11 Mon Sep 17 00:00:00 2001 From: Josh Avant <830519+joshavant@users.noreply.github.com> Date: Thu, 21 May 2026 18:53:30 -0700 Subject: [PATCH] fix: require configured subagent allowlist targets (#85154) * fix subagent allowlists to configured agents * add changelog for subagent allowlist fix --- CHANGELOG.md | 1 + docs/gateway/config-agents.md | 2 +- docs/gateway/config-tools.md | 2 +- docs/tools/subagents.md | 13 +- src/agents/openclaw-tools.agents.test.ts | 6 +- ...subagents.sessions-spawn.allowlist.test.ts | 28 ++- ...subagents.sessions-spawn.lifecycle.test.ts | 5 +- src/agents/subagent-target-policy.test.ts | 57 ++++-- src/agents/subagent-target-policy.ts | 27 ++- src/agents/tools/agents-list-tool.test.ts | 56 ++++++ src/agents/tools/agents-list-tool.ts | 3 +- src/commands/doctor/repair-sequencing.ts | 2 + .../doctor/shared/preview-warnings.ts | 23 +++ .../shared/stale-subagent-allowlist.test.ts | 123 ++++++++++++ .../doctor/shared/stale-subagent-allowlist.ts | 178 ++++++++++++++++++ 15 files changed, 489 insertions(+), 37 deletions(-) create mode 100644 src/commands/doctor/shared/stale-subagent-allowlist.test.ts create mode 100644 src/commands/doctor/shared/stale-subagent-allowlist.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d47b143ce3ec..20b2dabe98e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ Docs: https://docs.openclaw.ai - Codex app-server: mark missing turn completion after observed execution as replay-unsafe and release the session so follow-up turns can run. Fixes #84076. (#85107) Thanks @joshavant. - Codex app-server: add a dedicated post-tool raw assistant completion idle timeout config so trusted heavy turns can wait longer after tool handoff without weakening final assistant release. - Matrix: keep explicitly configured two-person rooms on the room route before stale `m.direct` or strict two-member DM fallback can bypass mention gating. Fixes #85017. (#85137) Thanks @joshavant. +- Agents/subagents: require explicit subagent allowlist targets to be configured agents so stale deleted-agent ids are omitted from `agents_list` and rejected by `sessions_spawn`. Fixes #84811. (#85154) Thanks @joshavant. - PDF tool: time out idle remote PDF body reads after 120 seconds so stalled remote documents return an error instead of wedging the session. Fixes #68649. (#84768) Thanks @luoyanglang. - Diagnostics/OpenTelemetry plugin: suppress handled OTLP exporter promise rejections so collector shutdowns no longer crash the Gateway. (#81085) Thanks @luoyanglang. - Agents/exec: omit raw command text and env values from denied exec failure logs while keeping safe correlation metadata. Fixes #85049. (#85140) Thanks @joshavant. diff --git a/docs/gateway/config-agents.md b/docs/gateway/config-agents.md index 258686a5e2e6..ee964d7e617b 100644 --- a/docs/gateway/config-agents.md +++ b/docs/gateway/config-agents.md @@ -1093,7 +1093,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 explicit `sessions_spawn.agentId` targets (`["*"]` = any configured target; default: same agent only). Include the requester id when self-targeted `agentId` calls should be allowed. +- `subagents.allowAgents`: allowlist of configured agent ids for explicit `sessions_spawn.agentId` targets (`["*"]` = any configured target; default: same agent only). Include the requester id when self-targeted `agentId` calls should be allowed. Stale entries whose agent config was deleted are rejected by `sessions_spawn` and omitted from `agents_list`; run `openclaw doctor --fix` to clean them up, or add a minimal `agents.list[]` entry if that target should remain spawnable while inheriting defaults. - 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/gateway/config-tools.md b/docs/gateway/config-tools.md index a759b32165e0..223f7e5c6738 100644 --- a/docs/gateway/config-tools.md +++ b/docs/gateway/config-tools.md @@ -433,7 +433,7 @@ Experimental built-in tool flags. Default off unless a strict-agentic GPT-5 auto ``` - `model`: default model for spawned sub-agents. If omitted, sub-agents inherit the caller's model. -- `allowAgents`: default allowlist of target agent ids for `sessions_spawn` when the requester agent does not set its own `subagents.allowAgents` (`["*"]` = any configured target; default: same agent only). +- `allowAgents`: default allowlist of configured target agent ids for `sessions_spawn` when the requester agent does not set its own `subagents.allowAgents` (`["*"]` = any configured target; default: same agent only). Stale entries whose agent config was deleted are rejected by `sessions_spawn` and omitted from `agents_list`; run `openclaw doctor --fix` to clean them up. - `runTimeoutSeconds`: default timeout (seconds) for `sessions_spawn` when the tool call omits `runTimeoutSeconds`. `0` means no timeout. - `announceTimeoutMs`: per-call timeout (milliseconds) for gateway `agent` announce delivery attempts. Default: `120000`. Transient retries can make the total announce wait longer than one configured timeout. - Per-subagent tool policy: `tools.subagents.tools.allow` / `tools.subagents.tools.deny`. diff --git a/docs/tools/subagents.md b/docs/tools/subagents.md index 73f8ef23d035..cc3036cf5790 100644 --- a/docs/tools/subagents.md +++ b/docs/tools/subagents.md @@ -190,7 +190,7 @@ Per-agent overrides use `agents.list[].subagents.delegationMode`. Optional human-readable label. - Spawn under another agent id when allowed by `subagents.allowAgents`. + Spawn under another configured agent id when allowed by `subagents.allowAgents`. `acp` is only for external ACP harnesses (`claude`, `droid`, `gemini`, `opencode`, or explicitly requested Codex ACP/acpx) and for `agents.list[]` entries whose `runtime.type` is `acp`. @@ -340,10 +340,10 @@ See [Configuration reference](/gateway/configuration-reference) and ### Allowlist - List of agent ids that can be targeted via explicit `agentId` (`["*"]` allows any configured target). 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. + List of configured agent ids that can be targeted via explicit `agentId` (`["*"]` allows any configured target). 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`. + Default configured target-agent allowlist used when the requester agent does not set its own `subagents.allowAgents`. Block `sessions_spawn` calls that omit `agentId` (forces explicit profile selection). Per-agent override: `agents.list[].subagents.requireAgentId`. @@ -362,6 +362,13 @@ Use `agents_list` to see which agent ids are currently allowed for model and embedded runtime metadata so callers can distinguish PI, Codex app-server, and other configured native runtimes. +`allowAgents` entries must point at configured agent ids in `agents.list[]`. +`["*"]` means any configured target agent plus the requester. If an agent config +is deleted but its id remains in `allowAgents`, `sessions_spawn` rejects that id +and `agents_list` omits it. Run `openclaw doctor --fix` to clean stale +allowlist entries, or add a minimal `agents.list[]` entry when the target should +remain spawnable while inheriting defaults. + ### Auto-archive - Sub-agent sessions are automatically archived after `agents.defaults.subagents.archiveAfterMinutes` (default `60`). diff --git a/src/agents/openclaw-tools.agents.test.ts b/src/agents/openclaw-tools.agents.test.ts index d755e75a4653..096c79c88f6a 100644 --- a/src/agents/openclaw-tools.agents.test.ts +++ b/src/agents/openclaw-tools.agents.test.ts @@ -126,7 +126,7 @@ describe("agents_list", () => { expect(agents?.map((agent) => agent.id)).toEqual(["main", "coder", "research"]); }); - it("marks allowlisted-but-unconfigured agents", async () => { + it("omits allowlisted-but-unconfigured agents", async () => { setConfigWithAgentList([ { id: "main", @@ -139,8 +139,6 @@ describe("agents_list", () => { const tool = createTool(); const result = await tool.execute("call4", {}); const agents = readAgentList(result); - expect(agents?.map((agent) => agent.id)).toEqual(["research"]); - const research = agents?.find((agent) => agent.id === "research"); - expect(research?.configured).toBe(false); + expect(agents?.map((agent) => agent.id)).toEqual([]); }); }); 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 c7f8c221d624..7a49b4d98fad 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn.allowlist.test.ts @@ -117,7 +117,7 @@ describe("subagent spawn allowlist + sandbox guards", () => { it("allows cross-agent spawning when configured", async () => { setConfig({ agents: { - list: [{ id: "main", subagents: { allowAgents: ["beta"] } }], + list: [{ id: "main", subagents: { allowAgents: ["beta"] } }, { id: "beta" }], }, }); const result = await spawn({ agentId: "beta" }); @@ -130,7 +130,7 @@ describe("subagent spawn allowlist + sandbox guards", () => { setConfig({ agents: { defaults: { subagents: { allowAgents: ["beta"] } }, - list: [{ id: "main" }], + list: [{ id: "main" }, { id: "beta" }], }, }); const result = await spawn({ agentId: "beta" }); @@ -161,21 +161,24 @@ describe("subagent spawn allowlist + sandbox guards", () => { expect(hoisted.callGatewayMock).not.toHaveBeenCalled(); }); - it("allows explicit unconfigured agent ids when allowlist also contains *", async () => { + it("rejects explicit unconfigured agent ids when allowlist also contains *", async () => { setConfig({ agents: { list: [{ id: "main", subagents: { allowAgents: ["*", "beta"] } }], }, }); const result = await spawn({ agentId: "beta" }); - expectStatus(result, "accepted"); - expectChildSessionKey(result, /^agent:beta:subagent:/); + expectStatus(result, "forbidden"); + expect(result.error ?? "").toBe( + 'agentId "beta" is not in the configured agent registry (allowed: main)', + ); + expect(hoisted.callGatewayMock).not.toHaveBeenCalled(); }); it("normalizes allowlisted agent ids", async () => { setConfig({ agents: { - list: [{ id: "main", subagents: { allowAgents: ["Research"] } }], + list: [{ id: "main", subagents: { allowAgents: ["Research"] } }, { id: "research" }], }, }); const result = await spawn({ agentId: "research" }); @@ -240,7 +243,10 @@ describe("subagent spawn allowlist + sandbox guards", () => { it("allows explicit agentId when requireAgentId is configured", async () => { setConfig({ agents: { - list: [{ id: "main", subagents: { allowAgents: ["worker"], requireAgentId: true } }], + list: [ + { id: "main", subagents: { allowAgents: ["worker"], requireAgentId: true } }, + { id: "worker" }, + ], }, }); const result = await spawn({ agentId: "worker" }); @@ -294,13 +300,17 @@ describe("subagent spawn allowlist + sandbox guards", () => { expectStatus(result, "accepted"); }); - it("allows allowlisted-but-unconfigured agentId", async () => { + it("rejects allowlisted-but-unconfigured agentId", async () => { setConfig({ agents: { list: [{ id: "main", subagents: { allowAgents: ["research"] } }], }, }); const result = await spawn({ agentId: "research" }); - expectStatus(result, "accepted"); + expectStatus(result, "forbidden"); + expect(result.error ?? "").toBe( + 'agentId "research" is not in the configured agent registry (allowed: none)', + ); + expect(hoisted.callGatewayMock).not.toHaveBeenCalled(); }); }); diff --git a/src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts b/src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts index 93c104033e0a..e57948377aba 100644 --- a/src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts +++ b/src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts @@ -122,7 +122,10 @@ async function executeBoundAccountSpawn(params: { setSessionsSpawnConfigOverride({ session: { mainKey: "main", scope: "per-sender" }, messages: { queue: { debounceMs: 0 } }, - agents: { defaults: { subagents: { allowAgents: ["bot-alpha"] } } }, + agents: { + defaults: { subagents: { allowAgents: ["bot-alpha"] } }, + list: [{ id: "main" }, { id: "bot-alpha" }], + }, bindings: params.bindings, }); setupSessionsSpawnGatewayMock({ diff --git a/src/agents/subagent-target-policy.test.ts b/src/agents/subagent-target-policy.test.ts index 2985bc2f3a41..e28774804a4e 100644 --- a/src/agents/subagent-target-policy.test.ts +++ b/src/agents/subagent-target-policy.test.ts @@ -41,6 +41,7 @@ describe("subagent target policy", () => { targetAgentId: "task-manager", requestedAgentId: "task-manager", allowAgents: ["planner", "checker"], + configuredAgentIds: ["task-manager", "planner", "checker"], }); expect(result.ok).toBe(false); if (result.ok) { @@ -65,6 +66,35 @@ describe("subagent target policy", () => { }); }); + it("filters explicit allowlists to configured target ids", () => { + expect( + resolveSubagentAllowedTargetIds({ + requesterAgentId: "main", + allowAgents: ["planner", "stale"], + configuredAgentIds: ["main", "planner"], + }), + ).toEqual({ + allowAny: false, + allowedIds: ["planner"], + }); + + const result = resolveSubagentTargetPolicy({ + requesterAgentId: "main", + targetAgentId: "stale", + requestedAgentId: "stale", + allowAgents: ["planner", "stale"], + configuredAgentIds: ["main", "planner"], + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("Expected target policy to reject stale explicit target"); + } + expect(result.allowedText).toBe("planner"); + expect(result.error).toBe( + 'agentId "stale" is not in the configured agent registry (allowed: planner)', + ); + }); + it("limits wildcard allowlists to configured agents plus the requester", () => { expect( resolveSubagentAllowedTargetIds({ @@ -97,7 +127,7 @@ describe("subagent target policy", () => { ); }); - it("preserves explicit targets when wildcard allowlists are mixed", () => { + it("filters explicit targets when wildcard allowlists are mixed", () => { expect( resolveSubagentAllowedTargetIds({ requesterAgentId: "main", @@ -106,17 +136,22 @@ describe("subagent target policy", () => { }), ).toEqual({ allowAny: true, - allowedIds: ["beta", "main", "planner"], + allowedIds: ["main", "planner"], }); - expect( - resolveSubagentTargetPolicy({ - requesterAgentId: "main", - targetAgentId: "beta", - requestedAgentId: "beta", - allowAgents: ["*", "beta"], - configuredAgentIds: ["main", "planner"], - }), - ).toEqual({ ok: true }); + const result = resolveSubagentTargetPolicy({ + requesterAgentId: "main", + targetAgentId: "beta", + requestedAgentId: "beta", + allowAgents: ["*", "beta"], + configuredAgentIds: ["main", "planner"], + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("Expected target policy to reject stale mixed explicit target"); + } + expect(result.error).toBe( + 'agentId "beta" is not in the configured agent registry (allowed: main, planner)', + ); }); }); diff --git a/src/agents/subagent-target-policy.ts b/src/agents/subagent-target-policy.ts index 95facb97d5c2..08b5fe7f29a1 100644 --- a/src/agents/subagent-target-policy.ts +++ b/src/agents/subagent-target-policy.ts @@ -26,6 +26,20 @@ function normalizeAllowAgents(allowAgents: readonly string[] | undefined): { }; } +function normalizeConfiguredAgentIds( + configuredAgentIds: readonly string[] | undefined, +): Set { + return new Set((configuredAgentIds ?? []).map((id) => normalizeAgentId(id)).filter(Boolean)); +} + +function filterConfiguredAllowedIds(params: { + allowedIds: readonly string[]; + configuredAgentIds?: readonly string[]; +}): string[] { + const configuredIds = normalizeConfiguredAgentIds(params.configuredAgentIds); + return params.allowedIds.filter((id) => configuredIds.has(id)); +} + export function resolveSubagentAllowedTargetIds(params: { requesterAgentId: string; allowAgents?: readonly string[]; @@ -40,10 +54,7 @@ export function resolveSubagentAllowedTargetIds(params: { }; } if (policy.allowAny) { - const configuredIds = (params.configuredAgentIds ?? []) - .map((id) => normalizeAgentId(id)) - .filter(Boolean); - configuredIds.push(...policy.allowedIds); + const configuredIds = Array.from(normalizeConfiguredAgentIds(params.configuredAgentIds)); if (requesterAgentId) { configuredIds.push(requesterAgentId); } @@ -54,7 +65,10 @@ export function resolveSubagentAllowedTargetIds(params: { } return { allowAny: false, - allowedIds: policy.allowedIds, + allowedIds: filterConfiguredAllowedIds({ + allowedIds: policy.allowedIds, + configuredAgentIds: params.configuredAgentIds, + }).toSorted((a, b) => a.localeCompare(b)), }; } @@ -80,7 +94,8 @@ export function resolveSubagentTargetPolicy(params: { return { ok: true }; } const allowedText = allowed.allowedIds.length > 0 ? allowed.allowedIds.join(", ") : "none"; - if (allowed.allowAny) { + const policy = normalizeAllowAgents(params.allowAgents); + if (allowed.allowAny || policy.allowedIds.includes(targetAgentId)) { return { ok: false, allowedText, diff --git a/src/agents/tools/agents-list-tool.test.ts b/src/agents/tools/agents-list-tool.test.ts index a3ac418ac983..59ac041be107 100644 --- a/src/agents/tools/agents-list-tool.test.ts +++ b/src/agents/tools/agents-list-tool.test.ts @@ -75,6 +75,32 @@ describe("agents_list tool", () => { }); }); + it("does not advertise stale allowlist-only targets as spawnable agents", async () => { + loadConfigMock.mockReturnValue({ + agents: { + list: [ + { + id: "main", + default: true, + subagents: { allowAgents: ["stale"] }, + }, + ], + }, + } satisfies OpenClawConfig); + + const result = await createAgentsListTool({ agentSessionKey: "agent:main:main" }).execute( + "call", + {}, + ); + const details = result.details as AgentListDetails; + + expect(details).toStrictEqual({ + requester: "main", + allowAny: false, + agents: [], + }); + }); + it("returns requester as the only target when no subagent allowlist is configured", async () => { loadConfigMock.mockReturnValue({ agents: { @@ -103,6 +129,36 @@ describe("agents_list tool", () => { }); }); + it("uses the implicit default agent as a configured target", async () => { + loadConfigMock.mockReturnValue({ + agents: { + defaults: { + subagents: { allowAgents: ["main"] }, + }, + }, + } satisfies OpenClawConfig); + + const result = await createAgentsListTool({ agentSessionKey: "agent:main:main" }).execute( + "call", + {}, + ); + const details = result.details as AgentListDetails; + + expect(details).toStrictEqual({ + requester: "main", + allowAny: false, + agents: [ + { + id: "main", + name: undefined, + configured: true, + model: undefined, + agentRuntime: { id: "codex", source: "implicit" }, + }, + ], + }); + }); + it("ignores legacy env-forced plugin runtime selections", 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 7a1c284e7f0a..8f98a7dadef7 100644 --- a/src/agents/tools/agents-list-tool.ts +++ b/src/agents/tools/agents-list-tool.ts @@ -6,6 +6,7 @@ import { parseAgentSessionKey, } from "../../routing/session-key.js"; import { resolveModelAgentRuntimeMetadata } from "../agent-runtime-metadata.js"; +import { listAgentIds } from "../agent-scope-config.js"; import { resolveAgentConfig, resolveAgentEffectiveModelPrimary } from "../agent-scope.js"; import { resolveDefaultModelForAgent } from "../model-selection.js"; import { resolveSubagentAllowedTargetIds } from "../subagent-target-policy.js"; @@ -58,7 +59,7 @@ export function createAgentsListTool(opts?: { cfg?.agents?.defaults?.subagents?.allowAgents; const configuredAgents = Array.isArray(cfg.agents?.list) ? cfg.agents?.list : []; - const configuredIds = configuredAgents.map((entry) => normalizeAgentId(entry.id)); + const configuredIds = listAgentIds(cfg); const configuredNameMap = new Map(); for (const entry of configuredAgents) { const name = entry?.name?.trim() ?? ""; diff --git a/src/commands/doctor/repair-sequencing.ts b/src/commands/doctor/repair-sequencing.ts index 7d0fc89a523e..cf79df7f2932 100644 --- a/src/commands/doctor/repair-sequencing.ts +++ b/src/commands/doctor/repair-sequencing.ts @@ -26,6 +26,7 @@ import { maybeRepairOpenPolicyAllowFrom } from "./shared/open-policy-allowfrom.j import { cleanupLegacyPluginDependencyState } from "./shared/plugin-dependency-cleanup.js"; import { repairStaleOAuthProfileShadows } from "./shared/stale-oauth-profile-shadows.js"; import { maybeRepairStalePluginConfig } from "./shared/stale-plugin-config.js"; +import { maybeRepairStaleSubagentAllowlists } from "./shared/stale-subagent-allowlist.js"; import { isUpdatePackageSwapInProgress } from "./shared/update-phase.js"; export async function runDoctorRepairSequence(params: { @@ -114,6 +115,7 @@ export async function runDoctorRepairSequence(params: { applyMutation(await maybeRepairAllowlistPolicyAllowFrom(state.candidate)); applyMutation(maybeRepairOpenPolicyAllowFrom(state.candidate)); applyMutation(maybeRepairGroupAllowFromFallback(state.candidate)); + applyMutation(maybeRepairStaleSubagentAllowlists(state.candidate)); const emptyAllowlistWarnings = scanEmptyAllowlistPolicyWarnings(state.candidate, { doctorFixCommand: params.doctorFixCommand, diff --git a/src/commands/doctor/shared/preview-warnings.ts b/src/commands/doctor/shared/preview-warnings.ts index 103ff318add7..37fb8ab460fe 100644 --- a/src/commands/doctor/shared/preview-warnings.ts +++ b/src/commands/doctor/shared/preview-warnings.ts @@ -47,6 +47,16 @@ function hasPluginLoadPaths(cfg: OpenClawConfig): boolean { return hasRecord(load) && Array.isArray(load.paths) && load.paths.length > 0; } +function hasSubagentAllowlistConfig(cfg: OpenClawConfig): boolean { + if (Array.isArray(cfg.agents?.defaults?.subagents?.allowAgents)) { + return true; + } + return listAgentRecords(cfg).some((agent) => { + const subagents = hasRecord(agent.subagents) ? agent.subagents : undefined; + return Array.isArray(subagents?.allowAgents); + }); +} + function hasExplicitChannelPluginBlockerConfig(cfg: OpenClawConfig): boolean { if (cfg.plugins?.enabled === false) { return true; @@ -436,6 +446,19 @@ export async function collectDoctorPreviewWarnings(params: { const { collectCodexRouteWarnings } = await import("./codex-route-warnings.js"); warnings.push(...collectCodexRouteWarnings({ cfg: params.cfg, env })); } + if (hasSubagentAllowlistConfig(params.cfg)) { + const { collectStaleSubagentAllowlistWarnings, scanStaleSubagentAllowlistReferences } = + await import("./stale-subagent-allowlist.js"); + const staleSubagentAllowlistHits = scanStaleSubagentAllowlistReferences(params.cfg); + if (staleSubagentAllowlistHits.length > 0) { + warnings.push( + collectStaleSubagentAllowlistWarnings({ + hits: staleSubagentAllowlistHits, + doctorFixCommand: params.doctorFixCommand, + }).join("\n"), + ); + } + } const { collectCodexNativeAssetWarnings } = await import("./codex-native-assets.js"); warnings.push(...(await collectCodexNativeAssetWarnings({ cfg: params.cfg, env }))); diff --git a/src/commands/doctor/shared/stale-subagent-allowlist.test.ts b/src/commands/doctor/shared/stale-subagent-allowlist.test.ts new file mode 100644 index 000000000000..b8841ff89cf9 --- /dev/null +++ b/src/commands/doctor/shared/stale-subagent-allowlist.test.ts @@ -0,0 +1,123 @@ +import { describe, expect, it } from "vitest"; +import type { OpenClawConfig } from "../../../config/types.openclaw.js"; +import { + collectStaleSubagentAllowlistWarnings, + maybeRepairStaleSubagentAllowlists, + scanStaleSubagentAllowlistReferences, +} from "./stale-subagent-allowlist.js"; + +describe("stale subagent allowlist doctor repair", () => { + it("detects stale default and per-agent subagent targets", () => { + const cfg = { + agents: { + defaults: { + subagents: { + allowAgents: ["planner", "stale-default"], + }, + }, + list: [ + { + id: "main", + subagents: { + allowAgents: ["planner", "stale-main"], + }, + }, + { id: "planner" }, + ], + }, + } as OpenClawConfig; + + expect(scanStaleSubagentAllowlistReferences(cfg)).toStrictEqual([ + { + pathLabel: "agents.defaults.subagents.allowAgents", + agentId: "stale-default", + normalizedAgentId: "stale-default", + }, + { + pathLabel: "agents.list.0.subagents.allowAgents", + agentId: "stale-main", + normalizedAgentId: "stale-main", + }, + ]); + }); + + it("keeps wildcard, configured OpenClaw agents, and configured ACP targets", () => { + const cfg = { + acp: { + defaultAgent: "claude", + allowedAgents: ["codex"], + }, + agents: { + defaults: { + subagents: { + allowAgents: ["*", "main", "planner", "codex", "claude", "writer", "stale"], + }, + }, + list: [ + { id: "main" }, + { id: "planner" }, + { + id: "writer-agent", + runtime: { type: "acp", acp: { agent: "writer" } }, + }, + ], + }, + } as OpenClawConfig; + + expect(scanStaleSubagentAllowlistReferences(cfg)).toStrictEqual([ + { + pathLabel: "agents.defaults.subagents.allowAgents", + agentId: "stale", + normalizedAgentId: "stale", + }, + ]); + }); + + it("repairs stale entries without widening an explicit empty allowlist", () => { + const cfg = { + agents: { + defaults: { + subagents: { + allowAgents: ["stale"], + }, + }, + list: [ + { + id: "main", + subagents: { + allowAgents: ["*", "planner", "stale-main"], + }, + }, + { id: "planner" }, + ], + }, + } as OpenClawConfig; + + const result = maybeRepairStaleSubagentAllowlists(cfg); + + expect(result.config.agents?.defaults?.subagents?.allowAgents).toStrictEqual([]); + expect(result.config.agents?.list?.[0]?.subagents?.allowAgents).toStrictEqual(["*", "planner"]); + expect(result.changes).toStrictEqual([ + "- agents.defaults.subagents.allowAgents: removed 1 stale subagent target id (stale)", + "- agents.list.0.subagents.allowAgents: removed 1 stale subagent target id (stale-main)", + ]); + }); + + it("formats preview warnings with the doctor fix command", () => { + const warnings = collectStaleSubagentAllowlistWarnings({ + hits: [ + { + pathLabel: "agents.defaults.subagents.allowAgents", + agentId: "research", + normalizedAgentId: "research", + }, + ], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toStrictEqual([ + '- agents.defaults.subagents.allowAgents: stale subagent target "research" is not in the configured agent registry.', + '- Run "openclaw doctor --fix" to remove stale subagent target ids, or add a configured agent or ACP target for each intended target.', + ]); + }); +}); diff --git a/src/commands/doctor/shared/stale-subagent-allowlist.ts b/src/commands/doctor/shared/stale-subagent-allowlist.ts new file mode 100644 index 000000000000..3acc4d641cde --- /dev/null +++ b/src/commands/doctor/shared/stale-subagent-allowlist.ts @@ -0,0 +1,178 @@ +import { listAgentIds } from "../../../agents/agent-scope-config.js"; +import type { OpenClawConfig } from "../../../config/types.openclaw.js"; +import { normalizeAgentId } from "../../../routing/session-key.js"; +import { normalizeOptionalString } from "../../../shared/string-coerce.js"; + +export type StaleSubagentAllowlistHit = { + pathLabel: string; + agentId: string; + normalizedAgentId: string; +}; + +function normalizeOptionalAgentId(value: string | undefined | null): string | undefined { + const trimmed = normalizeOptionalString(value) ?? ""; + if (!trimmed) { + return undefined; + } + return normalizeAgentId(trimmed); +} + +function collectConfiguredSubagentTargetIds(cfg: OpenClawConfig): Set { + const ids = new Set(listAgentIds(cfg)); + for (const agent of cfg.agents?.list ?? []) { + if (agent.runtime?.type !== "acp") { + continue; + } + const acpAgent = normalizeOptionalAgentId(agent.runtime.acp?.agent); + if (acpAgent) { + ids.add(acpAgent); + } + } + const defaultAcpAgent = normalizeOptionalAgentId(cfg.acp?.defaultAgent); + if (defaultAcpAgent) { + ids.add(defaultAcpAgent); + } + for (const entry of cfg.acp?.allowedAgents ?? []) { + if (entry.trim() === "*") { + continue; + } + const acpAgent = normalizeOptionalAgentId(entry); + if (acpAgent) { + ids.add(acpAgent); + } + } + return ids; +} + +function collectStaleAllowlistEntries(params: { + allowAgents: unknown; + pathLabel: string; + configuredTargetIds: ReadonlySet; +}): StaleSubagentAllowlistHit[] { + if (!Array.isArray(params.allowAgents)) { + return []; + } + const hits: StaleSubagentAllowlistHit[] = []; + const seen = new Set(); + for (const entry of params.allowAgents) { + if (typeof entry !== "string") { + continue; + } + const trimmed = entry.trim(); + if (!trimmed || trimmed === "*") { + continue; + } + const normalizedAgentId = normalizeAgentId(trimmed); + if (params.configuredTargetIds.has(normalizedAgentId)) { + continue; + } + const key = `${params.pathLabel}:${normalizedAgentId}`; + if (seen.has(key)) { + continue; + } + seen.add(key); + hits.push({ + pathLabel: params.pathLabel, + agentId: trimmed, + normalizedAgentId, + }); + } + return hits; +} + +export function scanStaleSubagentAllowlistReferences( + cfg: OpenClawConfig, +): StaleSubagentAllowlistHit[] { + const configuredTargetIds = collectConfiguredSubagentTargetIds(cfg); + const hits: StaleSubagentAllowlistHit[] = []; + hits.push( + ...collectStaleAllowlistEntries({ + allowAgents: cfg.agents?.defaults?.subagents?.allowAgents, + pathLabel: "agents.defaults.subagents.allowAgents", + configuredTargetIds, + }), + ); + const agents = Array.isArray(cfg.agents?.list) ? cfg.agents.list : []; + for (const [index, agent] of agents.entries()) { + hits.push( + ...collectStaleAllowlistEntries({ + allowAgents: agent?.subagents?.allowAgents, + pathLabel: `agents.list.${index}.subagents.allowAgents`, + configuredTargetIds, + }), + ); + } + return hits; +} + +export function collectStaleSubagentAllowlistWarnings(params: { + hits: readonly StaleSubagentAllowlistHit[]; + doctorFixCommand: string; +}): string[] { + if (params.hits.length === 0) { + return []; + } + return [ + ...params.hits.map( + (hit) => + `- ${hit.pathLabel}: stale subagent target "${hit.agentId}" is not in the configured agent registry.`, + ), + `- Run "${params.doctorFixCommand}" to remove stale subagent target ids, or add a configured agent or ACP target for each intended target.`, + ]; +} + +function filterAllowAgents(params: { + allowAgents: string[]; + staleTargetIds: ReadonlySet; +}): string[] { + return params.allowAgents.filter((entry) => { + const trimmed = entry.trim(); + return !trimmed || trimmed === "*" || !params.staleTargetIds.has(normalizeAgentId(trimmed)); + }); +} + +export function maybeRepairStaleSubagentAllowlists(cfg: OpenClawConfig): { + config: OpenClawConfig; + changes: string[]; +} { + const hits = scanStaleSubagentAllowlistReferences(cfg); + if (hits.length === 0) { + return { config: cfg, changes: [] }; + } + + const next = structuredClone(cfg); + const hitsByPath = new Map(); + for (const hit of hits) { + hitsByPath.set(hit.pathLabel, [...(hitsByPath.get(hit.pathLabel) ?? []), hit]); + } + + const defaultsHits = hitsByPath.get("agents.defaults.subagents.allowAgents") ?? []; + if (defaultsHits.length > 0 && Array.isArray(next.agents?.defaults?.subagents?.allowAgents)) { + const staleTargetIds = new Set(defaultsHits.map((hit) => hit.normalizedAgentId)); + next.agents.defaults.subagents.allowAgents = filterAllowAgents({ + allowAgents: next.agents.defaults.subagents.allowAgents, + staleTargetIds, + }); + } + + const agents = Array.isArray(next.agents?.list) ? next.agents.list : []; + for (const [index, agent] of agents.entries()) { + const pathLabel = `agents.list.${index}.subagents.allowAgents`; + const agentHits = hitsByPath.get(pathLabel) ?? []; + if (agentHits.length === 0 || !Array.isArray(agent?.subagents?.allowAgents)) { + continue; + } + const staleTargetIds = new Set(agentHits.map((hit) => hit.normalizedAgentId)); + agent.subagents.allowAgents = filterAllowAgents({ + allowAgents: agent.subagents.allowAgents, + staleTargetIds, + }); + } + + const changes = [...hitsByPath.entries()].map(([pathLabel, pathHits]) => { + const ids = pathHits.map((hit) => hit.agentId).join(", "); + return `- ${pathLabel}: removed ${pathHits.length} stale subagent target id${pathHits.length === 1 ? "" : "s"} (${ids})`; + }); + + return { config: next, changes }; +}