Skip to content

Commit 6830aa3

Browse files
joshavantjoshavant
committed
fix(signal): bind approval reactions from structured deliveries
1 parent a0b3977 commit 6830aa3

9 files changed

Lines changed: 910 additions & 198 deletions

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ export const signalApprovalNativeRuntime = createChannelApprovalNativeRuntimeAda
197197
conversationKey: entry.conversationKey,
198198
messageId: entry.messageId,
199199
approvalId: request.id,
200+
approvalKind: view.approvalKind,
200201
allowedDecisions: pendingPayload.reactionPayload.allowedDecisions,
201202
targetAuthorKeys: entry.targetAuthorKeys,
202203
route: {

‎extensions/signal/src/approval-reactions.test.ts‎

Lines changed: 287 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
1+
import {
2+
buildExecApprovalPendingReplyPayload,
3+
buildPluginApprovalPendingReplyPayload,
4+
} from "openclaw/plugin-sdk/approval-reply-runtime";
15
// Signal tests cover approval reactions plugin behavior.
26
import { beforeEach, describe, expect, it, vi } from "vitest";
37
import {
48
addSignalApprovalReactionHintToText,
5-
appendSignalApprovalReactionHintForOutboundMessage,
9+
addSignalApprovalReactionHintToStructuredPayload,
610
buildSignalApprovalReactionHint,
711
clearSignalApprovalReactionTargetsForTest,
812
maybeResolveSignalApprovalReaction,
9-
registerSignalApprovalReactionTargetForOutboundMessage,
13+
registerSignalApprovalReactionTargetForDeliveredPayload,
1014
registerSignalApprovalReactionTarget,
1115
resolveSignalApprovalReactionTargetWithPersistence,
1216
} from "./approval-reactions.js";
@@ -78,85 +82,334 @@ describe("Signal approval reactions", () => {
7882
).toBe(prompt);
7983
});
8084

81-
it("registers target-mode outbound approval prompts for reactions", async () => {
85+
it("registers delivered structured approval payloads for reactions", async () => {
8286
const cfg = {
8387
channels: {
8488
signal: {
8589
allowFrom: ["+15551230000"],
8690
},
8791
},
8892
approvals: {
89-
plugin: {
93+
exec: {
9094
enabled: true,
9195
mode: "targets" as const,
9296
targets: [{ channel: "signal", to: "+15551230000" }],
9397
},
9498
},
9599
};
96-
const text =
97-
"Plugin approval required\nID: plugin:abc\n\nReply with: /approve plugin:abc allow-once|deny";
98-
const textWithHint = appendSignalApprovalReactionHintForOutboundMessage({
100+
const payload = buildExecApprovalPendingReplyPayload({
101+
approvalId: "exec-structured-approval",
102+
approvalSlug: "exec-str",
103+
allowedDecisions: ["allow-once", "deny"],
104+
command: "printf test",
105+
host: "gateway",
106+
agentId: "main",
107+
sessionKey: "agent:main:signal:direct:+15551230000",
108+
});
109+
const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({
99110
cfg,
100111
accountId: "default",
101112
to: "+15551230000",
102-
text,
113+
payload,
103114
targetAuthor: "+15550009999",
104115
});
105116

106-
expect(textWithHint).toContain("React with:\n\n👍 Allow Once\n👎 Deny");
107117
expect(
108-
registerSignalApprovalReactionTargetForOutboundMessage({
118+
registerSignalApprovalReactionTargetForDeliveredPayload({
109119
cfg,
120+
target: {
121+
channel: "signal",
122+
to: "+15551230000",
123+
accountId: "default",
124+
},
125+
payload: deliveredPayload!,
126+
results: [
127+
{
128+
channel: "signal",
129+
messageId: "1700000000012",
130+
toJid: "+15551230000",
131+
},
132+
],
133+
targetAuthor: "+15550009999",
134+
}),
135+
).toBe(true);
136+
137+
await expect(
138+
resolveSignalApprovalReactionTargetWithPersistence({
110139
accountId: "default",
140+
conversationKey: "+15551230000",
141+
messageId: "1700000000012",
142+
reactionKey: "👍",
143+
targetAuthor: "+15550009999",
144+
}),
145+
).resolves.toEqual({
146+
approvalId: "exec-structured-approval",
147+
approvalKind: "exec",
148+
decision: "allow-once",
149+
route: {
150+
deliveryMode: "target",
111151
to: "+15551230000",
112-
messageId: "1700000000009",
113-
text: textWithHint,
152+
accountId: "default",
153+
agentId: "main",
154+
sessionKey: "agent:main:signal:direct:+15551230000",
155+
},
156+
});
157+
});
158+
159+
it("does not register metadata-only approval payloads without visible reaction hints", async () => {
160+
const cfg = {
161+
channels: {
162+
signal: {
163+
allowFrom: ["+15551230000"],
164+
},
165+
},
166+
approvals: {
167+
exec: {
168+
enabled: true,
169+
mode: "targets" as const,
170+
targets: [{ channel: "signal", to: "+15551230000" }],
171+
},
172+
},
173+
};
174+
const payload = buildExecApprovalPendingReplyPayload({
175+
approvalId: "exec-hidden-reaction",
176+
approvalSlug: "exec-hid",
177+
allowedDecisions: ["allow-once", "deny"],
178+
command: "printf hidden",
179+
host: "gateway",
180+
agentId: "main",
181+
sessionKey: "agent:main:signal:direct:+15551230000",
182+
});
183+
184+
expect(
185+
registerSignalApprovalReactionTargetForDeliveredPayload({
186+
cfg,
187+
target: {
188+
channel: "signal",
189+
to: "+15551230000",
190+
accountId: "default",
191+
},
192+
payload,
193+
results: [
194+
{
195+
channel: "signal",
196+
messageId: "1700000000015",
197+
},
198+
],
114199
targetAuthor: "+15550009999",
115200
}),
116-
).toBe(true);
201+
).toBe(false);
117202

118-
const handled = await maybeResolveSignalApprovalReaction({
203+
await expect(
204+
resolveSignalApprovalReactionTargetWithPersistence({
205+
accountId: "default",
206+
conversationKey: "+15551230000",
207+
messageId: "1700000000015",
208+
reactionKey: "👍",
209+
targetAuthor: "+15550009999",
210+
}),
211+
).resolves.toBeNull();
212+
});
213+
214+
it("registers only delivered chunks that contain visible reaction hints", async () => {
215+
const cfg = {
216+
channels: {
217+
signal: {
218+
allowFrom: ["+15551230000"],
219+
},
220+
},
221+
approvals: {
222+
exec: {
223+
enabled: true,
224+
mode: "targets" as const,
225+
targets: [{ channel: "signal", to: "+15551230000" }],
226+
},
227+
},
228+
};
229+
const payload = buildExecApprovalPendingReplyPayload({
230+
approvalId: "exec-chunked-reaction",
231+
approvalSlug: "exec-ch",
232+
allowedDecisions: ["allow-once", "deny"],
233+
command: "printf chunked",
234+
host: "gateway",
235+
agentId: "main",
236+
sessionKey: "agent:main:signal:direct:+15551230000",
237+
});
238+
const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({
119239
cfg,
120240
accountId: "default",
121-
conversationKey: "+15551230000",
122-
messageId: "1700000000009",
123-
reactionKey: "👍",
124-
actorId: "+15551230000",
241+
to: "+15551230000",
242+
payload,
125243
targetAuthor: "+15550009999",
126244
});
127245

128-
expect(handled).toBe(true);
129-
expect(resolverMocks.resolveSignalApproval).toHaveBeenCalledWith({
246+
expect(
247+
registerSignalApprovalReactionTargetForDeliveredPayload({
248+
cfg,
249+
target: {
250+
channel: "signal",
251+
to: "+15551230000",
252+
accountId: "default",
253+
},
254+
payload: deliveredPayload!,
255+
results: [
256+
{
257+
channel: "signal",
258+
messageId: "1700000000016",
259+
meta: {
260+
signalVisibleText: "Exec approval required\n\nReact with:\n\n👍 Allow Once\n👎 Deny",
261+
},
262+
},
263+
{
264+
channel: "signal",
265+
messageId: "1700000000017",
266+
meta: {
267+
signalVisibleText: "Continuation chunk without controls",
268+
},
269+
},
270+
],
271+
targetAuthor: "+15550009999",
272+
}),
273+
).toBe(true);
274+
275+
await expect(
276+
resolveSignalApprovalReactionTargetWithPersistence({
277+
accountId: "default",
278+
conversationKey: "+15551230000",
279+
messageId: "1700000000016",
280+
reactionKey: "👍",
281+
targetAuthor: "+15550009999",
282+
}),
283+
).resolves.toMatchObject({
284+
approvalId: "exec-chunked-reaction",
285+
decision: "allow-once",
286+
});
287+
await expect(
288+
resolveSignalApprovalReactionTargetWithPersistence({
289+
accountId: "default",
290+
conversationKey: "+15551230000",
291+
messageId: "1700000000017",
292+
reactionKey: "👍",
293+
targetAuthor: "+15550009999",
294+
}),
295+
).resolves.toBeNull();
296+
});
297+
298+
it("registers delivered structured plugin approval payloads using metadata kind", async () => {
299+
const cfg = {
300+
channels: {
301+
signal: {
302+
allowFrom: ["+15551230000"],
303+
},
304+
},
305+
approvals: {
306+
plugin: {
307+
enabled: true,
308+
mode: "targets" as const,
309+
targets: [{ channel: "signal", to: "+15551230000" }],
310+
},
311+
},
312+
};
313+
const payload = buildPluginApprovalPendingReplyPayload({
314+
request: {
315+
id: "plugin-structured-approval",
316+
request: {
317+
title: "Sensitive plugin action",
318+
description: "Needs approval",
319+
allowedDecisions: ["allow-once", "deny"],
320+
},
321+
createdAtMs: 1_000,
322+
expiresAtMs: 61_000,
323+
},
324+
nowMs: 1_000,
325+
});
326+
const deliveredPayload = addSignalApprovalReactionHintToStructuredPayload({
130327
cfg,
131-
approvalId: "plugin:abc",
328+
accountId: "default",
329+
to: "+15551230000",
330+
payload,
331+
targetAuthor: "+15550009999",
332+
});
333+
334+
expect(
335+
registerSignalApprovalReactionTargetForDeliveredPayload({
336+
cfg,
337+
target: {
338+
channel: "signal",
339+
to: "+15551230000",
340+
accountId: "default",
341+
},
342+
payload: deliveredPayload!,
343+
results: [
344+
{
345+
channel: "signal",
346+
messageId: "1700000000013",
347+
},
348+
],
349+
targetAuthor: "+15550009999",
350+
}),
351+
).toBe(true);
352+
353+
await expect(
354+
resolveSignalApprovalReactionTargetWithPersistence({
355+
accountId: "default",
356+
conversationKey: "+15551230000",
357+
messageId: "1700000000013",
358+
reactionKey: "👍",
359+
targetAuthor: "+15550009999",
360+
}),
361+
).resolves.toMatchObject({
362+
approvalId: "plugin-structured-approval",
363+
approvalKind: "plugin",
132364
decision: "allow-once",
133-
senderId: "+15551230000",
134-
gatewayUrl: undefined,
135365
});
136366
});
137367

138-
it("keeps target-mode outbound prompts manual when the target route is disabled", () => {
139-
const text =
140-
"Plugin approval required\nID: plugin:abc\n\nReply with: /approve plugin:abc allow-once|deny";
368+
it("does not register delivered structured approval payloads without explicit approvers", () => {
369+
const payload = buildExecApprovalPendingReplyPayload({
370+
approvalId: "exec-no-approvers",
371+
approvalSlug: "exec-no",
372+
allowedDecisions: ["allow-once", "deny"],
373+
command: "printf test",
374+
host: "gateway",
375+
});
376+
const deliveredPayload = {
377+
...payload,
378+
text: addSignalApprovalReactionHintToText({
379+
text: payload.text ?? "",
380+
allowedDecisions: ["allow-once", "deny"],
381+
}),
382+
};
141383

142384
expect(
143-
appendSignalApprovalReactionHintForOutboundMessage({
385+
registerSignalApprovalReactionTargetForDeliveredPayload({
144386
cfg: {
145-
channels: { signal: { allowFrom: ["+15551230000"] } },
387+
channels: {
388+
signal: {},
389+
},
146390
approvals: {
147-
plugin: {
148-
enabled: false,
391+
exec: {
392+
enabled: true,
149393
mode: "targets",
150394
targets: [{ channel: "signal", to: "+15551230000" }],
151395
},
152396
},
153397
},
154-
accountId: "default",
155-
to: "+15551230000",
156-
text,
398+
target: {
399+
channel: "signal",
400+
to: "+15551230000",
401+
accountId: "default",
402+
},
403+
payload: deliveredPayload,
404+
results: [
405+
{
406+
channel: "signal",
407+
messageId: "1700000000014",
408+
},
409+
],
157410
targetAuthor: "+15550009999",
158411
}),
159-
).toBe(text);
412+
).toBe(false);
160413
});
161414

162415
it("registers reaction state when only allow-always is available", async () => {

0 commit comments

Comments
 (0)