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
📝 WalkthroughWalkthroughAdds a new mail message-received event implementation, registers it, and migrates ChangesMail message received event
Event consume shutdown and mail watch migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)internal/event/consume/consume_test.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.37][ERROR]: unable to find a config; path internal/event/consume/listening_text_test.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.97][ERROR]: unable to find a config; path internal/event/consume/consume.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.76][ERROR]: unable to find a config; path
🔧 markdownlint-cli2 (0.22.1)skills/lark-event/SKILL.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) 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@7b31585e27cdafa0d3af1eb87545cba32ac76357🧩 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
| type mailWatchConsumeRuntime struct { | ||
| runtime *common.RuntimeContext | ||
| } | ||
|
|
||
| func (r *mailWatchConsumeRuntime) CallAPI(_ context.Context, method, path string, body interface{}) (json.RawMessage, error) { | ||
| apiPath := path | ||
| query := make(larkcore.QueryParams) | ||
| if u, err := url.Parse(path); err == nil && u.RawQuery != "" { | ||
| apiPath = u.Path | ||
| for key, values := range u.Query() { | ||
| if len(values) > 0 { | ||
| query.Set(key, values[0]) | ||
| } | ||
| } | ||
| } | ||
| data, err := r.runtime.DoAPIJSONTyped(method, apiPath, query, body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return json.Marshal(map[string]interface{}{"data": data}) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract duplicated mailWatchConsumeRuntime to a shared location.
The mailWatchConsumeRuntime type and its CallAPI method are duplicated across at least 5 files in this PR:
shortcuts/mail/mail_watch.go(this file, lines 52-72)events/mail/params.go:56-72events/mail/preconsume.go:56-72events/mail/message_received.go:56-72events/mail/message_received_test.go:56-72
Define this adapter once in a shared location (e.g., events/mail/runtime.go or internal/event/apiruntime.go) and import it in all consuming files to eliminate duplication and ensure consistency.
♻️ Suggested refactor
Create a new file events/mail/runtime.go:
package mail
import (
"context"
"encoding/json"
"net/url"
larkcore "github.com/larksuite/oapi-sdk-go/v3/core"
"github.com/larksuite/cli/shortcuts/common"
)
type ConsumeRuntime struct {
runtime *common.RuntimeContext
}
func (r *ConsumeRuntime) CallAPI(_ context.Context, method, path string, body interface{}) (json.RawMessage, error) {
apiPath := path
query := make(larkcore.QueryParams)
if u, err := url.Parse(path); err == nil && u.RawQuery != "" {
apiPath = u.Path
for key, values := range u.Query() {
if len(values) > 0 {
query.Set(key, values[0])
}
}
}
data, err := r.runtime.DoAPIJSONTyped(method, apiPath, query, body)
if err != nil {
return nil, err
}
return json.Marshal(map[string]interface{}{"data": data})
}Then in this file, replace lines 52-72 with:
-type mailWatchConsumeRuntime struct {
- runtime *common.RuntimeContext
-}
-
-func (r *mailWatchConsumeRuntime) CallAPI(_ context.Context, method, path string, body interface{}) (json.RawMessage, error) {
- apiPath := path
- query := make(larkcore.QueryParams)
- if u, err := url.Parse(path); err == nil && u.RawQuery != "" {
- apiPath = u.Path
- for key, values := range u.Query() {
- if len(values) > 0 {
- query.Set(key, values[0])
- }
- }
- }
- data, err := r.runtime.DoAPIJSONTyped(method, apiPath, query, body)
- if err != nil {
- return nil, err
- }
- return json.Marshal(map[string]interface{}{"data": data})
-}And update the consume.Run call sites (lines 271, 274):
- Runtime: &mailWatchConsumeRuntime{runtime: runtime},
+ Runtime: &mailpkg.ConsumeRuntime{Runtime: runtime},
...
- RemoteAPIClient: &mailWatchConsumeRuntime{runtime: runtime},
+ RemoteAPIClient: &mailpkg.ConsumeRuntime{Runtime: runtime},Apply the same refactor to all other files that duplicate this code.
🤖 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 `@shortcuts/mail/mail_watch.go` around lines 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.
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
| func TestMailWatchRejectsInvalidTimeout(t *testing.T) { | ||
| _, err := parseMailWatchTimeout("soon") | ||
| if err == nil { | ||
| t.Fatal("expected invalid timeout error") | ||
| } | ||
| var validationErr *errs.ValidationError | ||
| if !errors.As(err, &validationErr) { | ||
| t.Fatalf("expected validation error, got %T: %v", err, err) | ||
| } | ||
| if validationErr.Param != "--timeout" { | ||
| t.Fatalf("param = %q, want --timeout", validationErr.Param) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the test file
fd -t f "mail_watch_test.go" --exec wc -l {} +Repository: larksuite/cli
Length of output: 238
🏁 Script executed:
# Get outline of the test file to understand structure
ast-grep outline shortcuts/mail/mail_watch_test.goRepository: larksuite/cli
Length of output: 4111
🏁 Script executed:
# Read the specific test function and surrounding context
sed -n '270,295p' shortcuts/mail/mail_watch_test.goRepository: larksuite/cli
Length of output: 913
🏁 Script executed:
# Find the parseMailWatchTimeout function definition
rg "func parseMailWatchTimeout" -A 20 shortcuts/mail/Repository: larksuite/cli
Length of output: 1606
🏁 Script executed:
# Check what errs.ValidationError looks like and how ProblemOf works
rg "type ValidationError" -A 5 --type goRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
# Look for existing error-path test patterns to understand standards
rg "errs.ProblemOf" shortcuts/mail/ -B 3 -A 3 --type goRepository: larksuite/cli
Length of output: 12641
🏁 Script executed:
rg "func mailValidationParamError" -A 10 shortcuts/mail/Repository: larksuite/cli
Length of output: 923
🏁 Script executed:
# Check the test that does assert ValidationError category/subtype
sed -n '227,254p' shortcuts/mail/mail_watch_test.goRepository: larksuite/cli
Length of output: 937
🏁 Script executed:
rg "func mailValidationError" -A 5 shortcuts/mail/mail_errors.goRepository: larksuite/cli
Length of output: 460
🏁 Script executed:
sed -n '290,327p' shortcuts/mail/mail_watch_test.goRepository: larksuite/cli
Length of output: 1317
Strengthen the invalid-timeout test to assert full typed-error contract.
This test validates type and Param, but should also assert the errs.ProblemOf(err) metadata—specifically that Subtype == errs.SubtypeInvalidArgument—and that the wrapped cause is preserved (e.g., via errors.Is(err, <original parse error>)). See TestMailWatchOutputDirRejectsUnsafePathTyped (lines 227–254) and TestMailWatchOutputDirMkdirFailureTyped (lines 290+) for the established pattern in this file.
🤖 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 `@shortcuts/mail/mail_watch_test.go` around lines 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.
Sources: Coding guidelines, Learnings
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
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_v1, includingmsg_formatmodes and folder/label filtering.mail +watchto the unified event consumption framework, withdata/jsonoutput modes (and forced full format when using--output-dir).