feat(mail): support --send-separately and set_send_separately patch op#835
feat(mail): support --send-separately and set_send_separately patch op#835sysuljx wants to merge 3 commits into
Conversation
Add a draft-level "send separately" flag to the mail compose link: - +draft-create and +send accept --send-separately, which injects an X-Lms-Send-Separately: 1 EML header into the base64url-encoded raw body posted to drafts.create. The server reads the header at send time and delivers per-recipient copies so each To/Cc recipient only sees themselves; Bcc visibility is unchanged. - +draft-edit gains a set_send_separately patch op that upserts the header for "true"/"1" and removes it for "false"/"0". - Dry-run preview for both compose shortcuts surfaces a send_separately field so callers can confirm the flag took effect without sending. A single effective recipient triggers a stderr warning (the feature has no observable effect) but is not rejected so users can still add recipients later via +draft-edit. Backend errno 6002 paired with --send-separately is wrapped with a hint pointing at the most common cause (server not yet updated). - Reference docs (mail-draft-create / mail-send / mail-draft-edit), the mail skill-template, and the patch template emitted by --print-patch-template document the new flag and patch op plus a comparison with Bcc semantics. - Tests cover header injection, dry-run preview, single-recipient warning, patch apply (true/false/1/0/case variants), and Validate rejection for invalid values. sprint: S9
Replace internal backend service names in the helpers.go const comment and skill-template Send Separately concept entry with a neutral "mail backend" phrasing. Behavior is unchanged; the X-Lms-Send-Separately header semantics and per-recipient delivery contract are preserved. sprint: S9
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds a ChangesSend Separately Feature Implementation
Sequence Diagram (high-level flow) sequenceDiagram
participant CLI
participant EMLBuilder
participant Backend
CLI->>EMLBuilder: parse --send-separately flag / include in request
EMLBuilder->>Backend: POST draft with X-Lms-Send-Separately header (if set)
Backend-->>EMLBuilder: success / errno 6002 if unsupported
EMLBuilder-->>CLI: return success or API error with backend support hint (on 6002)
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
Suggested labels: Suggested reviewers:
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/mail
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
shortcuts/mail/draft/patch_send_separately_test.go (1)
23-51: ⚡ Quick winConsider adding an idempotent-upsert test when header already exists.
Current tests verify insertion/removal well, but not the “already present + set true again” path. A small case here would catch accidental duplicate-header regressions.
Optional extra test case
+func TestApplySetSendSeparatelyTrueKeepsSingleHeaderWhenAlreadyPresent(t *testing.T) { + snapshot := mustParseFixtureDraft(t, `Subject: Test +From: Alice <alice@example.com> +To: Bob <bob@example.com> +X-Lms-Send-Separately: 1 +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 + +hello +`) + err := Apply(&DraftCtx{FIO: testFIO}, snapshot, Patch{ + Ops: []PatchOp{{Op: "set_send_separately", Value: "true"}}, + }) + if err != nil { + t.Fatalf("Apply() error = %v", err) + } + if got := headerValue(snapshot.Headers, "X-Lms-Send-Separately"); got != "1" { + t.Fatalf("X-Lms-Send-Separately = %q, want %q", got, "1") + } + if c := strings.Count(snapshot.Headers.String(), "X-Lms-Send-Separately:"); c != 1 { + t.Fatalf("expected exactly one X-Lms-Send-Separately header, got %d", c) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/mail/draft/patch_send_separately_test.go` around lines 23 - 51, Add an idempotency test that re-applies set_send_separately with a true value when the X-Lms-Send-Separately header already exists: create a snapshot (mustParseFixtureDraft/fixtureSendSeparatelyDraft), pre-insert or ensure snapshot.Headers contains X-Lms-Send-Separately="1", call Apply(&DraftCtx{FIO: testFIO}, snapshot, Patch{Ops: []PatchOp{{Op: "set_send_separately", Value: "true"}}}), then assert no duplicate headers were added and headerValue(snapshot.Headers, "X-Lms-Send-Separately") still equals "1"; place this alongside TestApplySetSendSeparatelyTrueAddsHeader/TestApplySetSendSeparatelyOneAddsHeader to catch duplicate-header regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/helpers.go`:
- Around line 2101-2107: The current isMailErrno6002 uses strings.Contains which
can false-positive on numbers like "16002"; update isMailErrno6002 to match the
standalone error code using a regex word-boundary match (e.g., use
regexp.MatchString with pattern `\b6002\b`) so only the exact token "6002" (not
digits embedded in other numbers) triggers true; compile the regexp once
(package-level var) and replace the strings.Contains check in isMailErrno6002
with regexp.Match to avoid performance hits and ensure correct matching.
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 489-495: The test currently checks for `"send_separately"` and
then a separate `"true"` which can yield false positives; update the second
assertion to check the full key/value pair (e.g. assert out contains
`"send_separately": true` or `"send_separately":true`) so the dry-run preview
actually shows send_separately set to true; alternatively parse stdout into JSON
and assert that the send_separately field is present and equals true (use the
existing out/stdout variables to locate the change).
In `@shortcuts/mail/mail_draft_create.go`:
- Around line 177-180: The warning logic currently counts BCC when computing
total recipients, which can suppress the single-recipient warning; change the
computation to only include To and CC addresses (use countAddresses(input.To) +
countAddresses(input.CC)) so the warning about --send-separately triggers
correctly when there is only one visible To/Cc recipient; update the block
around total := ... and the subsequent if total == 1 check in
mail_draft_create.go (referencing countAddresses and input.To/input.CC)
accordingly.
In `@shortcuts/mail/mail_send.go`:
- Around line 188-191: The warning for --send-separately is incorrectly counting
Bcc recipients; change the visible-recipient calculation to only include To and
Cc by replacing total := countAddresses(to) + countAddresses(ccFlag) +
countAddresses(bccFlag) with a total that sums only countAddresses(to) and
countAddresses(ccFlag), so the fmt.Fprintf warning (runtime.IO().ErrOut) is
emitted based on visible recipients (To/Cc) rather than including bccFlag.
In `@skills/lark-mail/references/lark-mail-draft-edit.md`:
- Around line 187-188: Doc currently lists accepted values for the draft "send
separately" option as "true"/"false"/"1"/"0" but the implementation and tests
accept mixed-case boolean strings; update the description for the `value` of
`X-Lms-Send-Separately` to state values are case-insensitive and that any case
variation of "true"/"1" will write `X-Lms-Send-Separately: 1` while any case
variation of "false"/"0" will remove the header, and that other values will be
rejected by the `Validate` logic; reference the header name
`X-Lms-Send-Separately` and the `Validate` behavior in the text so readers know
the mapping and rejection rules.
---
Nitpick comments:
In `@shortcuts/mail/draft/patch_send_separately_test.go`:
- Around line 23-51: Add an idempotency test that re-applies set_send_separately
with a true value when the X-Lms-Send-Separately header already exists: create a
snapshot (mustParseFixtureDraft/fixtureSendSeparatelyDraft), pre-insert or
ensure snapshot.Headers contains X-Lms-Send-Separately="1", call
Apply(&DraftCtx{FIO: testFIO}, snapshot, Patch{Ops: []PatchOp{{Op:
"set_send_separately", Value: "true"}}}), then assert no duplicate headers were
added and headerValue(snapshot.Headers, "X-Lms-Send-Separately") still equals
"1"; place this alongside
TestApplySetSendSeparatelyTrueAddsHeader/TestApplySetSendSeparatelyOneAddsHeader
to catch duplicate-header regressions.
🪄 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: 89a11182-da9c-4ab2-9095-b54703e3e473
📒 Files selected for processing (13)
shortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_send_separately_test.goshortcuts/mail/helpers.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_test.goskill-template/domains/mail.mdskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-send.md
| if !strings.Contains(out, `"send_separately"`) { | ||
| t.Fatalf("expected dry-run preview to surface send_separately, got: %s", out) | ||
| } | ||
| if !strings.Contains(out, "true") { |
There was a problem hiding this comment.
Issue: This still has the same false-positive shape as the +draft-create dry-run test: checking for a standalone "true" can pass because of any unrelated boolean in the rendered JSON, even if send_separately is missing or false.
Severity: 💡 Suggestion
Suggested Fix: Assert the full key/value pair (for example "send_separately":true / "send_separately": true) or decode the dry-run payload and verify the preview boolean directly.
There was a problem hiding this comment.
Same key/value-pair assertion applied to mail_send_test.go in commit c7b8763.
| {"op": "set_reply_body", "shape": map[string]interface{}{"value": "string (user-authored content only, WITHOUT the quote block; quote block, signature, and attachment cards are auto-preserved; supports <img src=\"./local/path.png\" /> — local paths auto-resolved to inline MIME parts)"}}, | ||
| {"op": "set_header", "shape": map[string]interface{}{"name": "string", "value": "string"}}, | ||
| {"op": "remove_header", "shape": map[string]interface{}{"name": "string"}}, | ||
| {"op": "set_send_separately", "shape": map[string]interface{}{"value": "\"true\"|\"false\"|\"1\"|\"0\""}, "note": "marks draft as send-separately; on send, each To/Cc recipient sees only themselves. Stores as EML header X-Lms-Send-Separately: 1; \"false\"/\"0\" removes the header. Differs from Bcc: Bcc hides recipients from everyone, while send-separately makes every recipient appear sole."}, |
There was a problem hiding this comment.
Issue: The generated --print-patch-template output is now out of sync with the parser. PatchOp.Validate() accepts mixed-case boolean spellings for set_send_separately via strings.ToLower(strings.TrimSpace(op.Value)), but this template still documents only lowercase literals, which can mislead users and agents that rely on the emitted schema rather than the markdown docs.
Severity: 💡 Suggestion
Suggested Fix: Update the template wording here (and the mirrored entry in the headers group below) to say the true/false forms are case-insensitive, matching the runtime behavior and tests.
There was a problem hiding this comment.
Updated both --print-patch-template entries (top-level and headers group) to call out the case-insensitive accepted values, in commit c7b8763.
- Replace strings.Contains "6002" with a word-boundary regex to avoid matching unrelated numbers such as "16002". - Drop Bcc from the single-recipient warn count in +draft-create and +send; --send-separately only affects To/Cc visibility, so counting Bcc could suppress the warning when there is no practical split. - Tighten dry-run assertions in mail_draft_create_test.go and mail_send_test.go to match the "send_separately":true pair instead of a standalone "true" substring. - Note in lark-mail-draft-edit.md and the --print-patch-template output that set_send_separately accepts case-insensitive true/false (matches PatchOp.Validate strings.ToLower behavior). Harness-Role: integrator
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
==========================================
+ Coverage 65.77% 67.45% +1.67%
==========================================
Files 516 572 +56
Lines 48625 53702 +5077
==========================================
+ Hits 31985 36223 +4238
- Misses 13881 14467 +586
- Partials 2759 3012 +253 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c7b8763bf30d0ba7d117b2ee153d54343be28975🧩 Skill updatenpx skills add sysuljx/cli#harness/01krb2wwgq3k3aww91gevepq2z -y -g |
|
🤖 AI Review | CR 汇总 | 可合入（未发现问题） 审查覆盖 SA（安全）/ DR（业务）/ DA（对抗）三个视角。代码实现规范，与既有模式一致，测试覆盖充分。已有 CodeRabbit 评论均已 resolved，不重复。 |
|
🤖 AI Review | [P3 性能/可维护性]
误匹配场景：如果后端返回的错误消息字符串中因其他原因包含 修复建议: 使用精确错误码匹配替代正则： // 方案 A：若 CallAPI 返回结构化错误，直接类型断言
if apiErr, ok := err.(*output.APIError); ok && apiErr.Code == 6002 { ... }
// 方案 B：若只能匹配字符串，用更精确的模式
strings.Contains(err.Error(), "errno=6002") || strings.Contains(err.Error(), "\"code\":6002")如有疑问或认为判断不准确，欢迎直接回复讨论。 |
|
🤖 AI Review | CR 汇总 | 可合入（1 个 P3 建议项） P0 安全：无 SQL 注入、租户隔离、敏感信息泄露或路径穿越问题。header 值硬编码为 P3（本 PR）： 跨仓库关联： |
5 participants
Generated by an automation pipeline.
Sprints
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests