From eb502d9aef286d86f8f42869f871835ec66cb83d Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 21 Aug 2026 12:00:58 -0700 Subject: [PATCH] fix(claws): remove exec approvals when a Claw agent is removed (#127365) `claws remove` deleted the agent and reported status complete while its exec-approvals policy survived in the shared state DB, so a later agent reusing the id silently inherited the old allowlist. claws was the third agent-removal path and the only one that never opened an agent-deletion journal, which withAgentExecApprovalsRemoved requires as its fence. Open the journal around the config commit and run both commit branches inside the approvals fence, matching the gateway and CLI paths. Cron stays declaration-owned and unchanged. --- src/claws/lifecycle-config-removal.ts | 82 +++++++--- src/claws/lifecycle-remove-approvals.test.ts | 156 +++++++++++++++++++ 2 files changed, 214 insertions(+), 24 deletions(-) create mode 100644 src/claws/lifecycle-remove-approvals.test.ts diff --git a/src/claws/lifecycle-config-removal.ts b/src/claws/lifecycle-config-removal.ts index a7e4cc83c51f..eceb3989944c 100644 --- a/src/claws/lifecycle-config-removal.ts +++ b/src/claws/lifecycle-config-removal.ts @@ -1,5 +1,6 @@ import { createHash } from "node:crypto"; import { stableStringify } from "@openclaw/normalization-core"; +import { beginAgentDeletion } from "../agents/agent-lifecycle-registry.js"; import { listAgentEntries } from "../agents/agent-scope.js"; import { getRuntimeConfig } from "../config/config.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; @@ -7,7 +8,9 @@ import { AgentConfigPreconditionError, deleteAgentConfigEntry, } from "../gateway/server-methods/agents-config-mutations.js"; +import { withAgentExecApprovalsRemoved } from "../infra/exec-approvals.js"; import { normalizeAgentId } from "../routing/session-key.js"; +import { readAgentDeletionJournal } from "../state/agent-deletion-journal.js"; import { digestClawAgentConfig } from "./agent-config-digest.js"; import { deletionEffects, @@ -17,6 +20,25 @@ import { export type ConfigCommit = (transform: (config: OpenClawConfig) => OpenClawConfig) => Promise; +type ClawAgentConfigRemovalParams = { + agentId: string; + expectedDigest: string; + expectedRemovalSurfaceDigest: string; + expectedState: "present" | "missing"; + fallbackWorkspace: string; + config?: OpenClawConfig; + commitConfig?: ConfigCommit; + trashPath?: ClawTrashPath; + onModified: () => Error; +}; + +type ClawAgentConfigRemovalResult = { + agentRemoved: boolean; + cleanupTargets?: ClawCleanupTargets; + configBeforeDelete: OpenClawConfig; + nextConfig: OpenClawConfig; +}; + export { digestClawAgentConfig } from "./agent-config-digest.js"; export function digestClawAgentRemovalSurface(config: OpenClawConfig, agentId: string): string { @@ -32,31 +54,11 @@ export function digestClawAgentRemovalSurface(config: OpenClawConfig, agentId: s return `sha256:${createHash("sha256").update(stableStringify(surface)).digest("hex")}`; } -export async function claimClawAgentConfigRemoval(params: { - agentId: string; - expectedDigest: string; - expectedRemovalSurfaceDigest: string; - expectedState: "present" | "missing"; - fallbackWorkspace: string; - config?: OpenClawConfig; - commitConfig?: ConfigCommit; - trashPath?: ClawTrashPath; - onModified: () => Error; -}): Promise<{ - agentRemoved: boolean; - cleanupTargets?: ClawCleanupTargets; - configBeforeDelete: OpenClawConfig; - nextConfig: OpenClawConfig; -}> { +async function commitClawAgentConfigRemoval( + params: ClawAgentConfigRemovalParams, +): Promise { if (params.commitConfig) { - let result: - | { - agentRemoved: boolean; - cleanupTargets?: ClawCleanupTargets; - configBeforeDelete: OpenClawConfig; - nextConfig: OpenClawConfig; - } - | undefined; + let result: ClawAgentConfigRemovalResult | undefined; await params.commitConfig((config) => { const effects = deletionEffects(config, params.agentId, params.fallbackWorkspace); const agent = listAgentEntries(config).find((candidate) => candidate.id === params.agentId); @@ -149,3 +151,35 @@ export async function claimClawAgentConfigRemoval(params: { }; } } + +export async function claimClawAgentConfigRemoval(params: ClawAgentConfigRemovalParams) { + const config = params.config ?? getRuntimeConfig(); + const effects = deletionEffects(config, params.agentId, params.fallbackWorkspace); + // beginAgentDeletion takes over an existing journal row instead of refusing it, so rolling back + // a row this call did not open would erase another deletion's record. + const existingJournal = readAgentDeletionJournal(params.agentId); + const deletion = beginAgentDeletion({ + agentId: params.agentId, + workspaceDir: effects.workspace, + agentDir: effects.agentDir, + sessionsDir: effects.sessionsDir, + // Claw removal owns selective cleanup and may retain modified or untracked workspace entries, + // so the journal must not claim authority to trash them. + deleteFiles: existingJournal?.deleteFiles ?? false, + }); + try { + const result = await withAgentExecApprovalsRemoved(params.agentId, async () => + commitClawAgentConfigRemoval({ ...params, config }), + ); + deletion.commit(); + // The journal fences only the roster and approvals commit here; Claw's own filesystem cleanup + // runs afterwards and may legitimately end partial, so completion is recorded now. + deletion.finish(); + return result; + } catch (error) { + if (!existingJournal) { + deletion.rollback(); + } + throw error; + } +} diff --git a/src/claws/lifecycle-remove-approvals.test.ts b/src/claws/lifecycle-remove-approvals.test.ts new file mode 100644 index 000000000000..52d3d9c9b440 --- /dev/null +++ b/src/claws/lifecycle-remove-approvals.test.ts @@ -0,0 +1,156 @@ +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; +import { beginAgentDeletion } from "../agents/agent-lifecycle-registry.js"; +import { withTempHomeConfig, writeOpenClawConfig } from "../config/test-helpers.js"; +import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { loadExecApprovals, saveExecApprovals } from "../infra/exec-approvals.js"; +import { readAgentDeletionJournal } from "../state/agent-deletion-journal.js"; +import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js"; +import { captureEnv, setTestEnvValue } from "../test-utils/env.js"; +import { applyClawAddPlan } from "./add.js"; +import { + claimClawAgentConfigRemoval, + digestClawAgentRemovalSurface, +} from "./lifecycle-config-removal.js"; +import { applyClawRemovePlan, buildClawRemovePlan } from "./lifecycle-state.js"; +import { buildClawAddPlan } from "./lifecycle.js"; +import { parseClawManifest } from "./schema.js"; +import type { ClawSourceIdentity } from "./types.js"; + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); +const envSnapshot = captureEnv(["OPENCLAW_STATE_DIR"]); + +afterEach(() => { + closeOpenClawStateDatabaseForTest(); + envSnapshot.restore(); +}); + +async function buildApprovalFixture() { + const root = tempDirs.make("openclaw-claw-remove-approvals-"); + const parsed = parseClawManifest({ + schemaVersion: 1, + agent: { id: "worker", name: "Worker" }, + }); + if (!parsed.ok) { + throw new Error(JSON.stringify(parsed.diagnostics)); + } + const source: ClawSourceIdentity = { + kind: "package", + name: "@acme/worker", + version: "1.0.0", + packageRoot: root, + manifestPath: join(root, "openclaw.claw.json"), + integrityKind: "artifact", + integrity: "sha256:manifest", + byteLength: 100, + }; + return await buildClawAddPlan({ + manifest: parsed.manifest, + source, + context: { workspace: join(root, "workspace-worker") }, + }); +} + +describe("Claw exec approvals removal", () => { + it.each([ + { label: "config-file commit", useCommitConfig: false }, + { label: "commitConfig seam", useCommitConfig: true }, + ])("removes only the claw agent policy through the $label", async ({ useCommitConfig }) => { + const addPlan = await buildApprovalFixture(); + + await withTempHomeConfig({}, async ({ home }) => { + const env = { OPENCLAW_STATE_DIR: join(home, ".openclaw") }; + setTestEnvValue("OPENCLAW_STATE_DIR", env.OPENCLAW_STATE_DIR); + let config: OpenClawConfig = {}; + await applyClawAddPlan(addPlan, { + consentPlanIntegrity: addPlan.planIntegrity, + env, + commitConfig: async (transform) => { + config = transform(config); + }, + }); + await writeOpenClawConfig(home, config); + saveExecApprovals({ + version: 1, + agents: { + "*": { security: "deny" }, + worker: { + security: "allowlist", + allowlist: [{ pattern: "/usr/bin/rm" }], + }, + kept: { + security: "allowlist", + allowlist: [{ pattern: "/usr/bin/keep" }], + }, + }, + }); + const plan = useCommitConfig + ? await buildClawRemovePlan("worker", { env, config }) + : await buildClawRemovePlan("worker"); + const common = { + consentPlanIntegrity: plan.planIntegrity, + trashPath: async () => true, + }; + + const result = useCommitConfig + ? await applyClawRemovePlan(plan, { + ...common, + env, + config, + commitConfig: async (transform) => { + config = transform(config); + }, + }) + : await applyClawRemovePlan(plan, common); + + expect(result).toMatchObject({ status: "complete", agentRemoved: true }); + expect(loadExecApprovals().agents).toEqual({ + "*": { security: "deny" }, + kept: { + security: "allowlist", + allowlist: [expect.objectContaining({ pattern: "/usr/bin/keep" })], + }, + }); + expect(readAgentDeletionJournal("worker")).toMatchObject({ + cleanupCompleted: true, + deleteFiles: false, + }); + }); + }); + + // beginAgentDeletion takes over an existing journal row, so a failed Claw removal must not roll + // back a deletion another path started. + it.each([ + { label: "keeps a pre-existing journal", seedJournal: true }, + { label: "rolls back the journal it opened", seedJournal: false }, + ])("$label when the config commit fails", async ({ seedJournal }) => { + const root = tempDirs.make("openclaw-claw-remove-journal-"); + setTestEnvValue("OPENCLAW_STATE_DIR", join(root, "state")); + if (seedJournal) { + beginAgentDeletion({ + agentId: "worker", + agentDir: join(root, "agent"), + workspaceDir: join(root, "workspace"), + sessionsDir: join(root, "sessions"), + }); + } + + await expect( + claimClawAgentConfigRemoval({ + agentId: "worker", + expectedDigest: "sha256:unused", + expectedRemovalSurfaceDigest: digestClawAgentRemovalSurface({}, "worker"), + expectedState: "present", + fallbackWorkspace: join(root, "workspace"), + config: {}, + commitConfig: async () => { + throw new Error("claw commit failed"); + }, + onModified: () => new Error("claw agent modified"), + }), + ).rejects.toThrow("claw commit failed"); + + expect(readAgentDeletionJournal("worker") === undefined).toBe(!seedJournal); + }); +});