Improve IM bot setup and diagnostics / 优化 IM 机器人设置与诊断#4070
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6814433f65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tings # Conflicts: # desktop/frontend/src/components/SettingsPanel.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6951bca81f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
esengine
left a comment
There was a problem hiding this comment.
Thanks for the thorough work here — the independent per-connection runtime, the degraded-status handling, and the fixes from the earlier round (distinct group-user remembering + serialized config persistence, both with regression tests) all look good.
Before this goes in, the P1 from the latest Codex pass is a real blocker. I traced it end-to-end and it holds:
approvalCard(...)/askCard(...)are rendered with the session'schatType+userID, so in a group session the approve/deny/answer card is posted into the group.- In
handleCardAction,firstNonEmpty(payload.Event.Action.Value["user_id"], ...)trusts the requester id baked into the card before the real click operator. So any group member who can see the card — including one not in the user allowlist — can click a button and the inbound is attributed to the original requester. checkAllowlistthen gates on thatmsg.UserID, passes, and/approve <id>resolves against the group session's pending approvals.
Net effect: in a group with tool_approval_mode = ask and a strict allowlist, an unauthorized member can approve a pending tool call as the requester. That defeats the approval gate, which is the entire point of the gate. The two card-action tests currently bless this (msg.UserID == "allowed-user" taken from the card value), so they'd need to flip with the fix.
Fix: gate the allowlist on the actual operator (Operator.OperatorID.* / Operator.*) and treat value["user_id"] as routing-only, rejecting when the operator isn't allowed. TestHandleCardActionAcceptsDirectOperatorID already shows the operator id is present on the event, so this is a small change.
Also still open from the same pass — the P2 on internal/botruntime/runtime.go: --channels lark and --channels feishu both collapse to PlatformFeishu, and AdapterBindings filters by provider only, so with both a Feishu and a Lark connection enabled the selector starts both (wrong-tenant credentials can come up). Lower priority than the P1, but worth folding in since the per-connection Domain / runtime id is already threaded through.
Happy to merge as soon as the operator-auth fix lands.
Group approval/ask cards baked the requester id into value["user_id"] and trusted it ahead of the real button presser, so any group member who could see the card could click approve/deny/answer and have it attributed to the allowlisted requester, defeating the approval gate. Carry the actual operator id on InboundMessage and gate the allowlist on it, keeping value["user_id"] as routing-only so the action still resolves against the requester session.
--channels lark and --channels feishu both collapse to PlatformFeishu, and AdapterBindings filtered by provider only, so a workspace with both a Feishu and a Lark connection enabled started both adapters and could bring up the wrong-tenant credentials. Thread the explicitly requested feishu-family domains through to the binding builder and skip connections whose domain was not requested; nil (no channel given) keeps starting every enabled connection.
|
I pushed the two fixes on top of your branch so we can land this: P1 — operator auth on Feishu cards ( P2 — Feishu/Lark domain isolation (
|
esengine
left a comment
There was a problem hiding this comment.
Operator-auth fix and the Feishu/Lark domain isolation are in, and I synced main-v2 to clear the conflicts. Full CI is green on the merge head — cross-OS tests, race, desktop build, lint, govulncheck, and CodeQL all pass. The frontend merge keeps the prod bot exposure (dropping the dev-only isDevBuild gate is the intended behavior here) while preserving agentRunning and the allowlist deep-link. Thanks for the thorough runtime work — merging.
Summary
Verification
wails generate modulefrom the desktop modulepnpm --dir desktop/frontend buildgo test ./internal/bot ./internal/bot/feishu ./internal/bot/weixin ./internal/botruntime ./internal/cligo test ./internal/bot/weixin ./internal/botruntime ./internal/cli ./internal/bot/...go test ./internal/cli ./internal/botruntime ./internal/configgo test ./...go test .from the desktop modulegit diff --checkPROD_TEST_OPEN_DIST=0 ./prod_test darwin/arm64