Conversation
nikolasdehor
left a comment
There was a problem hiding this comment.
The WeCom integration adds useful channel support, but there are several issues that should be addressed before merging.
Missing error handling / security concerns:
-
No tests included: This PR adds ~1100 lines of Go code across
wecom.go(466 lines) andwecom_app.go(629 lines) with zero test files. At minimum, the AES decryption, signature verification, and XML parsing paths need unit tests — these are security-critical code paths. -
Config watcher (
pkg/config/watcher.go): This adds a 258-line file watcher that is unrelated to WeCom. It should be a separate PR. Mixing orthogonal features makes review harder and increases merge conflict surface. -
allow_fromfield inconsistency: The README docs useallow_from(snake_case) for WeCom but some existing channels useallowFrom(camelCase). The config struct should be consistent with the rest of the codebase. -
Missing
context.Contextpropagation: The WeCom HTTP handler should accept and pass throughcontext.Contextfor proper cancellation support, consistent with how other channels (LINE, MaixCam) handle it. -
Hardcoded timeout values: The 5-second WeCom reply deadline is mentioned in docs but I do not see a configurable timeout in the code or config struct. This should be a config option.
-
loop.gochanges: The diff shows 18 additions / 21 deletions inloop.go— these changes should be reviewed carefully to ensure they do not affect existing channel behavior. Please describe what changed in the agent loop and why it was necessary for WeCom support.
Please split the config watcher into a separate PR and add tests for the security-critical paths (AES decryption, signature verification).
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist