feat(event): implement mail event domain package and mail +watch improvements#1134
Open
xukuncx wants to merge 3 commits into
Open
feat(event): implement mail event domain package and mail +watch improvements#1134xukuncx wants to merge 3 commits into
xukuncx wants to merge 3 commits 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
…ept it Verification report INT-CLI-EventListMailEvent-01 failed with 'unknown flag: --as' because the root cobra command had no --as persistent flag. When --as appears before the subcommand name, cobra parses flags at root level and returns an error. Add IdentityType field to GlobalOptions and register --as in RegisterGlobalFlags via the root command's PersistentFlags(). This allows 'lark-cli --as user event list' to parse without error. Subcommands with their own local --as (event consume, api/service shortcuts) are unaffected: pflag.AddFlagSet skips flags already registered locally.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds mail event consumption: mail event key/handler, mailbox subscription pre-consume with rollback/cleanup, global ChangesMail Event Consumption with Identity Flag and Subscribe Lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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)
⚔️ Resolve merge conflicts
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shortcuts/mail/mail_watch.go (1)
193-208: ⚡ Quick winAdd focused tests for lock acquisition/bypass and migration-tip path.
The new concurrency gate and
--forcebypass are critical behavior changes, but these added branches are currently uncovered. Please add unit tests that exercise: lock-create failure, lock-held validation error,--forcebypass, and migration-tip emission path.Also applies to: 214-223
🤖 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, Add unit tests around the new concurrency gate in mail_watch: cover failure to create a lock by mocking lockfile.ForSubscribe to return an error, cover the lock-held branch by having ForSubscribe return a lock whose TryLock returns an error and assert output.ErrValidation is returned and contains the runtime.Config.AppID message, cover the --force bypass by setting runtime.Bool("force") to true and verifying ForSubscribe/TryLock are not called (or not required), and cover the migration-tip/emission path referenced in the same area; exercise functions lockfile.ForSubscribe, lock.TryLock, output.ErrValidation, and runtime.Bool/runtime.Config.AppID to simulate and assert each behavior.events/mail/register.go (1)
14-15: ⚡ Quick winExtract shared mail watch/consume scopes into one constant source.
The same 7-scope list is duplicated across layers with “MUST stay in sync” comments. Please centralize the scope slice (single exported/internal source) and reference it from both
events/mail/register.goandshortcuts/mail/mail_watch.goto prevent silent drift.Also applies to: 37-48
🤖 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/register.go` around lines 14 - 15, Create a single shared slice constant (e.g., MailWatchScopes []string) in a common package and replace the duplicated 7-item scope literals in both the Scopes field usage in events/mail/register.go and the scopes reference in shortcuts/mail/mail_watch.go to reference that single symbol (export it if used across packages or make it package-internal if both files share a package); ensure the new MailWatchScopes preserves the exact order and values and update imports/usages to reference this symbol so the two locations no longer duplicate the list.shortcuts/mail/mail_watch_event_consume_test.go (1)
64-84: 🏗️ Heavy liftTest name/intent says behavior, but assertions only check flag existence.
TestMailWatch_PrintSchemaDoesNotRequireForcecurrently cannot catch regressions in the real--print-output-schemaearly-return path (e.g., lockfile still being required), because it only verifies both flags are present. Please either (a) rename this test to reflect existence-only checks, or (b) execute the command path and assert it succeeds without--force.🤖 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 64 - 84, The test TestMailWatch_PrintSchemaDoesNotRequireForce only checks MailWatch.Flags contains "print-output-schema" and "force" but doesn't verify the early-return behavior; update the test to exercise the command path: simulate running MailWatch without "--force" but with "--print-output-schema" (invoke the command runner or the Run/Execute method used by MailWatch) and assert it returns success/early-terminates without requiring the lockfile, or alternatively rename the test to indicate it only verifies flag existence; reference MailWatch.Flags, the "print-output-schema" and "force" flag names, and the MailWatch Run/Execute entrypoint when making the change.
🤖 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-50: The current truncation in
events/mail/mail_message_received.go uses byte slicing on body (body =
body[:140]) which can split multi-byte UTF-8 characters; change the logic that
sets body_excerpt (or variable body) to slice by runes instead: convert body to
[]rune, take up to 140 runes, and convert back to string so multi-byte
characters remain intact and no panic occurs when shorter. Locate the truncation
near the body variable assignment and replace the byte-based slice with
rune-based slicing (e.g., rune conversion, min check, and reassembly) in the
same function handling mail message receipt.
- Around line 113-118: The cleanup closure is reusing the outer ctx (from
mailMessageReceivedPreConsume) which may be canceled on shutdown; change cleanup
to create a fresh timeout context for the unsubscribe calls (e.g., for each
subscribed[i] or once for the loop) and use that new ctx with rt.CallAPI,
ensuring you call cancel() after the requests; update references to use the new
local ctx instead of the outer ctx and keep using rt.CallAPI, subscribed and the
unsubscribe path as-is.
---
Nitpick comments:
In `@events/mail/register.go`:
- Around line 14-15: Create a single shared slice constant (e.g.,
MailWatchScopes []string) in a common package and replace the duplicated 7-item
scope literals in both the Scopes field usage in events/mail/register.go and the
scopes reference in shortcuts/mail/mail_watch.go to reference that single symbol
(export it if used across packages or make it package-internal if both files
share a package); ensure the new MailWatchScopes preserves the exact order and
values and update imports/usages to reference this symbol so the two locations
no longer duplicate the list.
In `@shortcuts/mail/mail_watch_event_consume_test.go`:
- Around line 64-84: The test TestMailWatch_PrintSchemaDoesNotRequireForce only
checks MailWatch.Flags contains "print-output-schema" and "force" but doesn't
verify the early-return behavior; update the test to exercise the command path:
simulate running MailWatch without "--force" but with "--print-output-schema"
(invoke the command runner or the Run/Execute method used by MailWatch) and
assert it returns success/early-terminates without requiring the lockfile, or
alternatively rename the test to indicate it only verifies flag existence;
reference MailWatch.Flags, the "print-output-schema" and "force" flag names, and
the MailWatch Run/Execute entrypoint when making the change.
In `@shortcuts/mail/mail_watch.go`:
- Around line 193-208: Add unit tests around the new concurrency gate in
mail_watch: cover failure to create a lock by mocking lockfile.ForSubscribe to
return an error, cover the lock-held branch by having ForSubscribe return a lock
whose TryLock returns an error and assert output.ErrValidation is returned and
contains the runtime.Config.AppID message, cover the --force bypass by setting
runtime.Bool("force") to true and verifying ForSubscribe/TryLock are not called
(or not required), and cover the migration-tip/emission path referenced in the
same area; exercise functions lockfile.ForSubscribe, lock.TryLock,
output.ErrValidation, and runtime.Bool/runtime.Config.AppID to simulate and
assert each behavior.
🪄 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: b9e870ec-1d47-4ac6-942e-d77c07eacd64
📒 Files selected for processing (11)
cmd/global_flags.gocmd/global_flags_test.goevents/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
Change-Type: ci-fix
|
yangbin.1005 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
--asflag to specify identity type (userorbot)mailboxfiltering (defaults to "me"), subscribing per-mailbox and auto-unsubscribing on exitmail +watchgains--forceto bypass app-level subscribe lockBehavior
Documentation
Tests