Add WeCom AIBot channel implementation and tests#893
Add WeCom AIBot channel implementation and tests#893reevoid wants to merge 21 commits intosipeed:mainfrom
Conversation
- Introduced WeCom AIBot channel configuration in config.go with relevant fields. - Implemented WeCom AIBot channel factory registration in init.go. - Created unit tests for WeCom AIBot channel functionalities including initialization, start/stop behavior, webhook path handling, message encryption/decryption, and signature generation. - Set default values for WeCom AIBot configuration in defaults.go.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new WeCom AI Bot (智能机器人) channel implementation that uses the official WeCom AI Bot API, which differs from the existing wecom channel by using a polling-based streaming protocol for responses rather than simple passive replies.
Changes:
- Adds
WeComAIBotConfigstruct andWeComAIBotfield inChannelsConfig, with default values defined - Implements the WeCom AI Bot channel in
aibot.go, featuring a streaming task queue, encryption/decryption, and a fallbackresponse_urldelivery mechanism when the stream deadline is reached - Adds unit tests, factory registration, channel manager integration, and an example config entry for the new channel
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/channels/wecom/aibot.go |
Core implementation of the WeCom AI Bot channel |
pkg/channels/wecom/aibot_test.go |
Unit tests for the new channel |
pkg/channels/wecom/init.go |
Registers the wecom_aibot factory |
pkg/channels/manager.go |
Adds initialization guard for wecom_aibot |
pkg/config/config.go |
Adds WeComAIBotConfig struct and field in ChannelsConfig |
pkg/config/defaults.go |
Sets default values for WeComAIBotConfig |
config/config.example.json |
Adds example wecom_aibot config block |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/channels/wecom/aibot.go
Outdated
| // generateSignature generates message signature using common function | ||
| func (c *WeComAIBotChannel) generateSignature(timestamp, nonce, encrypt string) string { | ||
| // Sort parameters | ||
| params := []string{c.config.Token, timestamp, nonce, encrypt} | ||
| sort.Strings(params) | ||
|
|
||
| // Concatenate | ||
| str := strings.Join(params, "") | ||
|
|
||
| // SHA1 hash | ||
| hash := sha1.Sum([]byte(str)) | ||
| return fmt.Sprintf("%x", hash) | ||
| } |
There was a problem hiding this comment.
The generateSignature method is a duplication of the logic inside verifySignature in common.go. Both sort the same parameters and compute a SHA1 hash. The generateSignature method could simply call an internal helper that both functions share, or generateSignature could be a package-level function in common.go reused here. This duplication makes maintenance harder as any future change to the signing algorithm would need to be applied in both places.
| "encoding_aes_key": "YOUR_43_CHAR_ENCODING_AES_KEY", | ||
| "webhook_host": "0.0.0.0", | ||
| "webhook_port": 18791, | ||
| "webhook_path": "/webhook/wecom-aibot", |
There was a problem hiding this comment.
The wecom_aibot entry in config.example.json is missing the allow_from and reply_timeout fields that are present in the WeComAIBotConfig struct (and in the defaults) and are included in every other WeCom channel entry (wecom and wecom_app). Omitting these from the example config makes it less discoverable for users who want to restrict which users the bot responds to.
| "webhook_path": "/webhook/wecom-aibot", | |
| "webhook_path": "/webhook/wecom-aibot", | |
| "allow_from": [], | |
| "reply_timeout": 5, |
pkg/config/config.go
Outdated
| WebhookPath string `json:"webhook_path" env:"PICOCLAW_CHANNELS_WECOM_AIBOT_WEBHOOK_PATH"` | ||
| AllowFrom FlexibleStringSlice `json:"allow_from" env:"PICOCLAW_CHANNELS_WECOM_AIBOT_ALLOW_FROM"` | ||
| ReplyTimeout int `json:"reply_timeout" env:"PICOCLAW_CHANNELS_WECOM_AIBOT_REPLY_TIMEOUT"` | ||
| MaxSteps int `json:"max_steps" env:"PICOCLAW_CHANNELS_WECOM_AIBOT_MAX_STEPS"` // Maximum streaming steps |
There was a problem hiding this comment.
The MaxSteps field in WeComAIBotConfig is defined in the config struct and set in defaults, but is never referenced in aibot.go. This means the configuration option has no effect. Either it should be used to control the number of processing steps (as suggested by the comment "Maximum streaming steps"), or it should be removed from the config to avoid confusing users.
| MaxSteps int `json:"max_steps" env:"PICOCLAW_CHANNELS_WECOM_AIBOT_MAX_STEPS"` // Maximum streaming steps |
| package wecom | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/sipeed/picoclaw/pkg/bus" | ||
| "github.com/sipeed/picoclaw/pkg/config" | ||
| ) | ||
|
|
||
| func TestNewWeComAIBotChannel(t *testing.T) { | ||
| t.Run("success with valid config", func(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| WebhookPath: "/webhook/test", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, err := NewWeComAIBotChannel(cfg, messageBus) | ||
| if err != nil { | ||
| t.Fatalf("Expected no error, got %v", err) | ||
| } | ||
|
|
||
| if ch == nil { | ||
| t.Fatal("Expected channel to be created") | ||
| } | ||
|
|
||
| if ch.Name() != "wecom_aibot" { | ||
| t.Errorf("Expected name 'wecom_aibot', got '%s'", ch.Name()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("error with missing token", func(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| _, err := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("Expected error for missing token, got nil") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("error with missing encoding key", func(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| _, err := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("Expected error for missing encoding key, got nil") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestWeComAIBotChannelStartStop(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, err := NewWeComAIBotChannel(cfg, messageBus) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create channel: %v", err) | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| // Test Start | ||
| if err := ch.Start(ctx); err != nil { | ||
| t.Fatalf("Failed to start channel: %v", err) | ||
| } | ||
|
|
||
| if !ch.IsRunning() { | ||
| t.Error("Expected channel to be running") | ||
| } | ||
|
|
||
| // Test Stop | ||
| if err := ch.Stop(ctx); err != nil { | ||
| t.Fatalf("Failed to stop channel: %v", err) | ||
| } | ||
|
|
||
| if ch.IsRunning() { | ||
| t.Error("Expected channel to be stopped") | ||
| } | ||
| } | ||
|
|
||
| func TestWeComAIBotChannelWebhookPath(t *testing.T) { | ||
| t.Run("default path", func(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, _ := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| expectedPath := "/webhook/wecom-aibot" | ||
| if ch.WebhookPath() != expectedPath { | ||
| t.Errorf("Expected webhook path '%s', got '%s'", expectedPath, ch.WebhookPath()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("custom path", func(t *testing.T) { | ||
| customPath := "/custom/webhook" | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| WebhookPath: customPath, | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, _ := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| if ch.WebhookPath() != customPath { | ||
| t.Errorf("Expected webhook path '%s', got '%s'", customPath, ch.WebhookPath()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestGenerateStreamID(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, _ := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| // Generate multiple IDs and check they are unique | ||
| ids := make(map[string]bool) | ||
| for i := 0; i < 100; i++ { | ||
| id := ch.generateStreamID() | ||
|
|
||
| if len(id) != 10 { | ||
| t.Errorf("Expected stream ID length 10, got %d", len(id)) | ||
| } | ||
|
|
||
| if ids[id] { | ||
| t.Errorf("Duplicate stream ID generated: %s", id) | ||
| } | ||
| ids[id] = true | ||
| } | ||
| } | ||
|
|
||
| func TestEncryptDecrypt(t *testing.T) { | ||
| // Use a valid 43-character base64 key (企业微信标准格式) | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFG", // 43 characters | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, _ := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| plaintext := "Hello, World!" | ||
| receiveid := "" | ||
|
|
||
| // Encrypt | ||
| encrypted, err := ch.encryptMessage(plaintext, receiveid) | ||
| if err != nil { | ||
| t.Fatalf("Failed to encrypt message: %v", err) | ||
| } | ||
|
|
||
| if encrypted == "" { | ||
| t.Fatal("Encrypted message is empty") | ||
| } | ||
|
|
||
| // Decrypt | ||
| decrypted, err := decryptMessageWithVerify(encrypted, cfg.EncodingAESKey, receiveid) | ||
| if err != nil { | ||
| t.Fatalf("Failed to decrypt message: %v", err) | ||
| } | ||
|
|
||
| if decrypted != plaintext { | ||
| t.Errorf("Expected decrypted message '%s', got '%s'", plaintext, decrypted) | ||
| } | ||
| } | ||
|
|
||
| func TestGenerateSignature(t *testing.T) { | ||
| cfg := config.WeComAIBotConfig{ | ||
| Enabled: true, | ||
| Token: "test_token", | ||
| EncodingAESKey: "testkey1234567890123456789012345678901234567", | ||
| } | ||
|
|
||
| messageBus := bus.NewMessageBus() | ||
| ch, _ := NewWeComAIBotChannel(cfg, messageBus) | ||
|
|
||
| timestamp := "1234567890" | ||
| nonce := "test_nonce" | ||
| encrypt := "encrypted_msg" | ||
|
|
||
| signature := ch.generateSignature(timestamp, nonce, encrypt) | ||
|
|
||
| if signature == "" { | ||
| t.Error("Generated signature is empty") | ||
| } | ||
|
|
||
| // Verify signature using verifySignature function | ||
| if !verifySignature(cfg.Token, signature, timestamp, nonce, encrypt) { | ||
| t.Error("Generated signature does not verify correctly") | ||
| } | ||
| } |
There was a problem hiding this comment.
The test file for aibot.go is significantly less comprehensive than the analogous bot_test.go. In bot_test.go, there are tests for HTTP handler behavior (TestWeComBotHandleVerification, TestWeComBotHandleMessageCallback, TestWeComBotProcessMessage), as well as allowlist and signature verification. The aibot_test.go is missing tests for: webhook handler endpoints (including error cases like missing parameters and invalid signatures), the Send() method's stream delivery behavior, and the getStreamResponse/handleStreamMessage polling logic. Given that this is a new, complex streaming protocol, these are important behaviors to test.
| // handleVerification handles the URL verification request from WeCom | ||
| func (c *WeComAIBotChannel) handleVerification( | ||
| ctx context.Context, | ||
| w http.ResponseWriter, | ||
| r *http.Request, | ||
| ) { | ||
| msgSignature := r.URL.Query().Get("msg_signature") | ||
| timestamp := r.URL.Query().Get("timestamp") | ||
| nonce := r.URL.Query().Get("nonce") | ||
| echostr := r.URL.Query().Get("echostr") | ||
|
|
||
| logger.DebugCF("wecom_aibot", "URL verification request", map[string]any{ | ||
| "msg_signature": msgSignature, | ||
| "timestamp": timestamp, | ||
| "nonce": nonce, | ||
| }) |
There was a problem hiding this comment.
Neither handleVerification nor handleMessageCallback validate that the required query parameters (msg_signature, timestamp, nonce) are non-empty before processing. In contrast, the existing bot.go handler returns HTTP 400 when any of these parameters are missing. This inconsistency means malformed requests will proceed to signature verification with empty values, which could lead to confusing error messages.
| } | ||
|
|
||
| // encryptEmptyResponse returns empty encrypted response | ||
| func (c *WeComAIBotChannel) encryptEmptyResponse(timestamp, nonce string) string { |
There was a problem hiding this comment.
The encryptEmptyResponse function accepts timestamp and nonce parameters but ignores them entirely, always returning an empty string. All callers pass values for these parameters but receive an empty string back, which is then written as an empty HTTP response body. This function should either use the parameters to build a properly encrypted empty response, or have its signature simplified to take no parameters since they are unused. Sending an empty body in response to a WeCom callback may cause the platform to retry or report an error.
| func (c *WeComAIBotChannel) encryptEmptyResponse(timestamp, nonce string) string { | |
| func (c *WeComAIBotChannel) encryptEmptyResponse(timestamp, nonce string) string { | |
| logger.DebugCF("wecom_aibot", "Returning empty encrypted response", map[string]any{ | |
| "timestamp": timestamp, | |
| "nonce": nonce, | |
| }) |
pkg/channels/wecom/aibot.go
Outdated
| if padding == 0 { | ||
| padding = blockSize | ||
| } |
There was a problem hiding this comment.
The pkcs7Pad function's if padding == 0 branch (line 970-972) is unreachable dead code. The expression blockSize - (len(data) % blockSize) produces values in the range [1, blockSize] — it can never be zero because len(data) % blockSize is in [0, blockSize-1]. When the result is blockSize (i.e., when the data length is a multiple of blockSize), a full block of padding is correctly added, which is already the desired behavior. The dead branch should be removed to avoid confusion.
| if padding == 0 { | |
| padding = blockSize | |
| } |
pkg/channels/wecom/aibot.go
Outdated
| // Echo back the image (simple demo behavior) | ||
| // streamID := c.generateStreamID() | ||
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) | ||
|
|
There was a problem hiding this comment.
Lines 622-625 contain commented-out code referencing a non-existent function encryptImageResponse. These stale comment lines should be removed to keep the code clean.
| // Echo back the image (simple demo behavior) | |
| // streamID := c.generateStreamID() | |
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) |
alexhoshina
left a comment
alexhoshina
left a comment
There was a problem hiding this comment.
StreamClosed tasks are only removed from streamTasks, not from chatTasks
In getStreamResponse(), when closeStreamOnly == true:
Only delete from streamTasks
delete(c.streamTasks, task.StreamID)
But the task remains in chatTasks. If the agent never replies (e.g., provider timeout, LLM error), this task will stay in chatTasks until cleanupOldTasks after 1 hour
before being cleaned up.
But more crucially: cleanupOldTasks only iterates through streamTasks for cleanup—for tasks that have StreamClosed=true and have been removed from streamTasks,
the first stage of cleanup does not touch them. Although the second stage (iterating through chatTasks) handles tasks that are Finished or have timed out, tasks with StreamClosed=true, Finished=false, and
ChatTasks created less than 1 hour ago will not be cleaned up. If a large number of requests time out in a short period, chatTasks will accumulate a significant number of dangling tasks.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/channels/wecom/aibot.go
Outdated
| sender := bus.SenderInfo{ | ||
| Platform: "wecom_aibot", | ||
| PlatformID: userID, | ||
| CanonicalID: "wecom_aibot:" + userID, |
There was a problem hiding this comment.
The CanonicalID for the sender is constructed manually as "wecom_aibot:" + userID instead of using identity.BuildCanonicalID("wecom_aibot", userID) (as done in bot.go line 407). The identity.BuildCanonicalID function lowercases and trims the platform name, which ensures consistent canonical IDs across all channels. Manual string concatenation skips this normalization and diverges from the convention used throughout the codebase.
pkg/channels/wecom/aibot.go
Outdated
| // Download and decrypt image | ||
| _, err := c.downloadAndDecryptImage(ctx, imageURL) | ||
| if err != nil { | ||
| logger.ErrorCF("wecom_aibot", "Failed to process image", map[string]any{ | ||
| "error": err, | ||
| "url": imageURL, | ||
| }) | ||
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Echo back the image (simple demo behavior) | ||
| // streamID := c.generateStreamID() | ||
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) | ||
|
|
There was a problem hiding this comment.
In handleImageMessage, when image download and decryption succeeds, the result is discarded with _ and the function still returns a "not yet supported" error message to the user. Meanwhile, the download is performed unconditionally, consuming network resources for every image message. Since image handling is not yet implemented, the download is wasteful and unnecessary. Remove the downloadAndDecryptImage call and the associated commented-out code (lines 623–625) until image support is actually implemented.
| // Download and decrypt image | |
| _, err := c.downloadAndDecryptImage(ctx, imageURL) | |
| if err != nil { | |
| logger.ErrorCF("wecom_aibot", "Failed to process image", map[string]any{ | |
| "error": err, | |
| "url": imageURL, | |
| }) | |
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | |
| MsgType: "stream", | |
| Stream: struct { | |
| ID string `json:"id"` | |
| Finish bool `json:"finish"` | |
| Content string `json:"content,omitempty"` | |
| MsgItem []struct { | |
| MsgType string `json:"msgtype"` | |
| Image *struct { | |
| Base64 string `json:"base64"` | |
| MD5 string `json:"md5"` | |
| } `json:"image,omitempty"` | |
| } `json:"msg_item,omitempty"` | |
| }{ | |
| ID: c.generateStreamID(), | |
| Finish: true, | |
| Content: fmt.Sprintf( | |
| "Image received (URL: %s), but image messages are not yet supported", | |
| imageURL, | |
| ), | |
| }, | |
| }) | |
| } | |
| // Echo back the image (simple demo behavior) | |
| // streamID := c.generateStreamID() | |
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) |
| | **DingTalk** | Medium (app credentials) | | ||
| | **LINE** | Medium (credentials + webhook URL) | | ||
| | **WeCom** | Medium (CorpID + webhook setup) | | ||
| | **WeCom AI Bot** | Medium (Token + AES key) | |
There was a problem hiding this comment.
The "WeCom" row in the channel difficulty table (line 306) has been replaced entirely with "WeCom AI Bot", removing the original WeCom Bot entry. Since WeCom Bot is still a supported channel and there is still a "WeCom Bot" setup section below in the same README, the table should include a separate row for WeCom Bot as it did before, and a new row for WeCom AI Bot should be added alongside it. The current diff creates an inconsistency between the table (which only lists WeCom AI Bot) and the Quick Setup section (which covers all three WeCom variants).
| } | ||
| req.Header.Set("Content-Type", "application/json; charset=utf-8") | ||
|
|
||
| client := &http.Client{Timeout: 15 * time.Second} |
There was a problem hiding this comment.
In sendViaResponseURL, the context.WithTimeout (line 826) and the http.Client.Timeout (line 835) are both set to 15 seconds independently. The http.Client.Timeout is redundant here because the request context already carries the 15-second deadline. Using both is not incorrect, but it creates a potential confusion — the effective timeout will be whichever expires first (in this case both are the same). Remove the Timeout field from the http.Client struct or use a longer value so the context deadline fully governs the behavior, consistent with the pattern used in other HTTP calls in the codebase.
| client := &http.Client{Timeout: 15 * time.Second} | |
| client := &http.Client{} |
| "reasoning_channel_id": "" | ||
| }, | ||
| "wecom_aibot": { | ||
| "_comment": "WeCom AI Bot (智能机器人) - Official WeCom AI Bot integration, supports proactive messaging and private chats.", |
There was a problem hiding this comment.
The comment says "supports proactive messaging and private chats" but the documentation in docs/channels/wecom/wecom_aibot/README.zh.md (feature comparison table, line 10) shows that WeCom AI Bot also supports group chats. The comment should be updated to mention group chat support.
| "_comment": "WeCom AI Bot (智能机器人) - Official WeCom AI Bot integration, supports proactive messaging and private chats.", | |
| "_comment": "WeCom AI Bot (智能机器人) - Official WeCom AI Bot integration, supports proactive messaging, private chats, and group chats.", |
pkg/channels/wecom/aibot.go
Outdated
| if padding == 0 { | ||
| padding = blockSize | ||
| } |
There was a problem hiding this comment.
In pkcs7Pad, the condition if padding == 0 { padding = blockSize } is dead code. The expression blockSize - (len(data) % blockSize) produces a value in the range [1, blockSize] because len(data) % blockSize is in the range [0, blockSize-1]; the result is therefore always at least 1 and never 0. The dead check is misleading. Remove it to clarify the intent.
| if padding == 0 { | |
| padding = blockSize | |
| } |
| **Option 3: WeCom AI Bot (AI Bot)** - Official AI Bot, streaming replies, supports group & private chat | ||
|
|
||
| See [WeCom App Configuration Guide](docs/wecom-app-configuration.md) for detailed setup instructions. | ||
| See [WeCom AI Bot Configuration Guide](docs/channels/wecom/wecom_aibot/README.zh.md) for detailed setup instructions. |
There was a problem hiding this comment.
The English README.md (and similarly README.fr.md, README.ja.md, README.pt-br.md, README.vi.md) now links the WeCom configuration guide to docs/channels/wecom/wecom_aibot/README.zh.md — a Chinese-language file. English-speaking users following the link will be directed to a Chinese guide. An English version of the documentation should be created (e.g. README.md in the same directory), or the link should point to the existing Chinese file with a note about the language.
| See [WeCom AI Bot Configuration Guide](docs/channels/wecom/wecom_aibot/README.zh.md) for detailed setup instructions. | |
| See [WeCom AI Bot Configuration Guide (Chinese)](docs/channels/wecom/wecom_aibot/README.zh.md) for detailed setup instructions (documentation currently available in Chinese only). |
| "webhook_host": "0.0.0.0", | ||
| "webhook_port": 18791, | ||
| "webhook_path": "/webhook/wecom-aibot", | ||
| "max_steps": 10, |
There was a problem hiding this comment.
The wecom_aibot example configuration is missing the allow_from and reply_timeout fields that are present in the wecom and wecom_app example configurations. While these fields have defaults, including them in the example config would help users understand that these options exist, consistent with the other WeCom channel examples.
| "max_steps": 10, | |
| "max_steps": 10, | |
| "allow_from": [], | |
| "reply_timeout": 5, |
…nce message handling limits
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/config/defaults.go
Outdated
| Token: "", | ||
| EncodingAESKey: "", | ||
| WebhookHost: "0.0.0.0", | ||
| WebhookPort: 18791, |
There was a problem hiding this comment.
The WeComAIBot channel is assigned the same default port (18791) as the LINE channel. If a user enables both channels simultaneously with default configurations, the second channel to start will fail to bind to the port.
The existing channels use distinct ports: MaixCam=18790, LINE=18791, WeComApp=18792, WeCom=18793. The WeComAIBot default port should be a new unique value (e.g., 18794) to avoid conflicts.
| WebhookPort: 18791, | |
| WebhookPort: 18794, |
config/config.example.json
Outdated
| "token": "YOUR_TOKEN", | ||
| "encoding_aes_key": "YOUR_43_CHAR_ENCODING_AES_KEY", | ||
| "webhook_host": "0.0.0.0", | ||
| "webhook_port": 18791, |
There was a problem hiding this comment.
The WeComAIBot example configuration uses port 18791, which conflicts with the default port for LINE. The example should use a unique port (e.g., 18794) to avoid misleading users into a port conflict when they also have LINE enabled.
| "webhook_port": 18791, | |
| "webhook_port": 18794, |
| // Send delivers the agent reply into the active streamTask for msg.ChatID. | ||
| // It writes into the earliest unfinished task in the queue (FIFO per chatID). |
There was a problem hiding this comment.
The Send() method is missing an IsRunning() guard. Both WeComBotChannel.Send() and WeComAppChannel.Send() check if !c.IsRunning() { return channels.ErrNotRunning } as their first statement. Without this guard, calling Send() on a stopped channel will proceed past the lock, potentially access c.ctx (which is nil before Start() is ever called) inside sendViaResponseURL, causing a nil-pointer panic. This was flagged in a previous review and acknowledged, but has not been fixed.
pkg/channels/wecom/aibot.go
Outdated
| if padding == 0 { | ||
| padding = blockSize | ||
| } |
There was a problem hiding this comment.
In pkcs7Pad, the condition if padding == 0 { padding = blockSize } is unreachable dead code. When len(data) % blockSize == 0, the expression blockSize - 0 evaluates to blockSize (not 0), so padding is already blockSize before the if check. The dead branch can be removed to keep the code clean.
| if padding == 0 { | |
| padding = blockSize | |
| } |
| if chatID == "" { | ||
| chatID = userID | ||
| } |
There was a problem hiding this comment.
There is a significant discrepancy between the documentation and the code. The documentation (docs/channels/wecom/wecom_aibot/README.zh.md lines 78-80 and README.md line 648) states the timeout occurs after "5 minutes 30 seconds" (5.5 min), but the actual deadline in handleTextMessage is only 30 seconds (time.Now().Add(30 * time.Second)). The code comment on line 483 says "WeCom stops sending stream-refresh callbacks after 6 minutes", but the deadline is set far shorter than that. Either the deadline should be increased (e.g., to ~5.5 minutes) to match the documented behavior, or the documentation should be corrected to reflect the 30-second actual limit.
| **超时处理**（任务超过 5 分 30 秒）： | ||
|
|
||
| 若 Agent 处理时间超过约 5 分 30 秒（企业微信最大轮询窗口为 6 分钟），PicoClaw 会： |
There was a problem hiding this comment.
The documentation in README.zh.md line 80 states the timeout occurs when "Agent 处理时间超过约 5 分 30 秒（企业微信最大轮询窗口为 6 分钟）", and the English README says "Long tasks (>5.5 min) automatically switch to response_url push delivery." However, the actual code in aibot.go sets a 30-second deadline, not 5.5 minutes. The documentation misleads users into expecting a much longer wait before fallback.
README.md
Outdated
| picoclaw gateway | ||
| ``` | ||
|
|
||
| > **Note**: WeCom AI Bot uses streaming pull protocol — no reply timeout concerns. Long tasks (>5.5 min) automatically switch to `response_url` push delivery. |
There was a problem hiding this comment.
The note states "Long tasks (>5.5 min) automatically switch to response_url push delivery", but the actual stream deadline in aibot.go is 30 seconds. The documentation is incorrect — tasks switch to response_url mode after 30 seconds, not 5.5 minutes.
… agent cancellation
… and improved cleanup logic
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" | ||
| b := make([]byte, 10) | ||
| for i := range b { | ||
| n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) |
There was a problem hiding this comment.
In generateStreamID(), the error from rand.Int is silently discarded with _. If the cryptographic random number generator fails, n will be nil and the subsequent n.Int64() call will panic with a nil pointer dereference. While crypto/rand failures are rare, they can happen in low-entropy environments. The error should be handled, similar to how encryptMessage() handles the same error at line 952.
| n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) | |
| n, err := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) | |
| if err != nil { | |
| // Log the error and fall back to a deterministic character to avoid panic | |
| logger.Errorf("failed to generate random stream ID character: %v", err) | |
| b[i] = letters[0] | |
| continue | |
| } |
pkg/channels/wecom/aibot.go
Outdated
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, |
There was a problem hiding this comment.
The image URL is embedded directly in the response message sent back to the user (at both lines 639-642 and 668-671). WeCom image URLs typically contain access tokens or session-specific credentials. Exposing the full URL to the end user via the chat message could leak authentication material. The response message should not include the URL itself — for example, just respond with "Image received, but image messages are not yet supported."
| // 1. Absolute expiry: task was created more than taskMaxLifetime ago. | ||
| // 2. Grace expiry: stream closed more than streamClosedGracePeriod ago | ||
| // (agent had enough time to reply; it is not coming back). | ||
| for chatID, queue := range c.chatTasks { | ||
| filtered := queue[:0] | ||
| for _, t := range queue { | ||
| absoluteExpired := t.CreatedTime.Before(cutoff) | ||
| graceExpired := t.StreamClosed && | ||
| !t.StreamClosedAt.IsZero() && | ||
| t.StreamClosedAt.Before(now.Add(-streamClosedGracePeriod)) | ||
| if !t.Finished && !absoluteExpired && !graceExpired { | ||
| filtered = append(filtered, t) |
There was a problem hiding this comment.
There are multiple data races between c.taskMu and task.mu protecting the same streamTask fields:
-
task.Finished: Written byremoveTask()undertask.mu(line 819-821), but read inSend()at line 192 underc.taskMuonly, and read incleanupOldTasks()at line 1171 underc.taskMuonly. -
task.StreamClosed/task.StreamClosedAt: Written bygetStreamResponse()undertask.mu(lines 783-786), but read incleanupOldTasks()at lines 1168-1170 underc.taskMuonly.
In all these cases, different goroutines access these fields under different locks, which means the accesses are not mutually exclusive. The fix is to consistently protect these fields under a single lock — either always use c.taskMu when reading/writing them, or always acquire task.mu in the cleanup and Send() paths.
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: "Unsupported message type: " + msg.MsgType, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The WeComAIBotStreamInfo named type was introduced (following feedback that was acknowledged as correct), and WeComAIBotStreamResponse.Stream now uses it. However, all call sites (at least 7 locations: lines 450, 579, 625, 654, 685, 723, 795) still construct the Stream field using anonymous struct literals instead of the named WeComAIBotStreamInfo type. This defeats the purpose of the refactor. All those sites should use WeComAIBotStreamInfo{ID: ..., Finish: ..., Content: ...} instead.
pkg/channels/wecom/aibot.go
Outdated
| // Download and decrypt image | ||
| _, err := c.downloadAndDecryptImage(ctx, imageURL) | ||
| if err != nil { | ||
| logger.ErrorCF("wecom_aibot", "Failed to process image", map[string]any{ | ||
| "error": err, | ||
| "url": imageURL, | ||
| }) | ||
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Echo back the image (simple demo behavior) | ||
| // streamID := c.generateStreamID() | ||
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) | ||
|
|
||
| // For now, just acknowledge receipt without echoing the image | ||
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Whether downloadAndDecryptImage succeeds or fails, both branches at lines 623-644 and 652-673 return the identical "Image received (URL: ...), but image messages are not yet supported" response. This means the function is called, it may download and attempt to decrypt (consuming network and CPU), but the result is completely discarded regardless. This makes downloadAndDecryptImage a dead code path in this function — either remove it or add real handling for the success case.
… streamTask comments
|
@alexhoshina I have addressed all the issues mentioned above. Please review again to see if it can be merged now. |
|
pkg/channels/wecom/aibot.go
Outdated
| imageURL := msg.Image.URL | ||
|
|
||
| // Download and decrypt image | ||
| _, err := c.downloadAndDecryptImage(ctx, imageURL) |
There was a problem hiding this comment.
After downloading the image fails, an error response is returned, but if it succeeds, the code also returns the exact same "not yet supported" message. The success path does not use the downloaded data—the decrypted imageData
is discarded (_). Moreover, even if it fails, the subsequent code will still execute (due to a missing return), resulting in sending two responses.
| for len(queue) > 0 && queue[0].Finished { | ||
| queue = queue[1:] | ||
| } |
There was a problem hiding this comment.
Only clean up the Finished tasks at the head of the queue during Send() calls; if there are finished tasks in the middle, do not clean them up. Long-term operation may cause the chatTasks queue to expand.
…update related tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: "Unsupported message type: " + msg.MsgType, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The WeComAIBotStreamInfo named type was introduced at line 105–116 precisely to avoid repeating the anonymous struct type at every call site (as flagged in prior review). However, none of the 7 call sites (lines 457, 586, 632, 661, 692, 730, 800) have been updated to use WeComAIBotStreamInfo{}. Each still repeats the full anonymous struct literal including all JSON tags. For example, the getStreamResponse function at line 800 still constructs the struct anonymously instead of using WeComAIBotStreamInfo{ID: ..., Finish: ..., Content: ...}. The type was defined but never actually applied, leaving the same maintainability problem the prior feedback was addressing.
| imageURL := msg.Image.URL | ||
|
|
||
| // Download and decrypt image | ||
| _, err := c.downloadAndDecryptImage(ctx, imageURL) | ||
| if err != nil { | ||
| logger.ErrorCF("wecom_aibot", "Failed to process image", map[string]any{ | ||
| "error": err, | ||
| "url": imageURL, | ||
| }) | ||
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Echo back the image (simple demo behavior) | ||
| // streamID := c.generateStreamID() | ||
| // return c.encryptImageResponse(streamID, timestamp, nonce, imageData) | ||
|
|
||
| // For now, just acknowledge receipt without echoing the image | ||
| return c.encryptResponse("", timestamp, nonce, WeComAIBotStreamResponse{ | ||
| MsgType: "stream", | ||
| Stream: struct { | ||
| ID string `json:"id"` | ||
| Finish bool `json:"finish"` | ||
| Content string `json:"content,omitempty"` | ||
| MsgItem []struct { | ||
| MsgType string `json:"msgtype"` | ||
| Image *struct { | ||
| Base64 string `json:"base64"` | ||
| MD5 string `json:"md5"` | ||
| } `json:"image,omitempty"` | ||
| } `json:"msg_item,omitempty"` | ||
| }{ | ||
| ID: c.generateStreamID(), | ||
| Finish: true, | ||
| Content: fmt.Sprintf( | ||
| "Image received (URL: %s), but image messages are not yet supported", | ||
| imageURL, | ||
| ), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
The handleImageMessage function (line 609) calls downloadAndDecryptImage to download and decrypt the image even when the result is always discarded with _. Whether the download succeeds or fails, the function returns the same "Image received (URL: ...), but image messages are not yet supported" response in both code paths (lines 626–651 and 659–680). The download and decrypt step is dead work — it consumes network bandwidth and CPU on every image message but the result is never used. The call should be removed until image support is actually implemented. The commented-out code at lines 654–656 also suggests this was known as incomplete.
…k management logic
…omAIBotMsgItem and WeComAIBotMsgItemImage types
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // encryptAESCBC encrypts plaintext using AES-CBC with the given key, mirroring | ||
| // decryptAESCBC. IV = aesKey[:aes.BlockSize]. The caller must PKCS7-pad the | ||
| // plaintext to a multiple of aes.BlockSize before calling. | ||
| func encryptAESCBC(aesKey, plaintext []byte) ([]byte, error) { | ||
| block, err := aes.NewCipher(aesKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create cipher: %w", err) | ||
| } | ||
| iv := aesKey[:aes.BlockSize] | ||
| ciphertext := make([]byte, len(plaintext)) | ||
| cipher.NewCBCEncrypter(block, iv).CryptBlocks(ciphertext, plaintext) | ||
| return ciphertext, nil | ||
| } |
There was a problem hiding this comment.
The pkcs7Pad function in common.go uses a blockSize parameter but the function argument shadows the package-level blockSize constant. Since encryptMessage in aibot.go always calls pkcs7Pad(frame, blockSize) passing the package-level constant (32), this will always pad to 32-byte blocks, which is correct. However, the AES block cipher expects input that's a multiple of 16 (AES block size), not 32. The encryptAESCBC function doesn't validate that its input is padded to a multiple of aes.BlockSize (16), whereas decryptAESCBC validates len(ciphertext) % aes.BlockSize != 0. Since blockSize (32) is a multiple of aes.BlockSize (16), this works correctly in practice. But there is a subtle mismatch: the caller pads to 32-byte multiples and the encrypt function expects AES block (16-byte) multiples. The WeCom spec requires 32-byte block padding, so this is technically correct, but adding the same validation to encryptAESCBC (or a note in the docstring) would make the expectation explicit.
| // downloadAndDecryptImage downloads and decrypts an encrypted image | ||
| func (c *WeComAIBotChannel) downloadAndDecryptImage( | ||
| ctx context.Context, | ||
| imageURL string, | ||
| ) ([]byte, error) { | ||
| // Download image | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, imageURL, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| client := &http.Client{ | ||
| Timeout: 15 * time.Second, | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to download image: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("download failed with status: %d", resp.StatusCode) | ||
| } | ||
|
|
||
| // Limit image download to 20 MB to prevent memory exhaustion | ||
| const maxImageSize = 20 << 20 // 20 MB | ||
| encryptedData, err := io.ReadAll(io.LimitReader(resp.Body, maxImageSize+1)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read image data: %w", err) | ||
| } | ||
| if len(encryptedData) > maxImageSize { | ||
| return nil, fmt.Errorf("image too large (exceeds %d MB)", maxImageSize>>20) | ||
| } | ||
|
|
||
| logger.DebugCF("wecom_aibot", "Image downloaded", map[string]any{ | ||
| "size": len(encryptedData), | ||
| }) | ||
|
|
||
| // Decode AES key | ||
| aesKey, err := decodeWeComAESKey(c.config.EncodingAESKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Decrypt image (AES-CBC with IV = first 16 bytes of key, PKCS7 padding stripped) | ||
| decryptedData, err := decryptAESCBC(aesKey, encryptedData) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt image: %w", err) | ||
| } | ||
|
|
||
| logger.DebugCF("wecom_aibot", "Image decrypted", map[string]any{ | ||
| "size": len(decryptedData), | ||
| }) | ||
|
|
||
| return decryptedData, nil | ||
| } |
There was a problem hiding this comment.
The downloadAndDecryptImage method is defined but never called anywhere. The handleImageMessage function currently just sends a "not yet supported" response without invoking this method. Since the image handling is marked as "not yet fully implemented" in the code, this function is dead code that will cause a linter warning (e.g., from staticcheck). Either remove it for now and add it back when image support is actually implemented, or add a // TODO: comment explaining when it will be used to suppress the warning.
| msgSignature := r.URL.Query().Get("msg_signature") | ||
| timestamp := r.URL.Query().Get("timestamp") | ||
| nonce := r.URL.Query().Get("nonce") | ||
| echostr := r.URL.Query().Get("echostr") | ||
|
|
||
| logger.DebugCF("wecom_aibot", "URL verification request", map[string]any{ | ||
| "msg_signature": msgSignature, | ||
| "timestamp": timestamp, | ||
| "nonce": nonce, | ||
| }) | ||
|
|
||
| // Verify signature | ||
| if !verifySignature(c.config.Token, msgSignature, timestamp, nonce, echostr) { |
There was a problem hiding this comment.
The handleVerification method does not validate that required URL query parameters (msg_signature, timestamp, nonce, echostr) are non-empty before proceeding. Both bot.go (line 199-201) and app.go (line 458-462) check for missing parameters first and return HTTP 400 Bad Request. Without this check, a request with all empty query params would pass verifySignature only if token is also empty (which can't happen since NewWeComAIBotChannel enforces a non-empty token), but the error returned would be a misleading "Signature verification failed" (HTTP 401) rather than "Missing parameters" (HTTP 400). More importantly, decryptMessageWithVerify would then be called with an empty echostr, resulting in an internal server error instead of a clear bad request response.
| "wecom_aibot": { | ||
| "_comment": "WeCom AI Bot (智能机器人) - Official WeCom AI Bot integration, supports proactive messaging and private chats.", | ||
| "enabled": false, | ||
| "token": "YOUR_TOKEN", | ||
| "encoding_aes_key": "YOUR_43_CHAR_ENCODING_AES_KEY", | ||
| "webhook_host": "0.0.0.0", | ||
| "webhook_port": 18791, | ||
| "webhook_path": "/webhook/wecom-aibot", | ||
| "max_steps": 10, | ||
| "welcome_message": "Hello! I'm your AI assistant. How can I help you today?", | ||
| "reasoning_channel_id": "" | ||
| } |
There was a problem hiding this comment.
The wecom_aibot section in config.example.json is missing the allow_from and reply_timeout fields. Every other channel in this file includes allow_from, and the other WeCom channels (wecom and wecom_app) both include reply_timeout. This inconsistency makes it less obvious to users that these configuration options exist for wecom_aibot. The fields are supported by the config struct (WeComAIBotConfig) and the channel implementation.
…calID for consistency
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | **DingTalk** | Médio (credenciais do app) | | ||
| | **LINE** | Médio (credenciais + webhook URL) | | ||
| | **WeCom** | Médio (CorpID + configuração webhook) | | ||
| | **WeCom AI Bot** | Médio (Token + chave AES) | |
There was a problem hiding this comment.
The WeCom table row was replaced with "WeCom AI Bot" (same as README.md), removing coverage of WeCom Bot and WeCom App from the overview table. The "See [WeCom AI Bot Configuration Guide]" link replaces the former WeCom App guide link, leaving no dedicated link for WeCom App or WeCom Bot detailed docs.
| | **DingTalk** | Moyen (identifiants de l'application) | | ||
| | **LINE** | Moyen (identifiants + URL de webhook) | | ||
| | **WeCom** | Moyen (CorpID + configuration webhook) | | ||
| | **WeCom AI Bot** | Moyen (Token + clé AES) | |
There was a problem hiding this comment.
The WeCom table row was replaced with "WeCom AI Bot", removing coverage of WeCom Bot and WeCom App from the overview table. The "See [WeCom AI Bot Configuration Guide]" link replaces the former WeCom App guide link, leaving no dedicated link for WeCom App or WeCom Bot detailed docs.
| "wecom_aibot": { | ||
| "_comment": "WeCom AI Bot (智能机器人) - Official WeCom AI Bot integration, supports proactive messaging and private chats.", | ||
| "enabled": false, | ||
| "token": "YOUR_TOKEN", | ||
| "encoding_aes_key": "YOUR_43_CHAR_ENCODING_AES_KEY", | ||
| "webhook_path": "/webhook/wecom-aibot", | ||
| "max_steps": 10, | ||
| "welcome_message": "Hello! I'm your AI assistant. How can I help you today?", | ||
| "reasoning_channel_id": "" | ||
| } | ||
| }, |
There was a problem hiding this comment.
The wecom_aibot entry in config.example.json omits allow_from and reply_timeout fields that are present in both the wecom and wecom_app example entries. For consistency and to make the example easier to use as a template, these fields should be included in the example, just as they are in the wecom and wecom_app blocks.
| streamID := c.generateStreamID() | ||
|
|
||
| // WeCom stops sending stream-refresh callbacks after 6 minutes. | ||
| // Set a slightly shorter deadline so we can send a timeout notice before it gives up. | ||
| deadline := time.Now().Add(30 * time.Second) | ||
|
|
||
| // Each task gets its own context derived from the channel lifetime context. | ||
| // Canceling taskCancel interrupts the agent goroutine when the task is removed. | ||
| taskCtx, taskCancel := context.WithCancel(c.ctx) | ||
|
|
||
| task := &streamTask{ | ||
| StreamID: streamID, | ||
| ChatID: chatID, | ||
| ResponseURL: msg.ResponseURL, | ||
| Question: content, | ||
| CreatedTime: time.Now(), | ||
| Deadline: deadline, | ||
| Finished: false, | ||
| answerCh: make(chan string, 1), | ||
| ctx: taskCtx, | ||
| cancel: taskCancel, | ||
| } | ||
|
|
||
| c.taskMu.Lock() | ||
| c.streamTasks[streamID] = task | ||
| c.chatTasks[chatID] = append(c.chatTasks[chatID], task) | ||
| c.taskMu.Unlock() | ||
|
|
||
| // Publish to agent asynchronously; agent will call Send() with reply. | ||
| // Use task.ctx (not c.ctx) so the agent goroutine is canceled when the task is removed. | ||
| go func() { | ||
| sender := bus.SenderInfo{ | ||
| Platform: "wecom_aibot", | ||
| PlatformID: userID, | ||
| CanonicalID: identity.BuildCanonicalID("wecom_aibot", userID), | ||
| DisplayName: userID, | ||
| } | ||
| peerKind := "direct" | ||
| if msg.ChatType == "group" { | ||
| peerKind = "group" | ||
| } | ||
| peer := bus.Peer{Kind: peerKind, ID: chatID} | ||
| metadata := map[string]string{ | ||
| "channel": "wecom_aibot", | ||
| "chat_type": msg.ChatType, | ||
| "msg_type": "text", | ||
| "msgid": msg.MsgID, | ||
| "aibotid": msg.AIBotID, | ||
| "stream_id": streamID, | ||
| "response_url": msg.ResponseURL, | ||
| } | ||
| c.HandleMessage(task.ctx, peer, msg.MsgID, userID, chatID, | ||
| content, nil, metadata, sender) | ||
| }() | ||
|
|
||
| // Return first streaming response immediately (finish=false, content empty) | ||
| return c.getStreamResponse(task, timestamp, nonce) |
There was a problem hiding this comment.
In handleTextMessage, a streamTask is created and added to the task maps before checking whether the sender is allowed. The allow-list check happens indirectly inside the goroutine via HandleMessage. If the sender is not allowed, HandleMessage returns early without ever sending a reply, so task.answerCh is never written. This means the task will sit in the map responding with finish=false until its 30-second deadline, at which point it transitions to response_url mode and remains in chatTasks for an additional 10 minutes (streamClosedGracePeriod) before cleanup. You should call IsAllowedSender before creating the stream task and return an appropriate encrypted response immediately if the sender is blocked, just like bot.go does at line 410.
| | **DingTalk** | Trung bình (app credentials) | | ||
| | **LINE** | Trung bình (credentials + webhook URL) | | ||
| | **WeCom** | Trung bình (CorpID + cấu hình webhook) | | ||
| | **WeCom AI Bot** | Trung bình (Token + khóa AES) | |
There was a problem hiding this comment.
The WeCom table row was replaced with "WeCom AI Bot" (same as README.md), removing coverage of WeCom Bot and WeCom App from the overview table. Additionally the "See [WeCom AI Bot Configuration Guide]" link replaces the former WeCom App guide link, leaving no dedicated link for WeCom App or WeCom Bot detailed docs.
|
|
||
| // HealthPath returns the health check endpoint path | ||
| func (c *WeComAIBotChannel) HealthPath() string { | ||
| return c.WebhookPath() + "/health" |
There was a problem hiding this comment.
The HealthPath() method returns c.WebhookPath() + "/health" (e.g., /webhook/wecom-aibot/health), which deviates from the established pattern in the same package: bot.go returns /health/wecom and app.go returns /health/wecom-app. Both use fixed, separate paths instead of composing them from the webhook path. Consider adopting the same convention (e.g., returning /health/wecom-aibot) for consistency.
| return c.WebhookPath() + "/health" | |
| return "/health/wecom-aibot" |
| for i := range b { | ||
| n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) |
There was a problem hiding this comment.
In generateStreamID, the error from rand.Int is silently ignored (assigned to _). If the call fails, n is nil and n.Int64() will panic. Compare this to packWeComFrame in common.go (line 109-113), which properly handles the same rand.Int error. Consider handling the error or using a simpler approach like crypto/rand.Read directly.
| for i := range b { | |
| n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) | |
| max := big.NewInt(int64(len(letters))) | |
| for i := range b { | |
| n, err := rand.Int(rand.Reader, max) | |
| if err != nil { | |
| logger.ErrorCF("wecom_aibot", "Failed to generate random stream ID", map[string]any{ | |
| "error": err, | |
| }) | |
| return "" | |
| } |
| | **DingTalk** | 普通（アプリ認証情報） | | ||
| | **LINE** | 普通（認証情報 + Webhook URL） | | ||
| | **WeCom** | 普通（CorpID + Webhook設定） | | ||
| | **WeCom AI Bot** | 普通（Token + AES キー） | |
There was a problem hiding this comment.
The WeCom table row was replaced with "WeCom AI Bot", removing coverage of WeCom Bot and WeCom App from the overview table. The "See [WeCom AI Bot Configuration Guide]" link replaces the former WeCom App guide link, leaving no dedicated link for WeCom App or WeCom Bot detailed docs.
| // downloadAndDecryptImage downloads and decrypts an encrypted image | ||
| func (c *WeComAIBotChannel) downloadAndDecryptImage( | ||
| ctx context.Context, | ||
| imageURL string, | ||
| ) ([]byte, error) { | ||
| // Download image | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, imageURL, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| client := &http.Client{ | ||
| Timeout: 15 * time.Second, | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to download image: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("download failed with status: %d", resp.StatusCode) | ||
| } | ||
|
|
||
| // Limit image download to 20 MB to prevent memory exhaustion | ||
| const maxImageSize = 20 << 20 // 20 MB | ||
| encryptedData, err := io.ReadAll(io.LimitReader(resp.Body, maxImageSize+1)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read image data: %w", err) | ||
| } | ||
| if len(encryptedData) > maxImageSize { | ||
| return nil, fmt.Errorf("image too large (exceeds %d MB)", maxImageSize>>20) | ||
| } | ||
|
|
||
| logger.DebugCF("wecom_aibot", "Image downloaded", map[string]any{ | ||
| "size": len(encryptedData), | ||
| }) | ||
|
|
||
| // Decode AES key | ||
| aesKey, err := decodeWeComAESKey(c.config.EncodingAESKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Decrypt image (AES-CBC with IV = first 16 bytes of key, PKCS7 padding stripped) | ||
| decryptedData, err := decryptAESCBC(aesKey, encryptedData) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt image: %w", err) | ||
| } | ||
|
|
||
| logger.DebugCF("wecom_aibot", "Image decrypted", map[string]any{ | ||
| "size": len(decryptedData), | ||
| }) | ||
|
|
||
| return decryptedData, nil | ||
| } |
There was a problem hiding this comment.
The downloadAndDecryptImage method (and the related WeComAIBotMsgItem / WeComAIBotMsgItemImage types) are never called or used anywhere in the codebase. handleImageMessage acknowledges image messages but does not decrypt or forward the image. This dead code increases maintenance burden and could confuse reviewers. Either wire the function into handleImageMessage to forward image content to the agent, or remove the function and the associated types until image support is fully implemented.
| if padding == 0 { | ||
| padding = blockSize | ||
| } |
There was a problem hiding this comment.
In pkcs7Pad, the expression blockSize - (len(data) % blockSize) evaluates to a value in the range [1, blockSize] — it is never 0. The guard if padding == 0 { padding = blockSize } is therefore unreachable dead code. Remove it to avoid confusion about the function's invariants.
| if padding == 0 { | |
| padding = blockSize | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| req.Header.Set("Content-Type", "application/json; charset=utf-8") | ||
|
|
||
| client := &http.Client{Timeout: 15 * time.Second} |
There was a problem hiding this comment.
In sendViaResponseURL, both a context timeout (context.WithTimeout(c.ctx, 15*time.Second)) and an http.Client{Timeout: 15*time.Second} are set to the same duration. Since the request is already bounded by the context timeout derived from c.ctx, the client.Timeout is redundant. The effective timeout is whichever fires first (they're both 15s, so it doesn't change behavior), but having two independent timeout mechanisms with identical values is confusing and unnecessary. The http.Client.Timeout should be removed, relying solely on the context for cancellation, which is the idiomatic Go pattern when using http.NewRequestWithContext.
| client := &http.Client{Timeout: 15 * time.Second} | |
| client := &http.Client{} |
| if padding == 0 { | ||
| padding = blockSize | ||
| } |
There was a problem hiding this comment.
The pkcs7Pad function contains unreachable code. The expression blockSize - (len(data) % blockSize) always yields a value in the range [1, blockSize] (since len(data) % blockSize is in [0, blockSize-1]), so padding can never be 0. The if padding == 0 branch (lines 172-174) is dead code and should be removed to avoid misleading readers into thinking a zero-padding case is possible.
| if padding == 0 { | |
| padding = blockSize | |
| } |
| picoclaw gateway | ||
| ``` | ||
|
|
||
| > **Lưu ý**: WeCom AI Bot sử dụng giao thức pull streaming — không lo timeout phản hồi. Tác vụ dài (>5,5 phút) tự động chuyển sang gửi qua `response_url`. |
There was a problem hiding this comment.
The timeout threshold is inconsistently documented across the localized READMEs. The code at aibot.go:510 uses a 30-second deadline (deadline := time.Now().Add(30 * time.Second)), and the English README correctly states ">30 seconds". However, the Vietnamese (README.vi.md), French (README.fr.md), and Portuguese (README.pt-br.md) READMEs all incorrectly state ">5.5 minutes" as the threshold. These should be updated to say ">30 seconds" to match the actual code behavior.
| picoclaw gateway | ||
| ``` | ||
|
|
||
| > **Note** : WeCom AI Bot utilise le protocole pull en streaming — pas de problème de timeout. Les tâches longues (>5,5 min) basculent automatiquement vers la livraison via `response_url`. |
There was a problem hiding this comment.
The timeout threshold stated here (">5,5 min") is incorrect. The actual code deadline is 30 seconds (aibot.go:510), and the English README.md correctly states ">30 seconds". This should be updated to ">30 secondes" to match the actual behavior.
| picoclaw gateway | ||
| ``` | ||
|
|
||
| > **Nota**: O WeCom AI Bot usa protocolo de pull em streaming — sem preocupações com timeout de resposta. Tarefas longas (>5,5 min) alternam automaticamente para entrega via `response_url`. |
There was a problem hiding this comment.
The timeout threshold stated here (">5,5 min") is incorrect. The actual code deadline is 30 seconds (aibot.go:510), and the English README.md correctly states ">30 seconds". This should be updated to ">30 segundos" to match the actual behavior.
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
📚 Technical Context (Skip for Docs)
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist