From 0f642d2ac1788bee92d756f8be6e8eb8a604ef7a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 20 Aug 2026 22:44:41 -0700 Subject: [PATCH] fix(agents): clean exec approvals on offline delete, and stop deleting main from taking the wildcard with it (#127037) Two defects in one lifecycle, found by following one thread. `agents delete` tries the Gateway first and falls back to a local path when the Gateway is unreachable -- its own header calls this "gateway delegation and local cleanup fallback". The Gateway path wraps deletion in `withAgentExecApprovalsRemoved`; the local fallback did not. So deleting an agent with no Gateway running left its exec approvals allowlist behind, silently. That residue is not inert. Verified end to end: approve a pattern for an agent, delete it through the offline path, recreate an agent with the same id, and `approvals get` shows the old allowlist live under the new agent. The operator never granted it. The offline path now uses the same journal-fenced, rollback-capable helper as the Gateway path. Removal rather than a warning: an exec approval is a latent authority grant, and leaving one behind contradicts the deletion contract the Gateway already enforces. `agents delete` already trashes the workspace and agent dir, so removing a policy entry is not out of character for it. The second defect is in the shared helper and affects the Gateway path too, which means it is shipped today. It matched policy keys with normalizeAgentId(policyKey) === key and `normalizeAgentId` falls back to `"main"` for anything it cannot represent. `"*"` normalizes to `""` and therefore returns `"main"`, so deleting the `main` agent also matched -- and removed -- the `"*"` wildcard allowlist. Strict normalization now skips unrepresentable keys, so valid aliases are still removed while the wildcard and unrelated agents survive. Sibling sweep of `pruneAgentConfig`, which already handled bindings, subagent allowlists, heartbeat/system-agent ownership, and Talk ownership: broadcast targets, hook mappings, and hook agent allowlists also retained the deleted id. Fixed, preserving `"*"` in the hook allowlist. Follow-ups deliberately not taken here, each needing its own owner or design: offline deletion still lacks the Gateway's transactional cron-job cleanup; plugin-owned routing and thread bindings need an agent-deletion lifecycle hook; remote node approvals need distributed cleanup or explicit warning semantics; approval `agentFilter` cleanup needs a disabled-state design, since removing its last entry would widen policy rather than narrow it. Production +45/-18. --- src/commands/agents.commands.delete.ts | 27 ++++++++++++++------------ src/commands/agents.config.ts | 27 ++++++++++++++++++++++++-- src/commands/agents.delete.test.ts | 18 +++++++++++++++++ src/commands/agents.test.ts | 23 ++++++++++++++++++++++ src/infra/exec-approvals-store.test.ts | 10 ++++++---- src/infra/exec-approvals-store.ts | 9 +++++---- 6 files changed, 92 insertions(+), 22 deletions(-) diff --git a/src/commands/agents.commands.delete.ts b/src/commands/agents.commands.delete.ts index 3b147e2666ca..a95fc8125033 100644 --- a/src/commands/agents.commands.delete.ts +++ b/src/commands/agents.commands.delete.ts @@ -42,6 +42,7 @@ import { isGatewayCredentialsRequiredError, isGatewayTransportError, } from "../gateway/call.js"; +import { withAgentExecApprovalsRemoved } from "../infra/exec-approvals.js"; import { normalizeAgentId, normalizeAgentIdStrict } from "../routing/session-key.js"; import { defaultRuntime, type RuntimeEnv, writeRuntimeJson } from "../runtime.js"; import { readAgentDeletionJournal } from "../state/agent-deletion-journal.js"; @@ -276,19 +277,21 @@ export async function agentsDeleteCommand( existingJournal ?? { agentId, agentDir, workspaceDir, sessionsDir, deleteFiles }, ); try { - if (configured) { - await replaceConfigFile({ - nextConfig: result.config, - ...(baseHash !== undefined ? { baseHash } : {}), - writeOptions: { - allowedAgentRosterRemovals: [agentId], - ...(opts.json ? { skipOutputLogs: true } : {}), - }, - }); - if (!opts.json) { - logConfigUpdated(runtime); + await withAgentExecApprovalsRemoved(agentId, async () => { + if (configured) { + await replaceConfigFile({ + nextConfig: result.config, + ...(baseHash !== undefined ? { baseHash } : {}), + writeOptions: { + allowedAgentRosterRemovals: [agentId], + ...(opts.json ? { skipOutputLogs: true } : {}), + }, + }); + if (!opts.json) { + logConfigUpdated(runtime); + } } - } + }); deletion.commit(); } catch (error) { if (!existingJournal) { diff --git a/src/commands/agents.config.ts b/src/commands/agents.config.ts index 43b4e10b812c..d91ece91c410 100644 --- a/src/commands/agents.config.ts +++ b/src/commands/agents.config.ts @@ -19,7 +19,7 @@ import { pinSurvivorWorkspaceForRosterCollapse } from "../config/agent-workspace import { listRouteBindings } from "../config/bindings.js"; import type { IdentityConfig } from "../config/types.base.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; -import { normalizeAgentId } from "../routing/session-key.js"; +import { normalizeAgentId, normalizeAgentIdStrict } from "../routing/session-key.js"; export type AgentSummary = { id: string; @@ -207,6 +207,10 @@ export function pruneAgentConfig( } { const id = normalizeAgentId(agentId); const clearedOwnerRefs: string[] = []; + const targetsDeletedAgent = (candidate: string) => { + const normalized = normalizeAgentIdStrict(candidate); + return normalized.ok && normalized.value === id; + }; const clearOwnerRef = (value: T | undefined, path: string) => { const owner = normalizeOptionalString(value?.agentId); if (!value || !owner || normalizeAgentId(owner) !== id) { @@ -220,7 +224,7 @@ export function pruneAgentConfig( const pruneAllowAgents = (allowAgents: string[] | undefined) => allowAgents?.filter((entry) => { const trimmed = entry.trim(); - return !trimmed || trimmed === "*" || normalizeAgentId(trimmed) !== id; + return !trimmed || !targetsDeletedAgent(trimmed); }); const nextAgentsList = []; for (const entry of agents) { @@ -277,6 +281,23 @@ export function pruneAgentConfig( } : undefined; const nextTalk = clearOwnerRef(cfg.talk, "talk.agentId"); + const nextBroadcast = cfg.broadcast + ? Object.fromEntries( + Object.entries(cfg.broadcast).map(([peerId, value]) => [ + peerId, + Array.isArray(value) ? value.filter((entry) => !targetsDeletedAgent(entry)) : value, + ]), + ) + : undefined; + const nextHooks = cfg.hooks + ? { + ...cfg.hooks, + allowedAgentIds: cfg.hooks.allowedAgentIds?.filter((entry) => !targetsDeletedAgent(entry)), + mappings: cfg.hooks.mappings?.filter( + (mapping) => !mapping.agentId || !targetsDeletedAgent(mapping.agentId), + ), + } + : undefined; const { list: _legacyList, ownership: _ownership, ...agentsConfig } = cfg.agents ?? {}; const nextAgentsConfig = cfg.agents ? { @@ -305,6 +326,8 @@ export function pruneAgentConfig( ...cfg, agents: nextAgentsConfig, bindings: filteredBindings.length > 0 ? filteredBindings : undefined, + broadcast: nextBroadcast, + hooks: nextHooks, talk: nextTalk, tools: nextTools, }; diff --git a/src/commands/agents.delete.test.ts b/src/commands/agents.delete.test.ts index 834a4898af62..79adb4c12460 100644 --- a/src/commands/agents.delete.test.ts +++ b/src/commands/agents.delete.test.ts @@ -19,6 +19,7 @@ import { } from "../config/sessions/session-accessor.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { GatewayTransportError } from "../gateway/transport-error.js"; +import { readExecApprovalsSnapshot, saveExecApprovals } from "../infra/exec-approvals.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; import { readAgentDeletionJournal } from "../state/agent-deletion-journal.js"; import { readAgentProvenance, recordAgentProvenance } from "../state/agent-provenance.js"; @@ -319,6 +320,14 @@ describe("agents delete command", () => { "agent:main:main": { sessionId: "sess-main", updatedAt: Date.now() }, }, }); + saveExecApprovals({ + version: 1, + agents: { + "*": { security: "deny" }, + main: { security: "allowlist", allowlist: [{ pattern: "/usr/bin/old" }] }, + ops: { security: "allowlist", allowlist: [{ pattern: "/usr/bin/keep" }] }, + }, + }); await agentsDeleteCommand({ id: "main", force: true, json: true }, runtime); @@ -326,6 +335,13 @@ describe("agents delete command", () => { expect(runtime.exit).not.toHaveBeenCalledWith(1); expect(configMocks.replaceConfigFile).toHaveBeenCalledOnce(); expectSessionStore(cfg, {}, "main"); + expect(readExecApprovalsSnapshot().file.agents).toEqual({ + "*": { security: "deny" }, + ops: { + security: "allowlist", + allowlist: [expect.objectContaining({ pattern: "/usr/bin/keep" })], + }, + }); }); }); @@ -570,6 +586,7 @@ describe("agents delete command", () => { "agent:main:main": { sessionId: "sess-main", updatedAt: now + 3 }, }, }); + expect(readExecApprovalsSnapshot().exists).toBe(false); await agentsDeleteCommand({ id: "ops", force: true, json: true }, runtime); @@ -591,6 +608,7 @@ describe("agents delete command", () => { expectSessionStore(cfg, { "agent:main:main": { sessionId: "sess-main", updatedAt: now + 3 }, }); + expect(readExecApprovalsSnapshot().exists).toBe(false); }); }); diff --git a/src/commands/agents.test.ts b/src/commands/agents.test.ts index 698ebf9c1f40..2c6d5059528d 100644 --- a/src/commands/agents.test.ts +++ b/src/commands/agents.test.ts @@ -451,6 +451,19 @@ describe("agents helpers", () => { { agentId: "work", match: { channel: "whatsapp" } }, { agentId: "home", match: { channel: "telegram" } }, ], + broadcast: { + strategy: "parallel", + "peer-1": ["work", "home"], + "peer-2": ["WORK"], + }, + hooks: { + allowedAgentIds: ["*", "work", "home"], + mappings: [ + { id: "work-hook", agentId: "WORK", action: "agent" }, + { id: "home-hook", agentId: "home", action: "agent" }, + { id: "default-hook", action: "agent" }, + ], + }, tools: { agentToAgent: { enabled: true, allow: ["work", "home"] }, }, @@ -463,6 +476,16 @@ describe("agents helpers", () => { expect(result.config.bindings).toStrictEqual([ { agentId: "home", match: { channel: "telegram" } }, ]); + expect(result.config.broadcast).toEqual({ + strategy: "parallel", + "peer-1": ["home"], + "peer-2": [], + }); + expect(result.config.hooks?.allowedAgentIds).toEqual(["*", "home"]); + expect(result.config.hooks?.mappings).toEqual([ + { id: "home-hook", agentId: "home", action: "agent" }, + { id: "default-hook", action: "agent" }, + ]); expect(result.config.tools?.agentToAgent?.allow).toEqual(["home"]); expect(result.config.agents?.defaults?.subagents?.allowAgents).toEqual(["home"]); expect(result.config.agents?.defaults?.heartbeat).toEqual({ every: "5m" }); diff --git a/src/infra/exec-approvals-store.test.ts b/src/infra/exec-approvals-store.test.ts index 3f10601567dd..38041f77bd49 100644 --- a/src/infra/exec-approvals-store.test.ts +++ b/src/infra/exec-approvals-store.test.ts @@ -261,18 +261,20 @@ describe("exec approvals SQLite store", () => { expect(commit).not.toHaveBeenCalled(); }); - it("removes one agent and preserves unrelated policy", async () => { + it("removes one agent and preserves wildcard and unrelated policy", async () => { saveExecApprovals({ version: 1, agents: { - removed: { security: "allowlist", allowlist: [{ pattern: "/usr/bin/old" }] }, + "*": { security: "deny" }, + main: { security: "allowlist", allowlist: [{ pattern: "/usr/bin/old" }] }, kept: { security: "allowlist", allowlist: [{ pattern: "/usr/bin/keep" }] }, }, }); - seedAgentDeletionJournal("removed"); + seedAgentDeletionJournal("main"); - await expect(withAgentExecApprovalsRemoved("removed", async () => "ok")).resolves.toBe("ok"); + await expect(withAgentExecApprovalsRemoved("main", async () => "ok")).resolves.toBe("ok"); expect(loadExecApprovals().agents).toEqual({ + "*": { security: "deny" }, kept: expect.objectContaining({ allowlist: [expect.objectContaining({ pattern: "/usr/bin/keep" })], }), diff --git a/src/infra/exec-approvals-store.ts b/src/infra/exec-approvals-store.ts index 5618bd44408c..bd7cca8fc9c3 100644 --- a/src/infra/exec-approvals-store.ts +++ b/src/infra/exec-approvals-store.ts @@ -4,7 +4,7 @@ import { AgentDeletionCommitUncertainError, } from "../agents/agent-lifecycle-registry.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; -import { normalizeAgentId } from "../routing/session-key.js"; +import { normalizeAgentId, normalizeAgentIdStrict } from "../routing/session-key.js"; import { readAgentDeletionJournal } from "../state/agent-deletion-journal.js"; import { withExistingOpenClawStateDatabaseReadOnly } from "../state/openclaw-state-db-readonly.js"; import { @@ -224,9 +224,10 @@ export async function withAgentExecApprovalsRemoved( if (!operationId) { throw new ExecApprovalsMutationFencedError(); } - const removedPolicyEntries = Object.entries(snapshot.file.agents ?? {}).filter( - ([policyKey]) => normalizeAgentId(policyKey) === key, - ); + const removedPolicyEntries = Object.entries(snapshot.file.agents ?? {}).filter(([policyKey]) => { + const normalizedPolicyKey = normalizeAgentIdStrict(policyKey); + return normalizedPolicyKey.ok && normalizedPolicyKey.value === key; + }); if (removedPolicyEntries.length > 0) { const updated = updateExecApprovalsInTransaction({ baseHash: snapshot.hash,