feat: add read receipt support (--request-receipt + +send-receipt)#595
Open
xzcong0820 wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add read receipt support (--request-receipt + +send-receipt)#595xzcong0820 wants to merge 1 commit intolarksuite:mainfrom
xzcong0820 wants to merge 1 commit intolarksuite:mainfrom
Conversation
github-actions
Bot
added
domain/mail
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RFC‑3798 read‑receipt support: eml builder gained disposition‑notification and read‑receipt flags; multiple mail shortcuts accept Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant API
participant Builder
User->>CLI: lark-cli mail +send-receipt --message-id=MSG --from=ME
CLI->>API: GET /messages?message_id=MSG
API-->>CLI: message + label_ids
alt message has READ_RECEIPT_REQUEST
CLI->>CLI: requireSenderForRequestReceipt(resolve sender)
CLI->>CLI: build localized subject/body & thread headers
CLI->>Builder: set From/To/Subject, In-Reply-To, References
Builder->>Builder: set Disposition-Notification-To & IsReadReceiptMail
Builder-->>CLI: raw EML
CLI->>API: POST /drafts.create (raw EML)
API-->>CLI: draft id
CLI->>API: POST /drafts.send (draft id)
API-->>CLI: send confirmation
CLI->>User: print receipt_for_message_id
else no request label
CLI-->>User: abort (no receipt request label)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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/emlbuilder/builder.go`:
- Around line 301-314: The DispositionNotificationTo method currently validates
the display name but not the addr, allowing CR/LF header injection via
mail.Address.String(); update Builder.DispositionNotificationTo to call
validateHeaderValue(addr) (set b.err and return on failure) before appending the
address, mirror the same validation for any other address-appending builders
that serialize via joinAddresses, and add a regression test similar to
TestBuild_DispositionNotificationTo_CRLFRejected that supplies a
CR/LF-containing addr to assert it is rejected.
In `@shortcuts/mail/mail_draft_create.go`:
- Around line 157-159: Add a shortcut-level unit test in
mail_draft_create_test.go that exercises the mail draft create flow with the
--request-receipt flag to ensure runtime.Bool("request-receipt") actually causes
bld.DispositionNotificationTo("", senderEmail) to be set; implement the test to
invoke the same CLI or handler path used by the draft-create shortcut (or call
the function that constructs the builder), supplying senderEmail and the
--request-receipt flag, then assert the produced EML includes a
Disposition-Notification-To header equal to senderEmail (or that the builder
serialized header matches), mirroring the behavior covered by
emlbuilder/builder_test.go but at the shortcut level.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 103-109: When request-receipt is specified but no draft sender can
be resolved, fail early instead of silently returning an empty patch: in
buildDraftEditPatch (and the counterpart code path in Execute that currently
appends the Disposition-Notification-To header only when draftFromEmail != ""),
detect runtime.Bool("request-receipt") && draftFromEmail == "" and return a
validation error indicating the request-receipt flag requires a draft From
address; update callers to propagate that error so the command reports failure
rather than a no-op success.
In `@skills/lark-mail/references/lark-mail-send-receipt.md`:
- Line 13: The confirmation rule for the +send-receipt command incorrectly
mentions the unsupported flag --confirm-send; update the documentation to remove
any reference to --confirm-send and instruct users to use --yes (as enforced by
the high-risk-write rule) so examples, parameter tables, and the +send-receipt
confirmation text consistently reference only --yes.
🪄 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: 3c45ec8e-de67-4c08-ae9d-49da71322420
📒 Files selected for processing (17)
shortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/mail/mail_thread.go (1)
96-124: Consider folding the post-output hint loop into the existing items loop.Lines 114-124 re-iterate
itemsand repeat the same envelope/inner unwrap already done at 97-106. You can collect the unwrappedmsgpointers during the first loop and then iterate that slice afterruntime.Out, eliminating the duplicated unwrap logic. Keeping the hints afterruntime.Out(so stdout JSON is emitted before stderr hints) is preserved.♻️ Proposed refactor
outs := make([]map[string]interface{}, 0, len(items)) + msgs := make([]map[string]interface{}, 0, len(items)) for _, item := range items { envelope, ok := item.(map[string]interface{}) if !ok { continue } - // Old API wraps each message inside a "message" sub-object; new API puts fields directly. msg := envelope if inner, ok := envelope["message"].(map[string]interface{}); ok { msg = inner } outs = append(outs, buildMessageOutput(msg, html)) + msgs = append(msgs, msg) } - // Sort by internal_date ascending. messages := sortThreadMessagesByInternalDate(outs) runtime.Out(mailThreadOutput{ThreadID: threadID, MessageCount: len(messages), Messages: messages}, nil) - for _, item := range items { - envelope, ok := item.(map[string]interface{}) - if !ok { - continue - } - msg := envelope - if inner, ok := envelope["message"].(map[string]interface{}); ok { - msg = inner - } - maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg) + for _, msg := range msgs { + maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_thread.go` around lines 96 - 124, The second loop re-parses each envelope; instead, in the first loop collect both the built outputs and the unwrapped message maps (e.g., append msg to a slice of map[string]interface{} alongside using buildMessageOutput) so you can call sortThreadMessagesByInternalDate and runtime.Out(mailThreadOutput{...}) and then iterate the collected unwrapped messages to call maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg). Keep the runtime.Out call before emitting hints to preserve output ordering and reuse the existing envelope/inner unwrap logic (functions referenced: buildMessageOutput, sortThreadMessagesByInternalDate, runtime.Out, maybeHintReadReceiptRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-mail/SKILL.md`:
- Line 330: The inline code span for the English prefix in the "+send-receipt"
table cell contains a trailing space (`Read Receipt: `) which triggers
markdownlint MD038; update the SKILL.md entry for `+send-receipt` to remove the
literal trailing space in the backticks by replacing it with an HTML space
entity (e.g. use `Read Receipt: `) or reformat the sentence so the trailing
space isn't inside the code span, ensuring the displayed text still includes the
space but the linter no longer flags it.
---
Nitpick comments:
In `@shortcuts/mail/mail_thread.go`:
- Around line 96-124: The second loop re-parses each envelope; instead, in the
first loop collect both the built outputs and the unwrapped message maps (e.g.,
append msg to a slice of map[string]interface{} alongside using
buildMessageOutput) so you can call sortThreadMessagesByInternalDate and
runtime.Out(mailThreadOutput{...}) and then iterate the collected unwrapped
messages to call maybeHintReadReceiptRequest(runtime, mailboxID,
strVal(msg["message_id"]), msg). Keep the runtime.Out call before emitting hints
to preserve output ordering and reuse the existing envelope/inner unwrap logic
(functions referenced: buildMessageOutput, sortThreadMessagesByInternalDate,
runtime.Out, maybeHintReadReceiptRequest).
🪄 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: de816432-eb92-457f-ad0c-68d3f3e72fa9
📒 Files selected for processing (8)
shortcuts/mail/helpers.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-send-receipt.md
✅ Files skipped from review due to trivial changes (2)
- shortcuts/mail/mail_send_receipt_test.go
- skills/lark-mail/references/lark-mail-send-receipt.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/mail/mail_send_receipt.go
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
ed27e7b to
3753ca0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
skills/lark-mail/SKILL.md (1)
330-330:⚠️ Potential issue | 🟡 MinorMD038: trailing space inside code span
`Read Receipt: `.Already flagged previously; replace the trailing space with
 (`Read Receipt: `) or reword to keep the literal space out of the backticks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/SKILL.md` at line 330, The markdown contains a code span with a trailing space in the literal `Read Receipt: ` (in the +send-receipt row) which triggers MD038; update the SKILL.md entry for `+send-receipt` to remove the trailing space inside backticks by either replacing the space with an HTML space entity (e.g., `Read Receipt: `) or rewording the sentence so the final space is not inside the code span, and ensure the subject prefix examples use the corrected code span.skills/lark-mail/references/lark-mail-send-receipt.md (1)
13-13:⚠️ Potential issue | 🟡 MinorRemove
--confirm-sendfrom the confirmation rule.The shortcut declares only
--message-id/--mailbox/--fromflags and is gated by thehigh-risk-writeframework which enforces--yes.--confirm-sendisn't a valid flag here and contradicts the parameter table on line 36 and the examples above. Change to reference only--yes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-send-receipt.md` at line 13, The confirmation-rule text currently references a non-existent flag `--confirm-send`; update the sentence so it only requires the existing `--yes` flag (since this shortcut only exposes `--message-id`, `--mailbox`, `--from` and is gated by the `high-risk-write` framework). Remove `--confirm-send` from the rule and any examples or descriptive text that mention it, ensuring the text and the parameter table/usage examples consistently reference `--yes` as the required confirmation flag.
🧹 Nitpick comments (1)
shortcuts/mail/helpers.go (1)
64-72: Suggested command in hint isn't shell-safe for values with spaces/special chars.
mailboxIDandmessageIDare interpolated with%sand only pass throughsanitizeForTerminal, which strips ANSI/CR/dangerous unicode but does not shell-escape. If a message ID ever contains whitespace or shell metacharacters, a user copy-pasting the printed command will parse it incorrectly. Consider wrapping the values in single quotes (matching the style used byhintSendDraft/hintMarkAsReadwhich embed values in single-quoted JSON), e.g.--mailbox '%s' --message-id '%s'. Low risk given message IDs are typically opaque tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 64 - 72, The printed hint in hintReadReceiptRequest currently interpolates mailboxID and messageID via sanitizeForTerminal but does not shell-quote them, so values with spaces or metacharacters can break a copy-pasted command; update the format string in hintReadReceiptRequest to wrap the mailbox and message-id arguments in single quotes (matching hintSendDraft/hintMarkAsRead style) and ensure you still call sanitizeForTerminal on mailboxID and messageID before embedding them.
🤖 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_forward.go`:
- Around line 128-130: When runtime.Bool("request-receipt") is set but
senderEmail is empty the code silently skips calling
bld.DispositionNotificationTo; add a defensive validation in the same scope that
checks if runtime.Bool("request-receipt") && senderEmail == "" (and
orig.headTo/other lookup produced no address) and return an explicit error (or
non-zero exit) indicating that a sender/from address is required for
--request-receipt; ensure this check occurs before attempting
bld.DispositionNotificationTo so callers see a clear failure instead of a silent
no-op.
---
Duplicate comments:
In `@skills/lark-mail/references/lark-mail-send-receipt.md`:
- Line 13: The confirmation-rule text currently references a non-existent flag
`--confirm-send`; update the sentence so it only requires the existing `--yes`
flag (since this shortcut only exposes `--message-id`, `--mailbox`, `--from` and
is gated by the `high-risk-write` framework). Remove `--confirm-send` from the
rule and any examples or descriptive text that mention it, ensuring the text and
the parameter table/usage examples consistently reference `--yes` as the
required confirmation flag.
In `@skills/lark-mail/SKILL.md`:
- Line 330: The markdown contains a code span with a trailing space in the
literal `Read Receipt: ` (in the +send-receipt row) which triggers MD038; update
the SKILL.md entry for `+send-receipt` to remove the trailing space inside
backticks by either replacing the space with an HTML space entity (e.g., `Read
Receipt: `) or rewording the sentence so the final space is not inside the
code span, and ensure the subject prefix examples use the corrected code span.
---
Nitpick comments:
In `@shortcuts/mail/helpers.go`:
- Around line 64-72: The printed hint in hintReadReceiptRequest currently
interpolates mailboxID and messageID via sanitizeForTerminal but does not
shell-quote them, so values with spaces or metacharacters can break a
copy-pasted command; update the format string in hintReadReceiptRequest to wrap
the mailbox and message-id arguments in single quotes (matching
hintSendDraft/hintMarkAsRead style) and ensure you still call
sanitizeForTerminal on mailboxID and messageID before embedding them.
🪄 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: ac0699e9-867d-4aef-a7a0-11555aab6207
📒 Files selected for processing (21)
shortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (8)
- shortcuts/mail/shortcuts.go
- shortcuts/mail/mail_draft_create.go
- shortcuts/mail/mail_reply.go
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-send.md
- shortcuts/mail/emlbuilder/builder_test.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/mail_send_receipt_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/mail/mail_messages.go
- skills/lark-mail/references/lark-mail-forward.md
- shortcuts/mail/mail_send.go
- shortcuts/mail/mail_thread.go
- skills/lark-mail/references/lark-mail-reply.md
- shortcuts/mail/emlbuilder/builder.go
- shortcuts/mail/mail_send_receipt.go
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
3753ca0 to
2d26f9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
shortcuts/mail/mail_send_receipt.go (1)
195-220: Optional:buildReceiptSubjectdoesn't enforce "prepend once".Doc says it "prepends the language-appropriate receipt prefix once", but there's no idempotency check — if the original subject already starts with "已读回执：" or "Read Receipt: " (e.g. a degenerate flow where a receipt gets receipted, or
origSubjectsomehow carries the prefix), the output will double-prefix. This is extremely unlikely in practice (receipts aren't typically receipt-requested) and matchesbuildReplySubject/buildForwardSubjectbehavior inmail_quote.go, so feel free to defer — just flagging that the docstring and the code disagree slightly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 195 - 220, The docstring promises buildReceiptSubject will prepend the language-appropriate receipt prefix only once, but the function currently always prepends it; update buildReceiptSubject to detect the appropriate prefix (use receiptMetaLabels(detectSubjectLang(original)).SubjectPrefix), trim the original subject, and only add the prefix if the trimmed original does not already start with that prefix (case/exact match as used elsewhere); ensure behavior mirrors buildReplySubject/buildForwardSubject by returning the trimmed original unchanged when the prefix is already present.
🤖 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/helpers.go`:
- Around line 70-87: The hint can be line-injected because sanitizeForTerminal
preserves '\n'; update hintReadReceiptRequest and shellQuoteForHint to use a
single-line sanitizer (either add a new singleLineSanitize function or extend
sanitizeForTerminal) that strips or replaces LF characters before further
processing so untrusted fromEmail, subject, mailboxID and messageID cannot
inject new lines; then pass singleLineSanitize(...) into
hintReadReceiptRequest's format arguments and into shellQuoteForHint (which
should call singleLineSanitize before replacing single quotes) so the embedded
hint and shell-quoted payload are always single-line and safe.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 103-112: The synthesized "Disposition-Notification-To" patch op
appended in buildDraftEditPatch/Execute (after requireSenderForRequestReceipt
resolves draftFromEmail) is not being validated; update the code that creates
the draftpkg.PatchOp with Op "set_header" and Name "Disposition-Notification-To"
to immediately validate the resulting patch/op (use patch.Validate() or
equivalent validation helper) after draftFromEmail is known and before appending
to patch.Ops, and return an error if validation fails so malformed or
control-character-containing addresses cannot bypass normal patch validation.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 29-36: The comment on the receiptMetaLabelSet struct incorrectly
describes the To field; update the doc-comment for To to state that it is the
label for the original recipient (the mailbox address that read the message),
not the original sender, to match how buildReceiptTextBody and
buildReceiptHTMLBody receive senderEmail as origRecipient (and avoid confusion
with origFromEmail).
In `@skills/lark-mail/references/lark-mail-send-receipt.md`:
- Around line 47-54: Add a fence language to the Chinese body example code block
(change the opening ``` to ```text) and remove trailing spaces inside the
English inline code spans on the en line so the inline tokens are `Your message
has been read. Details:`, `Subject:`, `To:`, `Sent:`, and `Read:` (no extra
spaces inside the backticks); update the fenced block and the en-line inline
code spans accordingly.
---
Nitpick comments:
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 195-220: The docstring promises buildReceiptSubject will prepend
the language-appropriate receipt prefix only once, but the function currently
always prepends it; update buildReceiptSubject to detect the appropriate prefix
(use receiptMetaLabels(detectSubjectLang(original)).SubjectPrefix), trim the
original subject, and only add the prefix if the trimmed original does not
already start with that prefix (case/exact match as used elsewhere); ensure
behavior mirrors buildReplySubject/buildForwardSubject by returning the trimmed
original unchanged when the prefix is already present.
🪄 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: 86605629-1f4b-4b16-9f45-eb52f592f625
📒 Files selected for processing (23)
shortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (7)
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-reply.md
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-send.md
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/emlbuilder/builder_test.go
- shortcuts/mail/mail_send_receipt_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_draft_create.go
- skills/lark-mail/references/lark-mail-forward.md
- shortcuts/mail/mail_messages.go
- skills/lark-mail/SKILL.md
- shortcuts/mail/mail_thread.go
- shortcuts/mail/mail_forward.go
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
2 times, most recently
from
d54e87f to
4d824b6
Compare
infeng
reviewed
Apr 23, 2026
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
4d824b6 to
374ee76
Compare
infeng
reviewed
Apr 23, 2026
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
374ee76 to
c2ac088
Compare
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_reply_all.go (1)
110-158:⚠️ Potential issue | 🟠 MajorValidate
--request-receiptagainst the resolved sender, not the fallback.Line 112 lets
orig.headTosatisfyrequireSenderForRequestReceipt, so--request-receiptcan proceed even when the sender was not actually resolved. That can put a read-receipt target from the original message intoDisposition-Notification-To; keep the existing fallback forFrom, but require the MDN target to be the explicitly/profile-resolved sender.🔒 Proposed fix
- senderEmail := resolveComposeSenderEmail(runtime) + resolvedSenderEmail := resolveComposeSenderEmail(runtime) + senderEmail := resolvedSenderEmail if senderEmail == "" { senderEmail = orig.headTo } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { + if err := requireSenderForRequestReceipt(runtime, resolvedSenderEmail); err != nil { return err } if runtime.Bool("request-receipt") { - bld = bld.DispositionNotificationTo("", senderEmail) + bld = bld.DispositionNotificationTo("", resolvedSenderEmail) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply_all.go` around lines 110 - 158, The code currently lets orig.headTo satisfy requireSenderForRequestReceipt because senderEmail is set to orig.headTo when resolveComposeSenderEmail(runtime) returns empty; change the logic to keep the fallback for the From header but ensure the MDN/receipt checks and Disposition-Notification-To use only the explicitly resolved sender: call resolveComposeSenderEmail(runtime) into a distinct variable (e.g., resolvedSender), use resolvedSender (not the fallback orig.headTo) when invoking requireSenderForRequestReceipt and when setting DispositionNotificationTo, while still using the existing senderEmail fallback for the From() call and other non-MDN uses.shortcuts/mail/helpers.go (1)
1734-1763:⚠️ Potential issue | 🟠 MajorDrop all terminal control bytes in hint sanitization.
Line 1762 removes LF, but
sanitizeForTerminalstill preserves non-CSI controls such as OSC sequences (ESC ] ... BEL) because only\x1b[escapes are stripped and most C0 runes are written through. Since read-receipt hints include untrusted email/API values, this can still manipulate terminals even without newline injection.🛡️ Proposed hardening
func sanitizeForTerminal(s string) string { s = ansiEscapeRe.ReplaceAllString(s, "") var b strings.Builder b.Grow(len(s)) for _, r := range s { - if r == '\r' { + if r == '\n' || r == '\t' { + b.WriteRune(r) + continue + } + if r < 0x20 || r == 0x7f { continue } if common.IsDangerousUnicode(r) { continue } b.WriteRune(r) } return b.String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 1734 - 1763, sanitizeForTerminal currently only strips CSI sequences via ansiEscapeRe and preserves many C0 control runes (including ESC/OSC), so terminals can still be manipulated; update sanitizeForTerminal to drop all control bytes by skipping runes where unicode.IsControl(r) (or explicit checks for '\x1b' / ESC and other C0s) while still preserving '\n' for multi-line output, and keep sanitizeForSingleLine behavior (it should call the updated sanitizeForTerminal then remove "\n"); reference sanitizeForTerminal, sanitizeForSingleLine, and ansiEscapeRe when making the change.
🧹 Nitpick comments (1)
shortcuts/mail/mail_draft_create.go (1)
178-188: Consider mirroringdraft-edit's explicitvalidateHeaderAddresscheck for consistency.The EML builder's
DispositionNotificationTomethod (line 316 inemlbuilder/builder.go) does validate the address withvalidateHeaderValueto prevent CR/LF injection and visual-spoofing. However,mail_draft_edit.go(lines 111–115) callsvalidateHeaderAddressexplicitly before appendingDisposition-Notification-To, making the intent visible and consistent across both code paths. Adding the same explicit check here would mirror that pattern and wouldn't regress ifresolveComposeSenderEmailever accepts a looser source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create.go` around lines 178 - 188, Add an explicit header address validation before appending the Disposition-Notification-To header: when runtime.Bool("request-receipt") is true, call validateHeaderAddress(senderEmail) (same check used in mail_draft_edit's draft-edit path) and return its error if any, then call bld = bld.DispositionNotificationTo("", senderEmail); keep the existing requireSenderForRequestReceipt(runtime, senderEmail) guard as-is so behavior is unchanged but intent and CR/LF/visual-spoof protection are explicit.
🤖 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_draft_create_test.go`:
- Around line 198-219: The test
TestBuildRawEMLForDraftCreate_RequestReceiptAddsHeader currently asserts only
that "sender@example.com" appears anywhere in the EML; change it to assert the
Disposition-Notification-To header value specifically by parsing the decoded EML
(use the same decodeBase64URL output) or by matching the full
"Disposition-Notification-To:" header line, and verify that its value equals the
sender ("sender@example.com"); update assertions in the test that calls
buildRawEMLForDraftCreate so the check targets the Disposition-Notification-To
header string rather than a generic substring match.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 241-250: The buildReceiptReferences function can append origSMTPID
even when it already appears in origRefs, causing duplicate message-IDs; update
buildReceiptReferences to detect if the SMTP id (normalized without surrounding
angle brackets) is already present in the trimmed origRefs (consider both bare
and <id> forms) and only append "<"+origSMTPID+">" when not found, keeping the
existing trimming and join behavior; use the function name
buildReceiptReferences and variables origRefs/origSMTPID to locate where to add
the presence check and normalization.
In `@skills/lark-mail/references/lark-mail-forward.md`:
- Line 75: Update the `--request-receipt` table row text to mention the
sender-resolution requirement: note that this flag can fail if the sender
address cannot be resolved and the CLI may prompt users to specify `--from`;
include a brief recovery tip telling agents/users to retry with `--from
<sender>` when prompted. Reference the `--request-receipt` flag in the docs and
add the short clarifying sentence immediately after the existing description so
readers see the failure mode and the explicit fix.
---
Outside diff comments:
In `@shortcuts/mail/helpers.go`:
- Around line 1734-1763: sanitizeForTerminal currently only strips CSI sequences
via ansiEscapeRe and preserves many C0 control runes (including ESC/OSC), so
terminals can still be manipulated; update sanitizeForTerminal to drop all
control bytes by skipping runes where unicode.IsControl(r) (or explicit checks
for '\x1b' / ESC and other C0s) while still preserving '\n' for multi-line
output, and keep sanitizeForSingleLine behavior (it should call the updated
sanitizeForTerminal then remove "\n"); reference sanitizeForTerminal,
sanitizeForSingleLine, and ansiEscapeRe when making the change.
In `@shortcuts/mail/mail_reply_all.go`:
- Around line 110-158: The code currently lets orig.headTo satisfy
requireSenderForRequestReceipt because senderEmail is set to orig.headTo when
resolveComposeSenderEmail(runtime) returns empty; change the logic to keep the
fallback for the From header but ensure the MDN/receipt checks and
Disposition-Notification-To use only the explicitly resolved sender: call
resolveComposeSenderEmail(runtime) into a distinct variable (e.g.,
resolvedSender), use resolvedSender (not the fallback orig.headTo) when invoking
requireSenderForRequestReceipt and when setting DispositionNotificationTo, while
still using the existing senderEmail fallback for the From() call and other
non-MDN uses.
---
Nitpick comments:
In `@shortcuts/mail/mail_draft_create.go`:
- Around line 178-188: Add an explicit header address validation before
appending the Disposition-Notification-To header: when
runtime.Bool("request-receipt") is true, call validateHeaderAddress(senderEmail)
(same check used in mail_draft_edit's draft-edit path) and return its error if
any, then call bld = bld.DispositionNotificationTo("", senderEmail); keep the
existing requireSenderForRequestReceipt(runtime, senderEmail) guard as-is so
behavior is unchanged but intent and CR/LF/visual-spoof protection are explicit.
🪄 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: a8f6d9d6-5a6e-4829-abed-551701be99d3
📒 Files selected for processing (26)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (7)
- skills/lark-mail/references/lark-mail-reply-all.md
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-send.md
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/draft/service.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/mail/mail_messages.go
- shortcuts/mail/mail_forward.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_send.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- skills/lark-mail/SKILL.md
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
c2ac088 to
28cb170
Compare
xzcong0820
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
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_reply_all.go (1)
113-171:⚠️ Potential issue | 🟡 MinorAdd integration test covering
+reply-all --request-receiptwith the pre-fallback sender guard.The code correctly validates the sender before falling back to
orig.headTo(lines 114–119), but this behavior change lacks shortcut-level test coverage. Existing tests (mail_send_time_integration_test.go,mail_reply_forward_inline_test.go,mail_confirm_send_scope_test.go) invokeMailReplyAllwithout the--request-receiptflag, and the isolated unit test inhelpers_test.go:1292covers only the validation function. Per coding guidelines, every behavior change must have an accompanying test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply_all.go` around lines 113 - 171, Add an integration test that exercises MailReplyAll invoked with the --request-receipt flag to verify the pre-fallback sender guard in requireSenderForRequestReceipt; specifically, write two cases: (1) simulate resolveComposeSenderEmail returning empty and assert MailReplyAll fails with the expected error (no fallback to orig.headTo), and (2) simulate resolveComposeSenderEmail returning a valid address and assert the built message includes Disposition-Notification-To set to that resolved sender (use the CLI invocation or call MailReplyAll and inspect the eml/emlbuilder output). Reference resolveComposeSenderEmail, requireSenderForRequestReceipt, MailReplyAll (or the command handler) and the emlbuilder.DispositionNotificationTo behavior when asserting results. Ensure the test is added to the integration test suite alongside existing mail_* integration tests.
♻️ Duplicate comments (1)
shortcuts/mail/mail_draft_create_test.go (1)
212-218:⚠️ Potential issue | 🟡 MinorAssert the DNT header value, not just any sender occurrence.
sender@example.comalready appears in theFrom:header, so this test can pass even ifDisposition-Notification-Tois empty or targets the wrong address. Match the full header line or parse the header value.Proposed tightening
- // Case-insensitive header match — net/mail uses canonical casing on write. - if !strings.Contains(eml, "Disposition-Notification-To:") { - t.Errorf("expected Disposition-Notification-To header when --request-receipt set; got EML:\n%s", eml) - } - if !strings.Contains(eml, "sender@example.com") { - t.Errorf("expected receipt address to be the sender; got EML:\n%s", eml) - } + if !strings.Contains(eml, "Disposition-Notification-To: <sender@example.com>") { + t.Errorf("expected Disposition-Notification-To targeting sender@example.com; got EML:\n%s", eml) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 212 - 218, The test currently checks for any occurrence of "sender@example.com" in eml which can be a false positive; update the assertion to verify the Disposition-Notification-To header value explicitly by matching the full header line or parsing headers from eml (e.g., assert strings.Contains(eml, "Disposition-Notification-To: sender@example.com") or parse with net/mail and compare msg.Header.Get("Disposition-Notification-To") == "sender@example.com") in the test case that builds eml so the DNT header is validated, not just the presence of the sender elsewhere.
🤖 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/emlbuilder/builder_test.go`:
- Around line 388-397: In TestBuild_DispositionNotificationTo_AddrBidiRejected
replace the literal U+202E right-to-left override characters used both in the
comment and in the DispositionNotificationTo test data with the escaped Unicode
form "\u202e" (matching the style used in shortcuts/mail/helpers_test.go) so the
comment and the string argument use "\\u202e" instead of embedding the raw
control character; update occurrences around the comment mentioning gmailcom
and the string "alice@gmail.com" referenced in the DispositionNotificationTo
call.
In `@shortcuts/mail/mail_send.go`:
- Around line 112-117: Add a shortcut-level test that exercises the +send
shortcut with the --request-receipt flag and verifies the produced EML contains
a Disposition-Notification-To header set to the resolved sender; follow the
pattern used in mail_draft_create_test.go. Specifically, write a test (e.g., in
mail_send_test.go) that sets up the runtime to resolve a sender email, invokes
the +send shortcut with runtime.Bool("request-receipt")/the flag enabled, and
asserts the output EML contains a Disposition-Notification-To header matching
the resolved sender (this verifies the path that uses
requireSenderForRequestReceipt and bld.DispositionNotificationTo). Ensure the
test covers the integration wiring rather than just the builder/helper level.
---
Outside diff comments:
In `@shortcuts/mail/mail_reply_all.go`:
- Around line 113-171: Add an integration test that exercises MailReplyAll
invoked with the --request-receipt flag to verify the pre-fallback sender guard
in requireSenderForRequestReceipt; specifically, write two cases: (1) simulate
resolveComposeSenderEmail returning empty and assert MailReplyAll fails with the
expected error (no fallback to orig.headTo), and (2) simulate
resolveComposeSenderEmail returning a valid address and assert the built message
includes Disposition-Notification-To set to that resolved sender (use the CLI
invocation or call MailReplyAll and inspect the eml/emlbuilder output).
Reference resolveComposeSenderEmail, requireSenderForRequestReceipt,
MailReplyAll (or the command handler) and the
emlbuilder.DispositionNotificationTo behavior when asserting results. Ensure the
test is added to the integration test suite alongside existing mail_*
integration tests.
---
Duplicate comments:
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 212-218: The test currently checks for any occurrence of
"sender@example.com" in eml which can be a false positive; update the assertion
to verify the Disposition-Notification-To header value explicitly by matching
the full header line or parsing headers from eml (e.g., assert
strings.Contains(eml, "Disposition-Notification-To: sender@example.com") or
parse with net/mail and compare msg.Header.Get("Disposition-Notification-To") ==
"sender@example.com") in the test case that builds eml so the DNT header is
validated, not just the presence of the sender elsewhere.
🪄 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: 04568eea-7c03-4c1f-a6f9-fb6e38a717d2
📒 Files selected for processing (26)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (10)
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-draft-create.md
- shortcuts/mail/shortcuts.go
- shortcuts/mail/mail_reply.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- skills/lark-mail/references/lark-mail-send.md
- skills/lark-mail/references/lark-mail-forward.md
- shortcuts/mail/draft/service.go
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/helpers.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_thread.go
- skills/lark-mail/SKILL.md
- shortcuts/mail/mail_send_receipt.go
- shortcuts/mail/mail_forward.go
- shortcuts/mail/helpers_test.go
- shortcuts/mail/mail_send_receipt_test.go
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
28cb170 to
3d9435f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/mail/mail_draft_create_test.go (1)
198-219:⚠️ Potential issue | 🟡 MinorAssertion still only proves the sender string appears somewhere in the EML.
strings.Contains(eml, "sender@example.com")passes even ifDisposition-Notification-Towere wired to an empty address or a different address — theFrom:header alone satisfies it. Match the full header value instead so a regression that drops or misroutes the receipt address is actually caught.🧪 Proposed tightening
- // Case-insensitive header match — net/mail uses canonical casing on write. - if !strings.Contains(eml, "Disposition-Notification-To:") { - t.Errorf("expected Disposition-Notification-To header when --request-receipt set; got EML:\n%s", eml) - } - if !strings.Contains(eml, "sender@example.com") { - t.Errorf("expected receipt address to be the sender; got EML:\n%s", eml) - } + if !strings.Contains(eml, "Disposition-Notification-To: <sender@example.com>") && + !strings.Contains(eml, "Disposition-Notification-To: sender@example.com") { + t.Errorf("expected Disposition-Notification-To targeting sender@example.com; got EML:\n%s", eml) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 198 - 219, Update the test TestBuildRawEMLForDraftCreate_RequestReceiptAddsHeader so it asserts the full Disposition-Notification-To header value instead of merely checking for the sender string anywhere in the EML: after decoding with decodeBase64URL(eml) search for the canonical header line (e.g. "Disposition-Notification-To: sender@example.com") or use a case-insensitive line/regex match to verify the header exists with the exact sender address produced by buildRawEMLForDraftCreate; this ensures the Disposition-Notification-To header is present and contains the correct address rather than relying on the From: header to satisfy the check.shortcuts/mail/mail_send_receipt.go (1)
29-36:⚠️ Potential issue | 🟡 MinorStruct doc for
Tostill describes the wrong address.The body's
To:line rendersorigRecipient, which theExecutepath wires tosenderEmailat L136–137 — i.e., the mailbox/reader who is sending the receipt, not the original sender. The receipt's envelope-To (set at L142 viaorigFromEmail) is a separate concern. A future maintainer reading this comment is likely to "fix" the call site to passorigFromEmail, which would flip the semantics and break the existing receipt-body tests.📝 Proposed doc fix
- To string // label for the address the receipt is sent to (= original mail's sender) + To string // label for the original recipient (i.e. the reader sending this receipt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 29 - 36, Update the struct field comment for receiptMetaLabelSet.To to accurately describe that the receipt body's "To:" line shows the mailbox/reader who is sending the receipt (the origRecipient wired to senderEmail in Execute), not the original sender; mention that the envelope recipient (origFromEmail) is set separately when sending the message so maintainers don't confuse body semantics with envelope-To.
🧹 Nitpick comments (1)
shortcuts/mail/mail_send_receipt_test.go (1)
78-79: Test comment contradicts the production behavior it is asserting.The comment here says "Language re-detected after strip; once CJK is stripped en prefix wins", but the assertion is the opposite (CJK wins → zh prefix), and
buildReceiptSubjectexplicitly detects language on the original subject before stripping (see the comment inmail_send_receipt.goaround L222–225). The test case is correct — only the explanatory comment is misleading.📝 Proposed tweak
- // Language re-detected after strip; once CJK is stripped en prefix wins. + // Language is detected on the ORIGINAL (pre-strip) subject, so CJK in + // the tail wins even when the stripped prefix was English. {"Read Receipt: 测试", "已读回执：测试"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt_test.go` around lines 78 - 79, The comment above the test case asserting "Read Receipt: 测试" is incorrect — update the explanatory comment to state that buildReceiptSubject detects language on the original subject before stripping, so CJK characters cause a zh prefix to be chosen (CJK wins), matching the test assertion; locate the test case near the {"Read Receipt: 测试", "已读回执：测试"} entry in mail_send_receipt_test.go and replace the misleading sentence with one that reflects buildReceiptSubject's actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 198-219: Update the test
TestBuildRawEMLForDraftCreate_RequestReceiptAddsHeader so it asserts the full
Disposition-Notification-To header value instead of merely checking for the
sender string anywhere in the EML: after decoding with decodeBase64URL(eml)
search for the canonical header line (e.g. "Disposition-Notification-To:
sender@example.com") or use a case-insensitive line/regex match to verify the
header exists with the exact sender address produced by
buildRawEMLForDraftCreate; this ensures the Disposition-Notification-To header
is present and contains the correct address rather than relying on the From:
header to satisfy the check.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 29-36: Update the struct field comment for receiptMetaLabelSet.To
to accurately describe that the receipt body's "To:" line shows the
mailbox/reader who is sending the receipt (the origRecipient wired to
senderEmail in Execute), not the original sender; mention that the envelope
recipient (origFromEmail) is set separately when sending the message so
maintainers don't confuse body semantics with envelope-To.
---
Nitpick comments:
In `@shortcuts/mail/mail_send_receipt_test.go`:
- Around line 78-79: The comment above the test case asserting "Read Receipt:
测试" is incorrect — update the explanatory comment to state that
buildReceiptSubject detects language on the original subject before stripping,
so CJK characters cause a zh prefix to be chosen (CJK wins), matching the test
assertion; locate the test case near the {"Read Receipt: 测试", "已读回执：测试"} entry
in mail_send_receipt_test.go and replace the misleading sentence with one that
reflects buildReceiptSubject's actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 259104db-a4f9-4f73-a9ca-c83b2aef7a38
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (5)
- skills/lark-mail/references/lark-mail-send.md
- skills/lark-mail/references/lark-mail-forward.md
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-draft-create.md
- shortcuts/mail/draft/service.go
🚧 Files skipped from review as they are similar to previous changes (9)
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/mail_forward.go
- shortcuts/mail/mail_thread.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_reply_all.go
- skills/lark-mail/SKILL.md
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_draft_edit.go
- shortcuts/mail/helpers_test.go
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
3d9435f to
3caa448
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_draft_edit.go (1)
60-73:⚠️ Potential issue | 🟡 MinorInclude
--request-receiptin the dry-run preview.For receipt-only edits,
buildDraftEditPatchreturns an empty patch and the realset_headerop is appended later inExecute, so dry-run reports an empty_patchwhile the actual command updatesDisposition-Notification-To.🧭 Proposed preview hint
- return common.NewDryRunAPI(). + api := common.NewDryRunAPI(). Desc("Edit an existing draft without sending it: first call drafts.get(format=raw) to fetch the current EML, parse it into MIME structure, apply either direct flags or the typed patch from patch-file, re-serialize the updated draft, and then call drafts.update. This is a minimal-edit pipeline rather than a full rebuild, so unchanged headers, attachments, and MIME subtrees are preserved where possible. Body edits must go through --patch-file using set_body or set_reply_body ops. It also has no optimistic locking, so concurrent edits to the same draft are last-write-wins."). GET(mailboxPath(mailboxID, "drafts", draftID)). Params(map[string]interface{}{"format": "raw"}). PUT(mailboxPath(mailboxID, "drafts", draftID)). Body(map[string]interface{}{ "raw": "<base64url-EML>", "_patch": patch.Summary(), "_notice": "This edit flow has no optimistic locking. If the same draft is changed concurrently, the last writer wins.", }) + if runtime.Bool("request-receipt") { + api = api.Set("_request_receipt", "will set Disposition-Notification-To from the draft From header after fetching the draft") + } + return apiAlso applies to: 349-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_edit.go` around lines 60 - 73, The dry-run preview currently shows an empty _patch because buildDraftEditPatch returns no ops for receipt-only edits and Execute appends the set_header for Disposition-Notification-To later; update the dry-run generation in the same code that calls buildDraftEditPatch so that when the request-receipt flag (or equivalent) is set you synthesize the corresponding set_header op into the preview (or include a clear _notice) so patch.Summary() reflects the Disposition-Notification-To change; ensure this mirrors the same logic used in Execute that appends the header op so the GET/PUT dry-run output accurately represents receipt-only edits.shortcuts/mail/helpers.go (1)
1769-1801:⚠️ Potential issue | 🟠 MajorStrip non-CSI terminal control sequences too.
ansiEscapeReonly removes CSI escapes (ESC [). Values printed by the new receipt hint still pass throughsanitizeForSingleLine, so an OSC sequence likeESC ] ... BELcan survive becausesanitizeForTerminalcurrently writes ESC/BEL and other C0 controls back out. Strip all non-printing controls except the intentional LF/tab cases.🛡️ Proposed hardening
-// ansiEscapeRe matches ANSI CSI escape sequences (ESC '[' ... <final byte>). -var ansiEscapeRe = regexp.MustCompile(`\x1b\[[0-9;]*[a-zA-Z]`) +// ansiEscapeRe matches ANSI CSI and OSC escape sequences. +var ansiEscapeRe = regexp.MustCompile(`\x1b(?:\[[0-?]*[ -/]*[`@-`~]|\][^\x07\x1b]*(?:\x07|\x1b\\))`) @@ for _, r := range s { - if r == '\r' { - continue - } - if common.IsDangerousUnicode(r) { + switch { + case r == '\n' || r == '\t': + b.WriteRune(r) + case r < 0x20 || r == 0x7f: + continue + case common.IsDangerousUnicode(r): continue + default: + b.WriteRune(r) } - b.WriteRune(r) } return b.String() }Add a regression case alongside
TestSanitizeForSingleLine:{"strips ANSI + LF", "\x1b[31mred\x1b[0m\nnext", "rednext"}, + {"strips OSC hyperlink", "\x1b]8;;https://evil.example\x07click\x1b]8;;\x07", "click"}, {"keeps tab", "a\tb", "a\tb"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 1769 - 1801, The current ansiEscapeRe only removes CSI sequences (ESC '[' ...) but sanitizeForTerminal still emits other non-printing controls (OSC, BEL, DEL, other C0s); update sanitizeForTerminal to drop all non-printing control runes except intentional LF ('\n') and TAB ('\t') and extend the ESC stripping to cover non-CSI sequences (e.g., OSC ESC ] ... BEL, ST ESC \, BEL '\x07', and any ESC ... final-byte sequences) so BEL/OSC/DEL don't survive; specifically, change the ansiEscapeRe to match broader ESC sequences and/or add logic in sanitizeForTerminal to skip runes where r < 0x20 && r != '\n' && r != '\t', skip r == 0x7f (DEL), and continue to call common.IsDangerousUnicode(r) so sanitizeForSingleLine (which wraps sanitizeForTerminal) guarantees only printable characters remain.
♻️ Duplicate comments (3)
shortcuts/mail/mail_draft_create_test.go (1)
212-218:⚠️ Potential issue | 🟡 MinorAssert the header value, not just the sender substring.
sender@example.comalready appears inFrom, so this test can pass even ifDisposition-Notification-Tois empty or points elsewhere. Match the full header line or parse the header value.🧪 Proposed tightening
- // Case-insensitive header match — net/mail uses canonical casing on write. - if !strings.Contains(eml, "Disposition-Notification-To:") { - t.Errorf("expected Disposition-Notification-To header when --request-receipt set; got EML:\n%s", eml) - } - if !strings.Contains(eml, "sender@example.com") { - t.Errorf("expected receipt address to be the sender; got EML:\n%s", eml) - } + if !strings.Contains(eml, "Disposition-Notification-To: <sender@example.com>") && + !strings.Contains(eml, "Disposition-Notification-To: sender@example.com") { + t.Errorf("expected Disposition-Notification-To targeting sender@example.com; got EML:\n%s", eml) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 212 - 218, The test is only checking for the substring "sender@example.com" which may come from the From header; replace the loose substring check with a strict assertion of the Disposition-Notification-To header value by either (a) checking for the full header line "Disposition-Notification-To: sender@example.com" in the EML string or (b) parsing eml into a mail.Message (using net/mail) and asserting msg.Header.Get("Disposition-Notification-To") == "sender@example.com"; update the test assertions near the existing checks for Disposition-Notification-To and sender to use one of these approaches so the header value is validated directly.shortcuts/mail/mail_send_receipt.go (2)
29-35:⚠️ Potential issue | 🟡 MinorFix the
Tocomment to match the generated receipt body.The body builders receive
senderEmailas the original recipient/reader, while the receipt itself is sent toorigFromEmail. This comment still describes the receipt destination and can lead to swapping the wrong address later.📝 Proposed doc fix
- To string // label for the address the receipt is sent to (= original mail's sender) + To string // label for the original recipient / reader shown in the receipt body🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 29 - 35, Update the comment on the receiptMetaLabelSet field To to correctly state it is the label for the original recipient/reader address (the value passed to body builders as senderEmail), not the receipt delivery address; note that the actual receipt is sent to origFromEmail while body builders use senderEmail. Reference: struct receiptMetaLabelSet and field To.
91-101:⚠️ Potential issue | 🟡 MinorKeep the dry-run profile call in lockstep with sender resolution.
Executeonly fetches profile when sender resolution falls back toprofile("me"); with--fromor a non-me--mailbox, it skips that API call. The dry-run currently always advertisesGET /profileformailboxID, including shared mailbox / alias cases where the real path will not call it.🔧 Proposed adjustment
DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { messageId := runtime.Str("message-id") mailboxID := resolveComposeMailboxID(runtime) - return common.NewDryRunAPI(). + api := common.NewDryRunAPI(). Desc("Send read receipt: fetch the original message → verify the READ_RECEIPT_REQUEST label is present → build a reply with subject \"已读回执：<original>\" (zh) or \"Read Receipt: <original>\" (en) picked by CJK detection on the original subject, In-Reply-To / References threading, and X-Lark-Read-Receipt-Mail: 1 → create draft and send. The backend extracts the private header, sets BodyExtra.IsReadReceiptMail, and DraftSend applies the READ_RECEIPT_SENT label to the outgoing message."). GET(mailboxPath(mailboxID, "messages", messageId)). - Params(map[string]interface{}{"format": messageGetFormat(false)}). - GET(mailboxPath(mailboxID, "profile")). + Params(map[string]interface{}{"format": messageGetFormat(false)}) + if runtime.Str("from") == "" && (runtime.Str("mailbox") == "" || runtime.Str("mailbox") == "me") { + api = api.GET(mailboxPath("me", "profile")) + } + return api. POST(mailboxPath(mailboxID, "drafts")). Body(map[string]interface{}{"raw": "<base64url-EML>"}). POST(mailboxPath(mailboxID, "drafts", "<draft_id>", "send")) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 91 - 101, The DryRun implementation for the SendReadReceipt shortcut (the DryRun func that builds the chain using mailboxPath(mailboxID, "messages", ...), resolveComposeMailboxID(runtime) and the GET(mailboxPath(mailboxID, "profile")) call) always includes the profile GET even when Execute would skip it for resolved senders (e.g. when --from or non-"me" mailbox is used); update DryRun to mirror the sender-resolution logic used in Execute: determine whether sender resolution falls back to profile("me") and only append GET(mailboxPath(mailboxID, "profile")) when that same condition is true, keeping mailboxPath, resolveComposeMailboxID, and messageGetFormat(false) usage intact.
🤖 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/emlbuilder/builder.go`:
- Around line 311-324: The display name passed to Disposition-Notification-To is
only checked by validateDisplayName (which currently rejects CR/LF) but not for
dangerous Unicode like Bidi (U+202E) or zero-width controls; update the
DispositionNotificationTo builder path to also run the same header-value
hardening used for addr (i.e. call validateHeaderValue(name) or incorporate the
dangerous-Unicode checks into validateDisplayName) before appending to
cp.dispositionNotificationTo, and add a regression test that calls
DispositionNotificationTo("Alice\u202e", "alice@example.com") expecting an error
for dangerous Unicode.
In `@shortcuts/mail/mail_forward.go`:
- Around line 132-137: Add a unit test for the +forward shortcut that enables
the --request-receipt flag and verifies the forwarded draft EML contains a
Disposition-Notification-To header set to the resolved sender (which for
+forward should fall back to the original message's To). Create a test that
builds an original message with a To address, invoke the forward shortcut with
no explicit sender (so requireSenderForRequestReceipt/runtime fallback logic is
exercised), run the shortcut to produce the draft, and assert the produced EML
includes Disposition-Notification-To: <original-To-address> by inspecting the
builder output produced via DispositionNotificationTo; place the test alongside
existing mail tests following the pattern in mail_draft_create_test.go and name
it to reflect testing request-receipt sender fallback for +forward.
---
Outside diff comments:
In `@shortcuts/mail/helpers.go`:
- Around line 1769-1801: The current ansiEscapeRe only removes CSI sequences
(ESC '[' ...) but sanitizeForTerminal still emits other non-printing controls
(OSC, BEL, DEL, other C0s); update sanitizeForTerminal to drop all non-printing
control runes except intentional LF ('\n') and TAB ('\t') and extend the ESC
stripping to cover non-CSI sequences (e.g., OSC ESC ] ... BEL, ST ESC \, BEL
'\x07', and any ESC ... final-byte sequences) so BEL/OSC/DEL don't survive;
specifically, change the ansiEscapeRe to match broader ESC sequences and/or add
logic in sanitizeForTerminal to skip runes where r < 0x20 && r != '\n' && r !=
'\t', skip r == 0x7f (DEL), and continue to call common.IsDangerousUnicode(r) so
sanitizeForSingleLine (which wraps sanitizeForTerminal) guarantees only
printable characters remain.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 60-73: The dry-run preview currently shows an empty _patch because
buildDraftEditPatch returns no ops for receipt-only edits and Execute appends
the set_header for Disposition-Notification-To later; update the dry-run
generation in the same code that calls buildDraftEditPatch so that when the
request-receipt flag (or equivalent) is set you synthesize the corresponding
set_header op into the preview (or include a clear _notice) so patch.Summary()
reflects the Disposition-Notification-To change; ensure this mirrors the same
logic used in Execute that appends the header op so the GET/PUT dry-run output
accurately represents receipt-only edits.
---
Duplicate comments:
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 212-218: The test is only checking for the substring
"sender@example.com" which may come from the From header; replace the loose
substring check with a strict assertion of the Disposition-Notification-To
header value by either (a) checking for the full header line
"Disposition-Notification-To: sender@example.com" in the EML string or (b)
parsing eml into a mail.Message (using net/mail) and asserting
msg.Header.Get("Disposition-Notification-To") == "sender@example.com"; update
the test assertions near the existing checks for Disposition-Notification-To and
sender to use one of these approaches so the header value is validated directly.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 29-35: Update the comment on the receiptMetaLabelSet field To to
correctly state it is the label for the original recipient/reader address (the
value passed to body builders as senderEmail), not the receipt delivery address;
note that the actual receipt is sent to origFromEmail while body builders use
senderEmail. Reference: struct receiptMetaLabelSet and field To.
- Around line 91-101: The DryRun implementation for the SendReadReceipt shortcut
(the DryRun func that builds the chain using mailboxPath(mailboxID, "messages",
...), resolveComposeMailboxID(runtime) and the GET(mailboxPath(mailboxID,
"profile")) call) always includes the profile GET even when Execute would skip
it for resolved senders (e.g. when --from or non-"me" mailbox is used); update
DryRun to mirror the sender-resolution logic used in Execute: determine whether
sender resolution falls back to profile("me") and only append
GET(mailboxPath(mailboxID, "profile")) when that same condition is true, keeping
mailboxPath, resolveComposeMailboxID, and messageGetFormat(false) usage intact.
🪄 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: e0a50c8e-74d0-40e8-b263-b1c385a22c2a
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (9)
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-forward.md
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-draft-edit.md
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/draft/service.go
- shortcuts/mail/mail_send_receipt_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_messages.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_thread.go
- skill-template/domains/mail.md
- shortcuts/mail/mail_reply_all.go
- skills/lark-mail/SKILL.md
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
3caa448 to
b64e2ad
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
shortcuts/mail/mail_send_receipt.go (1)
29-35:⚠️ Potential issue | 🟡 MinorFix the stale
Tofield comment.Line 33 says this label is for the address the receipt is sent to, but the body builders receive
senderEmailas the original recipient/reader. The generated receipt’sTo:line therefore describes the mailbox that read the original message, notorigFromEmail.Proposed doc fix
type receiptMetaLabelSet struct { SubjectPrefix string // "已读回执：" / "Read Receipt: " Lead string // first-line statement in the receipt body Subject string // label for the original mail subject - To string // label for the address the receipt is sent to (= original mail's sender) + To string // label for the original recipient (the reader sending this receipt) Sent string // label for the original send time Read string // label for the current read time (when the receipt was generated) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 29 - 35, The comment for the To field on receiptMetaLabelSet is incorrect: update the To field's comment to state it labels the mailbox that read the original message (the original recipient/reader, passed as senderEmail to the body builders) rather than origFromEmail/original sender. Edit the comment on the To field in receiptMetaLabelSet to mention senderEmail/original recipient/reader so it accurately describes the generated receipt's To: line.
🤖 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_send_receipt_test.go`:
- Around line 149-157: The test currently checks that the generated hint
includes several substrings but omits the required confirmation flag; update the
assertion so the suggested command always contains "--yes" by either adding
"--yes" to the []string of expected substrings in the loop (the slice iterated
over in the test) or by adding a separate explicit assertion that the hint
string contains "--yes" (e.g., assert.Contains(hint, "--yes")). Ensure you
modify the test in shortcuts/mail/mail_send_receipt_test.go where the expected
substrings are defined so the contract "+send-receipt --yes" is enforced.
- Around line 164-191: The test currently only checks LF ("\n") injection;
extend it to assert CR ("\r") and CRLF ("\r\n") cannot open tip lines. Modify
the existing t.Run block that calls maybeHintReadReceiptRequest (and uses msg,
rt, stderr, out) to set subject and head_from.mail_address with variants
containing "\r" and "\r\n" (or add separate subcases) and add assertions that
out does not contain "\rtip: proceed" nor "\r\ntip: proceed" (and still asserts
no "\ntip: proceed"); also ensure the loop over lines still detects only the
legitimate "tip: sender requested a read receipt" line. Use the same symbols
maybeHintReadReceiptRequest, msg, out, stderr in the updated checks.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 91-111: The code is using resolveComposeMailboxID (which falls
back to --from) for message lookup and draft send; replace calls to
resolveComposeMailboxID in DryRun and Execute with the mailbox-only resolver
(e.g., resolveMailboxID or the project’s resolver that does not fallback to
--from) so mailbox lookup/draft send use --mailbox or default "me" while leaving
resolveComposeMailboxID available for selecting the receipt sender header if
needed; update the two locations where resolveComposeMailboxID is called (the
DryRun mailboxID assignment and the Execute mailboxID assignment) to use the
non-fallback resolver.
---
Duplicate comments:
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 29-35: The comment for the To field on receiptMetaLabelSet is
incorrect: update the To field's comment to state it labels the mailbox that
read the original message (the original recipient/reader, passed as senderEmail
to the body builders) rather than origFromEmail/original sender. Edit the
comment on the To field in receiptMetaLabelSet to mention senderEmail/original
recipient/reader so it accurately describes the generated receipt's To: line.
🪄 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: 9950d8ec-a63b-47f9-a600-240cb3986b29
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (11)
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-send.md
- shortcuts/mail/emlbuilder/builder_test.go
- skills/lark-mail/references/lark-mail-forward.md
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/mail_draft_create_test.go
- shortcuts/mail/draft/service.go
- shortcuts/mail/emlbuilder/builder.go
- shortcuts/mail/mail_draft_edit.go
🚧 Files skipped from review as they are similar to previous changes (7)
- skills/lark-mail/references/lark-mail-reply.md
- shortcuts/mail/mail_forward.go
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_thread.go
- skill-template/domains/mail.md
- shortcuts/mail/mail_messages.go
- skills/lark-mail/SKILL.md
chanthuang
reviewed
Apr 23, 2026
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
b64e2ad to
fb06671
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/mail/emlbuilder/builder.go (1)
311-324:⚠️ Potential issue | 🟠 MajorApply dangerous-Unicode validation to the receipt display name too.
addris hardened withvalidateHeaderValue, butnameonly rejects CR/LF. A DNT display name containing bidi/zero-width controls can still be serialized intoDisposition-Notification-To, undermining the spoofing guard.Proposed hardening
if err := validateDisplayName(name); err != nil { b.err = err return b } + if err := validateHeaderValue(name); err != nil { + b.err = err + return b + } // addr ends up inside mail.Address.String() and written unescaped intoAdd a regression test alongside the DNT validation tests:
+func TestBuild_DispositionNotificationTo_NameBidiRejected(t *testing.T) { + _, err := New(). + From("", "alice@example.com"). + To("", "bob@example.com"). + Subject("hi"). + Date(fixedDate). + DispositionNotificationTo("Alice\u202e", "alice@example.com"). + TextBody([]byte("body")). + Build() + if err == nil || !strings.Contains(err.Error(), "dangerous Unicode") { + t.Fatalf("expected display-name dangerous-Unicode error, got %v", err) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/emlbuilder/builder.go` around lines 311 - 324, The display name is only checked with validateDisplayName (which rejects CR/LF) but not run through validateHeaderValue, so bidi/zero-width controls can still be injected into Disposition-Notification-To; after the existing validateDisplayName(name) check, call validateHeaderValue(name) and set b.err on failure (same pattern used for addr) before constructing the mail.Address and appending to cp.dispositionNotificationTo, keeping the existing use of copySlices, mail.Address{Name: name, Address: addr} and b.err handling.
🤖 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_send_receipt.go`:
- Around line 119-142: The To/From/CC/BCC/ReplyTo callers are passing address
strings directly into mail.Address without CR/LF/control-character validation;
update the emlbuilder methods To(), From(), CC(), BCC(), and ReplyTo() to call
validateHeaderValue() on the address parameter (the same way
DispositionNotificationTo() does) before constructing mail.Address, and return
an error if validation fails so header injection is prevented; keep Subject()
as-is since it already uses validateHeaderValue().
---
Duplicate comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 311-324: The display name is only checked with validateDisplayName
(which rejects CR/LF) but not run through validateHeaderValue, so
bidi/zero-width controls can still be injected into Disposition-Notification-To;
after the existing validateDisplayName(name) check, call
validateHeaderValue(name) and set b.err on failure (same pattern used for addr)
before constructing the mail.Address and appending to
cp.dispositionNotificationTo, keeping the existing use of copySlices,
mail.Address{Name: name, Address: addr} and b.err handling.
🪄 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: 261a6209-c6c4-4d7e-8c00-8d9d2afca246
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (8)
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-send.md
- shortcuts/mail/mail_draft_create_test.go
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/draft/service.go
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/mail/mail_message.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- skills/lark-mail/references/lark-mail-forward.md
- shortcuts/mail/mail_reply.go
- skill-template/domains/mail.md
- shortcuts/mail/mail_forward.go
- shortcuts/mail/mail_draft_create.go
- shortcuts/mail/mail_messages.go
- skills/lark-mail/SKILL.md
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
2 times, most recently
from
23e3e11 to
892de16
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai full review |
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_reply.go (1)
111-148:⚠️ Potential issue | 🟠 MajorValidate
--request-receiptbefore falling back toorig.headTo.Right now an unresolved sender is replaced with
orig.headTobeforerequireSenderForRequestReceiptruns, so+reply --request-receiptcan emitDisposition-Notification-Tofor an address from the original message instead of a resolved sender.🛠️ Proposed fix
- senderEmail := resolveComposeSenderEmail(runtime) + resolvedSender := resolveComposeSenderEmail(runtime) + if err := requireSenderForRequestReceipt(runtime, resolvedSender); err != nil { + return err + } + senderEmail := resolvedSender if senderEmail == "" { senderEmail = orig.headTo } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { - return err - } if runtime.Bool("request-receipt") { bld = bld.DispositionNotificationTo("", senderEmail) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply.go` around lines 111 - 148, The code currently falls back to orig.headTo before validating the request-receipt flag, so Disposition-Notification-To may be set to the original message address; fix by calling requireSenderForRequestReceipt(runtime, senderEmail) (or checking runtime.Bool("request-receipt") && senderEmail == "" and returning an error) immediately after obtaining senderEmail from resolveComposeSenderEmail(runtime) and before assigning senderEmail = orig.headTo, ensuring requireSenderForRequestReceipt validates the unresolved sender and preventing using orig.headTo as a substitute for the request-receipt address.
♻️ Duplicate comments (3)
shortcuts/mail/emlbuilder/builder.go (1)
311-324:⚠️ Potential issue | 🟠 MajorHarden the DNT display name with the same header-value validation.
nameis serialized intoDisposition-Notification-To, but this path only rejects CR/LF viavalidateDisplayName; Bidi and zero-width controls still bypass the new spoofing guard applied toaddr.🛡️ Proposed hardening
if err := validateDisplayName(name); err != nil { b.err = err return b } + if err := validateHeaderValue(name); err != nil { + b.err = err + return b + } // addr ends up inside mail.Address.String() and written unescaped intoAlso add a regression test with a display name such as
Alice\u202eexpecting a dangerous-Unicode error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/emlbuilder/builder.go` around lines 311 - 324, The Disposition-Notification-To path currently only runs validateDisplayName(name) which misses bidi/zero-width guards—call validateHeaderValue(name) as well before appending to cp.dispositionNotificationTo so the display name is rejected for dangerous Unicode like U+202E; update the builder method where mail.Address{Name: name, Address: addr} is appended (and keep setting b.err on failure), and add a regression test that constructs a DNT with a display name containing "\u202e" expecting a dangerous-Unicode/header-value validation error.shortcuts/mail/mail_send_receipt_test.go (2)
170-197:⚠️ Potential issue | 🟡 MinorCover CR and CRLF hint injection, not just LF.
A sanitizer that strips
\nbut leaves\rcould still produce terminal-control output that starts a forged prompt visually. As per coding guidelines, Every behavior change must have an accompanying test.🧪 Proposed test hardening
msg := map[string]interface{}{ "message_id": "msg-1", - "subject": "hi\ntip: go ahead", + "subject": "hi\r\ntip: go ahead", "label_ids": []interface{}{"READ_RECEIPT_REQUEST"}, - "head_from": map[string]interface{}{"mail_address": "alice@example.com\ntip: proceed"}, + "head_from": map[string]interface{}{"mail_address": "alice@example.com\r\ntip: proceed"}, } @@ - if strings.Contains(out, "\ntip: proceed") { - t.Errorf("LF in from address was not stripped; forged tip could open a new line:\n%s", out) + for _, bad := range []string{ + "\ntip: proceed", + "\rtip: proceed", + "\ntip: go ahead", + "\rtip: go ahead", + } { + if strings.Contains(out, bad) { + t.Errorf("line/control separator %q was not stripped; forged tip could open a new line:\n%s", bad, out) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt_test.go` around lines 170 - 197, Update the test "newline in from/subject cannot forge extra tip lines" to also guard against CR and CRLF injection: after calling maybeHintReadReceiptRequest(rt, "me", "msg-1", msg) and capturing out := stderr.String(), assert that out does not contain "\r" or "\r\n" (or at minimum that there is no carriage return character) in addition to the existing checks for "\n"; reference newOutputRuntime, maybeHintReadReceiptRequest, stderr and out when adding the extra assertions so the test fails if CR or CRLF are left in the sanitized output.
155-163:⚠️ Potential issue | 🟡 MinorAssert that the hinted command includes
--yes.The shortcut is
--yes-gated, but this test would still pass if the copy/paste hint omitted the required confirmation flag. As per coding guidelines, Every behavior change must have an accompanying test.🧪 Proposed test tightening
for _, want := range []string{ "READ_RECEIPT_REQUEST", "do NOT auto-send", "alice@example.com", "weekly report", "+send-receipt", + "--yes", "--mailbox 'me'", "--message-id 'msg-1'", } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt_test.go` around lines 155 - 163, The test currently checks that the hinted command contains several substrings but omits the required confirmation flag; update the expectation in the test (the slice of expected strings in mail_send_receipt_test.go) to include "--yes" (or add an explicit assertion that the generated hint contains "--yes") so the test fails if the copy/paste hint lacks the confirmation flag used by the shortcut.
🤖 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_reply.go`:
- Around line 111-148: The code currently falls back to orig.headTo before
validating the request-receipt flag, so Disposition-Notification-To may be set
to the original message address; fix by calling
requireSenderForRequestReceipt(runtime, senderEmail) (or checking
runtime.Bool("request-receipt") && senderEmail == "" and returning an error)
immediately after obtaining senderEmail from resolveComposeSenderEmail(runtime)
and before assigning senderEmail = orig.headTo, ensuring
requireSenderForRequestReceipt validates the unresolved sender and preventing
using orig.headTo as a substitute for the request-receipt address.
---
Duplicate comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 311-324: The Disposition-Notification-To path currently only runs
validateDisplayName(name) which misses bidi/zero-width guards—call
validateHeaderValue(name) as well before appending to
cp.dispositionNotificationTo so the display name is rejected for dangerous
Unicode like U+202E; update the builder method where mail.Address{Name: name,
Address: addr} is appended (and keep setting b.err on failure), and add a
regression test that constructs a DNT with a display name containing "\u202e"
expecting a dangerous-Unicode/header-value validation error.
In `@shortcuts/mail/mail_send_receipt_test.go`:
- Around line 170-197: Update the test "newline in from/subject cannot forge
extra tip lines" to also guard against CR and CRLF injection: after calling
maybeHintReadReceiptRequest(rt, "me", "msg-1", msg) and capturing out :=
stderr.String(), assert that out does not contain "\r" or "\r\n" (or at minimum
that there is no carriage return character) in addition to the existing checks
for "\n"; reference newOutputRuntime, maybeHintReadReceiptRequest, stderr and
out when adding the extra assertions so the test fails if CR or CRLF are left in
the sanitized output.
- Around line 155-163: The test currently checks that the hinted command
contains several substrings but omits the required confirmation flag; update the
expectation in the test (the slice of expected strings in
mail_send_receipt_test.go) to include "--yes" (or add an explicit assertion that
the generated hint contains "--yes") so the test fails if the copy/paste hint
lacks the confirmation flag used by the shortcut.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2cfe608-8d96-40f5-9bf3-24eacb7d37d4
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (11)
- shortcuts/mail/shortcuts.go
- skills/lark-mail/references/lark-mail-reply-all.md
- skills/lark-mail/references/lark-mail-forward.md
- skills/lark-mail/references/lark-mail-draft-create.md
- skills/lark-mail/references/lark-mail-send.md
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/mail_forward.go
- shortcuts/mail/draft/service.go
- shortcuts/mail/helpers.go
- shortcuts/mail/emlbuilder/builder_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_send.go
- shortcuts/mail/mail_reply_all.go
- skill-template/domains/mail.md
- skills/lark-mail/references/lark-mail-send-receipt.md
- shortcuts/mail/mail_draft_edit.go
- shortcuts/mail/mail_draft_create_test.go
- shortcuts/mail/mail_send_receipt.go
- skills/lark-mail/SKILL.md
✅ Actions performedFull review triggered. |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@944e4be1e117fda0cc3e450d2150a6595a191492🧩 Skill updatenpx skills add xzcong0820/larksuite-cli#feature/mail-read-receipt -y -g |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_reply.go (1)
111-148:⚠️ Potential issue | 🟠 MajorValidate receipt sender before falling back to
orig.headTo.Line 143 validates
senderEmailafter Lines 111-114 may replace an unresolved sender withorig.headTo. With--request-receipt, this can emitDisposition-Notification-Toto a header-derived address instead of failing when the sender cannot be resolved.Proposed fix
- senderEmail := resolveComposeSenderEmail(runtime) + resolvedSender := resolveComposeSenderEmail(runtime) + if err := requireSenderForRequestReceipt(runtime, resolvedSender); err != nil { + return err + } + senderEmail := resolvedSender if senderEmail == "" { senderEmail = orig.headTo } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { - return err - } if runtime.Bool("request-receipt") { - bld = bld.DispositionNotificationTo("", senderEmail) + bld = bld.DispositionNotificationTo("", resolvedSender) }Please add/update a test where
resolveComposeSenderEmailis empty butorig.headTois populated;--request-receiptshould return validation instead of building the DNT header. As per coding guidelines, Every behavior change must have an accompanying test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply.go` around lines 111 - 148, The code currently falls back to orig.headTo before validating the receipt sender, which allows a header-derived address to be used for Disposition-Notification-To; change the order so requireSenderForRequestReceipt(runtime, resolvedSender) is called using the raw result of resolveComposeSenderEmail(runtime) (i.e., before assigning senderEmail = orig.headTo), and only if validation passes or is not required, then apply the fallback to orig.headTo for normal From behavior; update the logic around DispositionNotificationTo("", senderEmail) to only run after validation and fallback; add a unit test where resolveComposeSenderEmail returns empty, orig.headTo is populated, and runtime.Bool("request-receipt") is true, asserting that request-receipt triggers requireSenderForRequestReceipt failure (an error) instead of building the DNT header.shortcuts/mail/mail_forward.go (1)
117-137:⚠️ Potential issue | 🟠 MajorValidate
--request-receiptbefore falling back to the originalToheader.Line 132 currently validates after Lines 117-120 replace an unresolved sender with
orig.headTo. That can addressDisposition-Notification-Toto a value from the original message headers instead of a sender address the caller controls.Proposed fix
- senderEmail := resolveComposeSenderEmail(runtime) + resolvedSender := resolveComposeSenderEmail(runtime) + if err := requireSenderForRequestReceipt(runtime, resolvedSender); err != nil { + return err + } + senderEmail := resolvedSender if senderEmail == "" { senderEmail = orig.headTo } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { - return err - } if runtime.Bool("request-receipt") { bld = bld.DispositionNotificationTo("", senderEmail) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_forward.go` around lines 117 - 137, The code falls back to orig.headTo before validating the --request-receipt requirement, which can cause Disposition-Notification-To to be set from the original message headers; change the order so the request-receipt check/validation happens before assigning orig.headTo. Specifically, in the flow around resolveComposeSenderEmail and sender fallback, if runtime.Bool("request-receipt") is true and senderEmail == "" call requireSenderForRequestReceipt (or return an error) before setting senderEmail = orig.headTo and before calling bld.DispositionNotificationTo; keep the rest of the logic (validateRecipientCount, emlbuilder usage) unchanged and reference resolveComposeSenderEmail, orig.headTo, requireSenderForRequestReceipt, and DispositionNotificationTo to locate where to reorder the checks.
♻️ Duplicate comments (4)
shortcuts/mail/mail_draft_create_test.go (1)
220-226:⚠️ Potential issue | 🟡 MinorTighten the Disposition-Notification-To assertion (unresolved from prior review).
strings.Contains(eml, "sender@example.com")still passes even if the address only appears in theFrom:header. Match the fullDisposition-Notification-To:line targeting the sender so a regression dropping or misrouting the receipt address is caught.🧪 Proposed tightening
- // Case-insensitive header match — net/mail uses canonical casing on write. - if !strings.Contains(eml, "Disposition-Notification-To:") { - t.Errorf("expected Disposition-Notification-To header when --request-receipt set; got EML:\n%s", eml) - } - if !strings.Contains(eml, "sender@example.com") { - t.Errorf("expected receipt address to be the sender; got EML:\n%s", eml) - } + if !strings.Contains(eml, "Disposition-Notification-To: <sender@example.com>") && + !strings.Contains(eml, "Disposition-Notification-To: sender@example.com") { + t.Errorf("expected Disposition-Notification-To targeting sender@example.com; got EML:\n%s", eml) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 220 - 226, The current test uses strings.Contains(eml, "sender@example.com") which can match the From: header; tighten the assertion to verify the actual Disposition-Notification-To header contains the sender address by matching the full header line (e.g. ensure eml contains "Disposition-Notification-To: sender@example.com" or use a regexp anchored to the header name) so the test checks the Disposition-Notification-To header specifically (look for the variable eml and the header name Disposition-Notification-To in the test).shortcuts/mail/mail_send.go (1)
38-38:⚠️ Potential issue | 🟡 Minor
+send --request-receiptintegration test still missing (unresolved from prior review).Builder and helper coverage exists, but there is still no shortcut-level test asserting that
+send --request-receiptyields an EML withDisposition-Notification-To:targeting the resolved sender (mirroring the+draft-createtest pattern).Also applies to: 112-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send.go` at line 38, Add a shortcut-level integration test that invokes the "+send --request-receipt" shortcut and asserts the produced EML includes a "Disposition-Notification-To:" header set to the resolved sender address (same pattern used by the existing "+draft-create" test); locate the test harness that exercises shortcut commands and create a new test case mirroring the "+draft-create" helper flow to call the send shortcut, resolve the sender (as the shortcut runtime would), capture the generated EML, and assert the header value equals that resolved sender; apply the same assertion pattern for the related cases referenced around lines 112-117 to ensure coverage for all relevant code paths.skills/lark-mail/references/lark-mail-forward.md (1)
75-75:⚠️ Potential issue | 🟡 MinorSender-resolution note still missing (unresolved from prior review).
The
--fromfallback hint suggested in the previous review has not been applied to this row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-forward.md` at line 75, The table row for `--request-receipt` is missing the sender-resolution fallback hint — update that row to mention the `--from` fallback so readers know which sender address will be used when none is explicit (e.g., "use --from as fallback"); specifically modify the `--request-receipt` description to append a brief sender-resolution note referencing `--from` so it mirrors the prior row's guidance.shortcuts/mail/emlbuilder/builder.go (1)
311-324:⚠️ Potential issue | 🟠 MajorApply the dangerous-Unicode guard to the DNT display name too.
Line 311 only rejects CR/LF in
name, while Line 319 applies the stronger spoofing guard toaddr. ADisposition-Notification-Todisplay name containing Bidi or zero-width controls can still reach the header.Proposed hardening
if err := validateDisplayName(name); err != nil { b.err = err return b } + if err := validateHeaderValue(name); err != nil { + b.err = err + return b + } // addr ends up inside mail.Address.String() and written unescaped intoAdd a regression test with
DispositionNotificationTo("Alice\u202e", "alice@example.com")expecting a dangerous-Unicode error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/emlbuilder/builder.go` around lines 311 - 324, The display name for Disposition-Notification-To is only checked with validateDisplayName but not the stronger dangerous-Unicode/CRLF guard; update the DispositionNotificationTo code path so you call validateHeaderValue(name) (or run name through the same validation as addr) and set b.err on failure before appending to cp.dispositionNotificationTo; also add a regression test calling DispositionNotificationTo("Alice\u202e", "alice@example.com") and assert it returns the dangerous-Unicode error to prevent Bidi/zero-width spoofing.
🧹 Nitpick comments (6)
skills/lark-mail/references/lark-mail-draft-create.md (1)
57-57: Consider noting the--fromfallback (same as other receipt-capable shortcut docs).If the sender cannot be resolved,
+draft-create --request-receipterrors out and asks for--from. Adding that hint keeps the doc consistent with the other receipt-aware shortcuts.📝 Proposed doc tweak
-| `--request-receipt` | 否 | 请求已读回执（RFC 3798 Message Disposition Notification）。在草稿 EML 里写 `Disposition-Notification-To: <sender>` 头，发送时生效。收件人的邮件客户端可能弹出提示、自动发送或忽略——送达不保证 | +| `--request-receipt` | 否 | 请求已读回执（RFC 3798 Message Disposition Notification）。在草稿 EML 里写 `Disposition-Notification-To: <sender>` 头，发送时生效；如果无法解析发件人地址，请显式指定 `--from`。收件人的邮件客户端可能弹出提示、自动发送或忽略——送达不保证 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-draft-create.md` at line 57, The docs for the `+draft-create --request-receipt` flag should note the `--from` fallback: update the `--request-receipt` line to add a short parenthetical or sentence that if the sender cannot be auto-resolved the command will error and require `--from`, mirroring other receipt-capable shortcut docs (mention `+draft-create`, `--request-receipt`, and `--from` explicitly so readers know what to pass).skills/lark-mail/references/lark-mail-reply-all.md (1)
79-79: Mention the sender-resolution fallback (parity with other receipt-capable shortcuts).
--request-receiptfails if the sender address cannot be resolved. Noting the--fromrecovery path here keeps reply-all consistent with the guidance being added to the other shortcut docs.📝 Proposed doc tweak
-| `--request-receipt` | 否 | 请求已读回执（RFC 3798 Message Disposition Notification）。在出站 EML 里写 `Disposition-Notification-To: <sender>` 头。收件人的邮件客户端可能弹出提示、自动发送或忽略——送达不保证 | +| `--request-receipt` | 否 | 请求已读回执（RFC 3798 Message Disposition Notification）。在出站 EML 里写 `Disposition-Notification-To: <sender>` 头；如果无法解析发件人地址，请显式指定 `--from`。收件人的邮件客户端可能弹出提示、自动发送或忽略——送达不保证 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-reply-all.md` at line 79, Update the `--request-receipt` documentation line to mention the sender-resolution fallback: state that `--request-receipt` requires a resolvable sender address and will fail if the sender cannot be resolved, and instruct users to supply `--from <address>` as a recovery/fallback to ensure the `Disposition-Notification-To` header is written; keep wording consistent with the other receipt-capable shortcut docs that reference `--from`.shortcuts/mail/mail_draft_create_test.go (1)
188-203: Optional: foldnewRuntimeWithFromAndRequestReceiptintonewRuntimeWithFrom.The two helpers are nearly identical; a single helper with an optional
requestReceiptbool (or variadic option) would avoid duplication as more receipt-related tests are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 188 - 203, The two helpers duplicate flag setup; replace newRuntimeWithFromAndRequestReceipt by folding its logic into newRuntimeWithFrom: add an optional parameter (e.g., requestReceipt bool) to newRuntimeWithFrom so it registers the "request-receipt" Bool flag and sets it when requestReceipt is true, remove the separate newRuntimeWithFromAndRequestReceipt function, and update any tests that call newRuntimeWithFromAndRequestReceipt to call newRuntimeWithFrom with the new requestReceipt argument; keep the existing flag names and use cobra.Command flags APIs as in the original helpers.shortcuts/mail/mail_send_receipt_test.go (1)
94-115: Consider adding a duplicate-ID case.
buildReceiptReferencestoday unconditionally appends<origSMTPID>even if already present inorigRefs(see line 245-254 inmail_send_receipt.go). A test like{"id already in refs", "<c@x>", "c@x", "<c@x>"}would lock in the current behavior and make future dedup changes (raised as a minor on the implementation) test-driven.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt_test.go` around lines 94 - 115, Add a test case to TestBuildReceiptReferences to cover the duplicate-ID scenario so future changes that deduplicate or alter behavior are caught: update the cases slice inside TestBuildReceiptReferences to include a case like {"id already in refs", "<c@x>", "c@x", "<c@x>"} that calls buildReceiptReferences(origRef, origID) and asserts the result equals the expected string; this ensures the current behavior of buildReceiptReferences is locked in by tests.shortcuts/mail/mail_send_receipt.go (1)
256-267:extractAddressPairsilently returns zeros for unexpected shapes.If
head_fromever arrives as[]interface{}(e.g., list form from a newer backend response) or ajson.Number, this returns empty strings and the caller at line 125-127 then rejects the whole message as "no sender address." That's a safe failure mode, but a targeted error message would help debugging. Optional — current behavior is defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 256 - 267, extractAddressPair currently returns empty strings for unexpected shapes; update it to handle []interface{} by extracting the first element (recursing into extractAddressPair or normalizing it to a string) and handle json.Number by converting it to its string representation, and add a default branch that logs a clear debug/error message including the concrete type when an unknown type is seen. Ensure you still return (email,name string) but attempt to coerce common variants (slice, json.Number, numeric) into an email string so the caller that rejects messages for "no sender address" gets more actionable data and logs show the unexpected payload shape (referencing extractAddressPair).shortcuts/mail/helpers_test.go (1)
1286-1310: Nit: checkos.WriteFileerrors in test fixtures.A failed
os.WriteFilewould cause subsequentvalidateComposeInlineAndAttachmentscalls to report "file not found" errors that mask the actual fixture setup failure. Cheap to guard:Proposed change
- os.WriteFile("img.png", []byte("png"), 0o644) + if err := os.WriteFile("img.png", []byte("png"), 0o644); err != nil { + t.Fatalf("setup: %v", err) + }Apply similarly at lines 1301 and 1309.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers_test.go` around lines 1286 - 1310, The test fixtures call os.WriteFile before invoking validateComposeInlineAndAttachments but ignore its error; update each fixture (the WriteFile calls that create "img.png", "malware.exe", and "report.pdf") to check and fail the test on os.WriteFile error (e.g., t.Fatalf or t.Fatalf with the returned error) so a write failure doesn't masquerade as a missing/blocked-file error from validateComposeInlineAndAttachments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-mail/references/lark-mail-send-receipt.md`:
- Line 11: Update the trigger rule sentence that currently checks for
`READ_RECEIPT_REQUEST` in `label_ids` (used when seeing `+message` / `+messages`
/ `+thread`) to also accept the numeric system label `-607`; specifically,
modify the text referencing `label_ids` and `READ_RECEIPT_REQUEST` so the rule
reads that the agent should prompt for a receipt when `label_ids` contains
either `READ_RECEIPT_REQUEST` or `-607`, ensuring ordinary messages still never
trigger this command.
- Line 43: The English subject prefix casing is inconsistent: the doc shows
"Read receipt:" but the implementation and backend guidance use "Read Receipt:".
Update the documentation text so the English prefix is exactly "Read Receipt:"
everywhere (including the example subject and the instruction to add to
MailPrefixConfig.SubjectPrefixListForAdvancedSearch), and verify references to
detectSubjectLang and GetRealSubject mention the exact prefix string so backend
aggregation/regex expectations match the documented value.
---
Outside diff comments:
In `@shortcuts/mail/mail_forward.go`:
- Around line 117-137: The code falls back to orig.headTo before validating the
--request-receipt requirement, which can cause Disposition-Notification-To to be
set from the original message headers; change the order so the request-receipt
check/validation happens before assigning orig.headTo. Specifically, in the flow
around resolveComposeSenderEmail and sender fallback, if
runtime.Bool("request-receipt") is true and senderEmail == "" call
requireSenderForRequestReceipt (or return an error) before setting senderEmail =
orig.headTo and before calling bld.DispositionNotificationTo; keep the rest of
the logic (validateRecipientCount, emlbuilder usage) unchanged and reference
resolveComposeSenderEmail, orig.headTo, requireSenderForRequestReceipt, and
DispositionNotificationTo to locate where to reorder the checks.
In `@shortcuts/mail/mail_reply.go`:
- Around line 111-148: The code currently falls back to orig.headTo before
validating the receipt sender, which allows a header-derived address to be used
for Disposition-Notification-To; change the order so
requireSenderForRequestReceipt(runtime, resolvedSender) is called using the raw
result of resolveComposeSenderEmail(runtime) (i.e., before assigning senderEmail
= orig.headTo), and only if validation passes or is not required, then apply the
fallback to orig.headTo for normal From behavior; update the logic around
DispositionNotificationTo("", senderEmail) to only run after validation and
fallback; add a unit test where resolveComposeSenderEmail returns empty,
orig.headTo is populated, and runtime.Bool("request-receipt") is true, asserting
that request-receipt triggers requireSenderForRequestReceipt failure (an error)
instead of building the DNT header.
---
Duplicate comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 311-324: The display name for Disposition-Notification-To is only
checked with validateDisplayName but not the stronger dangerous-Unicode/CRLF
guard; update the DispositionNotificationTo code path so you call
validateHeaderValue(name) (or run name through the same validation as addr) and
set b.err on failure before appending to cp.dispositionNotificationTo; also add
a regression test calling DispositionNotificationTo("Alice\u202e",
"alice@example.com") and assert it returns the dangerous-Unicode error to
prevent Bidi/zero-width spoofing.
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 220-226: The current test uses strings.Contains(eml,
"sender@example.com") which can match the From: header; tighten the assertion to
verify the actual Disposition-Notification-To header contains the sender address
by matching the full header line (e.g. ensure eml contains
"Disposition-Notification-To: sender@example.com" or use a regexp anchored to
the header name) so the test checks the Disposition-Notification-To header
specifically (look for the variable eml and the header name
Disposition-Notification-To in the test).
In `@shortcuts/mail/mail_send.go`:
- Line 38: Add a shortcut-level integration test that invokes the "+send
--request-receipt" shortcut and asserts the produced EML includes a
"Disposition-Notification-To:" header set to the resolved sender address (same
pattern used by the existing "+draft-create" test); locate the test harness that
exercises shortcut commands and create a new test case mirroring the
"+draft-create" helper flow to call the send shortcut, resolve the sender (as
the shortcut runtime would), capture the generated EML, and assert the header
value equals that resolved sender; apply the same assertion pattern for the
related cases referenced around lines 112-117 to ensure coverage for all
relevant code paths.
In `@skills/lark-mail/references/lark-mail-forward.md`:
- Line 75: The table row for `--request-receipt` is missing the
sender-resolution fallback hint — update that row to mention the `--from`
fallback so readers know which sender address will be used when none is explicit
(e.g., "use --from as fallback"); specifically modify the `--request-receipt`
description to append a brief sender-resolution note referencing `--from` so it
mirrors the prior row's guidance.
---
Nitpick comments:
In `@shortcuts/mail/helpers_test.go`:
- Around line 1286-1310: The test fixtures call os.WriteFile before invoking
validateComposeInlineAndAttachments but ignore its error; update each fixture
(the WriteFile calls that create "img.png", "malware.exe", and "report.pdf") to
check and fail the test on os.WriteFile error (e.g., t.Fatalf or t.Fatalf with
the returned error) so a write failure doesn't masquerade as a
missing/blocked-file error from validateComposeInlineAndAttachments.
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 188-203: The two helpers duplicate flag setup; replace
newRuntimeWithFromAndRequestReceipt by folding its logic into
newRuntimeWithFrom: add an optional parameter (e.g., requestReceipt bool) to
newRuntimeWithFrom so it registers the "request-receipt" Bool flag and sets it
when requestReceipt is true, remove the separate
newRuntimeWithFromAndRequestReceipt function, and update any tests that call
newRuntimeWithFromAndRequestReceipt to call newRuntimeWithFrom with the new
requestReceipt argument; keep the existing flag names and use cobra.Command
flags APIs as in the original helpers.
In `@shortcuts/mail/mail_send_receipt_test.go`:
- Around line 94-115: Add a test case to TestBuildReceiptReferences to cover the
duplicate-ID scenario so future changes that deduplicate or alter behavior are
caught: update the cases slice inside TestBuildReceiptReferences to include a
case like {"id already in refs", "<c@x>", "c@x", "<c@x>"} that calls
buildReceiptReferences(origRef, origID) and asserts the result equals the
expected string; this ensures the current behavior of buildReceiptReferences is
locked in by tests.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 256-267: extractAddressPair currently returns empty strings for
unexpected shapes; update it to handle []interface{} by extracting the first
element (recursing into extractAddressPair or normalizing it to a string) and
handle json.Number by converting it to its string representation, and add a
default branch that logs a clear debug/error message including the concrete type
when an unknown type is seen. Ensure you still return (email,name string) but
attempt to coerce common variants (slice, json.Number, numeric) into an email
string so the caller that rejects messages for "no sender address" gets more
actionable data and logs show the unexpected payload shape (referencing
extractAddressPair).
In `@skills/lark-mail/references/lark-mail-draft-create.md`:
- Line 57: The docs for the `+draft-create --request-receipt` flag should note
the `--from` fallback: update the `--request-receipt` line to add a short
parenthetical or sentence that if the sender cannot be auto-resolved the command
will error and require `--from`, mirroring other receipt-capable shortcut docs
(mention `+draft-create`, `--request-receipt`, and `--from` explicitly so
readers know what to pass).
In `@skills/lark-mail/references/lark-mail-reply-all.md`:
- Line 79: Update the `--request-receipt` documentation line to mention the
sender-resolution fallback: state that `--request-receipt` requires a resolvable
sender address and will fail if the sender cannot be resolved, and instruct
users to supply `--from <address>` as a recovery/fallback to ensure the
`Disposition-Notification-To` header is written; keep wording consistent with
the other receipt-capable shortcut docs that reference `--from`.
🪄 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: 8a8774eb-0e19-4e05-b361-dd263c1bffae
📒 Files selected for processing (27)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
892de16 to
6b57583
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 60.01% 61.24% +1.23%
==========================================
Files 405 430 +25
Lines 42830 45582 +2752
==========================================
+ Hits 25703 27916 +2213
- Misses 15108 15545 +437
- Partials 2019 2121 +102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
4 times, most recently
from
ec5c017 to
af6e318
Compare
github-actions
Bot
added
size/XL
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_reply.go (1)
111-148:⚠️ Potential issue | 🟠 Major
--request-receiptvalidation runs after theorig.headTofallback — may point the MDN at an address the caller does not control.At L111–114
senderEmailfalls back toorig.headTowhenresolveComposeSenderEmailreturns empty, andrequireSenderForRequestReceipt/DispositionNotificationToat L143–148 then use that post-fallback value. In a shared-mailbox or CC'd-only scenarioorig.headTois not necessarily an address the current caller owns, so the emittedDisposition-Notification-Towill solicit receipts to someone else's address.
mail_reply_all.go(L113–122) deliberately validates the pre-fallbackresolvedSenderfor this exact reason (see its comment: the MDN must point to an address the caller explicitly controls). Please mirror that pattern here so behavior is consistent across compose shortcuts.🛡️ Suggested fix
- senderEmail := resolveComposeSenderEmail(runtime) - if senderEmail == "" { - senderEmail = orig.headTo - } + resolvedSender := resolveComposeSenderEmail(runtime) + // Validate --request-receipt against the caller-controlled address + // BEFORE falling back to orig.headTo (which may be someone else in a + // shared-mailbox / CC'd scenario). Mirrors mail_reply_all.go. + if err := requireSenderForRequestReceipt(runtime, resolvedSender); err != nil { + return err + } + senderEmail := resolvedSender + if senderEmail == "" { + senderEmail = orig.headTo + } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { - return err - } - if runtime.Bool("request-receipt") { + if runtime.Bool("request-receipt") { bld = bld.DispositionNotificationTo("", senderEmail) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply.go` around lines 111 - 148, The request-receipt validation must run against the raw result of resolveComposeSenderEmail (before falling back to orig.headTo) so we don't set Disposition-Notification-To to an address the caller may not control; change the flow in the reply handler to capture the initial resolvedSender := resolveComposeSenderEmail(runtime), call requireSenderForRequestReceipt(runtime, resolvedSender) and only after that apply the fallback (senderEmail := resolvedSender; if senderEmail == "" { senderEmail = orig.headTo }) and use senderEmail in emlbuilder.From and DispositionNotificationTo; mirror the pre-fallback validation pattern used in mail_reply_all.go (check resolvedSender for receipt permission, then fallback for display/from).shortcuts/mail/mail_forward.go (1)
117-137:⚠️ Potential issue | 🟠 MajorSame issue as
mail_reply.go:--request-receiptuses the post-fallbacksenderEmail, which may beorig.headToand not an address the caller controls.
senderEmailat L117–120 falls back toorig.headTo;requireSenderForRequestReceipt/DispositionNotificationToat L132–137 then use that value. For a forwarded messageorig.headTois frequently an unrelated recipient of the original mail, so the emittedDisposition-Notification-Towould direct receipts to them.Align with
mail_reply_all.go's pattern — validate the pre-fallback resolved sender and refuse to set the MDN header if only theorig.headTofallback is available.🛡️ Suggested fix
- senderEmail := resolveComposeSenderEmail(runtime) - if senderEmail == "" { - senderEmail = orig.headTo - } + resolvedSender := resolveComposeSenderEmail(runtime) + // Validate BEFORE fallback so the MDN never points at orig.headTo + // (which for forwards is typically an unrelated original recipient). + if err := requireSenderForRequestReceipt(runtime, resolvedSender); err != nil { + return err + } + senderEmail := resolvedSender + if senderEmail == "" { + senderEmail = orig.headTo + } @@ - if err := requireSenderForRequestReceipt(runtime, senderEmail); err != nil { - return err - } - if runtime.Bool("request-receipt") { + if runtime.Bool("request-receipt") { bld = bld.DispositionNotificationTo("", senderEmail) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_forward.go` around lines 117 - 137, The code currently falls back senderEmail to orig.headTo and then uses that value for requireSenderForRequestReceipt and DispositionNotificationTo, which can set MDN headers to addresses the caller doesn't control; capture the pre-fallback resolved sender (e.g., resolvedSender := resolveComposeSenderEmail(runtime)) and use that original resolvedSender when calling requireSenderForRequestReceipt and when deciding to call bld.DispositionNotificationTo; keep the fallback-only senderEmail for From/other fields but refuse to set the MDN header (and skip requireSenderForRequestReceipt) if resolvedSender is empty (meaning only orig.headTo would be available).
♻️ Duplicate comments (6)
shortcuts/mail/mail_draft_create_test.go (1)
220-226:⚠️ Potential issue | 🟡 MinorTighten assertions to prove the address lands on the right header.
strings.Contains(eml, "sender@example.com")will also match theFrom:header, so a regression that drops or mis-targets theDisposition-Notification-Tovalue would still pass. Assert the full header line (e.g.Disposition-Notification-To: <sender@example.com>), which is whatnet/mail/the builder writes.🧪 Proposed tightening
- // Case-insensitive header match — net/mail uses canonical casing on write. - if !strings.Contains(eml, "Disposition-Notification-To:") { - t.Errorf("expected Disposition-Notification-To header when --request-receipt set; got EML:\n%s", eml) - } - if !strings.Contains(eml, "sender@example.com") { - t.Errorf("expected receipt address to be the sender; got EML:\n%s", eml) - } + if !strings.Contains(eml, "Disposition-Notification-To: <sender@example.com>") && + !strings.Contains(eml, "Disposition-Notification-To: sender@example.com") { + t.Errorf("expected Disposition-Notification-To targeting sender@example.com; got EML:\n%s", eml) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` around lines 220 - 226, The test currently checks for the receipt address with a loose contains on eml which can match From:; tighten the assertion to verify the exact header line produced by the mail builder by asserting that eml contains the full header "Disposition-Notification-To: <sender@example.com>" (use the eml variable in shortcuts/mail/mail_draft_create_test.go) so the test fails if the address is not placed on the Disposition-Notification-To header.shortcuts/mail/emlbuilder/builder.go (1)
311-324:⚠️ Potential issue | 🟠 MajorReject dangerous Unicode in the read-receipt display name too.
Line 311 only checks the display name for CR/LF, while Line 319 applies the stronger dangerous-Unicode guard to the address. Since both fields are emitted into
Disposition-Notification-To, apply the same header-value hardening tonameand add the corresponding regression test.Suggested hardening
if err := validateDisplayName(name); err != nil { b.err = err return b } + if err := validateHeaderValue(name); err != nil { + b.err = err + return b + } // addr ends up inside mail.Address.String() and written unescaped into🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/emlbuilder/builder.go` around lines 311 - 324, The display name is only checked with validateDisplayName but not against dangerous-Unicode/header-injection rules, while the address is validated with validateHeaderValue; update the builder code (around validateDisplayName and the block that appends to cp.dispositionNotificationTo in the EML builder) to also call validateHeaderValue(name) and set b.err if it returns an error before copying slices/appending, and add a regression test asserting that names containing CR/LF, bidi or zero-width code points are rejected for Disposition-Notification-To (use the same test pattern as the existing address validation tests).shortcuts/mail/mail_send_receipt.go (2)
33-33:⚠️ Potential issue | 🟡 MinorFix the
Tolabel comment to match the emitted body.Line 136 passes
senderEmailinto the body builder, so the body’sTo:field represents the original recipient / reader sending this receipt, not the receipt recipient / original sender.Proposed comment fix
- To string // label for the address the receipt is sent to (= original mail's sender) + To string // label for the original recipient / reader sending this receipt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` at line 33, The comment for the struct field To is inaccurate: update the comment on the To field (To string) to state that it represents the original recipient/reader (the address that sent the receipt content, i.e., the senderEmail passed into the body builder), not the receipt recipient/original sender; locate the To field in mail_send_receipt.go and replace the comment text accordingly so it matches how senderEmail is used when building the body.
91-109:⚠️ Potential issue | 🟠 MajorKeep
--fromfrom changing the mailbox used for lookup/send.
resolveComposeMailboxIDfalls back to--from, so+send-receipt --from alias@example.com --message-id ...can fetch/send underalias@example.cominstead of the documented default mailboxme. For this shortcut,--fromshould only select the receipt sender header; message lookup and draft APIs should use--mailboxorme.Proposed fix
DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { messageID := runtime.Str("message-id") - mailboxID := resolveComposeMailboxID(runtime) + mailboxID := resolveMailboxID(runtime) @@ Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { messageID := runtime.Str("message-id") - mailboxID := resolveComposeMailboxID(runtime) + mailboxID := resolveMailboxID(runtime)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt.go` around lines 91 - 109, The shortcut currently uses resolveComposeMailboxID (which falls back to --from) for message lookup and draft/send, causing --from to change the mailbox used; change the Execute (and DryRun) logic to derive two separate IDs: keep resolveComposeMailboxID (or the existing sender selection) for the receipt sender header, but for fetching the original message and creating/sending drafts use a mailbox ID that ignores --from (e.g., read runtime.Str("mailbox") and default to "me" if empty) — replace mailboxID := resolveComposeMailboxID(runtime) in DryRun/Execute with a lookupMailboxID obtained from runtime.Str("mailbox") || "me" and use that for mailboxPath calls while retaining the original resolveComposeMailboxID result only for setting the sender header.skills/lark-mail/references/lark-mail-send-receipt.md (1)
11-11:⚠️ Potential issue | 🟡 MinorInclude
-607in the critical trigger rule.Line 5 and later examples recognize the numeric system label, but this CRITICAL rule only names
READ_RECEIPT_REQUEST; agents seeing raw-607could skip the consent prompt.Proposed documentation fix
-1. **触发条件严格**：仅当拉信（`+message` / `+messages` / `+thread`）看到 `label_ids` 里有 `READ_RECEIPT_REQUEST` 时，才应该问用户是否发回执。对普通邮件**绝不**调用此命令。 +1. **触发条件严格**：仅当拉信（`+message` / `+messages` / `+thread`）看到 `label_ids` 里有 `READ_RECEIPT_REQUEST`（或系统 ID `-607`）时，才应该问用户是否发回执。对普通邮件**绝不**调用此命令。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-mail/references/lark-mail-send-receipt.md` at line 11, Update the critical trigger rule in the receipt-sending documentation to explicitly include the numeric system label "-607" alongside "READ_RECEIPT_REQUEST" so agents also match raw numeric labels; modify the rule text that currently references only READ_RECEIPT_REQUEST (and any instances of the trigger-check examples) to mention both "READ_RECEIPT_REQUEST" and "-607" so the consent prompt fires for either label.shortcuts/mail/mail_send_receipt_test.go (1)
155-198:⚠️ Potential issue | 🟡 MinorTighten the hint contract tests for
--yesand CR injection.Add the required confirmation flag to the expected hint and exercise CR/CRLF, not just LF; otherwise the test can miss regressions that omit
--yesor leave carriage-return line control intact.Proposed test hardening
for _, want := range []string{ "READ_RECEIPT_REQUEST", "do NOT auto-act", "alice@example.com", "weekly report", "+send-receipt", + "--yes", "+decline-receipt", "--mailbox 'me'", "--message-id 'msg-1'", @@ msg := map[string]interface{}{ "message_id": "msg-1", - "subject": "hi\ntip: go ahead", + "subject": "hi\r\ntip: go ahead", "label_ids": []interface{}{"READ_RECEIPT_REQUEST"}, - "head_from": map[string]interface{}{"mail_address": "alice@example.com\ntip: proceed"}, + "head_from": map[string]interface{}{"mail_address": "alice@example.com\r\ntip: proceed"}, } @@ - if strings.Contains(out, "\ntip: proceed") { - t.Errorf("LF in from address was not stripped; forged tip could open a new line:\n%s", out) + for _, bad := range []string{ + "\ntip: proceed", + "\rtip: proceed", + "\ntip: go ahead", + "\rtip: go ahead", + } { + if strings.Contains(out, bad) { + t.Errorf("line/control separator %q was not stripped; forged tip could open a new line:\n%s", bad, out) + } }As per coding guidelines, Every behavior change must have an accompanying test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send_receipt_test.go` around lines 155 - 198, The test must be tightened to require the confirmation flag and to exercise carriage-return variants: update the expected hint list in the first subtest to include the "--yes" token (so the hint must advertise the confirmation flag) and in the "newline in from/subject cannot forge extra tip lines" subtest, add additional message variants where subject and head_from contain "\r" and "\r\n" (not just "\n") and assert that neither "\r\ntip: proceed" nor "\rtip: proceed" appear in stderr; keep using the same helper maybeHintReadReceiptRequest to render the hint and maintain the existing checks for line-starting "tip:" lines.
🧹 Nitpick comments (4)
shortcuts/mail/mail_draft_create_test.go (1)
29-29: Checkos.WriteFileerrors in test fixtures.Several of the updated tests call
os.WriteFile(...)without checking the returned error (Lines 31, 88, 92, 113, 134, 252). If the write silently fails underchdirTemp, later assertions become misleading. A quickif err := os.WriteFile(...); err != nil { t.Fatal(err) }keeps failures honest. This pre-exists but is worth a cleanup pass while touching the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create_test.go` at line 29, Tests in shortcuts/mail/mail_draft_create_test.go call os.WriteFile in multiple places (e.g., inside TestBuildRawEMLForDraftCreate_ResolvesLocalImages and other test helpers used under chdirTemp) without checking the returned error, which can hide failures; update each os.WriteFile(...) call mentioned (around the lines referenced) to capture the error and fail the test on error (for example: if err := os.WriteFile(...); err != nil { t.Fatal(err) }) so filesystem write failures are reported immediately and subsequent assertions remain valid.shortcuts/mail/mail_send.go (1)
112-117:--request-receiptwiring looks correct.Sender is resolved via
resolveComposeSenderEmail, then gated throughrequireSenderForRequestReceiptbeforeDispositionNotificationTois set on the builder. Per the AI summary,shortcuts/mail/mail_request_receipt_integration_test.gonow covers the+sendend-to-end path, which addresses the previous request for shortcut-level test coverage.One minor suggestion: consider moving the
requireSenderForRequestReceiptcall up next tosenderEmail := resolveComposeSenderEmail(runtime)(around Line 93) so validation fails before any builder setup / signature resolution work. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_send.go` around lines 112 - 117, Move the requireSenderForRequestReceipt validation so it runs immediately after resolveComposeSenderEmail and before any builder or signature work: call requireSenderForRequestReceipt(runtime, senderEmail) right after senderEmail := resolveComposeSenderEmail(runtime) (instead of later near where bld.DispositionNotificationTo is set) so the check fails fast; keep the existing DispositionNotificationTo(bld) logic and only set it when runtime.Bool("request-receipt") is true.shortcuts/mail/mail_thread.go (1)
105-133: Duplicated envelope/message extraction — collect raw msgs in the first loop.The envelope→
msgnormalization at L123–131 duplicates the exact logic at L107–115. Consider capturing raw messages in the initial pass and iterating the cached slice at the end.♻️ Suggested refactor
outs := make([]map[string]interface{}, 0, len(items)) + rawMsgs := make([]map[string]interface{}, 0, len(items)) for _, item := range items { envelope, ok := item.(map[string]interface{}) if !ok { continue } msg := envelope if inner, ok := envelope["message"].(map[string]interface{}); ok { msg = inner } outs = append(outs, buildMessageOutput(msg, html)) + rawMsgs = append(rawMsgs, msg) } @@ runtime.Out(mailThreadOutput{ThreadID: threadID, MessageCount: len(messages), Messages: messages}, nil) - for _, item := range items { - envelope, ok := item.(map[string]interface{}) - if !ok { - continue - } - msg := envelope - if inner, ok := envelope["message"].(map[string]interface{}); ok { - msg = inner - } - maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg) - } + for _, msg := range rawMsgs { + maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_thread.go` around lines 105 - 133, The code duplicates envelope→msg normalization twice; in the loop that builds outs, also collect the normalized raw message objects into a slice (e.g., rawMsgs) and use that cached slice for both building outputs (call buildMessageOutput(msg, html)) and for later read-receipt hints (call maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]), msg)); keep sortThreadMessagesByInternalDate(outs) as-is but ensure you pass the normalized messages for hinting to avoid re-parsing envelope/message twice and reference the existing symbols items, buildMessageOutput, sortThreadMessagesByInternalDate, maybeHintReadReceiptRequest, and strVal to locate the code to change.shortcuts/mail/mail_decline_receipt_test.go (1)
31-42: Assert exact scopes, not only required-scope presence.This test would still pass if
+decline-receiptaccidentally gained an extra broad scope, even though scopes are part of the public auth prompt contract. Compare against an exact set instead.Suggested tightening
- required := map[string]bool{ + wantScopes := map[string]bool{ "mail:user_mailbox.message:modify": true, "mail:user_mailbox.message:readonly": true, "mail:user_mailbox:readonly": true, "mail:user_mailbox.message.body:read": true, } for _, s := range MailDeclineReceipt.Scopes { - delete(required, s) + if !wantScopes[s] { + t.Errorf("MailDeclineReceipt.Scopes has unexpected scope %q", s) + } + delete(wantScopes, s) } - if len(required) != 0 { - t.Errorf("MailDeclineReceipt.Scopes missing %v", required) + if len(wantScopes) != 0 { + t.Errorf("MailDeclineReceipt.Scopes missing %v", wantScopes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_decline_receipt_test.go` around lines 31 - 42, The test currently only ensures required scopes are present by deleting expected keys from a map, which would miss any extra unwanted scopes; change it to assert the exact set of scopes for MailDeclineReceipt by comparing MailDeclineReceipt.Scopes to the expected slice/map exactly (e.g., build an expected []string or map[string]bool with the four scopes and assert lengths match and every element matches, or compare serialized sorted slices for equality) so any added or removed scopes cause the test to fail; reference MailDeclineReceipt.Scopes and the current required set when making the replacement.
🤖 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/helpers.go`:
- Around line 104-106: The validation error for an empty senderEmail currently
tells users to "specify --from explicitly", which is incorrect for callers like
+draft-edit; update the message produced by output.ErrValidation in the
senderEmail check to give a generic recovery hint (e.g., ask the user to provide
a resolvable sender address or use the appropriate flag) so it applies to both
send and draft-edit flows — change the string returned in the if block that
references senderEmail and output.ErrValidation to a neutral hint that does not
mention --from.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 106-123: Tests are missing for the new --request-receipt
draft-edit path; add unit tests that exercise the logic in
requireSenderForRequestReceipt and the synthesized Disposition-Notification-To
patch insertion (where validateHeaderAddress guards draftFromEmail and patch.Ops
gets the draftpkg.PatchOp). Add tests for: 1) missing draft From (ensure
requireSenderForRequestReceipt returns the expected error), 2) unsafe draft From
(validateHeaderAddress failure yields ErrValidation), 3) request-receipt-only
success path (no other patch ops, but Execute results in a patch containing the
synthesized set_header Disposition-Notification-To), and 4) request-receipt
combined with an existing patch (existing patch.Ops plus the synthesized
draftpkg.PatchOp); place tests next to the draft edit tests and assert
errors/messages and final patch.Ops contents accordingly.
In `@shortcuts/mail/mail_request_receipt_integration_test.go`:
- Around line 117-328: Add tests for the +draft-edit request-receipt path
mirroring the existing +send/+reply tests: create an integration test
TestMailDraftEdit_RequestReceiptAddsHeader_Integration that uses
mailShortcutTestFactoryWithSendScope, stubMailboxProfile(reg, "me@example.com"),
registerDraftCaptureStubs(reg), runs runMountedMailShortcut with MailDraftEdit
and the --request-receipt flag, then assert the captured draft EML
(decodeCapturedRawEML on createStub.CapturedBody) contains
"Disposition-Notification-To:" and the sender address; also add a negative test
TestMailDraftEdit_RequestReceiptNoSender_FailsValidation that stubs an empty
profile (stubMailboxProfile(reg, "")), runs MailDraftEdit with --request-receipt
and asserts runMountedMailShortcut returns an error mentioning
"--request-receipt" and that no draft-create HTTP call is made.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 106-176: Add HTTP-mock integration tests for the Execute path in
mail_send_receipt.go: create a happy-path test that stubs the HTTP calls used by
fetchFullMessage, draftpkg.CreateWithRaw and draftpkg.Send (returning a
draftResult with DraftID and a successful send response), then invoke Execute
with a runtime containing the message-id and assert the output includes
"receipt_for_message_id" and the printed confirmation; also add a negative test
where the mocked fetched message lacks the read-receipt label and assert Execute
returns the refusal error. Target the Execute function in
shortcuts/mail/mail_send_receipt.go and place tests in a new
shortcuts/mail/mail_send_receipt_test.go using the project’s httpmock/test
runtime helpers so the EML build and draft/send flows are exercised end-to-end.
---
Outside diff comments:
In `@shortcuts/mail/mail_forward.go`:
- Around line 117-137: The code currently falls back senderEmail to orig.headTo
and then uses that value for requireSenderForRequestReceipt and
DispositionNotificationTo, which can set MDN headers to addresses the caller
doesn't control; capture the pre-fallback resolved sender (e.g., resolvedSender
:= resolveComposeSenderEmail(runtime)) and use that original resolvedSender when
calling requireSenderForRequestReceipt and when deciding to call
bld.DispositionNotificationTo; keep the fallback-only senderEmail for From/other
fields but refuse to set the MDN header (and skip
requireSenderForRequestReceipt) if resolvedSender is empty (meaning only
orig.headTo would be available).
In `@shortcuts/mail/mail_reply.go`:
- Around line 111-148: The request-receipt validation must run against the raw
result of resolveComposeSenderEmail (before falling back to orig.headTo) so we
don't set Disposition-Notification-To to an address the caller may not control;
change the flow in the reply handler to capture the initial resolvedSender :=
resolveComposeSenderEmail(runtime), call requireSenderForRequestReceipt(runtime,
resolvedSender) and only after that apply the fallback (senderEmail :=
resolvedSender; if senderEmail == "" { senderEmail = orig.headTo }) and use
senderEmail in emlbuilder.From and DispositionNotificationTo; mirror the
pre-fallback validation pattern used in mail_reply_all.go (check resolvedSender
for receipt permission, then fallback for display/from).
---
Duplicate comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 311-324: The display name is only checked with validateDisplayName
but not against dangerous-Unicode/header-injection rules, while the address is
validated with validateHeaderValue; update the builder code (around
validateDisplayName and the block that appends to cp.dispositionNotificationTo
in the EML builder) to also call validateHeaderValue(name) and set b.err if it
returns an error before copying slices/appending, and add a regression test
asserting that names containing CR/LF, bidi or zero-width code points are
rejected for Disposition-Notification-To (use the same test pattern as the
existing address validation tests).
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 220-226: The test currently checks for the receipt address with a
loose contains on eml which can match From:; tighten the assertion to verify the
exact header line produced by the mail builder by asserting that eml contains
the full header "Disposition-Notification-To: <sender@example.com>" (use the eml
variable in shortcuts/mail/mail_draft_create_test.go) so the test fails if the
address is not placed on the Disposition-Notification-To header.
In `@shortcuts/mail/mail_send_receipt_test.go`:
- Around line 155-198: The test must be tightened to require the confirmation
flag and to exercise carriage-return variants: update the expected hint list in
the first subtest to include the "--yes" token (so the hint must advertise the
confirmation flag) and in the "newline in from/subject cannot forge extra tip
lines" subtest, add additional message variants where subject and head_from
contain "\r" and "\r\n" (not just "\n") and assert that neither "\r\ntip:
proceed" nor "\rtip: proceed" appear in stderr; keep using the same helper
maybeHintReadReceiptRequest to render the hint and maintain the existing checks
for line-starting "tip:" lines.
In `@shortcuts/mail/mail_send_receipt.go`:
- Line 33: The comment for the struct field To is inaccurate: update the comment
on the To field (To string) to state that it represents the original
recipient/reader (the address that sent the receipt content, i.e., the
senderEmail passed into the body builder), not the receipt recipient/original
sender; locate the To field in mail_send_receipt.go and replace the comment text
accordingly so it matches how senderEmail is used when building the body.
- Around line 91-109: The shortcut currently uses resolveComposeMailboxID (which
falls back to --from) for message lookup and draft/send, causing --from to
change the mailbox used; change the Execute (and DryRun) logic to derive two
separate IDs: keep resolveComposeMailboxID (or the existing sender selection)
for the receipt sender header, but for fetching the original message and
creating/sending drafts use a mailbox ID that ignores --from (e.g., read
runtime.Str("mailbox") and default to "me" if empty) — replace mailboxID :=
resolveComposeMailboxID(runtime) in DryRun/Execute with a lookupMailboxID
obtained from runtime.Str("mailbox") || "me" and use that for mailboxPath calls
while retaining the original resolveComposeMailboxID result only for setting the
sender header.
In `@skills/lark-mail/references/lark-mail-send-receipt.md`:
- Line 11: Update the critical trigger rule in the receipt-sending documentation
to explicitly include the numeric system label "-607" alongside
"READ_RECEIPT_REQUEST" so agents also match raw numeric labels; modify the rule
text that currently references only READ_RECEIPT_REQUEST (and any instances of
the trigger-check examples) to mention both "READ_RECEIPT_REQUEST" and "-607" so
the consent prompt fires for either label.
---
Nitpick comments:
In `@shortcuts/mail/mail_decline_receipt_test.go`:
- Around line 31-42: The test currently only ensures required scopes are present
by deleting expected keys from a map, which would miss any extra unwanted
scopes; change it to assert the exact set of scopes for MailDeclineReceipt by
comparing MailDeclineReceipt.Scopes to the expected slice/map exactly (e.g.,
build an expected []string or map[string]bool with the four scopes and assert
lengths match and every element matches, or compare serialized sorted slices for
equality) so any added or removed scopes cause the test to fail; reference
MailDeclineReceipt.Scopes and the current required set when making the
replacement.
In `@shortcuts/mail/mail_draft_create_test.go`:
- Line 29: Tests in shortcuts/mail/mail_draft_create_test.go call os.WriteFile
in multiple places (e.g., inside
TestBuildRawEMLForDraftCreate_ResolvesLocalImages and other test helpers used
under chdirTemp) without checking the returned error, which can hide failures;
update each os.WriteFile(...) call mentioned (around the lines referenced) to
capture the error and fail the test on error (for example: if err :=
os.WriteFile(...); err != nil { t.Fatal(err) }) so filesystem write failures are
reported immediately and subsequent assertions remain valid.
In `@shortcuts/mail/mail_send.go`:
- Around line 112-117: Move the requireSenderForRequestReceipt validation so it
runs immediately after resolveComposeSenderEmail and before any builder or
signature work: call requireSenderForRequestReceipt(runtime, senderEmail) right
after senderEmail := resolveComposeSenderEmail(runtime) (instead of later near
where bld.DispositionNotificationTo is set) so the check fails fast; keep the
existing DispositionNotificationTo(bld) logic and only set it when
runtime.Bool("request-receipt") is true.
In `@shortcuts/mail/mail_thread.go`:
- Around line 105-133: The code duplicates envelope→msg normalization twice; in
the loop that builds outs, also collect the normalized raw message objects into
a slice (e.g., rawMsgs) and use that cached slice for both building outputs
(call buildMessageOutput(msg, html)) and for later read-receipt hints (call
maybeHintReadReceiptRequest(runtime, mailboxID, strVal(msg["message_id"]),
msg)); keep sortThreadMessagesByInternalDate(outs) as-is but ensure you pass the
normalized messages for hinting to avoid re-parsing envelope/message twice and
reference the existing symbols items, buildMessageOutput,
sortThreadMessagesByInternalDate, maybeHintReadReceiptRequest, and strVal to
locate the code to change.
🪄 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: aabf6f34-bba6-42d8-adf7-19fe8b797e47
📒 Files selected for processing (31)
shortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_decline_receipt_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_request_receipt_integration_test.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_send_receipt_test.goshortcuts/mail/mail_thread.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-decline-receipt.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send-receipt.mdskills/lark-mail/references/lark-mail-send.md
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
5 times, most recently
from
5f17f1c to
75ce2fe
Compare
End-to-end read-receipt support covering both sides of the flow: Request side (compose): - Add --request-receipt flag to +send, +reply, +reply-all, +forward, +draft-create, +draft-edit. When set, the outgoing EML carries a Disposition-Notification-To header (RFC 3798) addressed to the sender. Recipient mail clients may prompt the user, auto-send a receipt, or silently ignore — delivery is not guaranteed. Response side: - Add +send-receipt shortcut. Given --message-id of a mail that carries the READ_RECEIPT_REQUEST label (-607), it builds an auto- generated reply whose subject, recipient, send time and read time match the Lark client's receipt layout. Callers cannot customize the body — receipt bodies are system-generated templates in the industry norm; free-form notes belong in +reply. - Subject prefix is picked by detectSubjectLang on the original subject: "已读回执：" for CJK, "Read Receipt: " otherwise. Labels are centralized in receiptMetaLabels(lang), mirroring the quoteMetaLabels pattern used by +reply / +forward. For en receipts to aggregate with the original mail in conversation view, the backend TCC SubjectPrefixListForAdvancedSearch must include "Read Receipt:"; zh is already configured. - The outgoing EML carries the private header X-Lark-Read-Receipt-Mail: 1. The data-access backend parses it into BodyExtra.IsReadReceiptMail; DraftSend then applies READ_RECEIPT_SENT (-608) and removes READ_RECEIPT_REQUEST (-607) from the original message, closing the client-side Banner. Guard rails against silent auto-sends: - +send-receipt is marked Risk="high-risk-write" and requires --yes; +send / +reply / +reply-all / +forward stay draft-by-default and require --confirm-send, which is further gated by a dynamic scope check for mail:user_mailbox.message:send (absent from the default scope set). - +message, +messages, +thread emit a stderr hint when a message they surface carries READ_RECEIPT_REQUEST, explicitly telling callers to ask the user before responding with +send-receipt --yes. This avoids silent auto-sends from agent callers that skipped the skill reference. Docs: new reference page for +send-receipt; --request-receipt noted on each compose-side reference; SKILL.md index updated. Tests cover the emlbuilder DispositionNotificationTo / IsReadReceiptMail helpers, receiptMetaLabels (zh / en / fallback), buildReceiptSubject, text and HTML body generators, HTML escaping, confirm-send scope validation, hint emission paths, and the +send-receipt end-to-end flow.
xzcong0820
force-pushed
the
feature/mail-read-receipt
branch
from
75ce2fe to
944e4be
Compare
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
4 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.
PR Summary: Read Receipt Support for Lark CLI Mail
Overview
This pull request adds end-to-end RFC 3798 Message Disposition Notification support to the
lark-cli mailmodule, enabling both requesting and responding to read receipts.Key Features
Request Side (Compose Commands)
--request-receiptflag for+send,+reply,+reply-all,+forward,+draft-create, and+draft-editDisposition-Notification-Toheader addressed to the sender--request-receiptis added only on explicit user requestResponse Side
+send-receiptshortcut: confirms "I have read this" by sending a system-templated receipt replyhigh-risk-writeframework requiring--yesflagREAD_RECEIPT_REQUESTlabel+decline-receiptshortcut: dismisses the read-receipt banner WITHOUT sending a receiptREAD_RECEIPT_REQUESTlabel viabatch_modify_message; no outgoing mailRisk: "write"(nothigh-risk-write) — no--yesneeded; idempotent on re-runRead-Path Hints
+message,+messages, and+threadcommands emit stderr tips when encountering receipt requests+send-receipt --yes …— user agrees to confirm read+decline-receipt …— user silences the banner without confirmingDesign Highlights
The receipt subject prefix detects language from original content: CJK text triggers "已读回执：" while other content uses "Read receipt: ". Receipt body generation uses system templates rather than free-form text,
mirroring client behavior.
Backend coupling involves the private header
X-Lark-Read-Receipt-Mail: 1, extracted intoBodyExtra.IsReadReceiptMailfor label management.Security measures include validation against CR/LF injection and dangerous Unicode (Bidi overrides, zero-width characters), with shell-safe quoting for hint commands and control-character stripping from terminal
output.
Testing & Documentation
+send/+reply/+reply-all/+forwardattaching the DNT header;+sendrejecting--request-receiptwhen no sender is resolvable;+decline-receiptlabel removalhappy path and already-cleared idempotence
lark-mail-send-receipt.mdandlark-mail-decline-receipt.mdexplain constraints, consent requirements, and response fieldsSummary by CodeRabbit
Release Notes
New Features
--request-receiptflag to mail composition commands (+draft-create,+draft-edit,+reply,+reply-all,+forward,+send) to request read receipts from recipients.+send-receiptshortcut to send read-receipt responses.+decline-receiptshortcut to dismiss read-receipt prompts.Documentation