Signal: add REST API support for containerized deployments#16085
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "send": { | ||
| const recipients = (params.recipient as string[] | undefined) ?? []; | ||
| const groupId = params.groupId as string | undefined; | ||
| const formattedGroupId = groupId ? formatGroupIdForContainer(groupId) : undefined; | ||
| const finalRecipients = | ||
| recipients.length > 0 ? recipients : formattedGroupId ? [formattedGroupId] : []; |
There was a problem hiding this comment.
Container send ignores username targets
The "send" case only reads params.recipient and params.groupId, but sendMessageSignal in send.ts can also produce params.username (via buildTargetParams for username-type targets). When a username target is used in container mode, recipients will be empty and groupId will be undefined, resulting in finalRecipients = [] — the message is silently sent to nobody.
Consider handling the username param (or throwing an explicit error if the bbernhard container doesn't support username-based sends):
| case "send": { | |
| const recipients = (params.recipient as string[] | undefined) ?? []; | |
| const groupId = params.groupId as string | undefined; | |
| const formattedGroupId = groupId ? formatGroupIdForContainer(groupId) : undefined; | |
| const finalRecipients = | |
| recipients.length > 0 ? recipients : formattedGroupId ? [formattedGroupId] : []; | |
| const recipients = (params.recipient as string[] | undefined) ?? []; | |
| const usernames = (params.username as string[] | undefined) ?? []; | |
| const groupId = params.groupId as string | undefined; | |
| const formattedGroupId = groupId ? formatGroupIdForContainer(groupId) : undefined; | |
| const finalRecipients = | |
| recipients.length > 0 ? recipients : usernames.length > 0 ? usernames : formattedGroupId ? [formattedGroupId] : []; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/signal/client-adapter.ts
Line: 101:106
Comment:
**Container send ignores `username` targets**
The `"send"` case only reads `params.recipient` and `params.groupId`, but `sendMessageSignal` in `send.ts` can also produce `params.username` (via `buildTargetParams` for username-type targets). When a username target is used in container mode, `recipients` will be empty and `groupId` will be undefined, resulting in `finalRecipients = []` — the message is silently sent to nobody.
Consider handling the `username` param (or throwing an explicit error if the bbernhard container doesn't support username-based sends):
```suggestion
const recipients = (params.recipient as string[] | undefined) ?? [];
const usernames = (params.username as string[] | undefined) ?? [];
const groupId = params.groupId as string | undefined;
const formattedGroupId = groupId ? formatGroupIdForContainer(groupId) : undefined;
const finalRecipients =
recipients.length > 0 ? recipients : usernames.length > 0 ? usernames : formattedGroupId ? [formattedGroupId] : [];
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
The other e2e test ( Prompt To Fix With AIThis is a comment left during a code review.
Path: src/signal/monitor.tool-result.pairs-uuid-only-senders-uuid-allowlist-entry.e2e.test.ts
Line: 47:51
Comment:
**Missing adapter/container mocks will cause test hang**
`monitor.ts` now imports `adapterRpcRequest` and `checkAdapter` from `./client-adapter.js`, and uses `runSignalSseLoop` (which calls `streamSignalEventsAdapter`). The adapter's `resolveApiMode` defaults to `"auto"` when `apiMode` isn't in the config, which triggers `detectSignalApiMode` — this races `signalCheck` (mocked here to return `{}` with falsy `ok`) against `containerCheck` (unmocked, making a real HTTP request). Both probes fail, causing `Promise.any` to throw, and `runSignalSseLoop` enters an infinite reconnect loop.
The other e2e test (`sends-tool-summaries-responseprefix`) correctly handles this by mocking both `./client-adapter.js` and `./sse-reconnect.js`. This test should do the same, or alternatively set `apiMode: "native"` in the test config to skip auto-detection:
```
config = {
channels: {
signal: { autoStart: false, dmPolicy: "open", allowFrom: ["*"], apiMode: "native" },
},
};
```
How can I resolve this? If you propose a fix, please make it concise. |
|
Updated (cleaned formatting): Addressed both review points in
Validation:
|
|
Updated with concise summary:
Validation:
|
bfc1ccb to
f92900f
Compare
| * - "native": Use native signal-cli with JSON-RPC + SSE (/api/v1/rpc, /api/v1/events) | ||
| * - "container": Use bbernhard/signal-cli-rest-api with REST + WebSocket (/v2/send, /v1/receive/{account}) | ||
| */ | ||
| apiMode?: SignalApiMode; |
There was a problem hiding this comment.
apiMode defined at account level but only read from channel-global config
The apiMode field is defined in SignalAccountConfig (line 35), but client-adapter.ts:44 reads it exclusively from cfg.channels?.signal?.apiMode. Users might configure apiMode per account expecting it to work, but it will be ignored. Either remove it from SignalAccountConfig or update the implementation to respect account-level overrides.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/types.signal.ts
Line: 35:35
Comment:
`apiMode` defined at account level but only read from channel-global config
The `apiMode` field is defined in `SignalAccountConfig` (line 35), but `client-adapter.ts:44` reads it exclusively from `cfg.channels?.signal?.apiMode`. Users might configure `apiMode` per account expecting it to work, but it will be ignored. Either remove it from `SignalAccountConfig` or update the implementation to respect account-level overrides.
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
|
|
||
| // Cache auto-detected modes per baseUrl to avoid repeated network probes. | ||
| const detectedModeCache = new Map<string, "native" | "container">(); |
There was a problem hiding this comment.
Module-scoped cache never invalidated
The detectedModeCache persists for the lifetime of the process with no invalidation mechanism. If the Signal service configuration changes (e.g., switching from native to container) or if apiMode config is updated at runtime, the cache will serve stale values. Consider adding a cache invalidation method or TTL.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/signal/client-adapter.ts
Line: 32:32
Comment:
Module-scoped cache never invalidated
The `detectedModeCache` persists for the lifetime of the process with no invalidation mechanism. If the Signal service configuration changes (e.g., switching from native to container) or if `apiMode` config is updated at runtime, the cache will serve stale values. Consider adding a cache invalidation method or TTL.
How can I resolve this? If you propose a fix, please make it concise.| const groupId = (params.groupIds as string[] | undefined)?.[0] ?? undefined; | ||
| const reactionParams = { | ||
| baseUrl: opts.baseUrl, | ||
| account: (params.account as string) ?? "", | ||
| recipient, | ||
| emoji: (params.emoji as string) ?? "", | ||
| targetAuthor: (params.targetAuthor as string) ?? recipient, | ||
| targetTimestamp: params.targetTimestamp as number, | ||
| groupId, |
There was a problem hiding this comment.
Check if group ID needs formatting for container reactions
In the "send" case (lines 105-113), group IDs are formatted via formatGroupIdForContainer before being passed to the container. However, here in sendReaction, the groupId on line 172 is passed directly without formatting. Verify the bbernhard container's /v1/reactions endpoint expects the same group.{base64} format for consistency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/signal/client-adapter.ts
Line: 164:172
Comment:
Check if group ID needs formatting for container reactions
In the `"send"` case (lines 105-113), group IDs are formatted via `formatGroupIdForContainer` before being passed to the container. However, here in `sendReaction`, the `groupId` on line 172 is passed directly without formatting. Verify the bbernhard container's `/v1/reactions` endpoint expects the same `group.{base64}` format for consistency.
How can I resolve this? If you propose a fix, please make it concise.
d59c7ff to
3824ab5
Compare
|
Rebased onto latest Addressed greptile follow-ups:
Validation:
|
3175099 to
4d750d0
Compare
4d750d0 to
b30b6a0
Compare
b30b6a0 to
60d1e19
Compare
a4cb32c to
1ce3b01
Compare
openclaw-barnacle
Bot
removed
the
docs
Hua688
changed the title
Hua688
changed the title
0a3e66b to
0ebf2e5
Compare
58b06e5 to
5a3b0ae
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs maintainer review before merge. What this changes: This PR adds a Signal native/container adapter, a bbernhard signal-cli-rest-api REST/WebSocket client, Maintainer follow-up before merge: This is an open implementation PR with new transport and dependency behavior, raw payload logging, and missing live container proof, so the next action is maintainer review/update of the existing branch rather than an autonomous replacement fix PR. Best possible solution: Ship a plugin-local Signal transport adapter that preserves native signal-cli JSON-RPC/SSE behavior while adding documented bbernhard REST/WebSocket mode, with aligned config schema/help/docs, targeted tests, and live container proof. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 542821cd1e60. |
|
Updated to narrow the scope of this PR. I removed the Signal status reaction behavior from this branch for now, so #16085 focuses only on container REST/WebSocket support for
The status reaction work can be handled separately after this container support PR is reviewed. |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main always calls native Real behavior proof Next step before merge Security Review detailsBest possible solution: Ship the plugin-local Signal transport adapter after maintainer review, with native behavior preserved, config/docs aligned, targeted tests green, and redacted live bbernhard container proof attached to the PR. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: current main always calls native Is this the best way to solve the issue? Yes in direction: a Signal-plugin-local adapter is the right boundary for supporting native and container transports. It is not merge-ready until the PR has redacted real behavior proof and maintainer review of the new transport/dependency surface. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9cf0651e46f7. |
|
Updated this branch to address the current ClawSweeper/maintainer findings while keeping the original adapter boundary intact. What changed:
Why the adapter still reads config: The adapter is intentionally the only layer that knows whether Signal is using native JSON-RPC/SSE or the bbernhard REST/WebSocket container. Callers such as send/monitor/probe should continue to express Signal operations, not branch on This keeps Validation run locally:
Live container note: this branch has been exercised against a local |
|
Updated the branch for the latest ClawSweeper findings and rebased it onto current Fixes included:
Validation run locally:
Latest pushed head: |
🤖 PR Review SummaryStatus: Analysis
Conflict Details
Next Steps
Automated review by GitHub Manager Agent |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
3 participants
Summary
Linked Issue/PR
User-visible / Behavior Changes
Security Impact
Compatibility / Migration
Real behavior proof
Verification