Skip to content

Commit 719ce7f

Browse files
kevinslinkevinslin
authored
feat(signal): support reaction approvals (#85894)
* feat(signal): support reaction approvals * fix(signal): harden approval reaction bindings * fix(signal): quiet native approval prompt flow * test(prompts): refresh direct channel snapshots * fix(signal): suppress duplicate exec approval prompts * revert(reply): keep direct inbound metadata * docs: add signal approval changelog * test(prompts): restore direct channel snapshots * fix(signal): allow defaultTo approval reactions
1 parent 57748a6 commit 719ce7f

20 files changed

Lines changed: 3172 additions & 32 deletions

‎docs/channels/signal.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,21 @@ Config:
306306
- `minimal`/`extensive` enables agent reactions and sets the guidance level.
307307
- Per-account overrides: `channels.signal.accounts.<id>.actions.reactions`, `channels.signal.accounts.<id>.reactionLevel`.
308308

309+
## Approval reactions
310+
311+
Signal exec and plugin approval prompts use the top-level `approvals.exec` and
312+
`approvals.plugin` routing blocks. Signal does not have a
313+
`channels.signal.execApprovals` block.
314+
315+
- `👍` approves once.
316+
- `👎` denies.
317+
- Use `/approve <id> allow-always` when a request offers persistent approval.
318+
319+
Approval reaction resolution requires explicit Signal approvers from
320+
`channels.signal.allowFrom`, `channels.signal.defaultTo`, or the matching account-level fields.
321+
Direct same-chat exec approval prompts can still suppress the duplicate local `/approve` fallback
322+
without explicit approvers; no-approver group approvals keep the local fallback visible.
323+
309324
## Delivery targets (CLI/cron)
310325

311326
- DMs: `signal:+15551234567` (or plain E.164).

‎docs/tools/exec-approvals-advanced.md‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -277,12 +277,13 @@ Generic model:
277277

278278
- host exec policy still decides whether exec approval is required
279279
- `approvals.exec` controls forwarding approval prompts to other chat destinations
280-
- `channels.<channel>.execApprovals` controls whether that channel acts as a native approval client
280+
- `channels.<channel>.execApprovals` controls whether Discord, Slack, Telegram, and similar
281+
channel-specific native clients are enabled
281282
- Slack plugin approvals can use Slack's native approval client when the request comes from Slack
282283
and Slack plugin approvers resolve; `approvals.plugin` can also route plugin approvals to Slack
283284
sessions or targets even when Slack exec approvals are disabled
284-
- WhatsApp emoji approval delivery is gated by `approvals.exec` and `approvals.plugin`, while
285-
approval reactions require explicit WhatsApp approvers from `channels.whatsapp.allowFrom` or `"*"`
285+
- WhatsApp and Signal reaction approval delivery are gated by `approvals.exec` and
286+
`approvals.plugin`; they do not have `channels.<channel>.execApprovals` blocks
286287

287288
Native approval clients auto-enable DM-first delivery when all of these are true:
288289

@@ -301,6 +302,7 @@ FAQ: [Why are there two exec approval configs for chat approvals?](/help/faq-fir
301302
- Slack: `channels.slack.execApprovals.*`
302303
- Telegram: `channels.telegram.execApprovals.*`
303304
- WhatsApp: use `approvals.exec` and `approvals.plugin` to route approval prompts to WhatsApp
305+
- Signal: use `approvals.exec` and `approvals.plugin` to route approval prompts to Signal
304306

305307
These native approval clients add DM routing and optional channel fanout on top of the shared
306308
same-chat `/approve` flow and shared approval buttons.
@@ -321,6 +323,10 @@ Shared behavior:
321323
- WhatsApp emoji approvals handle both exec and plugin prompts only when the matching top-level
322324
forwarding family is enabled and routes to WhatsApp; target-only WhatsApp forwarding stays on
323325
the shared forwarding path unless it matches the same native origin target
326+
- Signal reaction approvals handle both exec and plugin prompts only when the matching top-level
327+
forwarding family is enabled and routes to Signal. Direct same-chat Signal exec approvals can
328+
suppress the local `/approve` fallback without explicit approvers; Signal reaction resolution
329+
still requires explicit Signal approvers from `channels.signal.allowFrom` or `defaultTo`.
324330
- Matrix native DM/channel routing and reaction shortcuts handle both exec and plugin approvals;
325331
plugin authorization still comes from `channels.matrix.dm.allowFrom`
326332
- Matrix native prompts include `com.openclaw.approval` custom event content on the first prompt

‎extensions/signal/src/approval-auth.ts‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,20 @@ function normalizeSignalApproverId(value: string | number): string | undefined {
1919
return e164.length > 1 ? e164 : undefined;
2020
}
2121

22+
export function getSignalApprovalApprovers(params: {
23+
cfg: Parameters<typeof resolveSignalAccount>[0]["cfg"];
24+
accountId?: string | null;
25+
}): string[] {
26+
const account = resolveSignalAccount(params).config;
27+
return resolveApprovalApprovers({
28+
allowFrom: account.allowFrom,
29+
defaultTo: account.defaultTo,
30+
normalizeApprover: normalizeSignalApproverId,
31+
});
32+
}
33+
2234
export const signalApprovalAuth = createResolvedApproverActionAuthAdapter({
2335
channelLabel: "Signal",
24-
resolveApprovers: ({ cfg, accountId }) => {
25-
const account = resolveSignalAccount({ cfg, accountId }).config;
26-
return resolveApprovalApprovers({
27-
allowFrom: account.allowFrom,
28-
defaultTo: account.defaultTo,
29-
normalizeApprover: normalizeSignalApproverId,
30-
});
31-
},
36+
resolveApprovers: getSignalApprovalApprovers,
3237
normalizeSenderId: (value) => normalizeSignalApproverId(value),
3338
});

‎extensions/signal/src/approval-handler.runtime.test.ts‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
const sendMocks = vi.hoisted(() => ({
4+
sendTypingSignal: vi.fn(),
5+
sendMessageSignal: vi.fn(),
6+
}));
7+
8+
vi.mock("./send.js", () => ({
9+
sendTypingSignal: sendMocks.sendTypingSignal,
10+
sendMessageSignal: sendMocks.sendMessageSignal,
11+
}));
12+
13+
const { signalApprovalNativeRuntime } = await import("./approval-handler.runtime.js");
14+
15+
describe("Signal approval native runtime", () => {
16+
beforeEach(() => {
17+
sendMocks.sendTypingSignal.mockReset().mockResolvedValue(true);
18+
sendMocks.sendMessageSignal.mockReset().mockResolvedValue({
19+
messageId: "1700000000000",
20+
timestamp: 1700000000000,
21+
receipt: { parts: [] },
22+
});
23+
});
24+
25+
it("uses the live Signal RPC context when delivering approval prompts", async () => {
26+
const prepared = await signalApprovalNativeRuntime.transport.prepareTarget({
27+
plannedTarget: { target: { to: "+15551230000" } },
28+
accountId: "default",
29+
context: { baseUrl: "http://127.0.0.1:18080", account: "+15550001111" },
30+
} as never);
31+
32+
expect(prepared?.target).toMatchObject({
33+
to: "+15551230000",
34+
accountId: "default",
35+
baseUrl: "http://127.0.0.1:18080",
36+
account: "+15550001111",
37+
});
38+
39+
await signalApprovalNativeRuntime.transport.deliverPending({
40+
cfg: {},
41+
preparedTarget: prepared!.target,
42+
pendingPayload: { text: "approval", allowedDecisions: ["allow-once"] },
43+
} as never);
44+
45+
expect(sendMocks.sendTypingSignal).toHaveBeenCalledWith("+15551230000", {
46+
cfg: {},
47+
accountId: "default",
48+
baseUrl: "http://127.0.0.1:18080",
49+
account: "+15550001111",
50+
});
51+
expect(sendMocks.sendMessageSignal).toHaveBeenCalledWith("+15551230000", "approval", {
52+
cfg: {},
53+
accountId: "default",
54+
baseUrl: "http://127.0.0.1:18080",
55+
account: "+15550001111",
56+
textMode: "plain",
57+
});
58+
});
59+
60+
it("only renders reaction hints when the Signal target author can be bound", async () => {
61+
const cfg = { channels: { signal: { allowFrom: ["+15551230000"] } } };
62+
const unbound = await signalApprovalNativeRuntime.transport.prepareTarget({
63+
plannedTarget: { target: { to: "+15551230000" } },
64+
accountId: "default",
65+
context: { baseUrl: "http://127.0.0.1:18080" },
66+
} as never);
67+
68+
await signalApprovalNativeRuntime.transport.deliverPending({
69+
cfg,
70+
preparedTarget: unbound!.target,
71+
pendingPayload: {
72+
text: "Exec approval required\nID: exec-1\n\nReply with: /approve exec-1 allow-once|deny",
73+
allowedDecisions: ["allow-once", "deny"],
74+
},
75+
} as never);
76+
77+
expect(sendMocks.sendMessageSignal).toHaveBeenLastCalledWith(
78+
"+15551230000",
79+
expect.not.stringContaining("React with:"),
80+
expect.any(Object),
81+
);
82+
83+
const bound = await signalApprovalNativeRuntime.transport.prepareTarget({
84+
plannedTarget: { target: { to: "+15551230000" } },
85+
accountId: "default",
86+
context: { baseUrl: "http://127.0.0.1:18080", account: "+15550001111" },
87+
} as never);
88+
89+
await signalApprovalNativeRuntime.transport.deliverPending({
90+
cfg,
91+
preparedTarget: bound!.target,
92+
pendingPayload: {
93+
text: "Exec approval required\nID: exec-1\n\nReply with: /approve exec-1 allow-once|deny",
94+
allowedDecisions: ["allow-once", "deny"],
95+
},
96+
} as never);
97+
98+
expect(sendMocks.sendMessageSignal).toHaveBeenLastCalledWith(
99+
"+15551230000",
100+
expect.stringContaining("React with:\n\n👍 Allow Once\n👎 Deny"),
101+
expect.any(Object),
102+
);
103+
});
104+
});

0 commit comments

Comments
 (0)