feat(signal): integrate native Signal messenger support#603
feat(signal): integrate native Signal messenger support#603quybquang wants to merge 1 commit intosipeed:mainfrom
Conversation
This commit introduces the independent picoclaw-signal-bridge daemon using Unix socket IPC for robust security and privacy. Updated Makefiles for a simplified build process and synchronized documentation across all 6 supported languages (EN, VI, ZH, JA, FR, PT-BR) to highlight Signal setup instructions.
There was a problem hiding this comment.
Pull request overview
This PR integrates native Signal messenger support into PicoClaw through an independent bridge daemon architecture. The design isolates AGPL-licensed Signal dependencies (libsignal, mautrix-signal) into a separate process (picoclaw-signal-bridge), keeping the main PicoClaw gateway MIT-licensed and CGo-free. Communication occurs via Unix socket IPC with a newline-delimited JSON protocol.
Changes:
- Added Signal channel support in the main gateway with automatic reconnection logic and compound "UUID|phone" sender ID format for improved UX
- Implemented standalone bridge daemon with device linking, message routing, and E164 phone number extraction
- Updated documentation across all 6 supported README languages (EN, ZH, VI, JA, FR, PT-BR) with consistent setup instructions
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/config.go | Added SignalConfig struct for bridge URL and allowlist configuration |
| pkg/config/defaults.go | Added default Signal configuration with Unix socket path |
| pkg/channels/signal.go | Implemented Signal channel with IPC client, reconnection logic, and message handling |
| pkg/channels/signal_proto.go | Defined IPC protocol structures for bridge communication |
| pkg/channels/signal_test.go | Added comprehensive tests for Signal channel, IPC protocol, and connection handling |
| pkg/channels/manager.go | Integrated Signal channel into channel manager initialization |
| contrib/picoclaw-signal-bridge/main.go | Bridge daemon entry point with device linking mode and graceful shutdown |
| contrib/picoclaw-signal-bridge/bridge.go | Core bridge logic for Signal protocol, message routing, and phone number extraction |
| contrib/picoclaw-signal-bridge/ipc.go | Unix socket IPC server for PicoClaw communication |
| contrib/picoclaw-signal-bridge/proto.go | IPC protocol types matching main gateway definitions |
| contrib/picoclaw-signal-bridge/go.mod | Bridge dependencies including mautrix-signal and libsignal |
| contrib/picoclaw-signal-bridge/go.sum | Dependency checksums and versions |
| contrib/picoclaw-signal-bridge/build.sh | Build script for compiling libsignal FFI and bridge binary |
| contrib/picoclaw-signal-bridge/README.md | Bridge-specific documentation and usage instructions |
| contrib/picoclaw-signal-bridge/LICENSE | AGPL-3.0 license for bridge component |
| contrib/picoclaw-signal-bridge/.gitignore | Ignore patterns for build artifacts and database files |
| README.md (+ 5 translations) | Added Signal setup instructions in all supported languages |
| Makefile | Added build-signal target for convenience |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LIBRARY_PATH="$LIB_DIR:$LIBRARY_PATH" \ | ||
| CGO_ENABLED=1 \ | ||
| CGO_LDFLAGS="-L$LIB_DIR -lsignal_ffi -lc++ -framework Security -framework Foundation" \ |
There was a problem hiding this comment.
The build script uses macOS-specific frameworks in CGO_LDFLAGS ("-framework Security -framework Foundation") which will cause build failures on Linux systems. Consider detecting the operating system and using platform-specific linker flags, or document that this build script is macOS-only and provide separate instructions for Linux builds.
| LIBRARY_PATH="$LIB_DIR:$LIBRARY_PATH" \ | |
| CGO_ENABLED=1 \ | |
| CGO_LDFLAGS="-L$LIB_DIR -lsignal_ffi -lc++ -framework Security -framework Foundation" \ | |
| OS_NAME="$(uname)" | |
| if [ "$OS_NAME" = "Darwin" ]; then | |
| CGO_LDFLAGS="-L$LIB_DIR -lsignal_ffi -lc++ -framework Security -framework Foundation" | |
| else | |
| CGO_LDFLAGS="-L$LIB_DIR -lsignal_ffi -lc++" | |
| fi | |
| LIBRARY_PATH="$LIB_DIR:$LIBRARY_PATH" \ | |
| CGO_ENABLED=1 \ | |
| CGO_LDFLAGS="$CGO_LDFLAGS" \ |
| db, err := sql.Open("sqlite3", dbPath+"?_journal_mode=WAL&_busy_timeout=5000") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open database: %w", err) | ||
| } | ||
|
|
||
| rawDB, err := dbutil.NewWithDB(db, "sqlite3") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create dbutil: %w", err) | ||
| } | ||
|
|
||
| container := store.NewStore(rawDB, nil) | ||
| if err := container.Upgrade(context.Background()); err != nil { | ||
| return nil, fmt.Errorf("failed to upgrade database: %w", err) | ||
| } | ||
|
|
||
| bridge := &Bridge{ | ||
| logger: logger, | ||
| dataDir: dataDir, | ||
| container: container, | ||
| } | ||
|
|
||
| devices, err := container.GetAllDevices(context.Background()) | ||
| if err == nil && len(devices) > 0 { | ||
| bridge.device = devices[0] | ||
| logger.Info().Str("number", bridge.device.Number).Msg("Loaded existing device session") | ||
| } | ||
|
|
||
| return bridge, nil |
There was a problem hiding this comment.
The database connection opened in NewBridge (line 42) is never explicitly closed. The sql.DB handle is wrapped in dbutil and stored in the container, but there's no cleanup mechanism when the bridge stops. This could lead to resource leaks, especially if the bridge is started and stopped multiple times. Consider adding a Close method to the Bridge that properly closes the database connection, and call it from the Stop method or main's defer.
|
|
||
| ## Build Prerequisites | ||
|
|
||
| - Go 1.25+ |
There was a problem hiding this comment.
The documentation states "Go 1.25+" as a prerequisite, but Go 1.25 does not exist. As of January 2025, the latest stable Go version is around 1.23.x. Update this to reference a valid Go version like "Go 1.22+" or "Go 1.23+".
| - Go 1.25+ | |
| - Go 1.22+ |
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| data, err := ipc.Receive() | ||
| if err != nil { | ||
| // Avoid log spam when PicoClaw isn't running by sleeping 1s | ||
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
|
|
||
| var outMsg SignalIPCOutbound | ||
| if err := json.Unmarshal(data, &outMsg); err != nil { | ||
| b.logger.Error().Err(err).Msg("Failed to parse IPC message") | ||
| continue | ||
| } | ||
|
|
||
| if outMsg.Type == "send" { | ||
| b.sendSignalMessage(ctx, outMsg) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The listenIPC goroutine (line 127) may not terminate properly on context cancellation if it's blocked in the Receive call (line 262). The select statement only checks ctx.Done() before calling Receive, but if Receive is blocking, it won't check again until Receive returns. When the bridge is stopped, this goroutine could remain blocked indefinitely. Consider using a reader with a deadline or making the IPC server's Receive method context-aware to ensure proper cleanup.
| for status := range statusChan { | ||
| b.logger.Info(). | ||
| Str("status", fmt.Sprintf("%v", status)). | ||
| Msg("Signal connection status changed") |
There was a problem hiding this comment.
The status monitoring goroutine (line 136-142) may leak if the statusChan is never closed or if the bridge is stopped before the channel is fully drained. When Stop is called, there's no mechanism to ensure this goroutine terminates. Consider passing the context to this goroutine and selecting on ctx.Done() in addition to reading from statusChan to ensure proper cleanup on shutdown.
| for status := range statusChan { | |
| b.logger.Info(). | |
| Str("status", fmt.Sprintf("%v", status)). | |
| Msg("Signal connection status changed") | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| // Context cancelled, stop monitoring to avoid goroutine leak. | |
| return | |
| case status, ok := <-statusChan: | |
| if !ok { | |
| // Channel closed, exit goroutine. | |
| return | |
| } | |
| b.logger.Info(). | |
| Str("status", fmt.Sprintf("%v", status)). | |
| Msg("Signal connection status changed") | |
| } |
| recipientUUID, err := uuid.Parse(msg.To) | ||
| if err != nil { | ||
| b.logger.Error().Err(err).Str("to", msg.To).Msg("Invalid recipient UUID") | ||
| return | ||
| } |
There was a problem hiding this comment.
The sendSignalMessage function expects msg.To to be a valid UUID string, but based on the bridge's design (line 201 in bridge.go), incoming messages use the format "UUID|+phone" for sender IDs. When PicoClaw tries to reply to a message using the sender ID as the recipient (which is common in chat applications), this will fail with "Invalid recipient UUID". Consider extracting just the UUID portion from compound identifiers before parsing, similar to how the BaseChannel.IsAllowed method handles compound IDs.
| reader := s.reader | ||
| s.mu.Unlock() | ||
|
|
||
| if reader == nil { | ||
| return nil, fmt.Errorf("no PicoClaw client connected") | ||
| } | ||
|
|
||
| line, err := reader.ReadBytes('\n') | ||
| if err != nil { | ||
| s.mu.Lock() | ||
| if s.conn != nil { | ||
| s.conn.Close() | ||
| s.conn = nil | ||
| s.reader = nil | ||
| } | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
Potential race condition in the Receive method. The reader is captured after releasing the lock (line 89-90), but the connection could be replaced or closed by acceptLoop (line 55-56) between the lock release and the actual read operation (line 96). If a new connection arrives during this window, the old reader could be reading from a closed or invalid connection. Consider holding the lock during the entire read operation, or use a connection-specific reader that is not shared.
| reader := s.reader | |
| s.mu.Unlock() | |
| if reader == nil { | |
| return nil, fmt.Errorf("no PicoClaw client connected") | |
| } | |
| line, err := reader.ReadBytes('\n') | |
| if err != nil { | |
| s.mu.Lock() | |
| if s.conn != nil { | |
| s.conn.Close() | |
| s.conn = nil | |
| s.reader = nil | |
| } | |
| s.mu.Unlock() | |
| defer s.mu.Unlock() | |
| if s.reader == nil { | |
| return nil, fmt.Errorf("no PicoClaw client connected") | |
| } | |
| line, err := s.reader.ReadBytes('\n') | |
| if err != nil { | |
| if s.conn != nil { | |
| s.conn.Close() | |
| s.conn = nil | |
| s.reader = nil | |
| } |
| c.mu.Lock() | ||
| conn := c.conn | ||
| c.mu.Unlock() | ||
|
|
||
| if conn == nil { | ||
| // Try to reconnect | ||
| if err := c.connect(); err != nil { | ||
| logger.DebugCF("signal", "Bridge reconnect failed", map[string]interface{}{ | ||
| "error": err.Error(), | ||
| "retry_in_s": delay.Seconds(), | ||
| }) | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-time.After(delay): | ||
| } | ||
|
|
||
| // Exponential backoff | ||
| delay *= 2 | ||
| if delay > signalReconnectMaxDelay { | ||
| delay = signalReconnectMaxDelay | ||
| } | ||
| continue | ||
| } | ||
| // Reset delay on successful connect | ||
| delay = signalReconnectBaseDelay | ||
| } | ||
|
|
||
| buf := make([]byte, signalReadBufferSize) | ||
| n, err := conn.Read(buf) |
There was a problem hiding this comment.
Potential race condition in the listen method. The connection is captured after releasing the lock (line 152-154), but the connection could be closed and set to nil by the Send method or Stop method between the lock release and the read operation (line 182). This could lead to reading from a nil or closed connection. Consider checking if conn is still valid after attempting to read, or restructure to ensure the connection remains valid during the read operation.
| git clone --depth 1 https://github.com/signalapp/libsignal.git "$TMPDIR/libsignal" | ||
| cd "$TMPDIR/libsignal" | ||
| cargo build --release -p libsignal-ffi |
There was a problem hiding this comment.
This build step clones and builds libsignal from the remote GitHub repository at whatever the current default branch is, which introduces a supply-chain risk: if the upstream repo or the fetch path is compromised, malicious code could be compiled and statically linked into picoclaw-signal-bridge without any change in your repo. To harden this, pin the clone to a specific immutable commit or signed release and enforce integrity verification (e.g., checksum/signature) or vendor the library so that builds are deterministic and not dependent on mutable remote state.
|
Note: PR #630 also implements Signal channel support via signal-cli. Both address the same feature. Suggesting the maintainer coordinate or pick one to avoid duplicate implementations. |
📝 Description
This PR introduces native, high-security Signal messenger support to PicoClaw.
To maintain PicoClaw's ultra-lightweight profile (
<10MB RAM) and prevent CGo runtime bloat in the main gateway, the Signal channel is implemented using a separate, independent daemon (picoclaw-signal-bridge). The bridge communicates with the main PicoClaw gateway seamlessly via a Unix Socket IPC architecture.Key Additions:
libsignalgoandsignalmeow. Keeps the main gateway 100% CGo-free and highly concurrent.+1234567890) from native UUID structures to massively simplify UX inallow_fromconfigs.make build-signaldirectly to the root Makefile for a simplified build process. Synchronized the detailed setup instructions across all 6 supported README languages (EN, VI, ZH, JA, FR, PT-BR).🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
libsignaldirectly into PicoClaw core would mandate CGo universally, inflating the binary size and increasing RAM usage significantly above the 10MB goal. The independent bridge daemon pattern isolates the crypto dependencies and maintains minimum footprint and robust privacy.🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots