Connect mail watch to event consume - #1542
Conversation
Register the mail message received EventKey and route the legacy watch shortcut through the unified consume path so subscription identity and cleanup are handled by the event framework. sprint: S1
github-actions
Bot
added
domain/mail
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new mail message-received event path, registers it, and moves mail watch plus event consume onto the unified consume framework with updated stdin-close behavior and docs. ChangesMail message received event
Event consume shutdown and mail watch migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@aca3c09c44cf72ff70fc24b9bec62de7db31e053🧩 Skill updatenpx skills add bubbmon233/cli#feat/4ae867d -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 52-72: The mailWatchConsumeRuntime type and its CallAPI method are
duplicated across five files. Create a new shared file in the events/mail
package (e.g., runtime.go) that defines this type once, then remove the
duplicate mailWatchConsumeRuntime definition from shortcuts/mail/mail_watch.go
and import the shared type instead. Apply the same refactoring to the other four
files mentioned (events/mail/params.go, events/mail/preconsume.go,
events/mail/message_received.go, and events/mail/message_received_test.go) to
ensure all files use the single shared definition of the CallAPI method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ff9cbb1-f3c4-497a-b600-7bacad945b4b
📒 Files selected for processing (9)
events/mail/message_received.goevents/mail/message_received_test.goevents/mail/params.goevents/mail/preconsume.goevents/mail/register.goevents/register.goshortcuts/mail/mail_watch.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-watch.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/mail_watch_test.go`:
- Around line 276-288: The TestMailWatchRejectsInvalidTimeout function currently
validates the error type and Param field, but it is missing assertions for the
complete typed-error contract. Add two additional assertions: first, use
errs.ProblemOf(err) to check that the Subtype field equals
errs.SubtypeInvalidArgument; second, use errors.Is to verify that the wrapped
cause (the original parse error from parseMailWatchTimeout) is preserved.
Reference the existing test patterns in
TestMailWatchOutputDirRejectsUnsafePathTyped and
TestMailWatchOutputDirMkdirFailureTyped to follow the established assertion
structure in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad20503c-17ef-4202-98b9-4c252094088e
📒 Files selected for processing (2)
shortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_watch.go
Defer stdin EOF cancellation until after consume setup so PreConsume can complete and last-subscriber cleanup can run. Timeout and max-events remain fallback bounds, while stdin EOF exits as a signal for non-TTY callers. sprint: S1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/event/consume/consume.go (1)
162-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
loopCtxfor exit reason reporting.
StdinClosednow cancels onlyloopCtx, but the deferred exit log still callsexitReason(ctx, ...). When stdin EOF stops an unbounded or not-yet-bounded run,ctx.Err()can remain nil, so stderr may not reportreason: signalas promised by the CLI/docs.🐛 Proposed fix
lastForKey := false var emitted atomic.Int64 startTime := time.Now() + exitCtx := ctx // On panic, run cleanup unconditionally — leaking server state is worse than // unsubscribing a still-live co-consumer (recoverable). defer func() { @@ } if !opts.Quiet && r == nil { - reason := exitReason(ctx, emitted.Load(), opts) + reason := exitReason(exitCtx, emitted.Load(), opts) fmt.Fprintf(errOut, "[event] exited — received %d event(s) in %s (reason: %s)\n", emitted.Load(), truncateDuration(time.Since(startTime)), reason) } @@ if opts.StdinClosed != nil { loopCtx, loopCancel = context.WithCancel(ctx) + exitCtx = loopCtx defer loopCancel() go func() {Also applies to: 180-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/event/consume/consume.go` around lines 162 - 164, The deferred exit log is still deriving the reason from the parent ctx instead of the loop-specific cancellation context, so update the exit-reason reporting in consume/consume.go to use loopCtx in the exitReason call. Make sure the deferred stderr message and any related exit logging paths that currently reference exitReason(ctx, ...) are switched to loopCtx so stdin EOF in unbounded or not-yet-bounded runs reports the expected signal reason consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/event/consume/consume.go`:
- Around line 162-164: The deferred exit log is still deriving the reason from
the parent ctx instead of the loop-specific cancellation context, so update the
exit-reason reporting in consume/consume.go to use loopCtx in the exitReason
call. Make sure the deferred stderr message and any related exit logging paths
that currently reference exitReason(ctx, ...) are switched to loopCtx so stdin
EOF in unbounded or not-yet-bounded runs reports the expected signal reason
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff36e332-465a-4bac-adce-f6c883d6b773
📒 Files selected for processing (8)
cmd/event/consume.gocmd/event/consume_stdin_test.gointernal/event/consume/bounded_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/listening_text_test.goshortcuts/mail/mail_watch.goskills/lark-event/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_watch.go
Refactor mail watch to use a shared event runtime, remove unreachable watch helpers, and strengthen invalid timeout typed-error assertions. Change-Type: ci-fix
github-actions
Bot
added
size/XL
| }, | ||
| func mailWatchConsumeOptions(runtime *common.RuntimeContext, params map[string]string, outputDir string, consumeOut io.Writer, timeout time.Duration) consume.Options { | ||
| apiRuntime := eventmail.NewRuntime(runtime) | ||
| return consume.Options{ |
There was a problem hiding this comment.
🤖 AI Review | [P1 稳定性] mail +watch 没有接入 stdin EOF 关闭信号
mailWatchConsumeOptions 只传了 MaxEvents、Timeout 和 IsTTY，没有像 cmd/event/consume.go 一样创建并传入 StdinClosed。因此 lark-cli mail +watch < /dev/null 或 AI 子进程关闭 stdin 时不会按新语义在 setup/PreConsume 后优雅退出，进程会继续挂住，server-side subscribe 也要等外部 kill 才能 cleanup；这和本 PR 更新的 flag/文档描述相反。
修复建议: 在 mail +watch 的 Execute 路径复用 event consume 的 stdin EOF watcher，非 TTY stdin EOF 后关闭 consume.Options.StdinClosed，并补一条 shortcut 级测试覆盖 stdin closed 后仍执行 cleanup。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| Match: matchMailMessageReceived, | ||
| Process: processMailMessageReceived, | ||
| PreConsume: mailSubscriptionPreConsume(EventTypeMessageReceived), | ||
| Scopes: []string{"mail:event", "mail:user_mailbox.event.mail_address:read", "mail:user_mailbox:readonly", "mail:user_mailbox.message:readonly", "mail:user_mailbox.message.address:read", "mail:user_mailbox.message.subject:read", "mail:user_mailbox.message.body:read"}, |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] EventKey scope 列表漏掉 folder/label 名称过滤依赖
这里的 Scopes 只声明了 event、profile 和 message 读取权限，但 normalizeMailMessageReceivedParams 在收到 folders 或 labels 参数时会调用 folders/list 或 labels/list 来把名称解析成 ID。实际场景里 event consume ... --param folders='["Projects"]' 会通过 scope preflight，随后在 NormalizeParams 阶段才因缺少 folder/label 权限失败，AI 也无法从 schema 中提前知道需要补授权。
修复建议: 将名称过滤所需的 folder/label scope 纳入 EventKey schema/preflight（或实现参数级条件 scope），并同步 mail +watch 的 scope 声明和测试，确保按名称过滤在启动前给出准确授权提示。
如有疑问或认为判断不准确，欢迎直接回复讨论。
|
🤖 AI Review | CR 汇总 | 有风险（1 个 P1，1 个 P2） 本轮增量审查基于已有 CodeRabbit 评论继续检查，新增 2 条问题：
建议修复后再合入。 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@events/mail/message_received.go`:
- Around line 47-50: The fetch path in message_received.go is turning every
fetchMessage error into a fetch failure event, including cancellation from stdin
EOF, timeout, or SIGTERM. Update messageReceived to check ctx cancellation via
errors.Is before calling mailMessageFetchFailure, and return the cancellation
error directly so unified consume can stop cleanly. Keep the fetch failure
payload only for real fetch errors, and add the standard-library errors import
if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 363adbd5-fced-42ff-9a56-6e242a19f0dc
📒 Files selected for processing (4)
events/mail/message_received.goevents/mail/message_received_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/mail/mail_watch_test.go
- events/mail/message_received_test.go
- shortcuts/mail/mail_watch.go
| message, err := fetchMessage(ctx, rt, fetchMailbox, messageID, fetchFormat) | ||
| if err != nil { | ||
| return json.Marshal(mailMessageFetchFailure(messageID, fetchFormat, err, body)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Let shutdown cancellation propagate instead of emitting a fetch failure.
Line 49 converts every fetchMessage error into an ok:false event. If stdin EOF, timeout, or SIGTERM cancels ctx during the GET, this produces a synthetic fetch_message_failed payload instead of letting unified consume stop cleanly.
Proposed fix
message, err := fetchMessage(ctx, rt, fetchMailbox, messageID, fetchFormat)
if err != nil {
+ if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+ return nil, err
+ }
return json.Marshal(mailMessageFetchFailure(messageID, fetchFormat, err, body))
}Also add the standard-library errors import if it is not already present.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message, err := fetchMessage(ctx, rt, fetchMailbox, messageID, fetchFormat) | |
| if err != nil { | |
| return json.Marshal(mailMessageFetchFailure(messageID, fetchFormat, err, body)) | |
| } | |
| message, err := fetchMessage(ctx, rt, fetchMailbox, messageID, fetchFormat) | |
| if err != nil { | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| return nil, err | |
| } | |
| return json.Marshal(mailMessageFetchFailure(messageID, fetchFormat, err, body)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@events/mail/message_received.go` around lines 47 - 50, The fetch path in
message_received.go is turning every fetchMessage error into a fetch failure
event, including cancellation from stdin EOF, timeout, or SIGTERM. Update
messageReceived to check ctx cancellation via errors.Is before calling
mailMessageFetchFailure, and return the cancellation error directly so unified
consume can stop cleanly. Keep the fetch failure payload only for real fetch
errors, and add the standard-library errors import if needed.
1 participant
Generated by the harness-coding skill.
Sprints
Source specs
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
mail.user_mailbox.event.message_received_v1support, includingmsg_formatmodes and folder/label filtering via normalized parameters.mail +watchto the unified event consumption framework, adding--timeoutand--output-dir(forces full output).event consumereferences to match unified framework behavior and schema inspection.reason: signal), ensuring cleanup happens correctly after setup.