fix(mail): add missing event scope for mail watch#357
fix(mail): add missing event scope for mail watch#357chanthuang wants to merge 2 commits intomainfrom
Conversation
The mail +watch shortcut requires scope mail:user_mailbox.event.mail_address:read to receive the mail_address field in WebSocket event payloads, but this scope was neither declared in the shortcut's Scopes list nor included in the auto-approve (recommend.allow) set. Without this scope, +watch events arrive without the mail_address field, which breaks mailbox filtering and fetch-mailbox resolution. - Add scope to mail +watch Scopes declaration - Add scope to scope_overrides.json recommend.allow list so that auth login --recommend requests it automatically
|
|
|
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)
📝 WalkthroughWalkthroughAdded the event-based scope Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
domain/mail
Greptile SummaryThis PR fixes two missing scope declarations for Confidence Score: 5/5Safe to merge — targeted scope fixes with no logic changes and no P0/P1 findings. Both scope additions are correct and complete: No files require special attention.
|
| Filename | Overview |
|---|---|
| shortcuts/mail/mail_watch.go | Adds mail:user_mailbox.event.mail_address:read and mail:user_mailbox:readonly to the shortcut's declared Scopes; both are functionally required and correctly scoped. |
| internal/registry/scope_overrides.json | Adds mail:user_mailbox.event.mail_address:read to recommend.allow, correctly overriding the platform's recommend: "false" so auth login --recommend requests it. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[auth login --recommend] --> B[LoadAutoApproveSet]
B --> C[scope_priorities.json recommend=true]
B --> D[scope_overrides.json recommend.allow]
D --> E[mail:user_mailbox.event.mail_address:read added by PR]
F[mail +watch --mailbox me] --> G[fetchMailboxPrimaryEmail\nrequires mail:user_mailbox:readonly]
F --> H[WebSocket connect]
H --> I[Event received with mail_address field]
I --> J[Filter by mailbox]
J --> K[Emit matching event to stdout]
Reviews (2): Last reviewed commit: "fix(mail): add missing mailbox profile s..." | Re-trigger Greptile
![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
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)
182-210:⚠️ Potential issue | 🟠 MajorUse
vfs.*instead ofos.*for filesystem operations.Lines 182-183 use
os.UserHomeDir()and line 203 usesos.MkdirAll(). As per coding guidelines, all filesystem access should usevfs.*instead ofos.*to support virtual filesystem abstractions for testing and sandboxing.📁 Proposed fix to use vfs package
Check if the
vfspackage is imported and available, then replace:- home, err := os.UserHomeDir() + home, err := vfs.UserHomeDir() if err != nil { return fmt.Errorf("cannot expand ~: %w", err)- if err := os.MkdirAll(outputDir, 0700); err != nil { + if err := vfs.MkdirAll(outputDir, 0700); err != nil { return fmt.Errorf("cannot create output directory %q: %w", outputDir, err)As per coding guidelines: Use
vfs.*instead ofos.*for all filesystem access.🤖 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 182 - 210, Replace direct os package filesystem calls with the vfs equivalents: call vfs.UserHomeDir() instead of os.UserHomeDir(), vfs.MkdirAll(outputDir, 0700) instead of os.MkdirAll(...), and use vfs.EvalSymlinks(outputDir) instead of filepath.EvalSymlinks so the code honors the virtual filesystem abstraction; keep the same error handling and assignment to outputDir, and ensure the vfs package is imported and used around the existing logic that also calls validate.SafeOutputPath and manipulates the outputDir variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Line 84: The Scopes array in mail_watch.go is missing the required
"mail:user_mailbox:readonly" permission which causes
fetchMailboxPrimaryEmail(runtime, "me") to fail when mailbox defaults to "me";
update the Scopes slice (the Scopes: []string{...} declaration) to include
"mail:user_mailbox:readonly" alongside the existing mail scopes so the default
mailbox resolution and event filtering have the necessary readonly mailbox
access.
---
Outside diff comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 182-210: Replace direct os package filesystem calls with the vfs
equivalents: call vfs.UserHomeDir() instead of os.UserHomeDir(),
vfs.MkdirAll(outputDir, 0700) instead of os.MkdirAll(...), and use
vfs.EvalSymlinks(outputDir) instead of filepath.EvalSymlinks so the code honors
the virtual filesystem abstraction; keep the same error handling and assignment
to outputDir, and ensure the vfs package is imported and used around the
existing logic that also calls validate.SafeOutputPath and manipulates the
outputDir variable.
🪄 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: 850c59a6-6d50-44a6-a11e-b639e9207771
📒 Files selected for processing (2)
internal/registry/scope_overrides.jsonshortcuts/mail/mail_watch.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6f5a4b8241415386075d10adecf16e87252a56c3🧩 Skill updatenpx skills add larksuite/cli#fix/mail-watch-event-scope -y -g |
The +watch shortcut calls fetchMailboxPrimaryEmail (GET user_mailboxes/me/profile) to resolve the mailbox address for event filtering, which requires scope mail:user_mailbox:readonly. All other mail shortcuts that call this API (send, reply, forward, draft-create, draft-edit) already declare this scope, but +watch did not.
chanthuang
requested review from
haidaodashushu and
liangshuo-1
and removed request for
haidaodashushu
2 participants
Summary
mail:user_mailbox.event.mail_address:readto mail +watch's declared Scopesscope_overrides.jsonrecommend.allow listProblem
The
mail +watchshortcut was missing themail:user_mailbox.event.mail_address:readscope. Without it, WebSocket event payloads arrive without themail_addressfield, which breaks:--mailboxfiltering (cannot match events to the subscribed mailbox)fetchMailboxresolution (falls back to the--mailboxflag value instead of the event's actual mailbox address)The scope was also absent from the auto-approve list, so
auth login --recommendwould not request it.Test plan
go build ./...passesgo test ./shortcuts/mail/... ./internal/registry/...passesauth login --recommendnow includesmail:user_mailbox.event.mail_address:readin requested scopesmail +watchevents containmail_addressfield after re-loginSummary by CodeRabbit