feat: Add Discord channel sidecar #3#9
Conversation
willamhou
left a comment
There was a problem hiding this comment.
Thanks for the Discord sidecar implementation @cugblbs! The overall structure follows the existing Slack/Webhook sidecar patterns well, and the test coverage is solid. However, there are a few issues that need to be addressed:
Must Fix
-
Unrelated socket path change should be a separate PR — This PR changes the SDK's
defaultSocketPathinsdk/channel/options.gofrom/var/run/claw/ipc.sockto/var/run/claw/bus.sock. While the change itself is arguably correct (aligning with the IPC Bus's actual socket path), it's unrelated to the Discord feature and could break existing channel sidecar deployments that rely on the current SDK default.Please split this into a separate fix PR. The Discord sidecar can set
IPC_SOCKET_PATHexplicitly via environment variable injection (which the controller already does inclaw_ipcbus.go:31), so it doesn't need to change the SDK default. -
CRD enum validation — Does this PR add
"discord"to theChannelTypeenum inapi/v1alpha1/channel_types.go? If not, the CRD admission webhook will reject anyClawChannelwithtype: discord. Please verify and add:ChannelTypeDiscord ChannelType = "discord"
Then run
make manifeststo regenerate the CRD YAML. -
builtinChannelImage()change — The current implementation uses a genericfmt.Sprintfpattern that automatically handles any channel type string:func builtinChannelImage(channelType clawv1alpha1.ChannelType) string { return fmt.Sprintf("ghcr.io/prismer-ai/claw-channel-%s:latest", string(channelType)) }
This already produces
ghcr.io/prismer-ai/claw-channel-discord:latestfor Discord. If you changed this to explicit switch/case, please ensure there's still a default fallback for custom channel types, or revert this change since it's unnecessary.
Should Fix
-
New dependency audit — The PR adds
github.com/bwmarrin/discordgo v0.28.1which pulls ingorilla/websocketandgolang.org/x/crypto. Please confirm:- Run
go mod tidyto clean up - Check for known vulnerabilities:
govulncheck ./...
- Run
-
Bot token security — The
DISCORD_TOKENenvironment variable contains a sensitive bot token. Make sure the sidecar container's env var is injected via the channel'sconfigfield (secretRef) rather than plaintext in the ClawChannel spec. The existing Slack sidecar pattern should be followed here.
Nit
- Dockerfile base image — Using
gcr.io/distroless/static-debian12:nonrootis good. Just make sure it matches the base images used by the other channel sidecars (Dockerfile.channel-slack,Dockerfile.channel-webhook) for consistency.
Please address items 1-3 and this will be ready for another round of review. Nice work on the test coverage!
|
@willamhou Thanks for the careful follow-up. I pushed an update and fully scoped the IPC/socket-path changes back out of the Discord PR. Addressed items:
The Discord adapter no longer depends on changing any shared SDK/controller default. The only Discord-specific socket-path handling left is local to I also added a small regression test around the SDK default socket path so this scope issue does not regress. Please take another look. |
willamhou
left a comment
There was a problem hiding this comment.
Great work — this is a high-quality PR. Architecture closely mirrors the Slack sidecar, tests are thorough (615 lines!), and the channel state tracker is a nice touch.
One issue to resolve before merge:
In sdk/channel/options_test.go, the default socket path is asserted as /var/run/claw/ipc.sock, but the rest of the codebase uses /var/run/claw/bus.sock (see cmd/ipcbus/main.go, cmd/channel-slack/main.go). Could you check if this introduces a path mismatch? If the channel SDK already defaults to ipc.sock, this test is correct but we may need a follow-up to align paths.
Minor suggestions (non-blocking):
- The custom
responseRecorder(lines 996-1027) can be replaced withhttptest.NewRecorder()from stdlib gorilla/websocketcomes in as an indirect dep from discordgo — not an issue, just noting we now have two WS libs in go.sum
Otherwise LGTM. Will approve once the socket path question is clarified.
|
Good catch. This was a real mismatch rather than just a test issue. I updated |
willamhou
left a comment
There was a problem hiding this comment.
Socket path issue resolved — both the Discord sidecar and the SDK now default to /var/run/claw/bus.sock, consistent with the IPC bus. Thanks for the quick follow-up!
LGTM, merging.
What
Add a built-in Discord channel sidecar under
cmd/channel-discord/and align channel/IPC Bus socket paths to use/var/run/claw/bus.sock.Why
Discord is a requested built-in integration, and issue #3 asks for a Discord sidecar that forwards messages between a Discord bot and the Claw runtime through the IPC Bus.
This PR also fixes a related socket-path mismatch that would prevent the new sidecar from connecting reliably in-cluster.
Fixes #3
How
cmd/channel-discord/usinggithub.com/bwmarrin/discordgoDISCORD_TOKENIPC_SOCKET_PATHwhen provided and defaulting to/var/run/claw/bus.socktext,user,thread)threadchannel"discord"/var/run/claw/bus.sockChecklist
make lintpassesmake testpassesType of Change