mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-23 10:55:31 -06:00
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.
This commit is contained in:
committed by
GitHub
parent
bed80ee645
commit
0f642d2ac1
@@ -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) {
|
||||
|
||||
@@ -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 = <T extends { agentId?: string }>(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,
|
||||
};
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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" });
|
||||
|
||||
@@ -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" })],
|
||||
}),
|
||||
|
||||
@@ -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<T>(
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user