feat(events): register mail message_received in unified pipeline#851
feat(events): register mail message_received in unified pipeline#851dingding0418 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a mail event key (mail.user_mailbox.event.message_received_v1) with an optional mailbox parameter and pre-consume subscribe/unsubscribe lifecycle, wires it into the global event registry, and updates skill docs and examples to document unified event consumption for mail. ChangesMail Event Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add events/mail/ following the IM pattern: the mail new-message-received
EventKey is now consumable via the unified event command, with PreConsume
opening the per-user mailbox subscription on startup and unsubscribing
on graceful shutdown.
Agents can now use a single contract for IM and mail:
lark-cli event consume mail.user_mailbox.event.message_received_v1 --as user
The existing `mail +watch` shortcut is unchanged for back-compat; its
SKILL reference now points to the unified entry as preferred for new
flows that already use the lark event contract (--max-events / --timeout
/ stderr ready-marker).
Tests cover registration invariants, scope/auth metadata, mailbox param
defaulting, subscribe/unsubscribe lifecycle, URL-escape path-traversal
defense, and error propagation.
Note on payload thickness: the mail event is intentionally thin in this
P0 — stdout carries only message_id, mail_address, mailbox_type,
subscriber. Subject/body/attachments need a follow-up `mail +message`
call. SKILL docs (lark-event, lark-mail) explain the asymmetry vs IM
(whose WS payload natively carries content) and show the typical
fetch-on-event pipeline.
Next steps (follow-up PRs to reach full feature parity with mail +watch):
- P1: add a Process function on the EventKey that fetches per-event
message metadata via OAPI on each event, so `event consume` emits
subject/from/snippet inline (matching mail +watch's default
--msg-format=metadata behaviour). Closes the IM-vs-mail payload
asymmetry agents currently see.
- P2: extract mail +watch's fetch + filter + format business logic into
a shared internal/mail/eventfmt package; expose --msg-format /
--folders / --labels as Params on the EventKey; mail +watch retires
to a thin alias that forwards to the unified path with sensible
defaults. After P2, mail +watch and event consume are functionally
interchangeable, fully unified.
- P3: add a lint rule (events/lint_test.go) pinning larkws and
event/dispatcher imports to events/, cmd/event/, shortcuts/event/
only — physical defense against future per-domain forks like the
one this PR removes the conceptual basis for.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
db7ba0d to
2107be8
Compare
|
Thanks for the PR, the unified entry point is a nice direction! While reading through I had a few questions I wanted to raise — happy to discuss.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #851 +/- ##
==========================================
+ Coverage 65.77% 65.88% +0.10%
==========================================
Files 516 518 +2
Lines 48625 48798 +173
==========================================
+ Hits 31985 32150 +165
- Misses 13881 13885 +4
- Partials 2759 2763 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2107be805bae18a01da57f0a672a9c5518d1eaf3🧩 Skill updatenpx skills add dingding0418/cli#feat/event-mail-pipeline -y -g |
|
Thanks for the careful review — these are all real concerns. Let me set the framing first: this PR and the planned follow-ups are all driven by an AI-friendly 1. Subscription dimension mismatch — real, framework-levelRoot cause: Wider observation: mail is the first EventKey in lark-cli where Proposed fix (Option A): lift the dedup key from one dimension to two.
Idempotency safety net: Feishu's 2. Feature parity gap — same abstraction solves it alongside #1
Key observation: every missing flag maps cleanly to an
Three hooks, three concerns:
After this, 3. Silent unsubscribe failure — fixing now, Option AFully agreed, this is a real bug. The current cleanup is Option A: change the framework's cleanup signature to return error. // internal/event/types.go
PreConsume func(ctx, rt, params) (cleanup func() error, err error)
// internal/event/consume/consume.go
case lastForKey:
fmt.Fprintf(errOut, "[event] running cleanup...\n")
if err := cleanup(); err != nil {
fmt.Fprintf(errOut,
"[event] WARN: cleanup failed: %v "+
"(server-side subscription may have leaked)\n", err)
} else {
fmt.Fprintf(errOut, "[event] cleanup done.\n")
}
mail cleanup implementation:
cleanup := func() error {
cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
if _, err := rt.CallAPI(cleanupCtx, "POST",
mailboxEventPath(mailbox, "unsubscribe"), body); err != nil {
return fmt.Errorf("unsubscribe mailbox=%s: %w", mailbox, err)
}
return nil
}
Wins: errors are explicit, the WARN line has a uniform format, the misleading "cleanup done" disappears on failure, and any future EventKey using PreConsume gets it
for free.
Will add a regression test: fakeClient returns a non-nil unsubErr, asserting cleanup() returns the wrapped error and the framework writes the WARN line. |
Summary
Register
mail.user_mailbox.event.message_received_v1as a first-class EventKey underevents/mail/, so mailbox new-message events can be consumed through the unifiedlark-cli event consumecontract instead of the standalonemail +watchfork. PreConsume hook auto-opens the per-user server-side subscription and cleanup auto-unsubscribes on graceful shutdown — agents now use the same command shape for IM and mail.Changes
events/mail/register.goregisteringmail.user_mailbox.event.message_received_v1withSchema.Native(SDK typelarkmail.P2UserMailboxEventMessageReceivedV1Data),mailboxparam (defaultme), required scopes (mail:event,mail:user_mailbox.event.mail_address:read),AuthTypes: [user], andRequiredConsoleEventsso missing console event subscription fails loudly at startupPreConsumeto callPOST /open-apis/mail/v1/user_mailboxes/<mailbox>/event/subscribeand return a cleanup that callsunsubscribe(with a freshcontext.WithTimeoutso cleanup still reaches the server after the parent ctx is cancelled)events/register.goevents/mail/register_test.go(10 tests): registration / native-key invariants, scope+auth metadata, mailbox param shape, default-mailbox path, custom mailbox in URL, slash escape (path-traversal defense), cleanup unsubscribe call, subscribe-failure must return nil cleanup, empty mailbox falls back tome, path helper assemblyskills/lark-event/SKILL.md: extend description to mention mail; add aPer-EventKey payload modelsection explaining the IM-vs-mail asymmetry; add a typicalevent consume → mail +messagepipeline exampleskills/lark-mail/SKILL.mdandreferences/lark-mail-watch.mdso the+watchrow spells out when to use which pathNote on payload thickness (intentional in this P0)
The mail event is intentionally thin:
event consume mail.user_mailbox.event.message_received_v1emitsmessage_id+mail_address+mailbox_type+subscriberonly. Subject/body/attachments need a follow-upmail +message --message-id <id>call.This is asymmetric with IM (whose WS payload natively carries
content); the asymmetry is a Feishu Open Platform API design choice, not a framework limitation. Closing the asymmetry is scoped to follow-up PRs (see Next Steps).Next Steps (follow-up PRs)
Processfunction on the EventKey that fetches per-event message metadata via OAPI, soevent consumeemits subject/from/snippet inline (matchingmail +watch's default--msg-format=metadata). Closes the IM-vs-mail payload asymmetry agents currently see.mail +watch's fetch + filter + format business logic into a sharedinternal/mail/eventfmtpackage; expose--msg-format/--folders/--labelsas Params on the EventKey;mail +watchretires to a thin alias forwarding to the unified path with sensible defaults. After P2 the two paths are fully interchangeable.events/lint_test.go) pinninglarkwsandevent/dispatcherimports toevents/,cmd/event/,shortcuts/event/only — physical defense against future per-domain forks like the one this PR removes the conceptual basis for.Test Plan
make unit-testpasses (full suite green)go vet ./...cleangolangci-lint v2.1.6 run ./events/...— 0 issueslark-cli event listshowsmail.user_mailbox.event.message_received_v1under── mail ──lark-cli event schema mail.user_mailbox.event.message_received_v1showsPre-consume: yes, scopes, console events, mailbox param, and the SDK-derived output schemalark-cli event consume mail.user_mailbox.event.message_received_v1 --as user --max-events 1 --timeout 10m— PreConsume opens subscription, NDJSON event streams to stdout when a real email arrives, cleanup unsubscribes on graceful exit (reason: limit)mail +watchstill works (verified to confirm back-compat)Notes
mail +watchshortcut is intentionally left in place — it offers richer payload modes (--msg-format minimal/metadata/plain_text_full/full) and folder/label filters that this minimal P0 unified path doesn't yet expose. Both paths share the same server-side subscription record; P1/P2 above explicitly target full feature parity so this duality is temporary.mail.user_mailbox.event.message_received_v1must be enabled (and the app published) in the developer console for events to flow — same prerequisite asmail +watch; the runtimeRequiredConsoleEventspredicate now warns if it is missing.