feat(event): implement mail event domain package and mail +watch improvements#899
feat(event): implement mail event domain package and mail +watch improvements#899bubbmon233 wants to merge 1 commit into
Conversation
…ovements - Add events/mail/ domain package: register.go wires mail.user_mailbox.event.message_received_v1 KeyDefinition (7 user scopes, AuthTypes: [user], PreConsume subscribe/rollback/cleanup, processMailMessageReceived flat output, parseMailboxes comma-split dedup default-me) - Wire mail.Keys() into events/register.go init(); remove "intentionally omitted" placeholder - Add --force flag and appID-level lockfile (shared with event +subscribe) to mail +watch - Add migration hint in mail +watch Description and Execute stderr (prefer event consume path) - Add test coverage: events/mail/ (20 tests), events/register_test.go (1 test), shortcuts/mail/mail_watch_event_consume_test.go (6 tests) - Update skills/lark-event/SKILL.md: mail event example + Topic index Mail row - Update skills/lark-mail/SKILL.md: +watch lockfile and migration tip sprint: S2
|
|
📝 WalkthroughWalkthroughThis PR implements mail event consumption for the Larksuite CLI by introducing a new "mail.user_mailbox.event.message_received_v1" event key with payload processing, multi-mailbox subscription management, PreConsume lifecycle handling, and integration with the global event registry. The mail watch shortcut gains appID-level single-instance locking shared with the event consumer, a ChangesMail event consumption implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
github-actions
Bot
added
domain/mail
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
+ Coverage 65.91% 65.96% +0.04%
==========================================
Files 520 522 +2
Lines 49277 49391 +114
==========================================
+ Hits 32483 32581 +98
- Misses 14022 14038 +16
Partials 2772 2772 ☔ 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@4eaf133cd4d9a15c8bbfe7068e6fe9f37294b5f0🧩 Skill updatenpx skills add bubbmon233/cli#feat/5b0c64b -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: 4
🧹 Nitpick comments (4)
skills/lark-event/SKILL.md (1)
150-150: ⚡ Quick winImprove clarity and consistency with line 57.
The phrase "PreConsume opens mailbox business subscribe + cleanup reverse unsubscribe" is less clear than the explanation in line 57. Consider aligning the wording.
✨ Suggested improvement
-| Mail | `events/mail/` (mail.user_mailbox.event.message_received_v1) | Receive new mail across one or more mailboxes; 7 user scopes; PreConsume opens mailbox business subscribe + cleanup reverse unsubscribe | +| Mail | `events/mail/` (mail.user_mailbox.event.message_received_v1) | Receive new mail across one or more mailboxes; 7 user scopes; PreConsume subscribes each mailbox via OAPI and unsubscribes on exit |🤖 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 `@skills/lark-event/SKILL.md` at line 150, The table row for "Mail" (the cell containing `events/mail/` and the phrase "PreConsume opens mailbox business subscribe + cleanup reverse unsubscribe") is unclear and should be reworded to match the clearer style used on line 57; update that cell to something like: "PreConsume: open mailbox and subscribe to business events; Cleanup: unsubscribe (reverse of subscribe)" so it reads consistently and explicitly references both the subscribe and unsubscribe steps for the mail.user_mailbox.event.message_received_v1 flow.skills/lark-mail/SKILL.md (1)
462-462: ⚡ Quick winClarify "bot event" terminology and improve readability.
Two issues:
Terminology confusion: The phrase "bot event mail.user_mailbox.event.message_received_v1 added" conflicts with the
--as userrequirement stated in the same sentence and in line 57 ofskills/lark-event/SKILL.md. From the PR context, this event requires "user-only auth type," so calling it a "bot event" is misleading. Consider rephrasing to "event subscription configured in the app console" or similar.Readability: This 570-character description packs multiple concerns (migration tip, scope requirements, feature comparison, locking semantics, concurrency warnings) into a single dense paragraph with nested parentheticals. The critical safety warning about concurrent execution is buried at the end. Consider breaking this into bullet points or shorter sentences to improve scanability.
✨ Suggested improvements
Option 1: Fix terminology only
-| [`+watch`](references/lark-mail-watch.md) | Watch for incoming mail events via WebSocket (requires scope mail:event and bot event mail.user_mailbox.event.message_received_v1 added). Run with --print-output-schema to see per-format field reference before parsing output. Startup now prints a tip recommending `lark-cli event consume mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...] --as user` as the unified entry with console preflight (auto-detects whether the app has subscribed mail event in console and whether the user token has the required 7 mail scopes). mail +watch keeps providing label/folder filter (--labels/--folders/--label-ids/--folder-ids) and msg-format multi-mode (--msg-format metadata\|minimal\|plain_text_full\|event\|full) and remains supported for those features. mail +watch and event +subscribe now share an appID-level subscribe lock (subscribe_<safe_appID>.lock) — running both concurrently for the same app is rejected unless --force is passed. event consume does NOT hold this lock (framework limitation); avoid running event consume mail.user_mailbox.event.message_received_v1 concurrently with mail +watch. | +| [`+watch`](references/lark-mail-watch.md) | Watch for incoming mail events via WebSocket (requires scope mail:event and the mail.user_mailbox.event.message_received_v1 event subscription configured in the app console). Run with --print-output-schema to see per-format field reference before parsing output. Startup now prints a tip recommending `lark-cli event consume mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...] --as user` as the unified entry with console preflight (auto-detects whether the app has subscribed mail event in console and whether the user token has the required 7 mail scopes). mail +watch keeps providing label/folder filter (--labels/--folders/--label-ids/--folder-ids) and msg-format multi-mode (--msg-format metadata\|minimal\|plain_text_full\|event\|full) and remains supported for those features. mail +watch and event +subscribe now share an appID-level subscribe lock (subscribe_<safe_appID>.lock) — running both concurrently for the same app is rejected unless --force is passed. event consume does NOT hold this lock (framework limitation); avoid running event consume mail.user_mailbox.event.message_received_v1 concurrently with mail +watch. |Option 2: Fix terminology and improve structure (recommended)
-| [`+watch`](references/lark-mail-watch.md) | Watch for incoming mail events via WebSocket (requires scope mail:event and bot event mail.user_mailbox.event.message_received_v1 added). Run with --print-output-schema to see per-format field reference before parsing output. Startup now prints a tip recommending `lark-cli event consume mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...] --as user` as the unified entry with console preflight (auto-detects whether the app has subscribed mail event in console and whether the user token has the required 7 mail scopes). mail +watch keeps providing label/folder filter (--labels/--folders/--label-ids/--folder-ids) and msg-format multi-mode (--msg-format metadata\|minimal\|plain_text_full\|event\|full) and remains supported for those features. mail +watch and event +subscribe now share an appID-level subscribe lock (subscribe_<safe_appID>.lock) — running both concurrently for the same app is rejected unless --force is passed. event consume does NOT hold this lock (framework limitation); avoid running event consume mail.user_mailbox.event.message_received_v1 concurrently with mail +watch. | +| [`+watch`](references/lark-mail-watch.md) | Watch for incoming mail events via WebSocket. Requires scope mail:event and the mail.user_mailbox.event.message_received_v1 event subscription configured in the app console. Startup prints a tip recommending the unified `lark-cli event consume mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...] --as user` command with console preflight. Keeps legacy features: label/folder filters (--labels/--folders/--label-ids/--folder-ids) and msg-format modes (metadata\|minimal\|plain_text_full\|event\|full). Locking: shares appID-level subscribe lock (subscribe_<safe_appID>.lock) with event +subscribe; concurrent use rejected unless --force. Warning: event consume does NOT hold this lock; avoid running event consume concurrently with mail +watch. Run --print-output-schema to see field reference. |🤖 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 `@skills/lark-mail/SKILL.md` at line 462, Change the misleading phrase "bot event mail.user_mailbox.event.message_received_v1 added" to language that reflects the event's user-only auth and console subscription requirement (e.g., "the mail.user_mailbox.event.message_received_v1 event must be subscribed in the app console and requires user-only auth and the mail:event scope"), and restructure the long single-paragraph description for readability: break into short sentences or bullet-like lines covering (1) scope and auth requirements (mention mail:event and --as user), (2) startup tip about using --print-output-schema and the recommended command example (event consume mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...] --as user), (3) supported flags for mail +watch (--labels/--folders/--label-ids/--folder-ids and --msg-format metadata|minimal|plain_text_full|event|full), and (4) locking/concurrency rules highlighting the safety warning up front about subscribe_<safe_appID>.lock, the --force override, and that event consume does NOT hold the lock.shortcuts/mail/mail_watch_event_consume_test.go (2)
60-84: 💤 Low valueTest body doesn't verify the documented behavior.
The doc comment and inline comment promise a "static check that print-output-schema runs the early-return path before the lockfile block", but the body only checks that both
print-output-schemaandforceexist inMailWatch.Flags(and explicitly disclaims any ordering assertion on line 83). This leaves the migration regression guard largely uncovered.Either tighten the name/comments to reflect what's actually asserted (presence of both flags), or strengthen the test to exercise the early-return — e.g., invoke
MailWatch.Executeviacmdutil.TestFactorywithprint-output-schema=trueand assert it returnsnilwithout ever attempting to acquire a lockfile.🤖 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_event_consume_test.go` around lines 60 - 84, The test TestMailWatch_PrintSchemaDoesNotRequireForce currently only checks presence of flags but promises to verify ordering/early-return; update the test to exercise the early-return instead of just checking MailWatch.Flags: use cmdutil.TestFactory to construct a command context and call MailWatch.Execute with the flag "print-output-schema" set to true, and assert the call returns nil (or expected success) and that no lockfile acquisition was attempted (mock or spy the lockfile acquire function used by the lockfile block, or inject a fake that would fail if called). Alternatively, if you prefer a minimal change, update the test comments to reflect it only asserts presence of flags (keep references to TestMailWatch_PrintSchemaDoesNotRequireForce and MailWatch.Flags) but the preferred fix is to invoke MailWatch.Execute via cmdutil.TestFactory and verify early-return and absence of lockfile acquisition.
97-116: ⚡ Quick winConsider sourcing
wantfrom the event key definition instead of hardcoding.The test docstring (lines 94–96) states it ensures MailWatch.Scopes "is aligned with the expected 7-item scope list", but the implementation hardcodes that list. Both
MailWatch.Scopes(inmail_watch.go:99) andevents/mail.Keys()[0].Scopescan be edited independently, defeating the sync guarantee the test name implies.The codebase already documents this concern:
events/mail/register.go:14–15states "MUST stay in sync with shortcuts/mail/mail_watch.go:98 Scopes field". Refactoring the test to sourcewantfromevents/mail.Keys()[0].Scopeswould link the two sources and prevent drift. There is no import cycle betweenshortcuts/mailandevents/mail, and the pattern is already used inevents/mail/mail_message_received_test.go:49.If refactoring is not preferred, rename the test to
TestMailWatch_ScopesExactListto clarify it verifies a literal list, not synchronization with the event key definition.🤖 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_event_consume_test.go` around lines 97 - 116, The test TestMailWatch_ScopesMatchEventKeyScopes currently hardcodes the expected scope list; instead either (preferred) make the test derive want from the canonical event key by importing events/mail and using events.Mail.Keys()[0].Scopes (compare MailWatch.Scopes to events/mail.Keys()[0].Scopes) so the test fails if they drift, or (if you intentionally want a literal-list test) rename the test to TestMailWatch_ScopesExactList and keep the hardcoded want to make the intent explicit; update assertions accordingly and ensure you reference MailWatch.Scopes and events/mail.Keys()[0].Scopes (or the new test name) when editing.
🤖 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/mail_message_received.go`:
- Around line 49-51: The current byte-slicing of the email body can split
multi-byte UTF-8 runes when building BodyExcerpt; change the truncation to be
rune-safe by converting body to runes (e.g., []rune(body)) and slicing to at
most 140 runes before converting back to string so BodyExcerpt contains up to
~140 characters without corrupting non-ASCII text (update the truncation at the
place that currently checks len(body) > 140 and assigns body = body[:140]).
- Around line 113-118: The cleanup closure should not reuse the pre-consume
captured ctx because it may be canceled; update cleanup (the function literal
that iterates subscribed and calls rt.CallAPI) to create and use a fresh context
(e.g. context.Background() or context.WithTimeout) for its unsubscribe POST
calls to
"/open-apis/mail/v1/user_mailboxes/"+url.PathEscape(subscribed[i])+"/event/unsubscribe"
via rt.CallAPI, so unsubscribes run even if the original ctx is canceled; ensure
you cancel any timeout context you create and keep the loop over subscribed and
the rt.CallAPI call site unchanged except for replacing ctx with the new local
context.
In `@events/register_test.go`:
- Around line 15-16: In TestMailKeysWiredUp add test-level config isolation by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the
test (before using event.Lookup); this ensures the test's config state is
hermetic—update the TestMailKeysWiredUp function to set the env var via t.Setenv
and then proceed to call
event.Lookup("mail.user_mailbox.event.message_received_v1").
In `@shortcuts/mail/mail_watch.go`:
- Around line 193-208: Replace the bare fmt.Errorf returned from
lockfile.ForSubscribe with a structured output error helper and distinguish lock
contention from I/O errors when calling lock.TryLock: call
lockfile.ForSubscribe(runtime.Config.AppID) and if it returns err wrap/return it
via the same output helper used elsewhere (e.g., output.ErrUnexpected or an
appropriate output.Err*), then when lock.TryLock() returns check errors.Is(err,
lockfile.ErrHeld) and only in that case return the user-facing
output.ErrValidation message about "another subscribe-class instance is already
running"; for any other TryLock error return or wrap it as an I/O/unexpected
error (e.g., output.ErrWithHint or output.ErrUnexpected) so creation,
contention, and other failures are clearly separated (refer to
lockfile.ForSubscribe, lock.TryLock, lockfile.ErrHeld, output.ErrValidation, and
output.ErrWithHint).
---
Nitpick comments:
In `@shortcuts/mail/mail_watch_event_consume_test.go`:
- Around line 60-84: The test TestMailWatch_PrintSchemaDoesNotRequireForce
currently only checks presence of flags but promises to verify
ordering/early-return; update the test to exercise the early-return instead of
just checking MailWatch.Flags: use cmdutil.TestFactory to construct a command
context and call MailWatch.Execute with the flag "print-output-schema" set to
true, and assert the call returns nil (or expected success) and that no lockfile
acquisition was attempted (mock or spy the lockfile acquire function used by the
lockfile block, or inject a fake that would fail if called). Alternatively, if
you prefer a minimal change, update the test comments to reflect it only asserts
presence of flags (keep references to
TestMailWatch_PrintSchemaDoesNotRequireForce and MailWatch.Flags) but the
preferred fix is to invoke MailWatch.Execute via cmdutil.TestFactory and verify
early-return and absence of lockfile acquisition.
- Around line 97-116: The test TestMailWatch_ScopesMatchEventKeyScopes currently
hardcodes the expected scope list; instead either (preferred) make the test
derive want from the canonical event key by importing events/mail and using
events.Mail.Keys()[0].Scopes (compare MailWatch.Scopes to
events/mail.Keys()[0].Scopes) so the test fails if they drift, or (if you
intentionally want a literal-list test) rename the test to
TestMailWatch_ScopesExactList and keep the hardcoded want to make the intent
explicit; update assertions accordingly and ensure you reference
MailWatch.Scopes and events/mail.Keys()[0].Scopes (or the new test name) when
editing.
In `@skills/lark-event/SKILL.md`:
- Line 150: The table row for "Mail" (the cell containing `events/mail/` and the
phrase "PreConsume opens mailbox business subscribe + cleanup reverse
unsubscribe") is unclear and should be reworded to match the clearer style used
on line 57; update that cell to something like: "PreConsume: open mailbox and
subscribe to business events; Cleanup: unsubscribe (reverse of subscribe)" so it
reads consistently and explicitly references both the subscribe and unsubscribe
steps for the mail.user_mailbox.event.message_received_v1 flow.
In `@skills/lark-mail/SKILL.md`:
- Line 462: Change the misleading phrase "bot event
mail.user_mailbox.event.message_received_v1 added" to language that reflects the
event's user-only auth and console subscription requirement (e.g., "the
mail.user_mailbox.event.message_received_v1 event must be subscribed in the app
console and requires user-only auth and the mail:event scope"), and restructure
the long single-paragraph description for readability: break into short
sentences or bullet-like lines covering (1) scope and auth requirements (mention
mail:event and --as user), (2) startup tip about using --print-output-schema and
the recommended command example (event consume
mail.user_mailbox.event.message_received_v1 --param mailbox=<email>[,<email>...]
--as user), (3) supported flags for mail +watch
(--labels/--folders/--label-ids/--folder-ids and --msg-format
metadata|minimal|plain_text_full|event|full), and (4) locking/concurrency rules
highlighting the safety warning up front about subscribe_<safe_appID>.lock, the
--force override, and that event consume does NOT hold the lock.
🪄 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: c1caadab-c3cd-44d3-8b74-dd904e676955
📒 Files selected for processing (9)
events/mail/mail_message_received.goevents/mail/mail_message_received_test.goevents/mail/register.goevents/register.goevents/register_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_event_consume_test.goskills/lark-event/SKILL.mdskills/lark-mail/SKILL.md
| if len(body) > 140 { | ||
| body = body[:140] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, does slicing a string by byte index (e.g., s[:140]) risk splitting UTF-8 runes, and what is the idiomatic way to truncate to N characters?
💡 Result:
Yes—slicing a Go string by byte index like s[:140] can split a UTF-8 encoded rune, because Go strings are byte sequences and slicing/indexing operates on byte offsets rather than characters/runes [1]. If you slice in the middle of a multi-byte UTF-8 sequence, the resulting string may contain invalid UTF-8 or partial characters when interpreted as text [1]. Idiomatic ways to truncate to N characters (runes) in Go: 1) Truncate to N runes by converting to []rune (simplest/idiomatic when you mean “characters”): r:= []rune(s) if len(r) > N { s = string(r[:N]) } This works because range over strings and rune-based operations treat UTF-8 correctly; the indices from range are byte positions [1]. 2) If you instead have an N-byte limit (not asked, but commonly confused with N “characters”), use utf8 helpers to avoid cutting mid-rune. The unicode/utf8 package provides RuneStart(b byte) to detect rune boundary bytes [2]. For example, after choosing an approximate byte index i, move left until s[i] is a valid rune start, then slice at that boundary (as illustrated by solutions using utf8.RuneStart) [3]. If you truly mean “truncate to N characters/runes”, use option (1).
Citations:
- 1: https://go.dev/blog/strings
- 2: https://pkg.go.dev/unicode/utf8@go1.22.1
- 3: https://stackoverflow.com/questions/52323044/split-string-based-on-byte-length-in-golang
🏁 Script executed:
# First, locate and examine the actual file
find . -name "mail_message_received.go" -type fRepository: larksuite/cli
Length of output: 96
🏁 Script executed:
# Read the file to understand the context around lines 49-51
cat -n events/mail/mail_message_received.go | head -70Repository: larksuite/cli
Length of output: 3327
Use rune-safe truncation for BodyExcerpt at lines 49–51.
The field is documented as "Body excerpt (first ~140 chars, plain text)" but truncates by byte index. This can split UTF-8 runes if the email body contains non-ASCII characters, producing corrupted text in the excerpt.
Suggested fix
body := envelope.Event.Body
- if len(body) > 140 {
- body = body[:140]
+ runes := []rune(body)
+ if len(runes) > 140 {
+ body = string(runes[:140])
}📝 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.
| if len(body) > 140 { | |
| body = body[:140] | |
| } | |
| body := envelope.Event.Body | |
| runes := []rune(body) | |
| if len(runes) > 140 { | |
| body = string(runes[:140]) | |
| } |
🤖 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/mail_message_received.go` around lines 49 - 51, The current
byte-slicing of the email body can split multi-byte UTF-8 runes when building
BodyExcerpt; change the truncation to be rune-safe by converting body to runes
(e.g., []rune(body)) and slicing to at most 140 runes before converting back to
string so BodyExcerpt contains up to ~140 characters without corrupting
non-ASCII text (update the truncation at the place that currently checks
len(body) > 140 and assigns body = body[:140]).
| cleanup := func() { | ||
| for i := len(subscribed) - 1; i >= 0; i-- { | ||
| _, _ = rt.CallAPI(ctx, "POST", | ||
| "/open-apis/mail/v1/user_mailboxes/"+url.PathEscape(subscribed[i])+"/event/unsubscribe", | ||
| map[string]interface{}{"event_type": 1}) | ||
| } |
There was a problem hiding this comment.
Avoid using the captured request context in cleanup at Lines 113-118.
cleanup currently reuses ctx from pre-consume. If that context is canceled on shutdown, unsubscribe calls can fail immediately, leaving stale subscriptions.
#!/bin/bash
# Verify whether cleanup is invoked after cancellation in consume lifecycle.
# Expected: call flow should show whether cleanup executes with canceled ctx.
rg -n -C4 'PreConsume|cleanup|lastForKey|context\.Cancel|cancel\(' --type goSuggested fix
import (
"context"
"encoding/json"
"fmt"
"net/url"
"strings"
+ "time"
@@
cleanup := func() {
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
for i := len(subscribed) - 1; i >= 0; i-- {
- _, _ = rt.CallAPI(ctx, "POST",
+ _, _ = rt.CallAPI(cleanupCtx, "POST",
"/open-apis/mail/v1/user_mailboxes/"+url.PathEscape(subscribed[i])+"/event/unsubscribe",
map[string]interface{}{"event_type": 1})
}
}🤖 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/mail_message_received.go` around lines 113 - 118, The cleanup
closure should not reuse the pre-consume captured ctx because it may be
canceled; update cleanup (the function literal that iterates subscribed and
calls rt.CallAPI) to create and use a fresh context (e.g. context.Background()
or context.WithTimeout) for its unsubscribe POST calls to
"/open-apis/mail/v1/user_mailboxes/"+url.PathEscape(subscribed[i])+"/event/unsubscribe"
via rt.CallAPI, so unsubscribes run even if the original ctx is canceled; ensure
you cancel any timeout context you create and keep the loop over subscribed and
the rt.CallAPI call site unchanged except for replacing ctx with the new local
context.
| func TestMailKeysWiredUp(t *testing.T) { | ||
| def, ok := event.Lookup("mail.user_mailbox.event.message_received_v1") |
There was a problem hiding this comment.
Add config-dir isolation in this unit test.
Initialize test config isolation at test start with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to keep test behavior hermetic and aligned with repo test standards.
As per coding guidelines: **/*_test.go: Use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.
🤖 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/register_test.go` around lines 15 - 16, In TestMailKeysWiredUp add
test-level config isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the start of the test (before using event.Lookup); this ensures
the test's config state is hermetic—update the TestMailKeysWiredUp function to
set the env var via t.Setenv and then proceed to call
event.Lookup("mail.user_mailbox.event.message_received_v1").
| if !runtime.Bool("force") { | ||
| lock, err := lockfile.ForSubscribe(runtime.Config.AppID) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create lock: %w", err) | ||
| } | ||
| if err := lock.TryLock(); err != nil { | ||
| return output.ErrValidation( | ||
| "another subscribe-class instance is already running for app %s\n"+ | ||
| " Only one subscribe-class process per app is allowed to prevent concurrent unsubscribe canceling each other's server-side subscription.\n"+ | ||
| " Recommended: use 'lark-cli event consume mail.user_mailbox.event.message_received_v1 --as user' for unified entry with console preflight.\n"+ | ||
| " Or pass --force to bypass this check (NOT recommended).", | ||
| runtime.Config.AppID, | ||
| ) | ||
| } | ||
| defer lock.Unlock() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect lockfile package to see what errors ForSubscribe / TryLock return,
# and which output.Err* helpers exist to model conflict-state errors.
fd -e go . internal/lockfile | xargs -r rg -n -C3 '\b(ForSubscribe|TryLock|Unlock|Err\w+)\b'
rg -n -C2 '^func Err\w+\b' internal/outputRepository: larksuite/cli
Length of output: 17671
Use structured error helpers and distinguish lock-contention from creation errors.
The code has two related issues:
-
Line 197 returns a bare
fmt.ErrorfforForSubscribeerrors, while the rest ofExecuteusesoutput.Err*helpers. This breaks the structured exit-code routing that the command relies on. -
Line 200 wraps
TryLockerrors inoutput.ErrValidation, but a held lockfile is a runtime-state conflict, not user-input validation. More critically,TryLockcan fail for two distinct reasons:- Lock contention (returns
ErrHeldsentinel) - I/O errors (e.g., "open lock file" on permission or filesystem failure)
The "already running" message is only correct when
errors.Is(err, lockfile.ErrHeld)is true. Other failures should not produce that message. - Lock contention (returns
Fix: Use a structured error helper for the ForSubscribe path, and for TryLock, check the error type—only emit the "already running" guidance when the error wraps lockfile.ErrHeld. For non-contention failures, return the underlying error or use output.ErrWithHint with a different message.
🤖 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 193 - 208, Replace the bare
fmt.Errorf returned from lockfile.ForSubscribe with a structured output error
helper and distinguish lock contention from I/O errors when calling
lock.TryLock: call lockfile.ForSubscribe(runtime.Config.AppID) and if it returns
err wrap/return it via the same output helper used elsewhere (e.g.,
output.ErrUnexpected or an appropriate output.Err*), then when lock.TryLock()
returns check errors.Is(err, lockfile.ErrHeld) and only in that case return the
user-facing output.ErrValidation message about "another subscribe-class instance
is already running"; for any other TryLock error return or wrap it as an
I/O/unexpected error (e.g., output.ErrWithHint or output.ErrUnexpected) so
creation, contention, and other failures are clearly separated (refer to
lockfile.ForSubscribe, lock.TryLock, lockfile.ErrHeld, output.ErrValidation, and
output.ErrWithHint).
2 participants
Generated by the harness-coding skill.
Sprints
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Improvements
--forceflag to bypass subscription lock when necessary.Documentation