fix(cli): stop claiming a write when approvals mutations are no-ops (#125960)

`openclaw approvals allowlist add|remove` printed "Writing local approvals."
from the shared target-resolution helper, before the mutation decision was
made. Both idempotent paths ("Already allowlisted.", "Pattern not found.")
returned without saving, so the CLI announced a write that never happened.
`approvals set` had the same problem: it announced the write and then rejected
unparseable input.

Move the announcement from `loadWritableSnapshotTarget` into the local branch
of `saveSnapshotTargeted`, the function that owns the write. Every caller of
the shared seam is fixed at once and exit codes are unchanged: idempotent add
and remove still leave the requested end state satisfied and exit 0.
This commit is contained in:
Peter Steinberger
2026-08-18 12:12:37 -07:00
committed by GitHub
parent 72783bcdc2
commit 7ae9cbf3bc
2 changed files with 41 additions and 5 deletions
+38 -2
View File
@@ -141,6 +141,10 @@ function expectGatewayCall(index: number, method: string, params: unknown) {
expect(call[2]).toEqual(params);
}
function loggedOutput(): string {
return defaultRuntime.log.mock.calls.map(([line]) => String(line ?? "")).join("\n");
}
function writtenJson(): Record<string, unknown> {
const value = firstMockArg(vi.mocked(defaultRuntime.writeJson));
return requireRecord(value, "written json");
@@ -311,7 +315,7 @@ describe("exec approvals CLI", () => {
await runApprovalsCommand(["approvals", "get"]);
const output = defaultRuntime.log.mock.calls.map(([line]) => String(line ?? "")).join("\n");
const output = loggedOutput();
expect(output).toContain("State");
expect(output).toContain("defaults (no stored overrides)");
expect(output).not.toContain("Exists");
@@ -326,7 +330,7 @@ describe("exec approvals CLI", () => {
await runApprovalsCommand(["approvals", "get"]);
const output = defaultRuntime.log.mock.calls.map(([line]) => String(line ?? "")).join("\n");
const output = loggedOutput();
const hasUnsafeControl = Array.from(output).some((char) => {
const codePoint = char.codePointAt(0) ?? -1;
return (
@@ -888,6 +892,37 @@ describe("exec approvals CLI", () => {
if (requireRecord(saved.agents, "saved agents")["*"] === undefined) {
throw new Error("Expected wildcard exec approval agent entry");
}
expect(loggedOutput()).toContain("Writing local approvals.");
});
it.each([
{
label: "an already-allowlisted add",
args: ["add", "/usr/bin/uptime"],
outcome: "Already allowlisted.",
},
{
label: "a remove of an absent pattern",
args: ["remove", "/usr/bin/never-added"],
outcome: "Pattern not found.",
},
])("reports $label without announcing a local write", async ({ args, outcome }) => {
localSnapshot.file = {
version: 1,
agents: { "*": { allowlist: [{ pattern: "/usr/bin/uptime", lastUsedAt: Date.now() }] } },
};
const updateExecApprovals = vi.mocked(execApprovals.updateExecApprovals);
updateExecApprovals.mockClear();
await runApprovalsCommand(["approvals", "allowlist", ...args]);
const output = loggedOutput();
expect(output).toContain(outcome);
expect(output).not.toContain("Writing local approvals.");
expect(updateExecApprovals).not.toHaveBeenCalled();
// Idempotent add/remove leave the requested end state satisfied: no failure exit.
expect(defaultRuntime.exit).not.toHaveBeenCalled();
expect(runtimeErrors).toHaveLength(0);
});
it("removes wildcard allowlist entry and prunes empty agent", async () => {
@@ -913,6 +948,7 @@ describe("exec approvals CLI", () => {
version: 1,
agents: {},
});
expect(loggedOutput()).toContain("Writing local approvals.");
expect(runtimeErrors).toHaveLength(0);
});
+3 -3
View File
@@ -324,9 +324,6 @@ async function loadWritableSnapshotTarget(opts: ExecApprovalsCliOpts): Promise<{
}> {
// Writes carry the base hash so gateway/node updates can reject stale snapshots.
const { snapshot, nodeId, source } = await loadSnapshotTarget(opts);
if (source === "local") {
defaultRuntime.log(theme.muted("Writing local approvals."));
}
const targetLabel = source === "local" ? "local" : nodeId ? `node:${nodeId}` : "gateway";
if (isNativeApprovalsSnapshot(snapshot) && !snapshot.enabled) {
exitWithError(
@@ -362,6 +359,9 @@ async function saveSnapshotTargeted(params: SaveSnapshotTargetedParams): Promise
});
next = await loadSnapshot(params.opts, params.nodeId);
} else if (params.source === "local") {
// Announced at the write, not at target resolution: no-op allowlist edits and
// rejected `set` input never reach here and must not claim a write happened.
defaultRuntime.log(theme.muted("Writing local approvals."));
next = await saveSnapshotLocal(params.file, params.baseHash);
} else {
next = await saveSnapshot(params.opts, params.nodeId, params.file, params.baseHash);