Skip to content

Commit ec905c1

Browse files
jesse-merhijesse-merhi
committed
fix(signal): harden setup transport probing
1 parent 00f6b48 commit ec905c1

44 files changed

Lines changed: 2873 additions & 1395 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎docs/.generated/config-baseline.sha256‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
84d1ca104fd7c97996299e9804cd21103fd2ced7acd0fe5f71a4fc7d6276dae9 config-baseline.json
1+
09ac06fea49a2e0086ef977d87530aacf6af8e2cd19513982a003b8df133f1ba config-baseline.json
22
0883f41da0e064a58ac557e244a97d6052c3d262225d309cffbbef9e7300f48e config-baseline.core.json
3-
87845735d2cfa660fb081d16544c92041110a31cddcd5be7462a0e54dc46b2f7 config-baseline.channel.json
4-
a7eac21a283d5eefa22a2b6168e1151f30519c19bc7efa2bd4ac2a4583ab0d59 config-baseline.plugin.json
3+
4058fdcf6e1f49b6c49e4c07ada7446bf0add9fead44877561e32d6bb13068a8 config-baseline.channel.json
4+
e61e1a692c531be5ee15c2469ba91f0f1a7c78cb32f37ffcce796d60538a57ce config-baseline.plugin.json

‎docs/.generated/plugin-sdk-api-baseline.sha256‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
0bbd37802d7330a5639480fd15a4f97131b996423136598fc8bd20719d6ad009 plugin-sdk-api-baseline.json
2-
7b889420c787ac6c16fff3f9ef7ff0136f7387c1c695b7991f458b5c72f57818 plugin-sdk-api-baseline.jsonl
1+
6c2888150a054a355ca5687b0d0b05e7b0c1537a3e2f159702b3386cbc4647ca plugin-sdk-api-baseline.json
2+
b831a450a6a4be5650b82239f74475456ce16f263b9607ff0961f490407a0d2a plugin-sdk-api-baseline.jsonl

‎docs/plugins/sdk-setup.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,18 @@ const setupWizard: ChannelSetupWizard = {
473473

474474
`ChannelSetupWizard` also supports `textInputs`, `dmPolicy`, `allowFrom`, `groupAccess`, `prepare`, `finalize`, and more. See the Discord plugin's `src/setup-core.ts` for a full bundled example.
475475

476+
Use `finalize` for validation or confirmation that must happen after the standard setup steps. It can return `{ cancelled: true }` to leave setup without saving any wizard-local config, recording the account, running post-write hooks, or showing the completion note. If the user accepted installation of an external plugin immediately before its wizard ran, OpenClaw keeps only that installation metadata so the installed package and config record stay consistent. Do not return `cfg` or `credentialValues` together with `cancelled`; cancelled results ignore those values.
477+
478+
```typescript
479+
finalize: async ({ prompter }) => {
480+
const save = await prompter.confirm({
481+
message: "Save this channel configuration?",
482+
initialValue: true,
483+
});
484+
return save ? undefined : { cancelled: true };
485+
},
486+
```
487+
476488
<AccordionGroup>
477489
<Accordion title="Shared allowFrom prompts">
478490
For DM allowlist prompts that only need the standard `note -> prompt -> parse -> merge -> patch` flow, prefer the shared setup helpers from `openclaw/plugin-sdk/setup`: `createPromptParsedAllowFromForAccount(...)`, `createTopLevelChannelParsedAllowFromPrompt(...)`, and `createNestedChannelParsedAllowFromPrompt(...)`.

‎extensions/msteams/src/setup-surface.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,9 @@ export const msteamsSetupWizard: ChannelSetupWizard = {
259259
// incoming cfg if the base ever returns void for forward-compat.
260260
const baseFinalize = msteamsSetupWizardBase.finalize;
261261
const baseResult = baseFinalize ? await baseFinalize(params) : undefined;
262+
if (baseResult?.cancelled) {
263+
return baseResult;
264+
}
262265
let next = baseResult?.cfg ?? params.cfg;
263266
const finalCreds = resolveMSTeamsCredentials(next.channels?.msteams);
264267
if (finalCreds?.type === "secret") {

‎extensions/signal/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
"clawhubSpec": "clawhub:@openclaw/signal",
4444
"npmSpec": "@openclaw/signal",
4545
"defaultChoice": "npm",
46-
"minHostVersion": ">=2026.6.11",
46+
"minHostVersion": ">=2026.6.9",
4747
"allowInvalidConfigRecovery": true
4848
},
4949
"compat": {

‎extensions/signal/src/accounts.test.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Signal tests cover accounts plugin behavior.
22
import { describe, expect, it } from "vitest";
33
import {
4+
listEnabledSignalAccounts,
45
listSignalAccountIds,
56
resolveDefaultSignalAccountId,
67
resolveSignalAccount,
@@ -48,4 +49,65 @@ describe("resolveSignalAccount", () => {
4849
expect(resolved.config.account).toBe("+15555550123");
4950
expect(resolved.configured).toBe(true);
5051
});
52+
53+
it("treats root server fields as shared defaults when only named accounts exist", () => {
54+
const cfg = {
55+
channels: {
56+
signal: {
57+
httpUrl: "http://127.0.0.1:8080",
58+
autoStart: false,
59+
accounts: {
60+
work: { account: "+15555550123" },
61+
},
62+
},
63+
},
64+
} as never;
65+
66+
expect(listSignalAccountIds(cfg)).toEqual(["work"]);
67+
expect(listEnabledSignalAccounts(cfg).map((account) => account.accountId)).toEqual(["work"]);
68+
expect(resolveSignalAccount({ cfg, accountId: "work" })).toMatchObject({
69+
baseUrl: "http://127.0.0.1:8080",
70+
configured: true,
71+
});
72+
});
73+
74+
it("keeps an explicit accountless default enumerable when named accounts coexist", () => {
75+
const cfg = {
76+
channels: {
77+
signal: {
78+
httpUrl: "http://127.0.0.1:8080",
79+
autoStart: false,
80+
accounts: {
81+
default: {},
82+
work: { account: "+15555550123" },
83+
},
84+
},
85+
},
86+
} as never;
87+
88+
expect(listSignalAccountIds(cfg)).toEqual(["default", "work"]);
89+
expect(listEnabledSignalAccounts(cfg).map((account) => account.accountId)).toEqual([
90+
"default",
91+
"work",
92+
]);
93+
expect(resolveSignalAccount({ cfg, accountId: "default" })).toMatchObject({
94+
baseUrl: "http://127.0.0.1:8080",
95+
configured: true,
96+
});
97+
});
98+
99+
it("does not infer a root account from apiMode alone", () => {
100+
const cfg = {
101+
channels: {
102+
signal: {
103+
apiMode: "native",
104+
accounts: {
105+
work: { account: "+15555550123" },
106+
},
107+
},
108+
},
109+
} as never;
110+
111+
expect(listSignalAccountIds(cfg)).toEqual(["work"]);
112+
});
51113
});

‎extensions/signal/src/accounts.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,21 @@ export type ResolvedSignalAccount = {
1919
config: SignalAccountConfig;
2020
};
2121

22+
function isSignalAccountConfigConfigured(config: SignalAccountConfig | undefined): boolean {
23+
return Boolean(
24+
normalizeOptionalString(config?.account) ||
25+
normalizeOptionalString(config?.configPath) ||
26+
normalizeOptionalString(config?.httpUrl) ||
27+
normalizeOptionalString(config?.cliPath) ||
28+
normalizeOptionalString(config?.httpHost) ||
29+
typeof config?.httpPort === "number" ||
30+
typeof config?.autoStart === "boolean",
31+
);
32+
}
33+
2234
const { listAccountIds, resolveDefaultAccountId } = createAccountListHelpers("signal", {
35+
// Top-level transport fields are shared defaults for named accounts. Only an account
36+
// identity implicitly creates the default; accountless defaults must use accounts.default.
2337
implicitDefaultAccount: {
2438
channelKeys: ["account"],
2539
},
@@ -52,15 +66,7 @@ export function resolveSignalAccount(params: {
5266
const host = normalizeOptionalString(merged.httpHost) ?? "127.0.0.1";
5367
const port = merged.httpPort ?? 8080;
5468
const baseUrl = normalizeOptionalString(merged.httpUrl) ?? `http://${host}:${port}`;
55-
const configured = Boolean(
56-
normalizeOptionalString(merged.account) ||
57-
normalizeOptionalString(merged.configPath) ||
58-
normalizeOptionalString(merged.httpUrl) ||
59-
normalizeOptionalString(merged.cliPath) ||
60-
normalizeOptionalString(merged.httpHost) ||
61-
typeof merged.httpPort === "number" ||
62-
typeof merged.autoStart === "boolean",
63-
);
69+
const configured = isSignalAccountConfigConfigured(merged);
6470
return {
6571
accountId,
6672
enabled,

‎extensions/signal/src/channel.ts‎

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
// Signal plugin module implements channel behavior.
21
import { DEFAULT_ACCOUNT_ID } from "openclaw/plugin-sdk/account-id";
32
import { buildDmGroupAccountAllowlistAdapter } from "openclaw/plugin-sdk/allowlist-config-edit";
43
import type { ChannelOutboundAdapter } from "openclaw/plugin-sdk/channel-contract";
@@ -42,6 +41,7 @@ import { signalMessageActions } from "./message-actions.js";
4241
import { looksLikeSignalTargetId, normalizeSignalMessagingTarget } from "./normalize.js";
4342
import { resolveSignalOutboundTarget } from "./outbound-session.js";
4443
import { materializeSignalPresentationFallback } from "./presentation-fallback.js";
44+
import { formatSignalCapabilitiesProbe } from "./probe-readiness.js";
4545
import { resolveSignalReactionLevel } from "./reaction-level.js";
4646
import { resolveSignalReplyContextWithPersistence } from "./reply-authors.js";
4747
import { signalSetupAdapter } from "./setup-core.js";
@@ -561,32 +561,7 @@ export const signalPlugin: ChannelPlugin<ResolvedSignalAccount, SignalProbe> =
561561
apiMode: account.config?.apiMode ?? "auto",
562562
});
563563
},
564-
formatCapabilitiesProbe: ({ probe }) => {
565-
if (!probe) {
566-
return [];
567-
}
568-
const readiness = (() => {
569-
switch (probe.readiness) {
570-
case "ready":
571-
return { text: "Signal readiness: ready", tone: "success" as const };
572-
case "account_missing":
573-
return { text: "Signal readiness: account missing", tone: "warn" as const };
574-
case "receive_unavailable":
575-
return { text: "Signal readiness: receive unavailable", tone: "warn" as const };
576-
case "unreachable":
577-
return { text: "Signal readiness: daemon unreachable", tone: "error" as const };
578-
default:
579-
return null;
580-
}
581-
})();
582-
if (!readiness) {
583-
return [];
584-
}
585-
return [
586-
readiness,
587-
...(probe.version ? [{ text: `Signal daemon: ${probe.version}` }] : []),
588-
];
589-
},
564+
formatCapabilitiesProbe: formatSignalCapabilitiesProbe,
590565
resolveAccountSnapshot: ({ account }) => ({
591566
accountId: account.accountId,
592567
name: account.name,

‎extensions/signal/src/client-adapter.test.ts‎

Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -166,28 +166,38 @@ describe("detectSignalApiMode", () => {
166166
expect(result).toBe("native");
167167
});
168168

169-
it("prefers native even when the container probe resolves first", async () => {
170-
mockNativeCheck.mockImplementation(
171-
() =>
172-
new Promise((resolve) => {
173-
setTimeout(() => resolve({ ok: true, status: 200 }), 1);
174-
}),
175-
);
176-
mockContainerCheck.mockResolvedValue({ ok: true, status: 200 });
169+
it("prefers native even when the container probe resolves more than 50ms first", async () => {
170+
vi.useFakeTimers();
171+
try {
172+
mockNativeCheck.mockImplementation(
173+
() =>
174+
new Promise((resolve) => {
175+
setTimeout(() => resolve({ ok: true, status: 200 }), 75);
176+
}),
177+
);
178+
mockContainerCheck.mockResolvedValue({ ok: true, status: 200 });
177179

178-
const result = await detectSignalApiMode("http://localhost:8080");
179-
expect(result).toBe("native");
180+
const result = detectSignalApiMode("http://localhost:8080");
181+
await vi.advanceTimersByTimeAsync(75);
182+
await expect(result).resolves.toBe("native");
183+
} finally {
184+
vi.useRealTimers();
185+
}
180186
});
181187

182-
it("returns container after the native preference grace when native does not respond", async () => {
188+
it("returns container after the native probe reaches its timeout", async () => {
183189
vi.useFakeTimers();
184190
try {
185-
mockNativeCheck.mockImplementation(() => new Promise(() => {}));
191+
mockNativeCheck.mockImplementation(
192+
() =>
193+
new Promise((resolve) => {
194+
setTimeout(() => resolve({ ok: false, status: null, error: "Timed out" }), 100);
195+
}),
196+
);
186197
mockContainerCheck.mockResolvedValue({ ok: true, status: 200 });
187198

188-
const result = detectSignalApiMode("http://localhost:8080");
189-
await Promise.resolve();
190-
await vi.advanceTimersByTimeAsync(50);
199+
const result = detectSignalApiMode("http://localhost:8080", 100);
200+
await vi.advanceTimersByTimeAsync(100);
191201
await expect(result).resolves.toBe("container");
192202
} finally {
193203
vi.useRealTimers();
@@ -626,7 +636,7 @@ describe("signalCheck", () => {
626636
status: 200,
627637
});
628638

629-
expect(mockNativeCheck).toHaveBeenCalledTimes(4);
639+
expect(mockNativeCheck).toHaveBeenCalledTimes(2);
630640
expect(mockContainerCheck).toHaveBeenCalledTimes(2);
631641
});
632642

@@ -645,7 +655,7 @@ describe("signalCheck", () => {
645655
status: 200,
646656
});
647657

648-
expect(mockNativeCheck).toHaveBeenCalledTimes(4);
658+
expect(mockNativeCheck).toHaveBeenCalledTimes(2);
649659
expect(mockContainerCheck).toHaveBeenCalledTimes(2);
650660
});
651661
});
@@ -806,6 +816,26 @@ describe("streamSignalEvents", () => {
806816
);
807817
});
808818

819+
it("reuses the detected native mode across stream reconnects", async () => {
820+
setApiMode("auto");
821+
mockNativeCheck.mockResolvedValue({ ok: true, status: 200 });
822+
mockContainerCheck.mockResolvedValue({ ok: true, status: 101 });
823+
mockNativeStreamEvents.mockResolvedValue(undefined);
824+
825+
const params = {
826+
baseUrl: "http://native-stream-cache.local:8080",
827+
account: "+14259798283",
828+
onEvent: vi.fn(),
829+
};
830+
await streamSignalEvents(params);
831+
await streamSignalEvents(params);
832+
833+
expect(mockNativeCheck).toHaveBeenCalledTimes(1);
834+
expect(mockContainerCheck).toHaveBeenCalledTimes(1);
835+
expect(mockNativeReceiveCheck).not.toHaveBeenCalled();
836+
expect(mockNativeStreamEvents).toHaveBeenCalledTimes(2);
837+
});
838+
809839
it("forwards timeout to container event stream", async () => {
810840
setApiMode("container");
811841
mockStreamContainerEvents.mockResolvedValue(undefined);
@@ -824,14 +854,11 @@ describe("streamSignalEvents", () => {
824854
it("revalidates an unvalidated cached container mode before streaming", async () => {
825855
setApiMode("auto");
826856
mockNativeCheck.mockResolvedValue({ ok: false, status: 404 });
827-
mockContainerCheck
828-
.mockResolvedValueOnce({ ok: true, status: 200 })
829-
.mockResolvedValueOnce({ ok: true, status: 200 })
830-
.mockResolvedValueOnce({
831-
ok: false,
832-
status: 200,
833-
error: "Signal container receive endpoint did not upgrade to WebSocket (HTTP 200)",
834-
});
857+
mockContainerCheck.mockResolvedValueOnce({ ok: true, status: 200 }).mockResolvedValueOnce({
858+
ok: false,
859+
status: 200,
860+
error: "Signal container receive endpoint did not upgrade to WebSocket (HTTP 200)",
861+
});
835862

836863
await expect(signalCheck("http://auto-cache.local:8080")).resolves.toEqual({
837864
ok: true,
@@ -870,7 +897,7 @@ describe("streamSignalEvents", () => {
870897
}),
871898
).rejects.toThrow("Signal API not reachable at http://auto-cache-no-account.local:8080");
872899
expect(mockStreamContainerEvents).not.toHaveBeenCalled();
873-
expect(mockContainerCheck).toHaveBeenCalledTimes(2);
900+
expect(mockContainerCheck).toHaveBeenCalledTimes(1);
874901
});
875902
});
876903

0 commit comments

Comments
 (0)