mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-25 03:45:46 -06:00
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.
This commit is contained in:
committed by
GitHub
parent
6aced3350d
commit
eb502d9aef
@@ -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<void>;
|
||||
|
||||
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<ClawAgentConfigRemovalResult> {
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user