feat(feishu): add CLI channel and setup guidance#2185
feat(feishu): add CLI channel and setup guidance#2185wade19990814-hue wants to merge 12 commits into
Conversation
|
A quick scope note for reviewers:
So the goal here is not to land "all of Feishu", but to make the first reviewable step small, explicit, and support-friendly. |
|
This is a sizable new channel (+2620/-50, 35 files) and it currently conflicts with main (DIRTY) — several PRs landed during this review pass. Please rebase onto current |
9016d70 to
72d3fca
Compare
|
Addressed the requested changes.
I also tightened the PR description so it reflects the actual reviewable scope more clearly. |
esengine
left a comment
There was a problem hiding this comment.
Reviewed the rebased version — solid, and it mirrors the (now-hardened) Telegram channel structure. Key security checks pass:
- Fail-closed access:
decideFeishuAccessgates on owner/allowlist and ends in{ accept: false, reason: "unauthorized" }— no open-mode fallback, so an unconfigured channel rejects everyone. - Authenticated ingress: events arrive over
Lark.WSClient(outbound, app-credential-authenticated long connection via@larksuiteoapi/node-sdk) throughEventDispatcherforim.message.receive_v1— there's no inbound webhook endpoint to forge POSTs to, so no signature-verification gap (the SDK's authenticated WS connection is the trust boundary, analogous to Telegram's polling). - Dispatch is gated on
acceptRemoteInputbefore a message reaches the agent, so the tool-driving surface is contained to authorized open_ids.
Official Lark SDK is a reasonable dependency, mirrors the proven Telegram pattern, tests cover access/channel/config/first-connect/slash, CI green. Merging.
|
Correction to my approval above — I went to merge but it's still marked draft, so GitHub blocked it. The review stands (fail-closed access, authenticated WSClient ingress, gated dispatch, CI green). Whenever you're ready, mark it 'Ready for review' (un-draft) and I'll merge — or let me know if you're still iterating. |
b8dbf23 to
7a18fb5
Compare
|
Pushed a follow-up markdown rendering fix in This specifically addresses the case where Feishu was showing an What changed:
I reran:
All are passing on this branch now. |
|
Follow-up fix pushed in This addressed a real multi-channel routing bug when QQ and Feishu were both enabled in CLI:
What changed:
So with this patch, QQ + Feishu can stay connected at the same time without QQ replies being dropped after Feishu has been used in the same CLI session. |
|
@esengine I think this one is finally in a good place. At this point I’ve gone through the Feishu CLI channel end-to-end and checked the remaining issues I could reasonably think of on this branch:
So from my side, this PR is no longer just “add a Feishu transport” — it now includes the follow-through needed to make the CLI flow, setup path, rendering, and mixed-channel behavior feel coherent with the current QQ work. I’m sure there can always be more future polish, but for the current intended scope, I believe I’ve checked the main integration and UX edges I could think of. Thanks again for the review guidance — especially on the access model. This one took a few rounds, but I think it finally landed in the right shape. |
a58f39f to
0bf8143
Compare
esengine
left a comment
There was a problem hiding this comment.
Thanks for the thorough iteration on this — the core Feishu channel is well structured and the security model is exactly what was asked for. I re-verified the rebased head directly against source and the fail-closed guarantees hold:
- decideFeishuAccess (src/feishu/access.ts) rejects an empty sender and ends in the {accept:false, reason:"unauthorized"} fallback for both the configured-but-unmatched and the wholly-unconfigured cases — no open / first-sender / bindRuntime mode. tests/feishu-access.test.ts pins decideFeishuAccess({}, ...) === reject.
- start() (channel.ts) additionally throws and refuses to launch when no owner and an empty allowlist, releasing the lock.
- Ingress is the authenticated Lark.Client + Lark.WSClient + EventDispatcher for im.message.receive_v1, p2p-gated, with a bounded dedup set — no forgeable inbound webhook.
- Dispatch is gated on decideFeishuAccess in handlePrivateMessage before onSubmitMessage reaches the agent, logging only a redacted openId. Nicely mirrors the hardened Telegram channel.
Two blockers before this can merge:
-
CI is RED on both build (ubuntu) and build (windows). You correctly bumped the slash-command counts 47 -> 48 for /feishu, but the live bare-slash surface is now 49, so tests/slash.test.ts ("suggestSlashCommands filters by prefix") and tests/ui-slash-suggestions.test.tsx ("renders the bare slash release command surface as 48 total commands") both fail: "expected length 48 but got 49". Please reconcile against current main and update the counts (and the "48 commands"/title) to the true value, then re-run npm run verify so both jobs go green. (The Windows-only auto-git-rollback timeout looks flaky/unrelated — a re-run should clear it.)
-
src/skills.ts line 1 picked up a stray UTF-8 BOM (the before the leading /** comment) — looks like an editor artifact. Please strip it so the file is plain UTF-8 again.
A couple of non-blocking notes: this PR also refactors the shared QQ/Telegram/Weixin submit path (App.tsx + new remote-incoming.ts) and carries a Windows symlink-type test fix — a bit beyond "add a Feishu channel"; splitting those out would keep the surface tight, but I'm fine keeping them if you note them in the description. Also: storing the bot appSecret in the main config.json (via saveFeishuConfig) matches the existing QQ/Weixin/Telegram convention, so no change needed here, but a future move to a dedicated secret store would be worthwhile across all channels. Finally, my earlier approval is stale (it cited green CI, which no longer holds) — I'll re-approve once CI is green.
ce62b03 to
95072c5
Compare
The static top-level import * as Lark from '@larksuiteoapi/node-sdk' caused the entire module tree (bot.ts → channel.ts → chat.tsx) to crash at load time on CI runners where the SDK failed to install (Windows). This cascaded into 50 test files failing with: Error: Failed to load url @larksuiteoapi/node-sdk Switch to async import() inside start() so the SDK is only resolved when a user actually connects to Feishu. If the package is absent, a clear error message tells the user how to install it. Moves Lark.Client construction from the constructor into start() so the dynamically loaded module is available. Adds a guard in sendPrivateMessage() for calls before start().
|
Holding for a focused review pass — 2800 lines across 29 files is more than I want to land in the release window. The Feishu CLI channel design looks reasonable on a skim; I'd just rather take it after the cut so the diff doesn't compete for attention with the other release PRs. Leaving open, will revisit. |
|
Totally understand, and I completely agree that keeping the release window clean is the priority! I just wanted to gently flag that since this branch has already absorbed multiple rebases (including proactive work like unifying the remote message loops to avoid future channel divergence), keeping it aligned with Whenever you have a window after the release cut, I’d really appreciate getting this reviewed and landed. If there’s any way I can help make the review smoother or if you'd like me to break anything down, just let me know! Thanks again for all your guidance. |
Summary
/feishu connect|status|disconnectownerOpenIdor allowlist); no first-sender auto-bindingWhy this scope
Feishu support has been requested explicitly in:
This PR only takes the smallest reviewable slice of that demand: a CLI-first text channel with staged onboarding and status/teardown, without mixing in desktop work or a larger remote-control surface.
Notes
main.ownerOpenIdnor an allowlist is configured, the channel will not accept messages.feishuhelper is included so users can get setup/troubleshooting guidance in-product, similar to the recent QQ workflow improvements.Testing
npm run verify