fix(mail): on-demand scope checks and watch event filtering#198
Merged
haidaodashushu merged 2 commits intomainfrom Apr 2, 2026
Merged
fix(mail): on-demand scope checks and watch event filtering#198haidaodashushu merged 2 commits intomainfrom
haidaodashushu merged 2 commits intomainfrom
Conversation
- Remove mail:user_mailbox.folder:read from watch's static Scopes; add validateFolderReadScope and validateLabelReadScope that check permissions on-demand when listMailboxFolders/listMailboxLabels is called (same pattern as validateConfirmSendScope). - Resolve --mailbox me to real email address via profile API for event filtering, preventing other users' mail events from being processed. Block startup if resolution fails, with proper error type distinction. - Add unsubscribe cleanup (guarded by sync.Once) on all exit paths: SIGINT/SIGTERM, profile resolution failure, and WebSocket failure. - Remove bot from AuthTypes since bot tokens cannot subscribe. - Include profile lookup in dry-run output and update tests. - Update fetchMailboxPrimaryEmail to return error for diagnostics. - Update documentation for on-demand scope requirements.
github-actions
bot
added
domain/mail
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors mail shortcut error handling and scope validation, changes MailWatch to user-only auth with narrowed scopes, adds profile-based mailbox resolution and robust unsubscription flow, updates tests to expect an extra profile API call, and clarifies permission requirements in documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime
participant TokenStore
participant ProfileAPI as "Mail API\n(GET /me/profile)"
participant WS as "WebSocket\n(event stream)"
Runtime->>TokenStore: read stored scopes
Runtime->>ProfileAPI: GET /me/profile (dry-run or --mailbox=me)
ProfileAPI-->>Runtime: profile (primary_email) / error
Runtime->>WS: subscribe to mail events (mail:event)
WS-->>Runtime: message_received event (with from/to, labels, folder)
Runtime->>Runtime: resolve mailbox filter (case-insensitive compare)
alt matches filter
Runtime->>Runtime: deliver event to shortcut
else not match
Runtime-->>Runtime: ignore event
end
Runtime->>Runtime: on shutdown or WS failure, call unsubscribe (sync.Once)
Runtime->>WS: unsubscribe (best-effort)
WS-->>Runtime: unsubscribe ack / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c8152b392c7cbe8acab76bfef3564d4094681672🧩 Skill updatenpx skills add larksuite/cli#fix/mail-watch-scope-on-demand -y -g |
Greptile SummaryThis PR improves Key changes:
Two concerns worth addressing before merge:
Confidence Score: 3/5Not safe to merge as-is: mandatory profile resolution may silently break existing users who lack mail:user_mailbox:readonly, and the sync.Once error-return pattern has a data race. Two P1 issues: (1) a data race on unsubErr between the signal-handler goroutine and the main goroutine's WebSocket-error path, and (2) profile resolution is now mandatory for the default --mailbox me path but mail:user_mailbox:readonly was removed from the static Scopes list — users who granted only the previously-required scopes will hit a hard failure at startup after subscribing, with no upfront prompt to acquire the new scope. shortcuts/mail/mail_watch.go — unsubErr race and mandatory profile resolution; shortcuts/mail/helpers.go — silent error drop in fetchCurrentUserEmail Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as mail +watch
participant API as Lark API
participant WS as WebSocket
User->>CLI: mail +watch --mailbox me
CLI->>API: POST /mailboxes/me/event/subscribe
API-->>CLI: 200 OK (subscribed)
CLI->>API: GET /mailboxes/me/profile
alt profile OK
API-->>CLI: primary_email_address
CLI->>WS: Connect & listen
WS-->>CLI: mail event (mail_address=X)
Note over CLI: EqualFold(X, resolved email)?
alt match
CLI->>API: GET /mailboxes/{email}/messages/{id}
API-->>CLI: message payload
CLI->>User: emit NDJSON
else no match
CLI->>CLI: discard event
end
User->>CLI: Ctrl+C (SIGINT)
CLI->>API: POST /mailboxes/me/event/unsubscribe
API-->>CLI: OK
CLI->>User: exit 0
else profile error (permission)
API-->>CLI: 403
CLI->>API: POST /mailboxes/me/event/unsubscribe (cleanup)
CLI->>User: error + hint to grant mail:user_mailbox:readonly
end
Reviews (2): Last reviewed commit: "fix(mail): preserve original error in en..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Return the original error directly for non-permission failures instead of wrapping with fmt.Errorf, so structured exit codes (ExitNetwork, ExitAPI) are preserved for scripting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
shortcuts/mail/mail_watch.go (1)
248-250:⚠️ Potential issue | 🟠 MajorResolve
mebefore the subscribe write.The mailbox address is now a startup prerequisite, but
event/subscribeis still called first. If/me/profilefails and the best-effort unsubscribe also fails, the command can leave a live subscription behind on a path that never started watching.Also applies to: 263-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch.go` around lines 248 - 250, The current code calls runtime.CallAPI("POST", mailboxPath(mailbox, "event", "subscribe"), ...) before ensuring the mailbox identity (/me/profile) is resolved, which can leave a stray subscription if /me/profile later fails; modify the flow in mail_watch.go so that you call the /me/profile resolution (the routine that fetches/resolves the mailbox address) and verify it succeeds before invoking mailboxPath(...) and runtime.CallAPI for subscribe, and handle errors from the profile resolution first (returning an error) so wrapWatchSubscribeError is only used after a successful resolve; apply the same change for the second subscribe site referenced (the block around lines 263-269) so both subscribe calls happen only after /me/profile has completed successfully.
🧹 Nitpick comments (3)
shortcuts/mail/helpers.go (1)
253-256: Don't collapse the new profile lookup error back to"".
fetchCurrentUserEmailandfetchSelfEmailSetdiscard thefetchMailboxPrimaryEmailerror, so callers inshortcuts/mail/mail_draft_create.go, Lines 124-129, andshortcuts/mail/mail_reply_all.go, Lines 86-102, still can't distinguish missing scope from transient profile failures.Also applies to: 263-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 253 - 256, fetchCurrentUserEmail (and similarly fetchSelfEmailSet) currently swallows the error from fetchMailboxPrimaryEmail and returns an empty string, preventing callers like the functions in mail_draft_create.go and mail_reply_all.go from distinguishing missing-scope vs transient failures; change fetchCurrentUserEmail and fetchSelfEmailSet to propagate the error instead of discarding it (e.g., change signatures to return (string, error) and return fetchMailboxPrimaryEmail(runtime, "me") directly), and update the callers (the uses in mail_draft_create.go and mail_reply_all.go) to handle the returned error accordingly so they can react differently to scope-missing vs transient profile errors.shortcuts/mail/mail_watch.go (1)
84-85: Pureeventmode still over-requires message read scopes.
Executeskips message fetches for--msg-format eventruns without filters or--output-dir, but the shortcut still statically requiresmail:user_mailbox.message:readonlyand the field-read scopes. Consider moving those checks on-demand too so the least-privileged event-stream path stays usable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch.go` around lines 84 - 85, The Scopes declaration currently always requires mailbox message read scopes even though Execute can run in pure event mode without fetching messages; update the logic so the static Scopes slice no longer includes mail:user_mailbox.message:readonly and the field-read scopes by default, and instead perform on-demand scope checks inside Execute (or helper called by Execute) right before any code that fetches or accesses message fields (e.g., where message fetch branches or Write to output-dir occur) so that runs with --msg-format event and no filters/output-dir do not require message read scopes while other code paths still validate required scopes.shortcuts/mail/mail_watch_test.go (1)
86-217: Add coverage for the new startup failure branches.These assertions only pin the dry-run request order. The new
enhanceProfileErrorpath and best-effortevent/unsubscribecleanup inMailWatch.Executestill look untested, which leaves the main regression surface of this PR without dedicated coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch_test.go` around lines 86 - 217, Tests only assert dry-run ordering; add unit tests that exercise the runtime startup failure branches in MailWatch.Execute: simulate a profile fetch error to trigger enhanceProfileError and assert the returned error wraps/contains that profile error, and simulate a startup failure path that ensures the cleanup POST to event/unsubscribe is called as a best-effort (even when the profile fetch or subscription fails). Locate MailWatch.Execute and enhanceProfileError to wire in test doubles (using runtimeForMailWatchTest or the existing dryRunAPIsForMailWatchTest helpers) that return controlled failures and verify the unsubscribe POST (mailboxPath("me","event","unsubscribe")) is attempted and that errors are surfaced/mapped by enhanceProfileError accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 248-250: The current code calls runtime.CallAPI("POST",
mailboxPath(mailbox, "event", "subscribe"), ...) before ensuring the mailbox
identity (/me/profile) is resolved, which can leave a stray subscription if
/me/profile later fails; modify the flow in mail_watch.go so that you call the
/me/profile resolution (the routine that fetches/resolves the mailbox address)
and verify it succeeds before invoking mailboxPath(...) and runtime.CallAPI for
subscribe, and handle errors from the profile resolution first (returning an
error) so wrapWatchSubscribeError is only used after a successful resolve; apply
the same change for the second subscribe site referenced (the block around lines
263-269) so both subscribe calls happen only after /me/profile has completed
successfully.
---
Nitpick comments:
In `@shortcuts/mail/helpers.go`:
- Around line 253-256: fetchCurrentUserEmail (and similarly fetchSelfEmailSet)
currently swallows the error from fetchMailboxPrimaryEmail and returns an empty
string, preventing callers like the functions in mail_draft_create.go and
mail_reply_all.go from distinguishing missing-scope vs transient failures;
change fetchCurrentUserEmail and fetchSelfEmailSet to propagate the error
instead of discarding it (e.g., change signatures to return (string, error) and
return fetchMailboxPrimaryEmail(runtime, "me") directly), and update the callers
(the uses in mail_draft_create.go and mail_reply_all.go) to handle the returned
error accordingly so they can react differently to scope-missing vs transient
profile errors.
In `@shortcuts/mail/mail_watch_test.go`:
- Around line 86-217: Tests only assert dry-run ordering; add unit tests that
exercise the runtime startup failure branches in MailWatch.Execute: simulate a
profile fetch error to trigger enhanceProfileError and assert the returned error
wraps/contains that profile error, and simulate a startup failure path that
ensures the cleanup POST to event/unsubscribe is called as a best-effort (even
when the profile fetch or subscription fails). Locate MailWatch.Execute and
enhanceProfileError to wire in test doubles (using runtimeForMailWatchTest or
the existing dryRunAPIsForMailWatchTest helpers) that return controlled failures
and verify the unsubscribe POST (mailboxPath("me","event","unsubscribe")) is
attempted and that errors are surfaced/mapped by enhanceProfileError
accordingly.
In `@shortcuts/mail/mail_watch.go`:
- Around line 84-85: The Scopes declaration currently always requires mailbox
message read scopes even though Execute can run in pure event mode without
fetching messages; update the logic so the static Scopes slice no longer
includes mail:user_mailbox.message:readonly and the field-read scopes by
default, and instead perform on-demand scope checks inside Execute (or helper
called by Execute) right before any code that fetches or accesses message fields
(e.g., where message fetch branches or Write to output-dir occur) so that runs
with --msg-format event and no filters/output-dir do not require message read
scopes while other code paths still validate required scopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0982ebe0-1172-4f59-a75c-11dc0396a124
📒 Files selected for processing (4)
shortcuts/mail/helpers.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goskills/lark-mail/references/lark-mail-watch.md
|
|
infeng
approved these changes
Apr 2, 2026
3 tasks
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
* fix(mail): on-demand scope checks, event filtering, and watch lifecycle - Remove mail:user_mailbox.folder:read from watch's static Scopes; add validateFolderReadScope and validateLabelReadScope that check permissions on-demand when listMailboxFolders/listMailboxLabels is called (same pattern as validateConfirmSendScope). - Resolve --mailbox me to real email address via profile API for event filtering, preventing other users' mail events from being processed. Block startup if resolution fails, with proper error type distinction. - Add unsubscribe cleanup (guarded by sync.Once) on all exit paths: SIGINT/SIGTERM, profile resolution failure, and WebSocket failure. - Remove bot from AuthTypes since bot tokens cannot subscribe. - Include profile lookup in dry-run output and update tests. - Update fetchMailboxPrimaryEmail to return error for diagnostics. - Update documentation for on-demand scope requirements. * fix(mail): preserve original error in enhanceProfileError fallback Return the original error directly for non-permission failures instead of wrapping with fmt.Errorf, so structured exit codes (ExitNetwork, ExitAPI) are preserved for scripting.
This was referenced Apr 5, 2026
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.
Summary
mail:user_mailbox.folder:readfrom watch's static Scopes; addvalidateFolderReadScopeandvalidateLabelReadScopethat check permissions on-demand only whenlistMailboxFolders/listMailboxLabelsis actually called (same pattern asvalidateConfirmSendScope).--mailbox meto real email address via profile API for event filtering, preventing other users' mail events from being processed. Block startup if resolution fails, with error type distinction (permission vs transient).unsubscribecleanup (guarded bysync.Once) on all exit paths: SIGINT/SIGTERM, profile resolution failure, and WebSocket connection failure.fetchMailboxPrimaryEmailto return error for proper diagnostics.Test plan
mail +watchworks withoutmail:user_mailbox.folder:readscopemail +watch --folders '["inbox"]'works (system folder, no scope needed)mail +watch --folders '["custom"]'prompts forfolder:readscope when missingmail +watch --labels '["custom"]'prompts formessage:modifyscope when missingmail:user_mailbox:readonly, startup fails with clear error--dry-runshows profile lookup stepSummary by CodeRabbit
New Features
Bug Fixes
--mailbox=meto correctly filter eventsTests
Documentation