mirror of
https://github.com/openclaw/openclaw.git
synced 2026-06-03 11:04:06 +00:00
fix: route denied exec approval followups to sessions
Routes denied async exec approval followups through the originating main session before using direct external fallback. Keeps strict inline-eval timeout denials fail-closed, while preserving suppression for subagent, cron, and no-session denial cases. Refs #88167. Verification: - git diff --check origin/main...refs/remotes/pr/88417 - .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main - gh pr checks 88417 --repo openclaw/openclaw --watch=false Co-authored-by: brokemac79 <martin_cleary@yahoo.co.uk>
This commit is contained in:
@@ -162,11 +162,16 @@ Approval-backed interpreter/runtime runs are intentionally conservative:
|
||||
When approvals are required, the exec tool returns immediately with an approval id. Use that id to
|
||||
correlate later approved-run system events (`Exec finished`, and `Exec running` when configured).
|
||||
If no decision arrives before the timeout, the request is treated as an approval timeout and
|
||||
surfaced as a terminal denial rather than an agent-waking system event.
|
||||
surfaced as a terminal host-command denial. For main-agent async approvals with an originating
|
||||
session, OpenClaw also resumes that session with an internal followup so the agent observes that
|
||||
the command did not run instead of later repairing a missing result.
|
||||
|
||||
### Followup delivery behavior
|
||||
|
||||
After an approved async exec finishes, OpenClaw sends a followup `agent` turn to the same session.
|
||||
Denied async approvals use the same main-session followup path for the denial status, but they do
|
||||
not register elevated runtime handoffs and they do not run the command. Denials without a resumable
|
||||
main session are either suppressed or reported through a safe direct route when one exists.
|
||||
|
||||
- If a valid external delivery target exists (deliverable channel plus target `to`), followup delivery uses that channel.
|
||||
- In webchat-only or internal-session flows with no external target, followup delivery stays session-only (`deliver: false`).
|
||||
|
||||
@@ -443,9 +443,13 @@ Exec lifecycle is surfaced as system messages:
|
||||
- `Exec finished`.
|
||||
|
||||
These are posted to the agent's session after the node reports the event.
|
||||
Denied exec approvals are terminal: OpenClaw can report the denial to the
|
||||
operator or direct chat route, but it does not post `Exec denied` back into the
|
||||
agent session or wake agent work.
|
||||
Denied exec approvals are terminal for the host command itself: the command
|
||||
does not run. For main-agent async approvals with an originating session,
|
||||
OpenClaw posts the denial back into that session as an internal followup so the
|
||||
agent can stop waiting on the async command and avoid a missing-result repair.
|
||||
If there is no session or the session cannot be resumed, OpenClaw can still
|
||||
report a concise denial to the operator or direct chat route. Denials for
|
||||
subagent sessions are not posted back into the subagent.
|
||||
Gateway-host exec approvals emit the same lifecycle events when the
|
||||
command finishes (and optionally when running longer than the threshold).
|
||||
Approval-gated execs reuse the approval id as the `runId` in these
|
||||
@@ -453,11 +457,12 @@ messages for easy correlation.
|
||||
|
||||
## Denied approval behavior
|
||||
|
||||
When an async exec approval is denied, OpenClaw treats the request as terminal.
|
||||
It can show a concise denial to the operator or direct chat route, but it does
|
||||
not send denial guidance back through the agent session. That keeps a denied
|
||||
command from becoming another model turn and prevents the agent from reusing
|
||||
output from an earlier run of the same command.
|
||||
When an async exec approval is denied, OpenClaw treats the host command as
|
||||
terminal and fail-closed. For main-agent sessions, the denial is delivered as an
|
||||
internal session followup that tells the agent the async command did not run.
|
||||
That preserves transcript continuity without exposing stale command output. If
|
||||
session delivery is unavailable, OpenClaw falls back to a concise operator or
|
||||
direct-chat denial when a safe route exists.
|
||||
|
||||
## Implications
|
||||
|
||||
|
||||
@@ -113,17 +113,26 @@ describe("exec approval followup", () => {
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("suppresses denied followups for normal sessions", async () => {
|
||||
await expect(
|
||||
sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-main",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: "webchat",
|
||||
resultText: "Exec denied (gateway id=req-denied-main, user-denied): uname -a",
|
||||
}),
|
||||
).resolves.toBe(false);
|
||||
it("routes denied followups through the originating main session", async () => {
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-main",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: "webchat",
|
||||
resultText: "Exec denied (gateway id=req-denied-main, user-denied): uname -a",
|
||||
});
|
||||
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
const agentArgs = expectGatewayAgentFollowup({
|
||||
sessionKey: "agent:main:main",
|
||||
deliver: false,
|
||||
channel: "webchat",
|
||||
idempotencyKey: "exec-approval-followup:req-denied-main",
|
||||
});
|
||||
expect(agentArgs.message).toContain("An async command did not run.");
|
||||
expect(agentArgs.message).toContain(
|
||||
"Exec denied (gateway id=req-denied-main, user-denied): uname -a",
|
||||
);
|
||||
expect(agentArgs.message).not.toContain("missing tool result");
|
||||
expect(agentArgs.message).not.toContain("transcript repair");
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -287,7 +296,9 @@ describe("exec approval followup", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("uses safe direct denied copy without resuming the session", async () => {
|
||||
it("falls back to safe direct denied copy when session resume fails", async () => {
|
||||
vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing"));
|
||||
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-resume-failed",
|
||||
sessionKey: "agent:main:telegram:-100123",
|
||||
@@ -302,10 +313,12 @@ describe("exec approval followup", () => {
|
||||
content: "Command did not run: approval timed out.",
|
||||
idempotencyKey: "exec-approval-followup:req-denied-resume-failed",
|
||||
});
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
expect(callGatewayTool).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("uses safe direct denied copy for nested-parentheses denial metadata without resuming the session", async () => {
|
||||
it("falls back to safe direct denied copy for nested-parentheses denial metadata", async () => {
|
||||
vi.mocked(callGatewayTool).mockRejectedValueOnce(new Error("session missing"));
|
||||
|
||||
await sendExecApprovalFollowup({
|
||||
approvalId: "req-denied-resume-failed-nested",
|
||||
sessionKey: "agent:main:telegram:-100123",
|
||||
@@ -321,7 +334,7 @@ describe("exec approval followup", () => {
|
||||
content: "Command did not run: approval timed out.",
|
||||
idempotencyKey: "exec-approval-followup:req-denied-resume-failed-nested",
|
||||
});
|
||||
expect(callGatewayTool).not.toHaveBeenCalled();
|
||||
expect(callGatewayTool).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("suppresses denied followups for subagent sessions", async () => {
|
||||
|
||||
@@ -292,21 +292,12 @@ export async function sendExecApprovalFollowup(
|
||||
? normalizedTurnSourceChannel
|
||||
: undefined;
|
||||
|
||||
if (isDenied) {
|
||||
if (!sessionKey || shouldSuppressExecDeniedFollowup(sessionKey)) {
|
||||
return false;
|
||||
}
|
||||
return await sendDirectFollowupFallback({
|
||||
approvalId: params.approvalId,
|
||||
deliveryTarget,
|
||||
resultText,
|
||||
sessionError: null,
|
||||
allowDenied: true,
|
||||
});
|
||||
}
|
||||
|
||||
let sessionError: unknown = null;
|
||||
|
||||
if (isDenied && (!sessionKey || shouldSuppressExecDeniedFollowup(sessionKey))) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (sessionKey && params.direct !== true) {
|
||||
try {
|
||||
const agentArgs = buildAgentFollowupArgs({
|
||||
@@ -342,6 +333,24 @@ export async function sendExecApprovalFollowup(
|
||||
}
|
||||
}
|
||||
|
||||
if (isDenied) {
|
||||
if (
|
||||
await sendDirectFollowupFallback({
|
||||
approvalId: params.approvalId,
|
||||
deliveryTarget,
|
||||
resultText,
|
||||
sessionError,
|
||||
allowDenied: true,
|
||||
})
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
if (sessionError) {
|
||||
throw new Error(`Session followup failed: ${formatUnknownError(sessionError)}`);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
if (
|
||||
await sendDirectFollowupFallback({
|
||||
approvalId: params.approvalId,
|
||||
|
||||
@@ -332,6 +332,7 @@ describe("processGatewayAllowlist", () => {
|
||||
askFallback: "full" | "allowlist";
|
||||
approvedByAsk: boolean;
|
||||
}) {
|
||||
buildExecApprovalFollowupTargetMock.mockImplementation((value) => value);
|
||||
resolveExecHostApprovalContextMock.mockReturnValue({
|
||||
approvals: { allowlist: [], file: { version: 1, agents: {} } },
|
||||
hostSecurity: params.security,
|
||||
@@ -355,6 +356,7 @@ describe("processGatewayAllowlist", () => {
|
||||
security: params.security,
|
||||
ask: "always",
|
||||
strictInlineEval: true,
|
||||
sessionKey: "agent:main:main",
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1117,7 +1119,12 @@ EOF`,
|
||||
expect(result.pendingResult?.details.status).toBe("approval-pending");
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
null,
|
||||
expect.objectContaining({
|
||||
approvalId: "req-1",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: undefined,
|
||||
direct: false,
|
||||
}),
|
||||
"Exec denied (gateway id=req-1, approval-timeout): python3 -c 'print(1)'",
|
||||
);
|
||||
});
|
||||
@@ -1135,7 +1142,12 @@ EOF`,
|
||||
expect(result.pendingResult?.details.status).toBe("approval-pending");
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
null,
|
||||
expect.objectContaining({
|
||||
approvalId: "req-1",
|
||||
sessionKey: "agent:main:main",
|
||||
turnSourceChannel: undefined,
|
||||
direct: false,
|
||||
}),
|
||||
"Exec denied (gateway id=req-1, approval-timeout): python3 -c 'print(1)'",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -168,7 +168,7 @@ vi.mock("./bash-tools.exec-host-shared.js", () => ({
|
||||
buildDefaultExecApprovalRequestArgs: vi.fn(() => ({})),
|
||||
createAndRegisterDefaultExecApprovalRequest: createAndRegisterDefaultExecApprovalRequestMock,
|
||||
shouldResolveExecApprovalUnavailableInline: vi.fn(() => false),
|
||||
buildExecApprovalFollowupTarget: vi.fn(() => ({ approvalId: "approval-1" })),
|
||||
buildExecApprovalFollowupTarget: vi.fn((value) => value),
|
||||
resolveApprovalDecisionOrUndefined: resolveApprovalDecisionOrUndefinedMock,
|
||||
createExecApprovalDecisionState: createExecApprovalDecisionStateMock,
|
||||
enforceStrictInlineEvalApprovalBoundary: enforceStrictInlineEvalApprovalBoundaryMock,
|
||||
@@ -1365,7 +1365,10 @@ describe("executeNodeHostCommand", () => {
|
||||
expect(autoReviewer).not.toHaveBeenCalled();
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
{ approvalId: "approval-1" },
|
||||
expect.objectContaining({
|
||||
approvalId: "approval-1",
|
||||
sessionKey: "requested-session",
|
||||
}),
|
||||
"Exec denied (node=node-1 id=approval-1, approval-timeout): bun ./script.ts",
|
||||
);
|
||||
});
|
||||
@@ -1638,7 +1641,10 @@ describe("executeNodeHostCommand", () => {
|
||||
expect(autoReviewer).not.toHaveBeenCalled();
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
{ approvalId: "approval-1" },
|
||||
expect.objectContaining({
|
||||
approvalId: "approval-1",
|
||||
sessionKey: "requested-session",
|
||||
}),
|
||||
"Exec denied (node=node-1 id=approval-1, approval-timeout): echo 'unterminated",
|
||||
);
|
||||
});
|
||||
@@ -1693,7 +1699,10 @@ describe("executeNodeHostCommand", () => {
|
||||
expect(result.details?.status).toBe("approval-pending");
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
{ approvalId: "approval-1" },
|
||||
expect.objectContaining({
|
||||
approvalId: "approval-1",
|
||||
sessionKey: "requested-session",
|
||||
}),
|
||||
"Exec denied (node=node-1 id=approval-1, approval-timeout): bun ./script.ts",
|
||||
);
|
||||
});
|
||||
@@ -2057,7 +2066,10 @@ describe("executeNodeHostCommand", () => {
|
||||
expect(result.details?.status).toBe("approval-pending");
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledWith(
|
||||
{ approvalId: "approval-1" },
|
||||
expect.objectContaining({
|
||||
approvalId: "approval-1",
|
||||
sessionKey: "requested-session",
|
||||
}),
|
||||
"Exec denied (node=node-1 id=approval-1, approval-timeout): python3 -c 'print(1)'",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -1141,7 +1141,7 @@ describe("exec approvals", () => {
|
||||
expect(agentCalls[0]?.message).toContain("webchat-ok");
|
||||
});
|
||||
|
||||
it("does not spawn a gateway followup agent when approval is denied", async () => {
|
||||
it("routes denied approval status through the originating session", async () => {
|
||||
const agentCalls: Array<Record<string, unknown>> = [];
|
||||
|
||||
vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => {
|
||||
@@ -1172,8 +1172,19 @@ describe("exec approvals", () => {
|
||||
});
|
||||
|
||||
expect(result.details.status).toBe("approval-pending");
|
||||
await new Promise((resolve) => setTimeout(resolve, 25));
|
||||
expect(agentCalls).toHaveLength(0);
|
||||
const approvalId = (result.details as { approvalId?: string }).approvalId;
|
||||
expect(typeof approvalId).toBe("string");
|
||||
await expect.poll(() => agentCalls.length, { timeout: 3000, interval: 1 }).toBe(1);
|
||||
expectRecordFields(agentCalls[0], {
|
||||
sessionKey: "agent:main:main",
|
||||
deliver: false,
|
||||
idempotencyKey: `exec-approval-followup:${approvalId}`,
|
||||
});
|
||||
expect(agentCalls[0]?.message).toContain("An async command did not run.");
|
||||
expect(agentCalls[0]?.message).toContain(
|
||||
`Exec denied (gateway id=${approvalId}, user-denied): echo ok`,
|
||||
);
|
||||
expect(sendMessage).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requires a separate approval for each elevated command after allow-once", async () => {
|
||||
|
||||
Reference in New Issue
Block a user