feat(mail): support calendar events in emails#646
Open
chanthuang wants to merge 5 commits intomainfrom
Open
Conversation
github-actions
Bot
added
domain/mail
📝 WalkthroughWalkthroughAdds calendar-invite support: ICS generation/parsing, CLI event flags and validation, MIME-level insertion/removal of Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLIParser
participant Val as Validator
participant ICS as ICSBuilder
participant EML as EMLBuilder
participant MIME as MIMEHandler
User->>CLI: invoke command with --event-*
CLI->>Val: validate event flags & times
Val-->>CLI: ok / error
CLI->>ICS: Build VCALENDAR (organizer/attendees, UID, DTSTART/DTEND)
ICS-->>CLI: CalendarICS bytes
CLI->>EML: supply body + CalendarICS
EML->>MIME: insert text/calendar part or wrap in multipart/mixed
MIME-->>EML: updated MIME tree
EML-->>CLI: composed EML
CLI-->>User: send / draft result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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 |
chanthuang
force-pushed
the
feat/mail-calendar
branch
from
d0d5d3f to
b784861
Compare
chanthuang
changed the title
chanthuang
force-pushed
the
feat/mail-calendar
branch
from
b784861 to
5684fb6
Compare
chanthuang
changed the title
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5ed9bf7003cd0a92f170d2754e94e5bdfb16c2b0🧩 Skill updatenpx skills add larksuite/cli#feat/mail-calendar -y -g |
chanthuang
force-pushed
the
feat/mail-calendar
branch
from
5684fb6 to
6c3edc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
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_draft_edit.go (1)
414-430:⚠️ Potential issue | 🟡 MinorExpose calendar patch ops in the patch template.
+draft-edit --patch-filenow supportsset_calendarandremove_calendar, but--print-patch-templatestill omits them fromsupported_ops, so callers won’t discover the documented patch-file path.Proposed fix
{"op": "insert_signature", "shape": map[string]interface{}{"signature_id": "string (run mail +signature to list IDs)"}}, {"op": "remove_signature", "shape": map[string]interface{}{}, "note": "removes existing signature from the HTML body"}, + {"op": "set_calendar", "shape": map[string]interface{}{"event_summary": "string", "event_start": "ISO 8601 string", "event_end": "ISO 8601 string", "event_location": "string(optional)"}, "note": "attendees are derived from draft To/Cc; Bcc is excluded"}, + {"op": "remove_calendar", "shape": map[string]interface{}{}},🤖 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 414 - 430, The patch template's supported_ops list in mail_draft_edit.go omits the new calendar operations, so update the supported_ops slice (the literal that currently lists ops like "set_subject" and "add_attachment") to include entries for "set_calendar" (shape: {"value":"string (iCal or calendar data)"} or similar to other body ops) and "remove_calendar" (shape: {} or {"target":"string"} if applicable), matching the names used by draft-edit --patch-file handling; ensure the new entries follow the existing map[string]interface{} pattern so --print-patch-template will display 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/draft/model.go`:
- Around line 343-349: The set_calendar branch currently only checks for
non-empty op.EventSummary/op.EventStart/op.EventEnd; update the case
"set_calendar" block to also parse op.EventStart and op.EventEnd as ISO
timestamps (e.g. time.Parse(time.RFC3339) or RFC3339Nano) and return an error if
parsing fails, and ensure the parsed end time is strictly after the start time
(return an error if end <= start); place these checks after the empty-string
checks and before accepting the patch so op.EventStart/op.EventEnd validation
mirrors the compose event flags.
In `@shortcuts/mail/draft/patch_calendar.go`:
- Around line 79-83: The removeCalendarPart function currently only removes
calendar parts from children via removePartByMediaType and misses the case where
snapshot.Body itself is a leaf with MediaType "text/calendar" (which can be
created by setCalendarPart); update removeCalendarPart to check
snapshot.Body.MediaType == calendarMediaType and set snapshot.Body = nil (or
replace appropriately) when it matches, while retaining the existing
child-removal logic using removePartByMediaType; then add a regression test that
constructs a DraftSnapshot{Body: &Part{MediaType: "text/calendar"}} and asserts
that calling removeCalendarPart removes the body (and any other expected state),
ensuring the new behavior is covered.
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 692-696: The calendar MIME part must include method and name
params on its Content-Type; modify the calendar builder to carry the calendar
METHOD and optional filename from CalendarBody (or add a setter) into the
Builder so that Build() can pass them to writeSingleBodyPartHeaders; update
writeSingleBodyPartHeaders to accept optional contentTypeParams (or explicit
method and name args) and emit Content-Type: text/calendar; method=REQUEST;
charset=UTF-8; name="invite.ics" and also set Content-Disposition/filename
accordingly when b.calendarBody indicates a filename.
In `@shortcuts/mail/helpers.go`:
- Around line 1186-1188: The code exposes normalized.CalendarEvent as
out["calendar_event"] but printMessageOutputSchema still omits that field;
update the schema provider (e.g., function printMessageOutputSchema) to include
"calendar_event" and its subfields: "calendar_event.method",
"calendar_event.uid", "calendar_event.summary", "calendar_event.start",
"calendar_event.end", "calendar_event.location", "calendar_event.organizer", and
"calendar_event.attendees" so schema-driven callers can discover them, and add a
corresponding schema test assertion (following existing tests style) that
verifies the presence and expected types/structure of calendar_event and its
subfields whenever normalized.CalendarEvent is present.
- Around line 2030-2032: Update the event-mode validation to treat
--event-location the same as --event-summary/--event-start/--event-end: when
checking mutual exclusion with --send-time, include
runtime.Str("event-location") in the condition alongside
runtime.Str("event-summary"), runtime.Str("event-start"), and
runtime.Str("event-end") so that providing only --event-location triggers the
same error and is not silently ignored; update the error branch that returns
fmt.Errorf("--send-time and --event-* are mutually exclusive: ...") to cover
location as well. Also add unit tests that assert: (1) providing only
--event-location is treated as event-mode and rejected with the mutual-exclusion
error, and (2) providing --send-time together with --event-location produces the
same mutual-exclusion error; apply the same change to the other validation block
referenced (the one around the second similar check).
In `@shortcuts/mail/ics/builder.go`:
- Around line 116-126: quoteCNParam currently only strips double-quote
characters and then quotes on punctuation, but it doesn't remove control
characters like CR/LF which can break ICS lines; update quoteCNParam to remove
all control characters (e.g., CR, LF and other Unicode control codepoints) from
the input before stripping quotes and applying quoting logic so that
ORGANIZER/ATTENDEE parameters cannot inject newlines or properties. Locate the
quoteCNParam function and apply a pass that filters out characters where
unicode.IsControl(r) (or equivalent) before continuing with the existing
strings.ReplaceAll and strings.ContainsAny steps, then return the
cleaned-and-possibly-quoted string.
In `@shortcuts/mail/ics/ics_test.go`:
- Around line 250-259: Add explicit assertions to verify the second attendee and
the event end time so DTEND is not ignored nor Bob mis-parsed: after the
existing checks for event.Attendees[0] and Start, assert that event.Attendees[1]
== "bob@example.com" (using t.Errorf/t.Fatalf consistent with the file) and
assert event.End equals the expected wantEnd (e.g., time.Date(2026, 4, 20, 7, 0,
0, 0, time.UTC)) to ensure DTEND is parsed into event.End; reference the
event.Attendees slice and the event.End field in your new assertions.
- Around line 99-123: The test TestBuild_AutoUIDIsPureUUID should validate the
UID is a real UUID v4, not just length/no-@; after extracting the UID from the
Build(event) output (the UID line found in strings.Split on "\r\n"), parse it
with a UUID parser (e.g., github.com/google/uuid Parse or equivalent) and assert
parsing succeeds and uuid.Version() == 4; update the test to fail if parsing
errors or the version is not 4, while keeping the existing checks for missing
UID and no "@".
- Around line 177-198: Update TestBuild_CNWithSpecialCharsIsQuoted to add an
organizer/attendee name containing a double-quote and another name containing a
CR/LF injection attempt so the test verifies parameter escaping; use the
existing Build and unfoldLines helpers and assert that the ICS contains a quoted
CN with the internal quote escaped (e.g., CN="Evil \"Name\"" or equivalent
escaping used by Build) and that the payload does NOT contain the injected
property (e.g., no literal "ATTACH:malicious") nor raw CR/LF characters inside
the CN parameter; reference the TestBuild_CNWithSpecialCharsIsQuoted test, the
Event/Address structs, and the Build/unfoldLines functions when adding these
assertions.
- Around line 201-212: The test TestBuild_LineFolding permits unbounded
continuation lines by only checking lines that don't start with a space; update
the test to force multiple folds (increase Summary length) and enforce the
75-octet limit on every physical line including those starting with a space.
Modify TestBuild_LineFolding to use a longer Summary (so Build triggers multiple
folds) and change the validation loop over strings.Split(ics, "\r\n") to assert
len(line) <= 75 for all lines (no skip on strings.HasPrefix(line, " ")),
referencing the TestBuild_LineFolding test and the Build(event) call so the test
fails if any physical line exceeds 75 octets.
In `@shortcuts/mail/ics/parser.go`:
- Around line 115-122: splitProperty currently uses strings.Index which splits
on the first colon even if it is inside quoted parameter values (e.g., CN="Doe:
Jane"), so change splitProperty to scan the line and find the first colon that
is not inside double quotes (ignore escaped quotes if relevant) and return the
parts accordingly; update the function splitProperty to iterate chars, toggle an
inQuotes flag on '"' and only treat ':' as the separator when inQuotes is false.
Also add a parser unit test that feeds the line `ATTENDEE;CN="Doe:
Jane":MAILTO:jane@example.com` and asserts the name-with-params and value are
parsed so the MAILTO: part is preserved, as every behavior change must include a
test.
- Around line 129-143: The parseICSTime function currently checks for "TZID="
case-sensitively and uses TrimRight which can leave trailing parameters; update
parseICSTime to locate the TZID parameter case-insensitively (e.g., normalize
propName to lower or use strings.ToLower) and extract the timezone value up to
the next parameter delimiter (stop at the first ';' or ':' after the TZID=
token) before calling time.LoadLocation; ensure you still attempt
time.ParseInLocation with the extracted tzName and add a unit test covering a
header like "DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000" and a
lowercase "tzid" variant to validate behavior.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 343-371: The current flag-detection only checks
runtime.Str("set-event-summary") to enter calendar mode, so other flags can be
ignored or bypass mutual exclusion; change hasEventSet to true if any of
runtime.Str("set-event-summary"), runtime.Str("set-event-start"),
runtime.Str("set-event-end"), or runtime.Str("set-event-location") are
non-empty, and ensure the mutual-exclusion check with
runtime.Bool("remove-event") uses this new hasEventSet; when hasEventSet is true
validate that summary, start, and end are all provided (using
parseEventTimeRange for start/end validation and prefixEventRangeError for error
messages) before appending the draftpkg.PatchOp, and if remove-event is true
while any set-event-* is present return an ErrValidation; add unit tests
covering: set-event-start/start-only without summary, set-event-location
combined with --remove-event, and a successful fully-specified set-event case.
In `@shortcuts/mail/mail_send.go`:
- Around line 77-79: The validateEventFlags helper currently omits
--event-location from the "any event flag" check and allows using --send-time
with event mode; update validateEventFlags to include event-location in the set
of event-related flags (so checks like "anyEventFlagSet" consider
--event-location) and add a validation branch that rejects using --send-time
when runtime is in event mode (return a clear error message); locate and modify
the validateEventFlags function and its checks for --send-time,
--event-location, and the existing event-mode/any-event logic to enforce these
rules.
In `@skill-template/domains/mail.md`:
- Around line 224-227: The doc line claiming the backend will reject requests
when an email contains Bcc and a calendar is inaccurate given current behavior:
update the constraint text so it states that Bcc recipients are excluded from
calendar ATTENDEE (calendar generation only uses To/Cc) while the compose paths
still add Bcc to the EML, and remove the statement that the backend will reject
the request; alternatively, if you prefer rejection, implement matching
validation in the CLI that detects simultaneous use of Bcc with calendar flags
(`--event-*`/`--send-time`) and fails early — reference the flags `--event-*`
and `--send-time`, the recipient type `Bcc`, and the calendar `ATTENDEE`
handling to locate the relevant doc and code paths.
In `@skills/lark-mail/references/lark-mail-forward.md`:
- Around line 73-78: Update the flags table to document calendar-invite
constraints: add a concise note stating that the event flags (`--event-summary`,
`--event-start`, `--event-end`, `--event-location`) cannot be used together with
scheduled sending (`--send-time`) and that calendar invitations created by these
event flags will only make To and Cc recipients attendees (Bcc recipients will
not be added as attendees); also clarify that `--confirm-send` is required to
actually send invites and that `--send-time` must not be supplied when any
`--event-*` flag is present.
In `@skills/lark-mail/references/lark-mail-send.md`:
- Around line 80-85: Update the `--event-summary`, `--event-start`,
`--event-end`, `--event-location` and related `--event-*` flag documentation to
note two constraints: 1) these `--event-*` flags are mutually exclusive with
`--send-time` (you cannot schedule a calendar invite and also use
`--send-time`), and 2) Bcc recipients are not added as `ATTENDEE` entries in the
embedded calendar invite (Bcc should not be treated as event participants).
Mention these constraints near the existing flag descriptions (e.g., next to
`--event-summary`/`--event-start`/`--event-end`) and ensure the
`--confirm-send`/`--send-time` lines are updated to reflect the exclusivity with
event flags.
---
Outside diff comments:
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 414-430: The patch template's supported_ops list in
mail_draft_edit.go omits the new calendar operations, so update the
supported_ops slice (the literal that currently lists ops like "set_subject" and
"add_attachment") to include entries for "set_calendar" (shape: {"value":"string
(iCal or calendar data)"} or similar to other body ops) and "remove_calendar"
(shape: {} or {"target":"string"} if applicable), matching the names used by
draft-edit --patch-file handling; ensure the new entries follow the existing
map[string]interface{} pattern so --print-patch-template will display 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: 6a4c3381-0d4b-4a95-97d3-69beaba3d4f5
📒 Files selected for processing (25)
shortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/patch_calendar_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/ics/builder.goshortcuts/mail/ics/ics_test.goshortcuts/mail/ics/parser.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.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-forward.mdskills/lark-mail/references/lark-mail-message.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
Comment on lines
+79
to
+83
| func removeCalendarPart(snapshot *DraftSnapshot) { | ||
| if snapshot.Body == nil { | ||
| return | ||
| } | ||
| removePartByMediaType(snapshot.Body, calendarMediaType) |
There was a problem hiding this comment.
Handle standalone calendar bodies in remove_calendar.
setCalendarPart can create snapshot.Body as a root text/calendar leaf, but removeCalendarPart only removes matching children. remove_calendar therefore no-ops for calendar-only drafts.
Proposed fix
func removeCalendarPart(snapshot *DraftSnapshot) {
if snapshot.Body == nil {
return
}
+ if strings.EqualFold(snapshot.Body.MediaType, calendarMediaType) {
+ snapshot.Body = nil
+ return
+ }
removePartByMediaType(snapshot.Body, calendarMediaType)
}Please add a regression test for removing from a DraftSnapshot{Body: &Part{MediaType: "text/calendar"}}. As per coding guidelines, Every behavior change must have an accompanying test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func removeCalendarPart(snapshot *DraftSnapshot) { | |
| if snapshot.Body == nil { | |
| return | |
| } | |
| removePartByMediaType(snapshot.Body, calendarMediaType) | |
| func removeCalendarPart(snapshot *DraftSnapshot) { | |
| if snapshot.Body == nil { | |
| return | |
| } | |
| if strings.EqualFold(snapshot.Body.MediaType, calendarMediaType) { | |
| snapshot.Body = nil | |
| return | |
| } | |
| removePartByMediaType(snapshot.Body, calendarMediaType) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/draft/patch_calendar.go` around lines 79 - 83, The
removeCalendarPart function currently only removes calendar parts from children
via removePartByMediaType and misses the case where snapshot.Body itself is a
leaf with MediaType "text/calendar" (which can be created by setCalendarPart);
update removeCalendarPart to check snapshot.Body.MediaType == calendarMediaType
and set snapshot.Body = nil (or replace appropriately) when it matches, while
retaining the existing child-removal logic using removePartByMediaType; then add
a regression test that constructs a DraftSnapshot{Body: &Part{MediaType:
"text/calendar"}} and asserts that calling removeCalendarPart removes the body
(and any other expected state), ensuring the new behavior is covered.
Comment on lines
+1186
to
+1188
| if normalized.CalendarEvent != nil { | ||
| out["calendar_event"] = normalized.CalendarEvent | ||
| } |
There was a problem hiding this comment.
Keep --print-output-schema in sync with calendar_event.
This exposes calendar_event in +message output, but printMessageOutputSchema still omits the field and subfields, so schema-driven callers won’t discover it. Please add calendar_event, calendar_event.method, calendar_event.uid, calendar_event.summary, calendar_event.start, calendar_event.end, calendar_event.location, calendar_event.organizer, and calendar_event.attendees, plus a schema test assertion. 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/helpers.go` around lines 1186 - 1188, The code exposes
normalized.CalendarEvent as out["calendar_event"] but printMessageOutputSchema
still omits that field; update the schema provider (e.g., function
printMessageOutputSchema) to include "calendar_event" and its subfields:
"calendar_event.method", "calendar_event.uid", "calendar_event.summary",
"calendar_event.start", "calendar_event.end", "calendar_event.location",
"calendar_event.organizer", and "calendar_event.attendees" so schema-driven
callers can discover them, and add a corresponding schema test assertion
(following existing tests style) that verifies the presence and expected
types/structure of calendar_event and its subfields whenever
normalized.CalendarEvent is present.
Comment on lines
+2030
to
+2032
| if runtime.Str("event-summary") != "" { | ||
| return fmt.Errorf("--send-time and --event-* are mutually exclusive: a calendar invitation must be sent immediately so recipients can respond before the event") | ||
| } |
There was a problem hiding this comment.
Include --event-location in event-mode validation.
The validators only look at summary/start/end. --event-location by itself is accepted and ignored, and --send-time --event-location also bypasses the documented mutual exclusion.
Proposed fix
+func hasAnyEventFlag(runtime *common.RuntimeContext) bool {
+ return strings.TrimSpace(runtime.Str("event-summary")) != "" ||
+ strings.TrimSpace(runtime.Str("event-start")) != "" ||
+ strings.TrimSpace(runtime.Str("event-end")) != "" ||
+ strings.TrimSpace(runtime.Str("event-location")) != ""
+}
+
func validateSendTime(runtime *common.RuntimeContext) error {
sendTime := runtime.Str("send-time")
if sendTime == "" {
return nil
}
- if runtime.Str("event-summary") != "" {
+ if hasAnyEventFlag(runtime) {
return fmt.Errorf("--send-time and --event-* are mutually exclusive: a calendar invitation must be sent immediately so recipients can respond before the event")
}
@@
func validateEventFlags(runtime *common.RuntimeContext) error {
- summary := runtime.Str("event-summary")
- start := runtime.Str("event-start")
- end := runtime.Str("event-end")
+ summary := strings.TrimSpace(runtime.Str("event-summary"))
+ start := strings.TrimSpace(runtime.Str("event-start"))
+ end := strings.TrimSpace(runtime.Str("event-end"))
+ location := strings.TrimSpace(runtime.Str("event-location"))
- hasAny := summary != "" || start != "" || end != ""
+ hasAny := summary != "" || start != "" || end != "" || location != ""
hasAll := summary != "" && start != "" && end != ""Please add tests for --event-location alone and with --send-time. As per coding guidelines, Every behavior change must have an accompanying test.
Also applies to: 2214-2224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/helpers.go` around lines 2030 - 2032, Update the event-mode
validation to treat --event-location the same as
--event-summary/--event-start/--event-end: when checking mutual exclusion with
--send-time, include runtime.Str("event-location") in the condition alongside
runtime.Str("event-summary"), runtime.Str("event-start"), and
runtime.Str("event-end") so that providing only --event-location triggers the
same error and is not silently ignored; update the error branch that returns
fmt.Errorf("--send-time and --event-* are mutually exclusive: ...") to cover
location as well. Also add unit tests that assert: (1) providing only
--event-location is treated as event-mode and rejected with the mutual-exclusion
error, and (2) providing --send-time together with --event-location produces the
same mutual-exclusion error; apply the same change to the other validation block
referenced (the one around the second similar check).
Comment on lines
+77
to
+79
| if err := validateEventFlags(runtime); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Enforce the documented event validation in the shared helper.
validateEventFlags currently ignores both --send-time and --event-location, so scheduled calendar sends can pass validation and --event-location alone is silently ignored. Include event-location in the “any event flag” check and reject event mode with --send-time.
Proposed helper fix
func validateEventFlags(runtime *common.RuntimeContext) error {
summary := runtime.Str("event-summary")
start := runtime.Str("event-start")
end := runtime.Str("event-end")
+ location := runtime.Str("event-location")
- hasAny := summary != "" || start != "" || end != ""
+ hasAny := summary != "" || start != "" || end != "" || location != ""
hasAll := summary != "" && start != "" && end != ""
if hasAny && !hasAll {
return fmt.Errorf("--event-summary, --event-start, and --event-end must all be provided together")
}
+ if hasAny && runtime.Str("send-time") != "" {
+ return fmt.Errorf("--event-* cannot be used with --send-time")
+ }
if summary == "" {
return nil
}🤖 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 77 - 79, The validateEventFlags
helper currently omits --event-location from the "any event flag" check and
allows using --send-time with event mode; update validateEventFlags to include
event-location in the set of event-related flags (so checks like
"anyEventFlagSet" consider --event-location) and add a validation branch that
rejects using --send-time when runtime is in event mode (return a clear error
message); locate and modify the validateEventFlags function and its checks for
--send-time, --event-location, and the existing event-mode/any-event logic to
enforce these rules.
Comment on lines
+73
to
78
| | `--event-summary <text>` | 否 | 日程标题。设置此参数即在邮件中嵌入日程邀请。需同时设置 `--event-start` 和 `--event-end` | | ||
| | `--event-start <time>` | 条件必填 | 日程开始时间（ISO 8601） | | ||
| | `--event-end <time>` | 条件必填 | 日程结束时间（ISO 8601） | | ||
| | `--event-location <text>` | 否 | 日程地点 | | ||
| | `--confirm-send` | 否 | 确认发送转发（默认只保存草稿）。仅在用户明确确认后使用 | | ||
| | `--send-time <timestamp>` | 否 | 定时发送时间，Unix 时间戳（秒）。需至少为当前时间 + 5 分钟。配合 `--confirm-send` 使用可定时发送邮件 | |
There was a problem hiding this comment.
Document the calendar invite constraints next to the new flags.
This table introduces --event-* beside --send-time but omits the mutual exclusion and Bcc-attendee behavior. Add a short note that event flags cannot be combined with scheduled send, and that only To/Cc become attendees.
🤖 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` around lines 73 - 78,
Update the flags table to document calendar-invite constraints: add a concise
note stating that the event flags (`--event-summary`, `--event-start`,
`--event-end`, `--event-location`) cannot be used together with scheduled
sending (`--send-time`) and that calendar invitations created by these event
flags will only make To and Cc recipients attendees (Bcc recipients will not be
added as attendees); also clarify that `--confirm-send` is required to actually
send invites and that `--send-time` must not be supplied when any `--event-*`
flag is present.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
==========================================
- Coverage 62.89% 61.35% -1.54%
==========================================
Files 428 431 +3
Lines 37280 45719 +8439
==========================================
+ Hits 23448 28052 +4604
- Misses 11721 15539 +3818
- Partials 2111 2128 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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/emlbuilder/builder.go (1)
20-29:⚠️ Potential issue | 🟡 MinorUpdate the package MIME diagram to match the new calendar placement.
The implementation now puts
text/calendaras amultipart/mixedpeer to the body, but the package comment still shows it insidemultipart/alternativeand saysmultipart/mixedis only for attachments.Also applies to: 679-704
🤖 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 20 - 29, Update the package comment diagram in shortcuts/mail/emlbuilder/builder.go to reflect that text/calendar is now a multipart/mixed peer to the body rather than inside multipart/alternative: edit the MIME structure under the Build() comment so multipart/mixed is shown as parent for attachments and the calendar part (text/calendar appears alongside the body/related parts), and remove the inaccurate note that multipart/mixed is only when attachments exist; reference the Build() comment block and the MIME diagram lines to make the diagram and explanatory text match the current implementation.shortcuts/mail/mail_draft_edit.go (1)
414-430:⚠️ Potential issue | 🟡 MinorExpose
set_calendarandremove_calendarin the patch template.
--patch-filesupport is part of this feature, but--print-patch-templatestill does not list the new calendar ops, making them hard for schema-driven callers to discover.Suggested addition
{"op": "insert_signature", "shape": map[string]interface{}{"signature_id": "string (run mail +signature to list IDs)"}}, {"op": "remove_signature", "shape": map[string]interface{}{}, "note": "removes existing signature from the HTML body"}, + {"op": "set_calendar", "shape": map[string]interface{}{"event_summary": "string", "event_start": "ISO 8601 string", "event_end": "ISO 8601 string", "event_location": "string(optional)"}}, + {"op": "remove_calendar", "shape": map[string]interface{}{}, "note": "removes existing text/calendar invitation"},Please add a template assertion for these ops. As per coding guidelines, Every behavior change must have an accompanying test.
Also applies to: 465-472
🤖 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 414 - 430, The patch-template for mail draft edits is missing entries for the new "set_calendar" and "remove_calendar" operations; update the supported_ops template in the mail draft edit generator (the map literal named "supported_ops" in mail_draft_edit.go) to include {"op":"set_calendar", "shape": {...}} and {"op":"remove_calendar", "shape": {...}} matching the schema used by other ops, add the same entries where the second supported_ops list is constructed (the later block referenced in the comment), and add/adjust unit tests that assert --print-patch-template output contains "set_calendar" and "remove_calendar" so the behavior change is covered by tests.
♻️ Duplicate comments (9)
shortcuts/mail/emlbuilder/builder.go (1)
692-704:⚠️ Potential issue | 🟡 MinorCalendar MIME parts still need invite parameters.
text/calendaris still emitted with onlycharset=UTF-8; add the calendarmethodparameter and an.icsfilename/name for better client interoperability.Also applies to: 924-928
🤖 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 692 - 704, The text/calendar part is emitted without the iTIP parameters and filename; update the calendar part emission so the Content-Type includes the method parameter (e.g. method=REQUEST or the appropriate b.calendarMethod) and the name parameter (e.g. name="invite.ics"), and ensure a corresponding Content-Disposition/filename is set for clients; modify the two call sites that currently call writeSingleBodyPartHeaders(&buf, "text/calendar", b.calendarBody) (the branch inside the multipart outer block and the lone-calendar branch) to supply the calendar-specific Content-Type string (including method and name) or to call an updated helper that accepts method/name, and ensure writeSingleBodyPartHeaders (or an overloaded variant) writes the Content-Type header with method and name and a Content-Disposition with filename.shortcuts/mail/helpers.go (2)
2023-2032:⚠️ Potential issue | 🟠 MajorTreat all
--event-*flags as enabling event mode.Validation still checks only
event-summaryfor--send-timemutual exclusion and ignoresevent-locationinvalidateEventFlags;--event-locationalone can be accepted and ignored, and--send-time --event-locationcan bypass the intended conflict.Proposed fix
+func hasAnyEventFlag(runtime *common.RuntimeContext) bool { + return strings.TrimSpace(runtime.Str("event-summary")) != "" || + strings.TrimSpace(runtime.Str("event-start")) != "" || + strings.TrimSpace(runtime.Str("event-end")) != "" || + strings.TrimSpace(runtime.Str("event-location")) != "" +} + func validateSendTime(runtime *common.RuntimeContext) error { sendTime := runtime.Str("send-time") if sendTime == "" { return nil } - if runtime.Str("event-summary") != "" { + if hasAnyEventFlag(runtime) { return fmt.Errorf("--send-time and --event-* are mutually exclusive: a calendar invitation must be sent immediately so recipients can respond before the event") } @@ - summary := runtime.Str("event-summary") - start := runtime.Str("event-start") - end := runtime.Str("event-end") + summary := strings.TrimSpace(runtime.Str("event-summary")) + start := strings.TrimSpace(runtime.Str("event-start")) + end := strings.TrimSpace(runtime.Str("event-end")) + location := strings.TrimSpace(runtime.Str("event-location")) - hasAny := summary != "" || start != "" || end != "" + hasAny := summary != "" || start != "" || end != "" || location != ""Please add tests for
--event-locationalone and with--send-time. As per coding guidelines, Every behavior change must have an accompanying test.Also applies to: 2214-2224
60-105:⚠️ Potential issue | 🟡 MinorKeep
--print-output-schemain sync withcalendar_event.
buildMessageOutputnow exposescalendar_event, but the schema still omitscalendar_eventand its subfields, so schema-driven callers will not discover the new output. As per coding guidelines, Every behavior change must have an accompanying test.Also applies to: 1186-1188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 60 - 105, The printMessageOutputSchema function is missing the newly added calendar_event field and its subfields introduced by buildMessageOutput; update the schema map in printMessageOutputSchema to include "calendar_event" and all its subfields (matching the structure and names used in buildMessageOutput) so schema-driven callers can discover them, and add/update the corresponding unit/integration test to assert that --print-output-schema includes calendar_event and the expected subfields (reference symbols: printMessageOutputSchema and buildMessageOutput).shortcuts/mail/ics/parser.go (2)
132-143:⚠️ Potential issue | 🟠 MajorParse
TZIDparameters case-insensitively and stop at the next parameter.
strings.Contains(propName, "TZID=")misses lowercase/mixed-case params, andstrings.TrimRight(parts[1], ";")leaves trailing params like;VALUE=DATE-TIME, makingtime.LoadLocationfail.Proposed fix
- if strings.Contains(propName, "TZID=") { - parts := strings.SplitN(propName, "TZID=", 2) - if len(parts) == 2 { - tzName := strings.TrimRight(parts[1], ";") - if loc, err := time.LoadLocation(tzName); err == nil { - if t, err := time.ParseInLocation("20060102T150405", value, loc); err == nil { - return t - } + for _, param := range strings.Split(propName, ";")[1:] { + key, val, ok := strings.Cut(param, "=") + if !ok || !strings.EqualFold(key, "TZID") { + continue + } + if loc, err := time.LoadLocation(val); err == nil { + if t, err := time.ParseInLocation("20060102T150405", value, loc); err == nil { + return t } } }Please cover
DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000and lowercasetzid. 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/ics/parser.go` around lines 132 - 143, The TZID extraction is case-sensitive and can include trailing params; update the check to find "TZID=" case-insensitively (e.g., use strings.Index(strings.ToUpper(propName), "TZID=") or strings.EqualFold for the key) and then extract the value by locating the start after "TZID=" and taking characters up to the next ';' or ':' (not just trimming ';') so trailing params like ";VALUE=DATE-TIME" are excluded; then TrimSpace and trim optional surrounding quotes before calling time.LoadLocation and time.ParseInLocation (keep existing formats), and add a unit test covering "DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000" and a lowercase "tzid" variant to validate the behavior.
115-122:⚠️ Potential issue | 🟠 MajorSplit on the first unquoted colon.
The comment says this handles quoted colons, but
strings.Indexsplits inside values such asCN="Doe: Jane", which can lose theMAILTO:value for organizer/attendee parsing.Proposed fix
func splitProperty(line string) (string, string) { - idx := strings.Index(line, ":") - if idx < 0 { - return line, "" + inQuotes := false + escaped := false + for i, r := range line { + if escaped { + escaped = false + continue + } + switch r { + case '\\': + escaped = true + case '"': + inQuotes = !inQuotes + case ':': + if !inQuotes { + return line[:i], line[i+1:] + } + } } - return line[:idx], line[idx+1:] + return line, "" }Please add a parser test for
ATTENDEE;CN="Doe: Jane":MAILTO:jane@example.com. 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/ics/parser.go` around lines 115 - 122, splitProperty currently uses strings.Index and will split on colons inside quoted parameter values (e.g., CN="Doe: Jane"), so update splitProperty to scan the line byte-by-byte and return the position of the first colon that is not inside double quotes (track entering/exiting quote state and ignore escaped quotes) and then split at that index; after changing the implementation of splitProperty, add a unit test that verifies parsing of the example "ATTENDEE;CN=\"Doe: Jane\":MAILTO:jane@example.com" so the function returns the name-with-params "ATTENDEE;CN=\"Doe: Jane\"" and value "MAILTO:jane@example.com".skills/lark-mail/references/lark-mail-forward.md (1)
73-78:⚠️ Potential issue | 🟡 MinorDocument the calendar invite constraints next to these flags.
This still omits that
--event-*cannot be combined with--send-time, To/Cc become attendees, Bcc is excluded, and--confirm-sendis required to send immediately.🤖 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` around lines 73 - 78, Update the documentation lines for the calendar-related flags to include the missing constraints: state that the `--event-summary`, `--event-start`, `--event-end`, and `--event-location` (collectively the `--event-*` flags) cannot be used together with `--send-time`; clarify that recipients in To and Cc are treated as event attendees while Bcc recipients are excluded from the calendar invite; and note that `--confirm-send` is required to actually send the invite immediately (otherwise it only saves a draft), while `--send-time` must be used with `--confirm-send` for scheduled sending and must be at least current time + 5 minutes. Ensure these constraints are added adjacent to the existing flag descriptions for `--event-summary`, `--event-start`, `--event-end`, `--event-location`, `--confirm-send`, and `--send-time`.shortcuts/mail/mail_draft_edit.go (1)
343-371:⚠️ Potential issue | 🟠 MajorValidate every
--set-event-*flag, not just summary.
hasEventSetonly checks--set-event-summary, so--set-event-start,--set-event-end, or--set-event-locationcan be silently ignored and can bypass--remove-eventmutual exclusion.Proposed fix
- hasEventSet := runtime.Str("set-event-summary") != "" + summary := strings.TrimSpace(runtime.Str("set-event-summary")) + start := strings.TrimSpace(runtime.Str("set-event-start")) + end := strings.TrimSpace(runtime.Str("set-event-end")) + location := strings.TrimSpace(runtime.Str("set-event-location")) + hasEventSet := summary != "" || start != "" || end != "" || location != "" hasEventRemove := runtime.Bool("remove-event") if hasEventSet && hasEventRemove { - return patch, output.ErrValidation("--set-event-summary and --remove-event are mutually exclusive") + return patch, output.ErrValidation("--set-event-* and --remove-event are mutually exclusive") } if hasEventSet { - summary := runtime.Str("set-event-summary") - start := runtime.Str("set-event-start") - end := runtime.Str("set-event-end") if summary == "" || start == "" || end == "" { return patch, output.ErrValidation("--set-event-summary, --set-event-start, and --set-event-end must all be provided together") } @@ EventSummary: summary, EventStart: start, EventEnd: end, - EventLocation: runtime.Str("set-event-location"), + EventLocation: location, })Please add coverage for start/location-only inputs and
--remove-eventcombinations. 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_draft_edit.go` around lines 343 - 371, The current check uses hasEventSet := runtime.Str("set-event-summary") != "" so flags like --set-event-start, --set-event-end or --set-event-location can be provided alone or with --remove-event; update the validation to consider any set-event-* flag as "set" (e.g., check runtime.Str("set-event-summary") != "" || runtime.Str("set-event-start") != "" || runtime.Str("set-event-end") != "" || runtime.Str("set-event-location") != "") and then require summary, start and end to be non-empty (use the existing parseEventTimeRange(start, end) and prefixEventRangeError helpers) before appending the draftpkg.PatchOp; also ensure mutual exclusion with runtime.Bool("remove-event") still applies and add unit tests covering: start-only, location-only, summary+remove-event combos and the valid full set-event-* path.shortcuts/mail/draft/patch_calendar.go (1)
79-84:⚠️ Potential issue | 🟠 Major
remove_calendarno-ops for calendar-only drafts.
setCalendarPartcan producesnapshot.Bodyas a roottext/calendarleaf (Lines 32-40), butremoveCalendarPartonly descends into children viaremovePartByMediaType, which iteratesroot.Childrenand never matchesrootitself. A draft whose body is a standalone calendar part will not be cleared.Proposed fix
func removeCalendarPart(snapshot *DraftSnapshot) { if snapshot.Body == nil { return } + if strings.EqualFold(snapshot.Body.MediaType, calendarMediaType) { + snapshot.Body = nil + return + } removePartByMediaType(snapshot.Body, calendarMediaType) }Please add a regression test for
DraftSnapshot{Body: &Part{MediaType: "text/calendar"}}. 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/draft/patch_calendar.go` around lines 79 - 84, removeCalendarPart currently only looks into snapshot.Body.Children via removePartByMediaType and misses the case where snapshot.Body itself is a text/calendar leaf; update removeCalendarPart to check snapshot.Body.MediaType (or use removePartByMediaType on the root including root match) and clear snapshot.Body when it equals calendarMediaType, and add a unit test asserting that DraftSnapshot{Body: &Part{MediaType: "text/calendar"}} is cleared by removeCalendarPart; reference functions/types: removeCalendarPart, removePartByMediaType, setCalendarPart, DraftSnapshot, Part, calendarMediaType.shortcuts/mail/ics/builder.go (1)
118-128:⚠️ Potential issue | 🟠 Major
quoteCNParamstill permits CR/LF and other control chars.
strings.ReplaceAll(s,", "")strips only double quotes; a display name containing\r,\n, or other C0/C1 control chars can break the foldedORGANIZER/ATTENDEEline and inject a fresh iCalendar property (e.g.\nATTENDEE:mailto:evil@...).Namehere typically flows fromFrom/To/Ccaddresses which may originate from untrusted headers, so this is a real injection sink even thoughSUMMARY/LOCATIONare already escaped.Proposed fix
func quoteCNParam(s string) string { - s = strings.ReplaceAll(s, `"`, "") + s = strings.Map(func(r rune) rune { + if r == '"' || r == '\r' || r == '\n' || r < 0x20 || r == 0x7f { + return -1 + } + return r + }, s) if strings.ContainsAny(s, ",:;") { return `"` + s + `"` } return s }As per coding guidelines, "Every behavior change must have an accompanying test" — please add a CN test covering
\r,\n, and embedded"to lock this down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/ics/builder.go` around lines 118 - 128, The quoteCNParam function currently only strips DQUOTE but leaves CR/LF and other control characters, enabling header injection via CN values; update quoteCNParam to remove or replace all C0/C1 control characters (e.g., strip characters <= 0x1F and 0x7F–0x9F) and still remove embedded double-quotes before quoting so the returned param cannot contain CR/LF or other non-printable controls, and add unit tests for quoteCNParam covering inputs with "\r", "\n", and embedded "\"" to ensure the sanitized output never contains control chars or raw quotes.
🤖 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/draft/patch_calendar.go`:
- Around line 60-77: The wrapper part retains stale headers because the code
does a shallow copy with original := *snapshot.Body and then mutates
snapshot.Body.MediaType without clearing snapshot.Body.Headers, causing
serialize.go:orderedPartHeaders() to prefer the old Content-Type and other
headers; fix by clearing or reinitializing snapshot.Body.Headers (and any
header-related fields like Content-Transfer-Encoding/Content-ID/PartID) after
changing MediaType in the function that builds the wrapper (the block that sets
snapshot.Body.MediaType = "multipart/mixed" and snapshot.Body.Children),
ensuring the serializer will generate a fresh Content-Type from MediaType; add a
unit test TestSetCalendar_WrapsTextHTMLInMultipartMixed that parses a text/html
draft, calls the calendar wrapper code, serializes the result, and asserts the
outer part's Content-Type equals "multipart/mixed".
In `@skills/lark-mail/references/lark-mail-draft-create.md`:
- Around line 57-60: Add a clear note to the calendar-related flags
(`--event-summary`, `--event-start`, `--event-end`, `--event-location`)
explaining that attendee entries for generated calendar invites are derived only
from To and Cc recipients and that Bcc recipients are intentionally excluded;
update the description for the event flags to explicitly state this constraint
so agents do not imply hidden/Bcc attendees will appear in the invite.
---
Outside diff comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 20-29: Update the package comment diagram in
shortcuts/mail/emlbuilder/builder.go to reflect that text/calendar is now a
multipart/mixed peer to the body rather than inside multipart/alternative: edit
the MIME structure under the Build() comment so multipart/mixed is shown as
parent for attachments and the calendar part (text/calendar appears alongside
the body/related parts), and remove the inaccurate note that multipart/mixed is
only when attachments exist; reference the Build() comment block and the MIME
diagram lines to make the diagram and explanatory text match the current
implementation.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 414-430: The patch-template for mail draft edits is missing
entries for the new "set_calendar" and "remove_calendar" operations; update the
supported_ops template in the mail draft edit generator (the map literal named
"supported_ops" in mail_draft_edit.go) to include {"op":"set_calendar", "shape":
{...}} and {"op":"remove_calendar", "shape": {...}} matching the schema used by
other ops, add the same entries where the second supported_ops list is
constructed (the later block referenced in the comment), and add/adjust unit
tests that assert --print-patch-template output contains "set_calendar" and
"remove_calendar" so the behavior change is covered by tests.
---
Duplicate comments:
In `@shortcuts/mail/draft/patch_calendar.go`:
- Around line 79-84: removeCalendarPart currently only looks into
snapshot.Body.Children via removePartByMediaType and misses the case where
snapshot.Body itself is a text/calendar leaf; update removeCalendarPart to check
snapshot.Body.MediaType (or use removePartByMediaType on the root including root
match) and clear snapshot.Body when it equals calendarMediaType, and add a unit
test asserting that DraftSnapshot{Body: &Part{MediaType: "text/calendar"}} is
cleared by removeCalendarPart; reference functions/types: removeCalendarPart,
removePartByMediaType, setCalendarPart, DraftSnapshot, Part, calendarMediaType.
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 692-704: The text/calendar part is emitted without the iTIP
parameters and filename; update the calendar part emission so the Content-Type
includes the method parameter (e.g. method=REQUEST or the appropriate
b.calendarMethod) and the name parameter (e.g. name="invite.ics"), and ensure a
corresponding Content-Disposition/filename is set for clients; modify the two
call sites that currently call writeSingleBodyPartHeaders(&buf, "text/calendar",
b.calendarBody) (the branch inside the multipart outer block and the
lone-calendar branch) to supply the calendar-specific Content-Type string
(including method and name) or to call an updated helper that accepts
method/name, and ensure writeSingleBodyPartHeaders (or an overloaded variant)
writes the Content-Type header with method and name and a Content-Disposition
with filename.
In `@shortcuts/mail/helpers.go`:
- Around line 60-105: The printMessageOutputSchema function is missing the newly
added calendar_event field and its subfields introduced by buildMessageOutput;
update the schema map in printMessageOutputSchema to include "calendar_event"
and all its subfields (matching the structure and names used in
buildMessageOutput) so schema-driven callers can discover them, and add/update
the corresponding unit/integration test to assert that --print-output-schema
includes calendar_event and the expected subfields (reference symbols:
printMessageOutputSchema and buildMessageOutput).
In `@shortcuts/mail/ics/builder.go`:
- Around line 118-128: The quoteCNParam function currently only strips DQUOTE
but leaves CR/LF and other control characters, enabling header injection via CN
values; update quoteCNParam to remove or replace all C0/C1 control characters
(e.g., strip characters <= 0x1F and 0x7F–0x9F) and still remove embedded
double-quotes before quoting so the returned param cannot contain CR/LF or other
non-printable controls, and add unit tests for quoteCNParam covering inputs with
"\r", "\n", and embedded "\"" to ensure the sanitized output never contains
control chars or raw quotes.
In `@shortcuts/mail/ics/parser.go`:
- Around line 132-143: The TZID extraction is case-sensitive and can include
trailing params; update the check to find "TZID=" case-insensitively (e.g., use
strings.Index(strings.ToUpper(propName), "TZID=") or strings.EqualFold for the
key) and then extract the value by locating the start after "TZID=" and taking
characters up to the next ';' or ':' (not just trimming ';') so trailing params
like ";VALUE=DATE-TIME" are excluded; then TrimSpace and trim optional
surrounding quotes before calling time.LoadLocation and time.ParseInLocation
(keep existing formats), and add a unit test covering
"DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000" and a lowercase
"tzid" variant to validate the behavior.
- Around line 115-122: splitProperty currently uses strings.Index and will split
on colons inside quoted parameter values (e.g., CN="Doe: Jane"), so update
splitProperty to scan the line byte-by-byte and return the position of the first
colon that is not inside double quotes (track entering/exiting quote state and
ignore escaped quotes) and then split at that index; after changing the
implementation of splitProperty, add a unit test that verifies parsing of the
example "ATTENDEE;CN=\"Doe: Jane\":MAILTO:jane@example.com" so the function
returns the name-with-params "ATTENDEE;CN=\"Doe: Jane\"" and value
"MAILTO:jane@example.com".
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 343-371: The current check uses hasEventSet :=
runtime.Str("set-event-summary") != "" so flags like --set-event-start,
--set-event-end or --set-event-location can be provided alone or with
--remove-event; update the validation to consider any set-event-* flag as "set"
(e.g., check runtime.Str("set-event-summary") != "" ||
runtime.Str("set-event-start") != "" || runtime.Str("set-event-end") != "" ||
runtime.Str("set-event-location") != "") and then require summary, start and end
to be non-empty (use the existing parseEventTimeRange(start, end) and
prefixEventRangeError helpers) before appending the draftpkg.PatchOp; also
ensure mutual exclusion with runtime.Bool("remove-event") still applies and add
unit tests covering: start-only, location-only, summary+remove-event combos and
the valid full set-event-* path.
In `@skills/lark-mail/references/lark-mail-forward.md`:
- Around line 73-78: Update the documentation lines for the calendar-related
flags to include the missing constraints: state that the `--event-summary`,
`--event-start`, `--event-end`, and `--event-location` (collectively the
`--event-*` flags) cannot be used together with `--send-time`; clarify that
recipients in To and Cc are treated as event attendees while Bcc recipients are
excluded from the calendar invite; and note that `--confirm-send` is required to
actually send the invite immediately (otherwise it only saves a draft), while
`--send-time` must be used with `--confirm-send` for scheduled sending and must
be at least current time + 5 minutes. Ensure these constraints are added
adjacent to the existing flag descriptions for `--event-summary`,
`--event-start`, `--event-end`, `--event-location`, `--confirm-send`, and
`--send-time`.
🪄 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: 9ff4c3b8-92f8-4c4a-a289-f3ba6c9162a4
📒 Files selected for processing (25)
shortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/patch_calendar_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/ics/builder.goshortcuts/mail/ics/ics_test.goshortcuts/mail/ics/parser.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.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-forward.mdskills/lark-mail/references/lark-mail-message.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (4)
- skills/lark-mail/references/lark-mail-message.md
- skills/lark-mail/references/lark-mail-reply.md
- skills/lark-mail/references/lark-mail-reply-all.md
- skill-template/domains/mail.md
🚧 Files skipped from review as they are similar to previous changes (9)
- shortcuts/mail/draft/patch.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/emlbuilder/builder_test.go
- skills/lark-mail/references/lark-mail-send.md
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_forward.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_send.go
Comment on lines
+60
to
+77
| // Single leaf or non-mixed multipart (e.g. multipart/alternative, | ||
| // multipart/related): wrap in multipart/mixed so the body and the | ||
| // calendar invitation are independent peers. | ||
| original := *snapshot.Body | ||
| snapshot.Body.MediaType = "multipart/mixed" | ||
| snapshot.Body.MediaParams = nil | ||
| snapshot.Body.Body = nil | ||
| snapshot.Body.Children = []*Part{ | ||
| &original, | ||
| { | ||
| MediaType: calendarMediaType, | ||
| MediaParams: map[string]string{"charset": "UTF-8"}, | ||
| Body: icsData, | ||
| Dirty: true, | ||
| }, | ||
| } | ||
| snapshot.Body.Dirty = true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where Part.Headers and MediaType are serialized into MIME bytes.
rg -nP --type=go -C4 '\b(Headers|MediaType|MediaParams)\b' shortcuts/mail/draft/ | rg -n 'serialize|Serialize|encode|Encode|Write|Render'
fd -e go . shortcuts/mail/draft | xargs rg -nP -C3 'func.*\(.*\*Part\).*(Serialize|Encode|Render|Write)'
# Check for any test that wraps a non-mixed body and inspects resulting Content-Type.
rg -nP --type=go -C5 'multipart/mixed' shortcuts/mail/draft/Repository: larksuite/cli
Length of output: 39466
🏁 Script executed:
#!/bin/bash
# Find tests in patch_calendar_test.go that might verify serialization
rg -nP --type=go -A 20 'func Test.*[Cc]alendar' shortcuts/mail/draft/patch_calendar_test.go | head -100
# Check if any test inspects Headers or Content-Type after wrapping
rg -nP --type=go -C 5 'wrap.*calendar|calendar.*wrap' shortcuts/mail/draft/patch_calendar_test.go
# Search for tests that serialize and inspect the result
rg -nP --type=go -C 5 'Serialize|RawEntity' shortcuts/mail/draft/patch_calendar_test.goRepository: larksuite/cli
Length of output: 3394
🏁 Script executed:
#!/bin/bash
# Find the exact structure of the wrapping code to see if Headers are modified
sed -n '60,80p' shortcuts/mail/draft/patch_calendar.go
cat -n shortcuts/mail/draft/patch_calendar.go | sed -n '40,80p'Repository: larksuite/cli
Length of output: 2275
🏁 Script executed:
#!/bin/bash
# Look at what the test does after applying the calendar patch
sed -n '47,80p' shortcuts/mail/draft/patch_calendar_test.goRepository: larksuite/cli
Length of output: 1095
🏁 Script executed:
#!/bin/bash
# Check the serialize.go orderedPartHeaders function more closely
sed -n '118,170p' shortcuts/mail/draft/serialize.goRepository: larksuite/cli
Length of output: 1795
🏁 Script executed:
#!/bin/bash
# Look for any test that verifies serialized Content-Type after wrapping
rg -nP --type=go -B 5 -A 10 'multipart/mixed.*Content-Type|Content-Type.*multipart/mixed' shortcuts/mail/draft/patch_calendar_test.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if TestSetCalendar_AddsCalendarPartToHTMLDraft serializes the result
sed -n '47,80p' shortcuts/mail/draft/patch_calendar_test.go | grep -i "serial\|Raw\|Bytes\|Marshal"Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find if any test in patch_calendar_test.go calls Serialize on the snapshot after patching
rg -nP --type=go -A 30 'TestSetCalendar_AddsCalendarPartToHTMLDraft' shortcuts/mail/draft/patch_calendar_test.goRepository: larksuite/cli
Length of output: 969
🏁 Script executed:
#!/bin/bash
# Check what the Part struct looks like and confirm Headers is a slice
rg -nP --type=go -A 30 'type Part struct' shortcuts/mail/draft/model.go | head -50Repository: larksuite/cli
Length of output: 991
🏁 Script executed:
#!/bin/bash
# Check if there are any integration tests that serialize after calendar wrapping
rg -nP --type=go -B 5 -A 20 'Serialize|RawEntity' shortcuts/mail/draft/patch_calendar_test.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for the existing test pattern used in serialize_test.go for round-tripping
sed -n '85,130p' shortcuts/mail/draft/serialize_test.goRepository: larksuite/cli
Length of output: 1507
🏁 Script executed:
#!/bin/bash
# Check if there's a test that parses a text/html body and wraps it, then serializes
rg -nP --type=go -C 8 'text/html.*wrap\|wrap.*text/html' shortcuts/mail/draft/Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Trace through what happens when the wrapper is serialized
# Check if snapshot.Body.Headers remains unmodified after the wrapping
cat shortcuts/mail/draft/patch_calendar.go | sed -n '60,77p'Repository: larksuite/cli
Length of output: 598
🏁 Script executed:
#!/bin/bash
# Write a test to verify the bug - create a minimal proof-of-concept
python3 << 'EOF'
# Analyze the shallow copy aliasing in the wrapping code
print("=== Shallow Copy Analysis ===")
print("Line 63: original := *snapshot.Body")
print(" -> Creates a shallow copy of the Part struct")
print(" -> original.Headers and snapshot.Body.Headers point to SAME underlying slice")
print()
print("Lines 64-75: Modify snapshot.Body in place:")
print(" - snapshot.Body.MediaType = 'multipart/mixed'")
print(" - snapshot.Body.MediaParams = nil")
print(" - snapshot.Body.Body = nil")
print(" - snapshot.Body.Children = [...]")
print(" BUT: snapshot.Body.Headers is NOT modified!")
print()
print("Result:")
print(" - snapshot.Body now has MediaType='multipart/mixed'")
print(" - But snapshot.Body.Headers still contains the original's headers")
print(" - Including the original Content-Type header")
print()
print("=== Serialization Impact ===")
print("In serialize.go orderedPartHeaders (line 119):")
print(" contentTypeValue := existingHeaderValue(part.Headers, 'Content-Type')")
print(" if contentTypeValue == '' {")
print(" contentTypeValue = mime.FormatMediaType(part.MediaType, ...)")
print(" }")
print()
print("Since snapshot.Body.Headers still contains the original Content-Type,")
print("the serializer will emit that stale header instead of constructing")
print("a new multipart/mixed Content-Type from MediaType.")
EOFRepository: larksuite/cli
Length of output: 1102
🏁 Script executed:
#!/bin/bash
# Confirm that orderedPartHeaders uses existing header value
sed -n '118,130p' shortcuts/mail/draft/serialize.goRepository: larksuite/cli
Length of output: 485
Wrapper part retains stale headers when MediaType is changed.
When the code does original := *snapshot.Body (shallow copy) and then changes snapshot.Body.MediaType to "multipart/mixed" without clearing snapshot.Body.Headers, the serializer will emit the original body's Content-Type header verbatim instead of constructing a new one for the multipart container. In serialize.go:orderedPartHeaders() (line 119), it checks for an existing Content-Type header first before using MediaType; stale headers like "text/html; charset=UTF-8", Content-Transfer-Encoding, Content-ID, and PartID will leak onto the wrapper, producing malformed MIME.
The proposed fix is correct:
Proposed fix
original := *snapshot.Body
snapshot.Body.MediaType = "multipart/mixed"
snapshot.Body.MediaParams = nil
snapshot.Body.Body = nil
+ snapshot.Body.Headers = nil
+ snapshot.Body.ContentDisposition = ""
+ snapshot.Body.ContentDispositionArg = nil
+ snapshot.Body.ContentID = ""
+ snapshot.Body.TransferEncoding = ""
+ snapshot.Body.PartID = ""
snapshot.Body.Children = []*Part{
&original,Also add a test that serializes the wrapped result and verifies the wrapper's Content-Type header is correct (e.g., TestSetCalendar_WrapsTextHTMLInMultipartMixed that parses a text/html draft, applies calendar, serializes, and checks the outer part's Content-Type is multipart/mixed).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Single leaf or non-mixed multipart (e.g. multipart/alternative, | |
| // multipart/related): wrap in multipart/mixed so the body and the | |
| // calendar invitation are independent peers. | |
| original := *snapshot.Body | |
| snapshot.Body.MediaType = "multipart/mixed" | |
| snapshot.Body.MediaParams = nil | |
| snapshot.Body.Body = nil | |
| snapshot.Body.Children = []*Part{ | |
| &original, | |
| { | |
| MediaType: calendarMediaType, | |
| MediaParams: map[string]string{"charset": "UTF-8"}, | |
| Body: icsData, | |
| Dirty: true, | |
| }, | |
| } | |
| snapshot.Body.Dirty = true | |
| } | |
| // Single leaf or non-mixed multipart (e.g. multipart/alternative, | |
| // multipart/related): wrap in multipart/mixed so the body and the | |
| // calendar invitation are independent peers. | |
| original := *snapshot.Body | |
| snapshot.Body.MediaType = "multipart/mixed" | |
| snapshot.Body.MediaParams = nil | |
| snapshot.Body.Body = nil | |
| snapshot.Body.Headers = nil | |
| snapshot.Body.ContentDisposition = "" | |
| snapshot.Body.ContentDispositionArg = nil | |
| snapshot.Body.ContentID = "" | |
| snapshot.Body.TransferEncoding = "" | |
| snapshot.Body.PartID = "" | |
| snapshot.Body.Children = []*Part{ | |
| &original, | |
| { | |
| MediaType: calendarMediaType, | |
| MediaParams: map[string]string{"charset": "UTF-8"}, | |
| Body: icsData, | |
| Dirty: true, | |
| }, | |
| } | |
| snapshot.Body.Dirty = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/draft/patch_calendar.go` around lines 60 - 77, The wrapper
part retains stale headers because the code does a shallow copy with original :=
*snapshot.Body and then mutates snapshot.Body.MediaType without clearing
snapshot.Body.Headers, causing serialize.go:orderedPartHeaders() to prefer the
old Content-Type and other headers; fix by clearing or reinitializing
snapshot.Body.Headers (and any header-related fields like
Content-Transfer-Encoding/Content-ID/PartID) after changing MediaType in the
function that builds the wrapper (the block that sets snapshot.Body.MediaType =
"multipart/mixed" and snapshot.Body.Children), ensuring the serializer will
generate a fresh Content-Type from MediaType; add a unit test
TestSetCalendar_WrapsTextHTMLInMultipartMixed that parses a text/html draft,
calls the calendar wrapper code, serializes the result, and asserts the outer
part's Content-Type equals "multipart/mixed".
Comment on lines
+57
to
+60
| | `--event-summary <text>` | 否 | 日程标题。设置此参数即在邮件中嵌入日程邀请。需同时设置 `--event-start` 和 `--event-end` | | ||
| | `--event-start <time>` | 条件必填 | 日程开始时间（ISO 8601） | | ||
| | `--event-end <time>` | 条件必填 | 日程结束时间（ISO 8601） | | ||
| | `--event-location <text>` | 否 | 日程地点 | |
There was a problem hiding this comment.
Document attendee mapping for calendar drafts.
These flags create ATTENDEE entries from To/Cc only, while Bcc recipients are intentionally excluded. Please add that constraint here so agents do not promise hidden attendees.
Suggested doc addition
| `--event-location <text>` | 否 | 日程地点 |
+
+说明：日程邀请会将 `--to` 和 `--cc` 收件人写入 ATTENDEE；`--bcc` 收件人不会写入日程参与人。📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `--event-summary <text>` | 否 | 日程标题。设置此参数即在邮件中嵌入日程邀请。需同时设置 `--event-start` 和 `--event-end` | | |
| | `--event-start <time>` | 条件必填 | 日程开始时间（ISO 8601） | | |
| | `--event-end <time>` | 条件必填 | 日程结束时间（ISO 8601） | | |
| | `--event-location <text>` | 否 | 日程地点 | | |
| | `--event-summary <text>` | 否 | 日程标题。设置此参数即在邮件中嵌入日程邀请。需同时设置 `--event-start` 和 `--event-end` | | |
| | `--event-start <time>` | 条件必填 | 日程开始时间（ISO 8601） | | |
| | `--event-end <time>` | 条件必填 | 日程结束时间（ISO 8601） | | |
| | `--event-location <text>` | 否 | 日程地点 | | |
| 说明：日程邀请会将 `--to` 和 `--cc` 收件人写入 ATTENDEE；`--bcc` 收件人不会写入日程参与人。 |
🤖 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` around lines 57 - 60,
Add a clear note to the calendar-related flags (`--event-summary`,
`--event-start`, `--event-end`, `--event-location`) explaining that attendee
entries for generated calendar invites are derived only from To and Cc
recipients and that Bcc recipients are intentionally excluded; update the
description for the event flags to explicitly state this constraint so agents do
not imply hidden/Bcc attendees will appear in the invite.
Change-Id: I3a09b75dbaa64c4b78c5ad30db028fb27768ebe1 Co-Authored-By: AI
Change-Id: I46c8c967042ebcf210778b2be61d418453c0eb28 Co-Authored-By: AI
Change-Id: Iabd8c870e8bcf6a3ac08923e650702bbd471bea5 Co-Authored-By: AI
chanthuang
force-pushed
the
feat/mail-calendar
branch
from
6c3edc5 to
5a35724
Compare
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_draft_edit.go (1)
414-430:⚠️ Potential issue | 🟠 MajorExpose calendar patch ops in
--print-patch-template.The new patch-file operations are implemented, but the template still omits
set_calendarandremove_calendar, so schema/template-driven agents won’t discover the PR’s draft-edit calendar path.Proposed template addition
{"op": "insert_signature", "shape": map[string]interface{}{"signature_id": "string (run mail +signature to list IDs)"}}, {"op": "remove_signature", "shape": map[string]interface{}{}, "note": "removes existing signature from the HTML body"}, + {"op": "set_calendar", "shape": map[string]interface{}{ + "event_summary": "string", + "event_start": "ISO 8601 timestamp", + "event_end": "ISO 8601 timestamp", + "event_location": "string(optional)", + }}, + {"op": "remove_calendar", "shape": map[string]interface{}{}, "note": "removes the existing text/calendar invitation"}, }, @@ { "group": "signature", "ops": []map[string]interface{}{ {"op": "insert_signature", "shape": map[string]interface{}{"signature_id": "string (run mail +signature to list IDs)"}}, {"op": "remove_signature", "shape": map[string]interface{}{}, "note": "removes existing signature and its preceding spacing from the HTML body"}, }, }, + { + "group": "calendar", + "ops": []map[string]interface{}{ + {"op": "set_calendar", "shape": map[string]interface{}{"event_summary": "string", "event_start": "ISO 8601 timestamp", "event_end": "ISO 8601 timestamp", "event_location": "string(optional)"}}, + {"op": "remove_calendar", "shape": map[string]interface{}{}}, + }, + },Add a template assertion for these ops. As per coding guidelines, Every behavior change must have an accompanying test.
Also applies to: 465-471
🤖 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 414 - 430, The supported_ops template in mail_draft_edit.go is missing the new set_calendar and remove_calendar operations, so update the supported_ops slice to include entries for {"op":"set_calendar","shape":{...}} and {"op":"remove_calendar","shape":{}} matching the established schema pattern (similar to set_body/remove_body entries) and add a unit/integration test asserting these ops appear in --print-patch-template output (the test should reference the supported_ops rendering path used in the existing tests around the block that currently covers lines ~414-471, e.g., the same template/assert helper used for other ops), ensuring agents discover the calendar draft-edit path.
♻️ Duplicate comments (8)
skills/lark-mail/references/lark-mail-draft-create.md (1)
57-60:⚠️ Potential issue | 🟡 MinorDocument how attendees are derived.
The event flag docs still don’t tell agents that generated calendar attendees come from
--toand--cconly. Add that note near the event flags so draft creation guidance matches the actual invite semantics.🤖 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` around lines 57 - 60, Update the event flags documentation so it states that calendar attendees for generated invites are derived only from the `--to` and `--cc` parameters; near the `--event-start`, `--event-end`, `--event-location` (and `--event-summary`) entries add a short sentence clarifying “Attendees are taken from `--to` and `--cc` only” so the draft creation guidance matches the actual invite semantics.shortcuts/mail/helpers.go (2)
63-105:⚠️ Potential issue | 🟡 MinorKeep
--print-output-schemain sync withcalendar_event.
buildMessageOutputnow emitscalendar_event, but the schema field list still omits it and its subfields, so schema-driven callers won’t discover the new output contract.Proposed schema addition
"body_preview": "First 100 characters of plaintext body content, for quick preview of core email content", "body_html": "Raw HTML body; omitted when --html=false", + "calendar_event": "Parsed calendar invitation object when the message contains body_calendar", + "calendar_event.method": "iCalendar METHOD, e.g. REQUEST", + "calendar_event.uid": "iCalendar UID", + "calendar_event.summary": "Event summary/title", + "calendar_event.start": "Event start time in RFC3339 format", + "calendar_event.end": "Event end time in RFC3339 format", + "calendar_event.location": "Event location", + "calendar_event.organizer": "Organizer email address", + "calendar_event.attendees": "Attendee email addresses", "attachments": "Unified list of regular attachments and inline images",Please add a schema test assertion for these fields. As per coding guidelines, Every behavior change must have an accompanying test.
Also applies to: 1186-1188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 63 - 105, buildMessageOutput now emits calendar_event but the printed schema (used by --print-output-schema) is missing calendar_event and its subfields; update the schema map in shortcuts/mail/helpers.go to include "calendar_event" and its subfields (e.g., start_time, end_time, title, location — match the exact keys emitted by buildMessageOutput) and add a unit test asserting the presence of these keys in the printed schema (follow existing schema test pattern referenced around the test covering fields list); ensure the test asserts both the top-level "calendar_event" field and at least one representative subfield to prevent regressions.
2025-2032:⚠️ Potential issue | 🟠 MajorTreat
--event-locationas an event flag during validation.
--event-locationalone is accepted and ignored, and--send-time --event-locationbypasses the documented--send-time/--event-*mutual exclusion. Use one shared “any event flag” check for both validators.Proposed fix
+func hasAnyEventFlag(runtime *common.RuntimeContext) bool { + return strings.TrimSpace(runtime.Str("event-summary")) != "" || + strings.TrimSpace(runtime.Str("event-start")) != "" || + strings.TrimSpace(runtime.Str("event-end")) != "" || + strings.TrimSpace(runtime.Str("event-location")) != "" +} + func validateSendTime(runtime *common.RuntimeContext) error { sendTime := runtime.Str("send-time") if sendTime == "" { return nil } - if runtime.Str("event-summary") != "" { + if hasAnyEventFlag(runtime) { return fmt.Errorf("--send-time and --event-* are mutually exclusive: a calendar invitation must be sent immediately so recipients can respond before the event") } @@ func validateEventFlags(runtime *common.RuntimeContext) error { - summary := runtime.Str("event-summary") - start := runtime.Str("event-start") - end := runtime.Str("event-end") + summary := strings.TrimSpace(runtime.Str("event-summary")) + start := strings.TrimSpace(runtime.Str("event-start")) + end := strings.TrimSpace(runtime.Str("event-end")) + location := strings.TrimSpace(runtime.Str("event-location")) - hasAny := summary != "" || start != "" || end != "" + hasAny := summary != "" || start != "" || end != "" || location != "" hasAll := summary != "" && start != "" && end != ""Please add tests for
--event-locationalone and--send-timewith only--event-location. As per coding guidelines, Every behavior change must have an accompanying test.Also applies to: 2214-2224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/helpers.go` around lines 2025 - 2032, The validator validateSendTime currently only checks runtime.Str("event-summary") as an “event flag” and therefore ignores --event-location; update validateSendTime to treat any event flag (including runtime.Str("event-location")) the same as runtime.Str("event-summary") by using a shared check (e.g., an isEventFlagPresent helper or a joint boolean that checks event-summary OR event-location) so that --send-time and any --event-* are mutually exclusive; then add unit tests covering (1) --event-location alone is accepted/ignored and (2) --send-time combined with only --event-location triggers the mutual-exclusion error (also apply the same shared-event-flag logic to the other validator referenced in the comment around the same area).shortcuts/mail/mail_draft_edit.go (1)
343-371:⚠️ Potential issue | 🟠 MajorValidate every
--set-event-*flag, not only summary.Line 347 only enables calendar mode when
--set-event-summaryis set.--set-event-start,--set-event-end, or--set-event-locationcan still be ignored, and--remove-event --set-event-location ...bypasses mutual exclusion.Proposed fix
- hasEventSet := runtime.Str("set-event-summary") != "" + summary := strings.TrimSpace(runtime.Str("set-event-summary")) + start := strings.TrimSpace(runtime.Str("set-event-start")) + end := strings.TrimSpace(runtime.Str("set-event-end")) + location := strings.TrimSpace(runtime.Str("set-event-location")) + hasEventSet := summary != "" || start != "" || end != "" || location != "" hasEventRemove := runtime.Bool("remove-event") if hasEventSet && hasEventRemove { - return patch, output.ErrValidation("--set-event-summary and --remove-event are mutually exclusive") + return patch, output.ErrValidation("--set-event-* and --remove-event are mutually exclusive") } if hasEventSet { - summary := runtime.Str("set-event-summary") - start := runtime.Str("set-event-start") - end := runtime.Str("set-event-end") if summary == "" || start == "" || end == "" { return patch, output.ErrValidation("--set-event-summary, --set-event-start, and --set-event-end must all be provided together") } @@ EventSummary: summary, EventStart: start, EventEnd: end, - EventLocation: runtime.Str("set-event-location"), + EventLocation: location, })Please add tests for start/end/location without summary and for
--remove-eventcombined with any set-event flag. 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_draft_edit.go` around lines 343 - 371, The code currently only treats calendar mode as enabled when runtime.Str("set-event-summary") is non-empty; change the detection to consider any of the set-event flags (summary, start, end, location) so that hasEventSet := runtime.Str("set-event-summary") != "" || runtime.Str("set-event-start") != "" || runtime.Str("set-event-end") != "" || runtime.Str("set-event-location") != ""; enforce that when any set-event flag is present all of summary, start and end are provided and validate start/end with parseEventTimeRange and prefixEventRangeError as done now, and ensure hasEventSet is mutually exclusive with runtime.Bool("remove-event") so combinations like --remove-event --set-event-location are rejected; update the block that appends draftpkg.PatchOp{Op:"set_calendar", EventSummary, EventStart, EventEnd, EventLocation} accordingly, and add unit tests covering: start/end/location without summary, start without end, and any --remove-event combined with any --set-event-* flag to assert validation errors.shortcuts/mail/ics/parser.go (2)
117-123:⚠️ Potential issue | 🟠 Major
splitPropertystill splits on colons inside quoted parameter values.A valid RFC 5545 line like
ATTENDEE;CN="Doe: Jane":MAILTO:jane@example.comis split on the colon insideCN="Doe: Jane", soextractMailtonever sees theMAILTO:portion and returns"Jane"as part of value — the attendee is lost. The doc-comment even claims "The first unquoted colon is the separator" but the implementation ignores quotes.Proposed fix
func splitProperty(line string) (string, string) { - idx := strings.Index(line, ":") - if idx < 0 { - return line, "" + inQuotes := false + for i, r := range line { + switch r { + case '"': + inQuotes = !inQuotes + case ':': + if !inQuotes { + return line[:i], line[i+1:] + } + } } - return line[:idx], line[idx+1:] + return line, "" }Please add a regression test covering
ATTENDEE;CN="Doe: Jane":MAILTO:jane@example.com. 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/ics/parser.go` around lines 117 - 123, splitProperty incorrectly splits on colons inside quoted parameter values; update splitProperty to scan the line and return at the first colon that is not inside a quoted string (toggle an inQuotes flag on '"' but respect backslash-escaping so \" doesn't toggle), then return the segment before that colon and the rest after it; add a regression test that parses "ATTENDEE;CN=\"Doe: Jane\":MAILTO:jane@example.com" so extractMailto sees the MAILTO: portion and the attendee CN remains intact.
129-143:⚠️ Potential issue | 🟠 Major
parseICSTimeTZID handling is still case-sensitive and doesn't stop at the next parameter.
strings.Contains(propName, "TZID=")misses lowercase/mixed-case params, andstrings.TrimRight(parts[1], ";")keeps trailing params such as;VALUE=DATE-TIME, makingtime.LoadLocation("Asia/Shanghai;VALUE=DATE-TIME")fail on otherwise valid ICS (e.g.,DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000).Proposed fix
- if strings.Contains(propName, "TZID=") { - parts := strings.SplitN(propName, "TZID=", 2) - if len(parts) == 2 { - tzName := strings.TrimRight(parts[1], ";") - if loc, err := time.LoadLocation(tzName); err == nil { - if t, err := time.ParseInLocation("20060102T150405", value, loc); err == nil { - return t - } + for _, param := range strings.Split(propName, ";")[1:] { + key, val, ok := strings.Cut(param, "=") + if !ok || !strings.EqualFold(key, "TZID") { + continue + } + if loc, err := time.LoadLocation(val); err == nil { + if t, err := time.ParseInLocation("20060102T150405", value, loc); err == nil { + return t } } }Please cover
DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:...and a lowercasetzid=variant. 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/ics/parser.go` around lines 129 - 143, parseICSTime's TZID handling is case-sensitive and trims incorrectly (keeps trailing params), so update parseICSTime to detect TZID case-insensitively and to extract the tzName only up to the next parameter separator (e.g., ';' or ':'). Specifically, in parseICSTime locate "tzid=" using a lower-cased copy of propName (or strings.IndexFunc with case-insensitive logic), compute the substring after that index and then cut it at the first occurrence of ';' or ':' to get the bare tzName (also trim surrounding quotes/spaces) before calling time.LoadLocation; preserve the existing fallback parsing if LoadLocation or ParseInLocation fail. Add a unit test covering "DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000" and a lowercase "tzid=" variant as required.shortcuts/mail/ics/builder.go (1)
122-128:⚠️ Potential issue | 🟠 Major
quoteCNParamstill permits CR/LF injection intoORGANIZER/ATTENDEElines.Only
"is stripped. A display name containing\ror\n(e.g., from a reply to a message whose From address name was crafted by an attacker) will terminate the current property line and inject arbitrary ICS properties (e.g., a fakeATTENDEEorDTSTART). Strip all control characters before emitting parameter text.Proposed fix
func quoteCNParam(s string) string { - s = strings.ReplaceAll(s, `"`, "") + s = strings.Map(func(r rune) rune { + if r == '"' || r < 0x20 || r == 0x7f { + return -1 + } + return r + }, s) if strings.ContainsAny(s, ",:;") { return `"` + s + `"` } return s }Please add a test that constructs an
Address{Name: "Mallory\r\nATTENDEE;..."}and asserts the rendered ICS contains exactly oneATTENDEEline per configured attendee. 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/ics/builder.go` around lines 122 - 128, The function quoteCNParam currently only removes double quotes and still allows CR/LF (and other control characters) which enables header injection in ORGANIZER/ATTENDEE lines; update quoteCNParam to strip all ASCII control characters (at minimum '\r' and '\n', preferably Unicode categories Cc) before performing the quote logic so display names cannot terminate a property line, and use that sanitized string when rendering Address.Name in the code paths that build ORGANIZER/ATTENDEE; add a unit test that creates Address{Name: "Mallory\r\nATTENDEE;..."} and asserts the generated ICS contains exactly one ATTENDEE line per configured attendee (and no injected extra properties).shortcuts/mail/draft/patch_calendar.go (1)
63-76:⚠️ Potential issue | 🟠 MajorWrapper part still retains stale headers from the original leaf.
After
original := *snapshot.Body(shallow copy), the code mutatesMediaType/MediaParams/Body/Childrenonsnapshot.Bodybut leavesHeaders,ContentDisposition,ContentDispositionArg,ContentID,TransferEncoding, andPartIDintact on the new multipart/mixed wrapper. On serialization (orderedPartHeadersprefers an existingContent-Typeheader overMediaType), the wrapper will emit the original leaf'sContent-Type: text/html; charset=UTF-8(and friends) instead ofmultipart/mixed, producing malformed MIME.TestSetCalendar_AddsCalendarPartToHTMLDraftdoesn't catch this because it only inspectsfindPartByMediaType, not the serialized bytes.Proposed fix
original := *snapshot.Body snapshot.Body.MediaType = "multipart/mixed" snapshot.Body.MediaParams = nil snapshot.Body.Body = nil + snapshot.Body.Headers = nil + snapshot.Body.ContentDisposition = "" + snapshot.Body.ContentDispositionArg = nil + snapshot.Body.ContentID = "" + snapshot.Body.TransferEncoding = "" + snapshot.Body.PartID = "" snapshot.Body.Children = []*Part{ &original,Please also add a test that serializes after
set_calendaron atext/htmlleaf and asserts the outer part'sContent-Typeismultipart/mixed. 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/draft/patch_calendar.go` around lines 63 - 76, The multipart wrapper created in set_calendar by doing original := *snapshot.Body is a shallow copy so snapshot.Body still retains stale header fields (Headers, ContentDisposition, ContentDispositionArg, ContentID, TransferEncoding, PartID) causing orderedPartHeaders to emit the old Content-Type; fix it by ensuring the new wrapper either deep-copies and then clears/overrides those header-related fields on snapshot.Body (clear Headers, ContentDisposition, ContentDispositionArg, ContentID, TransferEncoding, PartID) or construct a fresh Part struct for the wrapper with only MediaType/MediaParams/Children/Dirty set and the original part appended as the only child; update references in patch_calendar.go where snapshot.Body is mutated and ensure orderedPartHeaders will see multipart/mixed; add a unit test (new or extend TestSetCalendar_AddsCalendarPartToHTMLDraft) that runs set_calendar on a text/html leaf, serializes the snapshot, and asserts the outer part's serialized Content-Type header equals multipart/mixed.
🤖 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_draft_edit.go`:
- Around line 414-430: The supported_ops template in mail_draft_edit.go is
missing the new set_calendar and remove_calendar operations, so update the
supported_ops slice to include entries for {"op":"set_calendar","shape":{...}}
and {"op":"remove_calendar","shape":{}} matching the established schema pattern
(similar to set_body/remove_body entries) and add a unit/integration test
asserting these ops appear in --print-patch-template output (the test should
reference the supported_ops rendering path used in the existing tests around the
block that currently covers lines ~414-471, e.g., the same template/assert
helper used for other ops), ensuring agents discover the calendar draft-edit
path.
---
Duplicate comments:
In `@shortcuts/mail/draft/patch_calendar.go`:
- Around line 63-76: The multipart wrapper created in set_calendar by doing
original := *snapshot.Body is a shallow copy so snapshot.Body still retains
stale header fields (Headers, ContentDisposition, ContentDispositionArg,
ContentID, TransferEncoding, PartID) causing orderedPartHeaders to emit the old
Content-Type; fix it by ensuring the new wrapper either deep-copies and then
clears/overrides those header-related fields on snapshot.Body (clear Headers,
ContentDisposition, ContentDispositionArg, ContentID, TransferEncoding, PartID)
or construct a fresh Part struct for the wrapper with only
MediaType/MediaParams/Children/Dirty set and the original part appended as the
only child; update references in patch_calendar.go where snapshot.Body is
mutated and ensure orderedPartHeaders will see multipart/mixed; add a unit test
(new or extend TestSetCalendar_AddsCalendarPartToHTMLDraft) that runs
set_calendar on a text/html leaf, serializes the snapshot, and asserts the outer
part's serialized Content-Type header equals multipart/mixed.
In `@shortcuts/mail/helpers.go`:
- Around line 63-105: buildMessageOutput now emits calendar_event but the
printed schema (used by --print-output-schema) is missing calendar_event and its
subfields; update the schema map in shortcuts/mail/helpers.go to include
"calendar_event" and its subfields (e.g., start_time, end_time, title, location
— match the exact keys emitted by buildMessageOutput) and add a unit test
asserting the presence of these keys in the printed schema (follow existing
schema test pattern referenced around the test covering fields list); ensure the
test asserts both the top-level "calendar_event" field and at least one
representative subfield to prevent regressions.
- Around line 2025-2032: The validator validateSendTime currently only checks
runtime.Str("event-summary") as an “event flag” and therefore ignores
--event-location; update validateSendTime to treat any event flag (including
runtime.Str("event-location")) the same as runtime.Str("event-summary") by using
a shared check (e.g., an isEventFlagPresent helper or a joint boolean that
checks event-summary OR event-location) so that --send-time and any --event-*
are mutually exclusive; then add unit tests covering (1) --event-location alone
is accepted/ignored and (2) --send-time combined with only --event-location
triggers the mutual-exclusion error (also apply the same shared-event-flag logic
to the other validator referenced in the comment around the same area).
In `@shortcuts/mail/ics/builder.go`:
- Around line 122-128: The function quoteCNParam currently only removes double
quotes and still allows CR/LF (and other control characters) which enables
header injection in ORGANIZER/ATTENDEE lines; update quoteCNParam to strip all
ASCII control characters (at minimum '\r' and '\n', preferably Unicode
categories Cc) before performing the quote logic so display names cannot
terminate a property line, and use that sanitized string when rendering
Address.Name in the code paths that build ORGANIZER/ATTENDEE; add a unit test
that creates Address{Name: "Mallory\r\nATTENDEE;..."} and asserts the generated
ICS contains exactly one ATTENDEE line per configured attendee (and no injected
extra properties).
In `@shortcuts/mail/ics/parser.go`:
- Around line 117-123: splitProperty incorrectly splits on colons inside quoted
parameter values; update splitProperty to scan the line and return at the first
colon that is not inside a quoted string (toggle an inQuotes flag on '"' but
respect backslash-escaping so \" doesn't toggle), then return the segment before
that colon and the rest after it; add a regression test that parses
"ATTENDEE;CN=\"Doe: Jane\":MAILTO:jane@example.com" so extractMailto sees the
MAILTO: portion and the attendee CN remains intact.
- Around line 129-143: parseICSTime's TZID handling is case-sensitive and trims
incorrectly (keeps trailing params), so update parseICSTime to detect TZID
case-insensitively and to extract the tzName only up to the next parameter
separator (e.g., ';' or ':'). Specifically, in parseICSTime locate "tzid=" using
a lower-cased copy of propName (or strings.IndexFunc with case-insensitive
logic), compute the substring after that index and then cut it at the first
occurrence of ';' or ':' to get the bare tzName (also trim surrounding
quotes/spaces) before calling time.LoadLocation; preserve the existing fallback
parsing if LoadLocation or ParseInLocation fail. Add a unit test covering
"DTSTART;TZID=Asia/Shanghai;VALUE=DATE-TIME:20260420T140000" and a lowercase
"tzid=" variant as required.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 343-371: The code currently only treats calendar mode as enabled
when runtime.Str("set-event-summary") is non-empty; change the detection to
consider any of the set-event flags (summary, start, end, location) so that
hasEventSet := runtime.Str("set-event-summary") != "" ||
runtime.Str("set-event-start") != "" || runtime.Str("set-event-end") != "" ||
runtime.Str("set-event-location") != ""; enforce that when any set-event flag is
present all of summary, start and end are provided and validate start/end with
parseEventTimeRange and prefixEventRangeError as done now, and ensure
hasEventSet is mutually exclusive with runtime.Bool("remove-event") so
combinations like --remove-event --set-event-location are rejected; update the
block that appends draftpkg.PatchOp{Op:"set_calendar", EventSummary, EventStart,
EventEnd, EventLocation} accordingly, and add unit tests covering:
start/end/location without summary, start without end, and any --remove-event
combined with any --set-event-* flag to assert validation errors.
In `@skills/lark-mail/references/lark-mail-draft-create.md`:
- Around line 57-60: Update the event flags documentation so it states that
calendar attendees for generated invites are derived only from the `--to` and
`--cc` parameters; near the `--event-start`, `--event-end`, `--event-location`
(and `--event-summary`) entries add a short sentence clarifying “Attendees are
taken from `--to` and `--cc` only” so the draft creation guidance matches the
actual invite semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16e94ce7-d8ad-4c27-8973-fedafea38766
📒 Files selected for processing (29)
shortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/patch_calendar_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/ics/builder.goshortcuts/mail/ics/ics_test.goshortcuts/mail/ics/parser.goshortcuts/mail/mail_calendar_shortcuts_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_edit_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_confirm_output_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-forward.mdskills/lark-mail/references/lark-mail-message.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-mail/references/lark-mail-message.md
- skills/lark-mail/references/lark-mail-reply-all.md
- skill-template/domains/mail.md
🚧 Files skipped from review as they are similar to previous changes (12)
- shortcuts/mail/emlbuilder/builder_test.go
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/draft/patch.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_send.go
- shortcuts/mail/mail_draft_create.go
- skills/lark-mail/references/lark-mail-forward.md
- shortcuts/mail/mail_forward.go
- shortcuts/mail/draft/model.go
- skills/lark-mail/references/lark-mail-reply.md
- shortcuts/mail/emlbuilder/builder.go
Change-Id: I4a48d017fdcce33bfb2283478ebdf88f2357c64f
…idateEventSendTimeExclusion The check was previously buried inside validateSendTime. Moving it to a dedicated function called explicitly in each shortcut's Validate makes the constraint visible at the flag-validation layer, addressing reviewer feedback that the enforcement location should be clear. Change-Id: I287340fbc33b9035c41c39ad2ba1920123446db1
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/mail/mail_send.go (1)
64-83: Nit:validateEventFlagsplacement differs from the other compose shortcuts.In
mail_reply.goandmail_reply_all.go,validateEventFlagsruns beforevalidateComposeInlineAndAttachments; here it runs after. Functionally harmless (both validators are independent), but moving the call to right aftervalidateSignatureWithPlainTextwould keep the Validate pipelines aligned across+send/+reply/+reply-all/+forward/+draft-createand make future maintenance easier.Proposed reorder
if err := validateSignatureWithPlainText(runtime.Bool("plain-text"), runtime.Str("signature-id")); err != nil { return err } + if err := validateEventFlags(runtime); err != nil { + return err + } if err := validateComposeInlineAndAttachments(runtime.FileIO(), runtime.Str("attach"), runtime.Str("inline"), runtime.Bool("plain-text"), runtime.Str("body")); err != nil { return err } - if err := validateEventFlags(runtime); err != nil { - return err - } return validatePriorityFlag(runtime)🤖 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 64 - 83, In the Validate pipeline, move the call to validateEventFlags so it runs immediately after validateSignatureWithPlainText and before validateComposeInlineAndAttachments; specifically, within the Validate function replace the current order where validateComposeInlineAndAttachments precedes validateEventFlags by invoking validateEventFlags right after validateSignatureWithPlainText (keeping validatePriorityFlag last) to align this +send shortcut with the existing +reply/+reply-all/+forward/+draft-create ordering.shortcuts/mail/helpers_test.go (1)
1034-1049: LGTM — mutual-exclusion and event-flag tests are thorough.Coverage is comprehensive: mutual-exclusion for
--send-timevs--event-*, start/end range parsing (ok / end<start / end==start / invalid start / invalid end),prefixEventRangeErrorrewriting including the pass-through branch, partial-field rejection forvalidateEventFlags, end-before-start ordering, invalid format, andbuildCalendarBodyFromArgsbehavior (nil on empty summary, attendee inclusion from To and CC, no-CC case).One small gap worth considering: the
validateEventFlagspartial-rejection table has no case for--event-locationalone. Given the helper currently ignores location entirely when no summary/start/end are provided (silent no-op), a test that pins down that behavior — whether as "location-only is rejected" or "location-only is silently ignored" — would make the contract explicit.🤖 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 1034 - 1049, Add a test that covers the missing case of "--event-location" alone: in shortcuts/mail/helpers_test.go create a test (e.g., TestValidateEventFlags_LocationOnly) that constructs a cobra command with only the "event-location" flag set, invokes validateEventFlags (or validateEventSendTimeExclusion if using that flow) and asserts the current/desired behavior explicitly—either expect a rejection error or (to match current behavior) expect no error and that buildCalendarBodyFromArgs still returns nil/no event body; reference validateEventFlags and buildCalendarBodyFromArgs so the test ties directly to those symbols and makes the contract for location-only inputs explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/mail/helpers_test.go`:
- Around line 1034-1049: Add a test that covers the missing case of
"--event-location" alone: in shortcuts/mail/helpers_test.go create a test (e.g.,
TestValidateEventFlags_LocationOnly) that constructs a cobra command with only
the "event-location" flag set, invokes validateEventFlags (or
validateEventSendTimeExclusion if using that flow) and asserts the
current/desired behavior explicitly—either expect a rejection error or (to match
current behavior) expect no error and that buildCalendarBodyFromArgs still
returns nil/no event body; reference validateEventFlags and
buildCalendarBodyFromArgs so the test ties directly to those symbols and makes
the contract for location-only inputs explicit.
In `@shortcuts/mail/mail_send.go`:
- Around line 64-83: In the Validate pipeline, move the call to
validateEventFlags so it runs immediately after validateSignatureWithPlainText
and before validateComposeInlineAndAttachments; specifically, within the
Validate function replace the current order where
validateComposeInlineAndAttachments precedes validateEventFlags by invoking
validateEventFlags right after validateSignatureWithPlainText (keeping
validatePriorityFlag last) to align this +send shortcut with the existing
+reply/+reply-all/+forward/+draft-create ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76f7a1e3-2d58-463a-b41e-59097fbd3bd1
📒 Files selected for processing (7)
shortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_calendar_shortcuts_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/mail/mail_calendar_shortcuts_test.go
- shortcuts/mail/helpers.go
- shortcuts/mail/mail_forward.go
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add support for embedding calendar invitations in outgoing emails and parsing calendar event data from received emails.
Sending calendar invitations
Use
--event-*flags with any compose command to attach an iCalendar invitation (text/calendar). Recipients receive a standard calendar invite and can accept or decline the event.New flags (available on
+send,+reply,+reply-all,+forward,+draft-create):--event-summary <text>--event-startand--event-end.--event-start <time>--event-summaryis set2026-05-10T14:00+08:00)--event-end <time>--event-summaryis set--event-location <text>Constraints:
--event-summary,--event-start, and--event-endmust all be provided together--event-*flags are mutually exclusive with--send-time(scheduled delivery)ToandCcrecipients are automatically added as attendees;Bccrecipients are notEditing calendar events in drafts
+draft-editgains two new patch operations via--patch-file:set_calendar— attach or replace the calendar invitation in a draftremove_calendar— remove the calendar invitation from a draftReading calendar invitations
When a received email contains a calendar invitation,
+messageoutput now includes acalendar_eventfield:{ "calendar_event": { "method": "REQUEST", "uid": "abc123", "summary": "Team sync", "start": "2026-05-10T06:00:00Z", "end": "2026-05-10T07:00:00Z", "location": "Meeting room 5F", "organizer": "sender@example.com", "attendees": ["alice@example.com", "bob@example.com"] } }Test plan
+send/+draft-create/+reply/+reply-all/+forwardwith--event-*flags produce valid RFC 5545 iCalendar outputToandCcrecipients appear asATTENDEEentries;Bccrecipients are excluded--event-*and--send-timecannot be used together (validated at input)set_calendarandremove_calendarpatch ops work correctly in+draft-edit+messagecorrectly parses and surfacescalendar_eventfrom received invitation emailsSummary by CodeRabbit
New Features
Documentation
Tests