feat(mail): add template management shortcuts and --template-id flag#642
feat(mail): add template management shortcuts and --template-id flag#642infeng wants to merge 3 commits intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive email template support to the mail service. It adds Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant MailAPI
participant DriveAPI
participant Backend as Mail Server
User->>CLI: mail +send --template-id ABC --body "custom"
CLI->>CLI: Validate template ID format
CLI->>MailAPI: Fetch template ABC
MailAPI-->>CLI: Template (subject, body, to/cc/bcc, attachments)
CLI->>DriveAPI: Upload files referenced in template
DriveAPI-->>CLI: File keys for attachments
CLI->>CLI: Merge template fields with user flags<br/>(user flags take precedence)
CLI->>CLI: Rewrite HTML img src → cid: references
CLI->>CLI: Classify attachments as SMALL/LARGE
CLI->>Backend: POST draft with merged fields<br/>+ LargeAttachmentIDsHeader
Backend-->>CLI: Draft created
User->>CLI: mail +template-create --template-content-file tmpl.html<br/>--attach file1.pdf
CLI->>CLI: Scan HTML for local img src paths
CLI->>DriveAPI: Upload img and --attach files
DriveAPI-->>CLI: File keys
CLI->>CLI: Rewrite img src to cid: references<br/>Build attachment payload
CLI->>MailAPI: POST mail.user_mailbox.templates.create
MailAPI-->>CLI: Template created (id, attachments)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/mail
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
==========================================
- Coverage 60.66% 59.37% -1.29%
==========================================
Files 416 419 +3
Lines 43989 44985 +996
==========================================
+ Hits 26686 26710 +24
- Misses 15233 16188 +955
- Partials 2070 2087 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c93b4b99ac37f13af44ae98ee578814ebead79e5🧩 Skill updatenpx skills add infeng/cli#harness/01kpmfk36nw9fhn08mab2sbjkb-v2 -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_forward.go (1)
374-383:⚠️ Potential issue | 🔴 CriticalTwo successive
Header(LargeAttachmentIDsHeader, ...)calls will drop one set of IDsLines 379 and 382 both call
bld.Header(draftpkg.LargeAttachmentIDsHeader, ...)with different values in immediate succession: first the forward-derivedlargeAttIDs(originals + user uploads), then the template-derived IDs. Unlessemlbuilder.Headeris strictly append-and-preserves-order and the server reads all values, one of the two header values is functionally discarded and large attachments from either the forwarded message/user uploads or the template will silently fail to reference from the draft.Merge the two ID lists into a single base64-JSON value and emit
Header(...)exactly once. This is the same underlying issue I flagged atmail_reply_all.go:265-267,mail_reply.go:255-257, andmail_send.go:216-218, but here it's most visible.🐛 Emit a single merged header
- if len(largeAttIDs) > 0 { - idsJSON, err := json.Marshal(largeAttIDs) - if err != nil { - return fmt.Errorf("failed to encode large attachment IDs: %w", err) - } - bld = bld.Header(draftpkg.LargeAttachmentIDsHeader, base64.StdEncoding.EncodeToString(idsJSON)) - } - if hdr, hdrErr := encodeTemplateLargeAttachmentHeader(templateLargeAttachmentIDs); hdrErr == nil && hdr != "" { - bld = bld.Header(draftpkg.LargeAttachmentIDsHeader, hdr) + // Merge forward-derived and template-derived large attachment IDs into one header. + for _, id := range templateLargeAttachmentIDs { + if id != "" { + largeAttIDs = append(largeAttIDs, largeAttID{ID: id}) + } + } + if len(largeAttIDs) > 0 { + idsJSON, err := json.Marshal(largeAttIDs) + if err != nil { + return fmt.Errorf("failed to encode large attachment IDs: %w", err) + } + bld = bld.Header(draftpkg.LargeAttachmentIDsHeader, base64.StdEncoding.EncodeToString(idsJSON)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_forward.go` around lines 374 - 383, Merge the two separate header writes so the draft gets a single LargeAttachmentIDsHeader: collect IDs from largeAttIDs and from encodeTemplateLargeAttachmentHeader(templateLargeAttachmentIDs) (decode the template header if needed or get its slice), combine them (dedupe if appropriate), json.Marshal the combined slice and base64.StdEncoding.EncodeToString the result, then call bld.Header(draftpkg.LargeAttachmentIDsHeader, <encodedCombined>) exactly once; keep existing error handling for json.Marshal and for encodeTemplateLargeAttachmentHeader, and ensure you only set the header when the final combined list is non-empty.shortcuts/mail/mail_draft_create.go (1)
40-82:⚠️ Potential issue | 🔴 CriticalRemove
Required: truefrom conditionally-required flags to allow--template-id-only flows.
subjectandbodyare declaredRequired: trueinmail_draft_create.go(lines 40–41), yet theValidatefunction (lines 77–82) attempts to make them optional when--template-idis provided. Since Cobra enforces required flags at parse time beforeValidateruns, the conditional logic is ineffective—users invoking+draft-create --template-id <id>alone will fail with "required flags not set" before reaching the custom validator.The same conflict exists in
+send(subject/body Required),+reply(body Required), and+reply-all(body Required). Only+forwardavoids this issue because body is not required. No test coverage was found for the--template-idfeature, confirming this code path has not been exercised.Fix: Drop
Required: truefrom subject/body and rely onValidatefor conditional enforcement (already in place for draft-create; add it to send/reply/reply-all). Update flag descriptions to note that flags are required unless--template-idis used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_draft_create.go` around lines 40 - 82, The subject/body flags are marked Required:true but your command-level Validate logic (validateTemplateID + the Validate function) already conditionally allows them when --template-id is given; remove Required:true from the subject and body flag declarations in the mail draft/create command (and likewise drop Required:true for subject/body on +send and body on +reply and +reply-all), update their Desc strings to note "required unless --template-id is used", and ensure each command has a Validate method that enforces presence of subject/body when template-id is empty (use the existing validateTemplateID and the same TrimSpace checks used in mail_draft_create.go's Validate to make the enforcement consistent).
🧹 Nitpick comments (7)
shortcuts/mail/mail_reply.go (1)
150-152: Fragile subject round-trip throughorig.subjectReassigning
orig.subject = sso that a laterbuildReplySubject(orig.subject)picks up the template subject works only as long asbuildReplySubjectis idempotent on"Re: …"prefixes (which is why thes != buildReplySubject(orig.subject)guard exists). A cleaner and harder-to-break approach is to compute the final subject directly frommerged.Subject, matching howmail_send.goalready consumes it. See the parallel comment ontemplate_compose.go:499-584about making subject handling consistent across the five shortcuts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_reply.go` around lines 150 - 152, The current code mutates orig.subject from merged.Subject and relies on buildReplySubject(orig.subject) being idempotent; instead compute the final subject directly from merged.Subject to avoid the fragile round-trip. Replace the orig.subject reassignment and its comparison with buildReplySubject(orig.subject) by computing finalSub := buildReplySubject(strings.TrimSpace(merged.Subject)) (or equivalent logic used in mail_send.go/template_compose.go) and use finalSub for downstream processing; reference symbols: merged.Subject, orig.subject, buildReplySubject, and ensure behavior matches mail_send.go subject handling.shortcuts/mail/template_compose.go (3)
64-71: Prefersort.Stringsover hand-rolled insertion sortThe justification in the comment ("to avoid importing sort in hot template path") doesn't hold —
sortis already pulled into the binary via many transitive imports, andlogTemplateInforuns at most a few times per command invocation, not on a hot path. Using the standard library here avoids a custom implementation and an extra function on the review surface.♻️ Use `sort.Strings`
import ( "context" "encoding/base64" "encoding/json" "fmt" "net/url" "path/filepath" "regexp" + "sort" "strconv" "strings" @@ - // Stable key order so log lines are diff-friendly. - sortStrings(keys) + // Stable key order so log lines are diff-friendly. + sort.Strings(keys) @@ -func sortStrings(s []string) { - // tiny insertion sort to avoid importing sort in hot template path. - for i := 1; i < len(s); i++ { - for j := i; j > 0 && s[j-1] > s[j]; j-- { - s[j-1], s[j] = s[j], s[j-1] - } - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/template_compose.go` around lines 64 - 71, Replace the custom insertion sort implementation in sortStrings with the standard library call sort.Strings to reduce custom code and surface area; import "sort" and change the body of sortStrings to call sort.Strings(s) (this function is used in the template path such as logTemplateInfo), ensuring the import is added and removing the hand-rolled loop.
646-655: Dead conditional branch inmergeTemplateBodyThe
if draftpkg.HTMLContainsLargeAttachment(draftBody) { // fall through … }block has an empty body and noelse, so the check is a no-op — the call result is discarded. Either remove the block or implement the intended behavior.♻️ Remove the dead branch
- // Regex replace: if the draft body already contains a quote block - // (some callers pre-compose it), insert template before it. - if draftpkg.HTMLContainsLargeAttachment(draftBody) { - // fall through — no quote heuristic; appending is safe. - } merged := draftpkg.InsertBeforeQuoteOrAppend(draftBody, tplContent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/template_compose.go` around lines 646 - 655, The empty conditional using draftpkg.HTMLContainsLargeAttachment(draftBody) is a no-op and should be removed; delete the whole if block and its comment so the code simply calls merged := draftpkg.InsertBeforeQuoteOrAppend(draftBody, tplContent) and returns merged or tplContent+draftBody as currently written, or alternatively, if the original intent was special handling when a large attachment exists, implement that logic inside mergeTemplateBody referencing draftpkg.HTMLContainsLargeAttachment, draftpkg.InsertBeforeQuoteOrAppend, draftBody and tplContent instead of leaving an empty branch.
365-388: Dead maps:pathToFileKey/pathToSizepopulated but never readOnly
pathToCIDis ever consulted for deduplication; the other two maps are written to but never queried. Either delete them or use them to skip re-uploads of duplicate paths (the current code callsuploadToDriveForTemplateagain on the first hit only becausepathToCIDshort-circuits before reaching the upload, so a rewrite of a differentRawSrcpointing to the samePathwould still short-circuit correctly — the file_key/size tracking is redundant).♻️ Drop the unused maps
imgs := parseLocalImgs(content) pathToCID := make(map[string]string) - pathToFileKey := make(map[string]string) - pathToSize := make(map[string]int64) for _, img := range imgs { @@ pathToCID[img.Path] = cid - pathToFileKey[img.Path] = fileKey - pathToSize[img.Path] = sz content = replaceImgSrcOnce(content, img.RawSrc, "cid:"+cid) builder.append(fileKey, filepath.Base(img.Path), cid, true, sz)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/template_compose.go` around lines 365 - 388, The maps pathToFileKey and pathToSize are populated but never read; remove their declarations and all assignments to them (the make(...) lines and the lines setting pathToFileKey[img.Path] = fileKey and pathToSize[img.Path] = sz) to eliminate dead state in the parseLocalImgs loop where pathToCID, uploadToDriveForTemplate, generateTemplateCID, replaceImgSrcOnce, and builder.append are used; alternatively, if you prefer to avoid re-uploads instead, check pathToFileKey[path] (or pathToSize) before calling uploadToDriveForTemplate and reuse the saved fileKey/sz when present—choose one approach and remove the unused code for the other.shortcuts/mail/mail_template_create.go (1)
84-84: Consider validating that a usable content source exists when inline attachments are expected
Validatedoes not require either--template-contentor--template-content-file, which is fine for recipient-only templates. However, nothing catches the case where the user passes only--template-content-file=<bad-path>— that error surfaces as an ugly runtime failure duringExecute's first call toresolveTemplateContentrather than during validation. Consider a quickruntime.FileIO().Stat(...)inValidatewhen--template-content-fileis set, so errors appear before any Drive work begins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_template_create.go` at line 84, Update the Validate method to check that when TemplateContentFile (the --template-content-file flag) is provided it points to a readable file: call runtime.FileIO().Stat(...) (or equivalent file-existence/readability check) inside Validate and return a descriptive validation error if Stat fails, so malformed paths are caught during Validate rather than later in Execute/resolveTemplateContent; keep existing behavior allowing neither template-content nor template-content-file when recipient-only templates are intended.shortcuts/mail/mail_template_update.go (2)
163-178: Preferdefer f.Close()for the patch file.
f.Close()is called manually afterio.ReadAll. If a future change introduces an early return betweenOpenandClose(orReadAllpanics), the file handle leaks.deferis the idiomatic guard.♻️ Proposed refactor
if pf := strings.TrimSpace(runtime.Str("patch-file")); pf != "" { f, err := runtime.FileIO().Open(pf) if err != nil { return output.ErrValidation("open --patch-file %s: %v", pf, err) } - buf, readErr := io.ReadAll(f) - f.Close() + defer f.Close() + buf, readErr := io.ReadAll(f) if readErr != nil { return output.ErrValidation("read --patch-file %s: %v", pf, readErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_template_update.go` around lines 163 - 178, Open file handles with immediate defer to ensure they are closed on all returns: after calling runtime.FileIO().Open(pf) in the block that reads the patch file, check err as before and then call defer f.Close() (instead of the current manual f.Close() after io.ReadAll); keep the existing io.ReadAll, json.Unmarshal into templatePatchFile, and applyTemplatePatchFile(tpl, &patch) logic unchanged, and optionally handle or ignore the error returned by Close to avoid leaking the file descriptor if later returns or panics occur.
60-66: Dry-run silently swallows--set-template-content-fileerrors.
resolveTemplateUpdateContentcan return an error (file missing/unreadable), but the dry-run discards it with_, _, _. If the file path is wrong, the resulting preview will show no upload steps for inline images and the user will only discover the issue duringExecute. Consider surfacing the error as a dry-run hint so the preview is predictable.♻️ Proposed refactor
- content, _, _ := resolveTemplateUpdateContent(runtime) + content, _, contentErr := resolveTemplateUpdateContent(runtime) + if contentErr != nil { + api = api.Set("warning", fmt.Sprintf("could not read --set-template-content-file: %v", contentErr)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_template_update.go` around lines 60 - 66, resolveTemplateUpdateContent errors are being ignored in the dry-run path (content, _, _ := resolveTemplateUpdateContent(runtime)), which causes previews to miss upload steps for invalid template file paths; change this to capture the error (e.g., content, _, err := resolveTemplateUpdateContent(runtime)) and, when err != nil during dry-run, surface it as a preview hint or warning (or return the error) so users see a clear message before Execute; update the block that calls parseLocalImgs, addTemplateUploadSteps, splitByComma and runtime.Str("attach") to handle and propagate/report the captured error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_template_create.go`:
- Around line 86-148: The new +template-create shortcut (Execute function) lacks
unit and dry-run tests: add tests that exercise resolveTemplateContent and
buildTemplatePayloadFromFlags behavior — specifically validate --name length
rejection, mutual exclusion of --template-content vs --template-content-file
(including file-read error propagation from resolveTemplateContent), and the
inline-image extraction + Drive-upload flow (mock Drive HTTP calls via
httpmock); also include a DryRun/E2E test mirroring other mail shortcuts that
invokes Execute in dry-run mode and asserts the runtime output and that
createTemplate is not called / is invoked as expected (mock
createTemplate/extractTemplatePayload where appropriate).
- Around line 36-73: The DryRun handler silently discards errors from
resolveTemplateContent (content, _, _ := resolveTemplateContent(runtime)),
causing misleading dry-run counts; change the call in the DryRun function to
capture the error (e.g. content, _, err := resolveTemplateContent(runtime)) and
if err != nil log a warning via the runtime/context logger (using
logTemplateInfo or runtime.Logger if available) that includes the error message
and short context (e.g. "template content load failed"), then proceed but ensure
counts are computed from the (possibly empty) content while the warning surfaces
the problem to the user; update any references in the same block
(attachments_total, inline_count) to use the captured content variable.
In `@shortcuts/mail/template_compose.go`:
- Around line 499-584: applyTemplate sets res.Subject but reply-all and forward
currently ignore it (they read runtime.Str("subject") into subjectOverride),
causing template subjects to be dropped; make behavior consistent by having
reply_all and forward consume the merged subject from applyTemplate unless the
user explicitly passed --subject. Concretely, in mail_reply_all.go and
mail_forward.go, replace the current subjectOverride usage of
runtime.Str("subject") with logic that prefers the applyTemplate result's
Subject (template-merged value) and only uses the runtime flag when non-empty
(matching the precedence implemented by applyTemplate); you can mirror the
approach used by mail_send.go and/or the round-trip in mail_reply.go so all
shortcuts use the merged.Subject from applyTemplate when userSubject is not
provided.
- Around line 576-584: The code in applyTemplate is collecting all attachment
IDs into the LargeAttachmentIDs slice regardless of attachment type; update the
loop that iterates tpl.Attachments to only append att.ID when att.AttachmentType
(or equivalent enum/field) indicates LARGE, so LargeAttachmentIDs truly contains
only large attachments; keep the LargeAttachmentIDs field name and callers
(encodeTemplateLargeAttachmentHeader in mail_send.go, mail_reply.go,
mail_reply_all.go, mail_forward.go) unchanged so headers written match what
decodeLargeAttachmentHeader expects.
- Around line 604-658: mergeTemplateBody correctly injects tpl.TemplateContent
(with <img src="cid:...">) but applyTemplate and templateApplyResult only
propagate LargeAttachmentIDs and drop inline attachment metadata, so inline
images never get added via AddFileInline and render broken; to fix, extend
templateApplyResult to include inline attachment info (e.g., struct fields for
IsInline, CID, and file path/file_key), update applyTemplate to populate those
inline entries for both LARGE and SMALL template attachments, and then update
the downstream send/draft code paths (where EML is built and headers like
X-Lms-Large-Attachment-Ids are set) to iterate over
templateApplyResult.inlineAttachments and call AddFileInline for each before
constructing/sending the EML so cid: references are satisfied.
In `@skills/lark-mail/references/lark-mail-templates.md`:
- Around line 115-117: The fenced code block containing the warning string
"warning: template to/cc/bcc are appended without de-duplication; you may see
repeated recipients. Use --to/--cc/--bcc to override, or run +template-update to
clear template addresses." needs a language tag to satisfy markdownlint MD040;
update the triple-backtick fence that surrounds that warning to use a language
like text (e.g., ```text) so renderers and linters pick sensible defaults.
- Line 39: The doc states the LARGE threshold as 25 MB but the implementation in
templateAttachmentBuilder.append uses 20*1024*1024; update the markdown to match
the code by changing both occurrences of "25 MB" to "20 MB" (so the description
of switching and the Q4 line read 20 MB) to reflect the logic in
templateAttachmentBuilder.append in template_compose.go where b.projectedSize >
20*1024*1024 sets b.largeBucket = true.
---
Outside diff comments:
In `@shortcuts/mail/mail_draft_create.go`:
- Around line 40-82: The subject/body flags are marked Required:true but your
command-level Validate logic (validateTemplateID + the Validate function)
already conditionally allows them when --template-id is given; remove
Required:true from the subject and body flag declarations in the mail
draft/create command (and likewise drop Required:true for subject/body on +send
and body on +reply and +reply-all), update their Desc strings to note "required
unless --template-id is used", and ensure each command has a Validate method
that enforces presence of subject/body when template-id is empty (use the
existing validateTemplateID and the same TrimSpace checks used in
mail_draft_create.go's Validate to make the enforcement consistent).
In `@shortcuts/mail/mail_forward.go`:
- Around line 374-383: Merge the two separate header writes so the draft gets a
single LargeAttachmentIDsHeader: collect IDs from largeAttIDs and from
encodeTemplateLargeAttachmentHeader(templateLargeAttachmentIDs) (decode the
template header if needed or get its slice), combine them (dedupe if
appropriate), json.Marshal the combined slice and
base64.StdEncoding.EncodeToString the result, then call
bld.Header(draftpkg.LargeAttachmentIDsHeader, <encodedCombined>) exactly once;
keep existing error handling for json.Marshal and for
encodeTemplateLargeAttachmentHeader, and ensure you only set the header when the
final combined list is non-empty.
---
Nitpick comments:
In `@shortcuts/mail/mail_reply.go`:
- Around line 150-152: The current code mutates orig.subject from merged.Subject
and relies on buildReplySubject(orig.subject) being idempotent; instead compute
the final subject directly from merged.Subject to avoid the fragile round-trip.
Replace the orig.subject reassignment and its comparison with
buildReplySubject(orig.subject) by computing finalSub :=
buildReplySubject(strings.TrimSpace(merged.Subject)) (or equivalent logic used
in mail_send.go/template_compose.go) and use finalSub for downstream processing;
reference symbols: merged.Subject, orig.subject, buildReplySubject, and ensure
behavior matches mail_send.go subject handling.
In `@shortcuts/mail/mail_template_create.go`:
- Line 84: Update the Validate method to check that when TemplateContentFile
(the --template-content-file flag) is provided it points to a readable file:
call runtime.FileIO().Stat(...) (or equivalent file-existence/readability check)
inside Validate and return a descriptive validation error if Stat fails, so
malformed paths are caught during Validate rather than later in
Execute/resolveTemplateContent; keep existing behavior allowing neither
template-content nor template-content-file when recipient-only templates are
intended.
In `@shortcuts/mail/mail_template_update.go`:
- Around line 163-178: Open file handles with immediate defer to ensure they are
closed on all returns: after calling runtime.FileIO().Open(pf) in the block that
reads the patch file, check err as before and then call defer f.Close() (instead
of the current manual f.Close() after io.ReadAll); keep the existing io.ReadAll,
json.Unmarshal into templatePatchFile, and applyTemplatePatchFile(tpl, &patch)
logic unchanged, and optionally handle or ignore the error returned by Close to
avoid leaking the file descriptor if later returns or panics occur.
- Around line 60-66: resolveTemplateUpdateContent errors are being ignored in
the dry-run path (content, _, _ := resolveTemplateUpdateContent(runtime)), which
causes previews to miss upload steps for invalid template file paths; change
this to capture the error (e.g., content, _, err :=
resolveTemplateUpdateContent(runtime)) and, when err != nil during dry-run,
surface it as a preview hint or warning (or return the error) so users see a
clear message before Execute; update the block that calls parseLocalImgs,
addTemplateUploadSteps, splitByComma and runtime.Str("attach") to handle and
propagate/report the captured error.
In `@shortcuts/mail/template_compose.go`:
- Around line 64-71: Replace the custom insertion sort implementation in
sortStrings with the standard library call sort.Strings to reduce custom code
and surface area; import "sort" and change the body of sortStrings to call
sort.Strings(s) (this function is used in the template path such as
logTemplateInfo), ensuring the import is added and removing the hand-rolled
loop.
- Around line 646-655: The empty conditional using
draftpkg.HTMLContainsLargeAttachment(draftBody) is a no-op and should be
removed; delete the whole if block and its comment so the code simply calls
merged := draftpkg.InsertBeforeQuoteOrAppend(draftBody, tplContent) and returns
merged or tplContent+draftBody as currently written, or alternatively, if the
original intent was special handling when a large attachment exists, implement
that logic inside mergeTemplateBody referencing
draftpkg.HTMLContainsLargeAttachment, draftpkg.InsertBeforeQuoteOrAppend,
draftBody and tplContent instead of leaving an empty branch.
- Around line 365-388: The maps pathToFileKey and pathToSize are populated but
never read; remove their declarations and all assignments to them (the make(...)
lines and the lines setting pathToFileKey[img.Path] = fileKey and
pathToSize[img.Path] = sz) to eliminate dead state in the parseLocalImgs loop
where pathToCID, uploadToDriveForTemplate, generateTemplateCID,
replaceImgSrcOnce, and builder.append are used; alternatively, if you prefer to
avoid re-uploads instead, check pathToFileKey[path] (or pathToSize) before
calling uploadToDriveForTemplate and reuse the saved fileKey/sz when
present—choose one approach and remove the unused code for the other.
🪄 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: c0b839cf-23e7-4df6-b934-e4a918262ac2
📒 Files selected for processing (12)
shortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/shortcuts.goshortcuts/mail/template_compose.goskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-templates.md
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | ||
| mailboxID := resolveComposeMailboxID(runtime) | ||
| content, _, _ := resolveTemplateContent(runtime) | ||
| logTemplateInfo(runtime, "create.dry_run", map[string]interface{}{ | ||
| "mailbox_id": mailboxID, | ||
| "is_plain_text_mode": runtime.Bool("plain-text"), | ||
| "name_len": len([]rune(runtime.Str("name"))), | ||
| "attachments_total": len(splitByComma(runtime.Str("attach"))) + len(parseLocalImgs(content)), | ||
| "inline_count": len(parseLocalImgs(content)), | ||
| "tos_count": countAddresses(runtime.Str("to")), | ||
| "ccs_count": countAddresses(runtime.Str("cc")), | ||
| "bccs_count": countAddresses(runtime.Str("bcc")), | ||
| }) | ||
| api := common.NewDryRunAPI(). | ||
| Desc("Create a new mail template. The command scans HTML for local <img src> references, uploads each inline image to Drive (≤20MB single upload_all; >20MB upload_prepare+upload_part+upload_finish), rewrites <img src> values to cid: references, uploads any non-inline --attach files the same way, and finally POSTs a Template payload to mail.user_mailbox.templates.create.") | ||
| // Surface the Drive upload steps explicitly so AI callers see the | ||
| // chunked vs single-part branch point for each local image. | ||
| for _, img := range parseLocalImgs(content) { | ||
| addTemplateUploadSteps(runtime, api, img.Path) | ||
| } | ||
| for _, p := range splitByComma(runtime.Str("attach")) { | ||
| addTemplateUploadSteps(runtime, api, p) | ||
| } | ||
| api = api.POST(templateMailboxPath(mailboxID)). | ||
| Body(map[string]interface{}{ | ||
| "template": map[string]interface{}{ | ||
| "name": runtime.Str("name"), | ||
| "subject": runtime.Str("subject"), | ||
| "template_content": "<rewritten-HTML-or-text>", | ||
| "is_plain_text_mode": runtime.Bool("plain-text"), | ||
| "tos": renderTemplateAddresses(runtime.Str("to")), | ||
| "ccs": renderTemplateAddresses(runtime.Str("cc")), | ||
| "bccs": renderTemplateAddresses(runtime.Str("bcc")), | ||
| "attachments": "<computed from uploads>", | ||
| }, | ||
| }) | ||
| return api | ||
| }, |
There was a problem hiding this comment.
DryRun silently swallows resolveTemplateContent error
If --template-content-file is set but unreadable, content, _, _ := resolveTemplateContent(runtime) hides the error, and the DryRun output happily reports attachments_total/inline_count based on an empty body. Surface the error so mail +template-create --dry-run --template-content-file nonexistent.html still signals a problem to the user.
🛠️ Propagate the error via a warning log
- content, _, _ := resolveTemplateContent(runtime)
+ content, _, rcErr := resolveTemplateContent(runtime)
+ if rcErr != nil {
+ fmt.Fprintf(runtime.IO().ErrOut, "warning: dry-run could not load template content: %v\n", rcErr)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/mail_template_create.go` around lines 36 - 73, The DryRun
handler silently discards errors from resolveTemplateContent (content, _, _ :=
resolveTemplateContent(runtime)), causing misleading dry-run counts; change the
call in the DryRun function to capture the error (e.g. content, _, err :=
resolveTemplateContent(runtime)) and if err != nil log a warning via the
runtime/context logger (using logTemplateInfo or runtime.Logger if available)
that includes the error message and short context (e.g. "template content load
failed"), then proceed but ensure counts are computed from the (possibly empty)
content while the warning surfaces the problem to the user; update any
references in the same block (attachments_total, inline_count) to use the
captured content variable.
| Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| mailboxID := resolveComposeMailboxID(runtime) | ||
| content, _, err := resolveTemplateContent(runtime) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| name := runtime.Str("name") | ||
| subject := runtime.Str("subject") | ||
| isPlainText := runtime.Bool("plain-text") | ||
| tos := renderTemplateAddresses(runtime.Str("to")) | ||
| ccs := renderTemplateAddresses(runtime.Str("cc")) | ||
| bccs := renderTemplateAddresses(runtime.Str("bcc")) | ||
|
|
||
| rewritten, atts, err := buildTemplatePayloadFromFlags( | ||
| ctx, runtime, name, subject, content, tos, ccs, bccs, | ||
| splitByComma(runtime.Str("attach")), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| inlineCount, largeCount := countAttachmentsByType(atts) | ||
| logTemplateInfo(runtime, "create.execute", map[string]interface{}{ | ||
| "mailbox_id": mailboxID, | ||
| "is_plain_text_mode": isPlainText, | ||
| "name_len": len([]rune(name)), | ||
| "attachments_total": len(atts), | ||
| "inline_count": inlineCount, | ||
| "large_count": largeCount, | ||
| "tos_count": len(tos), | ||
| "ccs_count": len(ccs), | ||
| "bccs_count": len(bccs), | ||
| }) | ||
|
|
||
| payload := &templatePayload{ | ||
| Name: name, | ||
| Subject: subject, | ||
| TemplateContent: rewritten, | ||
| IsPlainTextMode: isPlainText, | ||
| Tos: tos, | ||
| Ccs: ccs, | ||
| Bccs: bccs, | ||
| Attachments: atts, | ||
| } | ||
|
|
||
| resp, err := createTemplate(runtime, mailboxID, payload) | ||
| if err != nil { | ||
| return fmt.Errorf("create template failed: %w", err) | ||
| } | ||
| tpl, _ := extractTemplatePayload(resp) | ||
| out := map[string]interface{}{ | ||
| "template": tpl, | ||
| } | ||
| runtime.OutFormat(out, nil, func(w io.Writer) { | ||
| fmt.Fprintln(w, "Template created.") | ||
| if tpl != nil { | ||
| fmt.Fprintf(w, "template_id: %s\n", tpl.TemplateID) | ||
| fmt.Fprintf(w, "name: %s\n", tpl.Name) | ||
| fmt.Fprintf(w, "attachments: %d\n", len(tpl.Attachments)) | ||
| } | ||
| }) | ||
| return nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
Missing tests for the new +template-create shortcut
Per the repo coding guidelines ("Every behavior change must have an accompanying test"), this new shortcut should ship with at least one unit/DryRun test covering: --name length validation, the --template-content vs --template-content-file mutual-exclusion rule, the content-file read path (including the error-propagation behavior noted above), and the inline-image extraction/Drive-upload flow (mockable via httpmock). Adjacent dry-run E2E coverage under tests/cli_e2e/dryrun/ would also match the pattern used by other mail shortcuts.
As per coding guidelines: **/*.go: "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_template_create.go` around lines 86 - 148, The new
+template-create shortcut (Execute function) lacks unit and dry-run tests: add
tests that exercise resolveTemplateContent and buildTemplatePayloadFromFlags
behavior — specifically validate --name length rejection, mutual exclusion of
--template-content vs --template-content-file (including file-read error
propagation from resolveTemplateContent), and the inline-image extraction +
Drive-upload flow (mock Drive HTTP calls via httpmock); also include a
DryRun/E2E test mirroring other mail shortcuts that invokes Execute in dry-run
mode and asserts the runtime output and that createTemplate is not called / is
invoked as expected (mock createTemplate/extractTemplatePayload where
appropriate).
| // applyTemplate merges a fetched template with draft-derived and user-flag | ||
| // values. draftTo/Cc/Bcc are the addresses already on the draft (from the | ||
| // original message for reply/reply-all/forward, or the user flags for send/ | ||
| // draft-create). userTo/Cc/Bcc/Subject/Body are user-supplied flag values | ||
| // (empty string = not provided). | ||
| func applyTemplate( | ||
| kind templateShortcutKind, | ||
| tpl *templatePayload, | ||
| draftTo, draftCc, draftBcc string, | ||
| draftSubject string, | ||
| draftBody string, | ||
| userTo, userCc, userBcc, userSubject, userBody string, | ||
| ) templateApplyResult { | ||
| res := templateApplyResult{} | ||
|
|
||
| // Start with whatever is already in the draft (or the user-explicit | ||
| // draft-to values for send/draft-create). | ||
| effTo := draftTo | ||
| effCc := draftCc | ||
| effBcc := draftBcc | ||
| // User-flag --to/--cc/--bcc values override draft-derived values | ||
| // before template injection. | ||
| if userTo != "" { | ||
| effTo = userTo | ||
| } | ||
| if userCc != "" { | ||
| effCc = userCc | ||
| } | ||
| if userBcc != "" { | ||
| effBcc = userBcc | ||
| } | ||
|
|
||
| tplTo := joinTemplateAddresses(tpl.Tos) | ||
| tplCc := joinTemplateAddresses(tpl.Ccs) | ||
| tplBcc := joinTemplateAddresses(tpl.Bccs) | ||
|
|
||
| // Append template to/cc/bcc into draft to/cc/bcc. | ||
| effTo = appendAddrList(effTo, tplTo) | ||
| effCc = appendAddrList(effCc, tplCc) | ||
| effBcc = appendAddrList(effBcc, tplBcc) | ||
|
|
||
| res.To = effTo | ||
| res.Cc = effCc | ||
| res.Bcc = effBcc | ||
|
|
||
| // Q2: subject merging. User --subject wins, else draft non-empty wins, | ||
| // else template subject. | ||
| switch { | ||
| case strings.TrimSpace(userSubject) != "": | ||
| res.Subject = userSubject | ||
| case strings.TrimSpace(draftSubject) != "": | ||
| res.Subject = draftSubject | ||
| default: | ||
| res.Subject = tpl.Subject | ||
| } | ||
|
|
||
| // Q3: body merging. The shortcut-specific HTML/plain-text injection is | ||
| // handled by the caller; applyTemplate returns a merged body string that | ||
| // the caller can feed back into its compose pipeline. | ||
| res.Body = mergeTemplateBody(kind, tpl, draftBody, userBody) | ||
|
|
||
| // IsPlainTextMode propagation: template value wins. | ||
| res.IsPlainTextMode = tpl.IsPlainTextMode | ||
|
|
||
| // Q4: warn when reply / reply-all + template has to/cc/bcc (likely | ||
| // duplicates against the reply-derived recipients). | ||
| if (kind == templateShortcutReply || kind == templateShortcutReplyAll) && | ||
| (len(tpl.Tos) > 0 || len(tpl.Ccs) > 0 || len(tpl.Bccs) > 0) { | ||
| res.Warnings = append(res.Warnings, | ||
| "template to/cc/bcc are appended without de-duplication; "+ | ||
| "you may see repeated recipients. Use --to/--cc/--bcc to override, "+ | ||
| "or run +template-update to clear template addresses.") | ||
| } | ||
|
|
||
| // Collect template attachment ids for the caller (file_keys). The SEND | ||
| // path uses these as the X-Lms-Large-Attachment-Ids header entries for | ||
| // LARGE types; SMALL entries are reused by file_key server-side. | ||
| for _, att := range tpl.Attachments { | ||
| if att.ID == "" { | ||
| continue | ||
| } | ||
| res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID) | ||
| } | ||
|
|
||
| return res | ||
| } |
There was a problem hiding this comment.
merged.Subject is silently discarded in reply-all and forward shortcuts
applyTemplate computes res.Subject with a documented precedence (user subject → draft subject → template subject), but:
mail_send.gousesmerged.Subject(good).mail_reply.goapplies it via a round-trip throughorig.subject(works but awkward).mail_reply_all.goandmail_forward.gonever readmerged.Subject; they only honorruntime.Str("subject")throughsubjectOverride. When a template has a non-emptysubjectand the user did not pass--subject, the template subject is dropped.
Please make the behavior consistent across all five shortcuts (either always consume merged.Subject, or document that reply/reply-all/forward intentionally keep the Re:/Fw: auto-subject and only honor --subject). If the latter, res.Subject in applyTemplate is misleading and should be scoped accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/template_compose.go` around lines 499 - 584, applyTemplate
sets res.Subject but reply-all and forward currently ignore it (they read
runtime.Str("subject") into subjectOverride), causing template subjects to be
dropped; make behavior consistent by having reply_all and forward consume the
merged subject from applyTemplate unless the user explicitly passed --subject.
Concretely, in mail_reply_all.go and mail_forward.go, replace the current
subjectOverride usage of runtime.Str("subject") with logic that prefers the
applyTemplate result's Subject (template-merged value) and only uses the runtime
flag when non-empty (matching the precedence implemented by applyTemplate); you
can mirror the approach used by mail_send.go and/or the round-trip in
mail_reply.go so all shortcuts use the merged.Subject from applyTemplate when
userSubject is not provided.
| for _, att := range tpl.Attachments { | ||
| if att.ID == "" { | ||
| continue | ||
| } | ||
| res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID) | ||
| } | ||
|
|
||
| return res | ||
| } |
There was a problem hiding this comment.
Header likely to include SMALL template attachments, not just LARGE
applyTemplate aggregates every attachment ID regardless of AttachmentType, and the callers in mail_send.go, mail_reply.go, mail_reply_all.go, mail_forward.go feed the resulting slice into encodeTemplateLargeAttachmentHeader, which is then written as X-Lms-Large-Attachment-Ids on the outgoing EML. Based on the header name and the server-side decoder (decodeLargeAttachmentHeader in shortcuts/mail/draft/large_attachment_parse.go), that header is specifically for LARGE attachments — feeding SMALL file_keys into it is likely to cause the server to mis-classify all template attachments as large, render a download-card for SMALL items, or reject the entries entirely.
The field name LargeAttachmentIDs also advertises large-only semantics while the code collects all IDs, so either the filter or the field should be tightened.
🛠️ Filter to LARGE-only when aggregating template attachment IDs for the header
- for _, att := range tpl.Attachments {
- if att.ID == "" {
- continue
- }
- res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID)
- }
+ for _, att := range tpl.Attachments {
+ if att.ID == "" || att.AttachmentType != attachmentTypeLARGE {
+ continue
+ }
+ res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID)
+ }📝 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.
| for _, att := range tpl.Attachments { | |
| if att.ID == "" { | |
| continue | |
| } | |
| res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID) | |
| } | |
| return res | |
| } | |
| for _, att := range tpl.Attachments { | |
| if att.ID == "" || att.AttachmentType != attachmentTypeLARGE { | |
| continue | |
| } | |
| res.LargeAttachmentIDs = append(res.LargeAttachmentIDs, att.ID) | |
| } | |
| return res | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/template_compose.go` around lines 576 - 584, The code in
applyTemplate is collecting all attachment IDs into the LargeAttachmentIDs slice
regardless of attachment type; update the loop that iterates tpl.Attachments to
only append att.ID when att.AttachmentType (or equivalent enum/field) indicates
LARGE, so LargeAttachmentIDs truly contains only large attachments; keep the
LargeAttachmentIDs field name and callers (encodeTemplateLargeAttachmentHeader
in mail_send.go, mail_reply.go, mail_reply_all.go, mail_forward.go) unchanged so
headers written match what decodeLargeAttachmentHeader expects.
| func mergeTemplateBody(kind templateShortcutKind, tpl *templatePayload, draftBody, userBody string) string { | ||
| tplContent := tpl.TemplateContent | ||
| // If the user explicitly passed --body, that is the composer's own | ||
| // authoring area; we still inject the template content into the same | ||
| // area (draft_body = user_body for send/draft-create). | ||
| if userBody != "" { | ||
| draftBody = userBody | ||
| } | ||
|
|
||
| // Plain-text mode: simple append. | ||
| if tpl.IsPlainTextMode { | ||
| switch kind { | ||
| case templateShortcutSend, templateShortcutDraftCreate: | ||
| if strings.TrimSpace(draftBody) == "" { | ||
| return tplContent | ||
| } | ||
| return draftBody + "\n\n" + tplContent | ||
| default: | ||
| // reply/forward plain-text: prepend template before quote. | ||
| // emlbuilder composes quote separately so the draft body here | ||
| // is pure user-authored content; we just prepend. | ||
| if strings.TrimSpace(draftBody) == "" { | ||
| return tplContent | ||
| } | ||
| return tplContent + "\n\n" + draftBody | ||
| } | ||
| } | ||
|
|
||
| switch kind { | ||
| case templateShortcutSend, templateShortcutDraftCreate: | ||
| if strings.TrimSpace(draftBody) == "" { | ||
| return tplContent | ||
| } | ||
| return draftBody + tplContent | ||
| case templateShortcutReply, templateShortcutReplyAll, templateShortcutForward: | ||
| // At this compose layer, draftBody is the user-authored area only | ||
| // (the caller adds the quote block downstream). Inject template | ||
| // content at the head of that area so it lands above the future | ||
| // quote block. | ||
| if strings.TrimSpace(draftBody) == "" { | ||
| return tplContent | ||
| } | ||
| // Regex replace: if the draft body already contains a quote block | ||
| // (some callers pre-compose it), insert template before it. | ||
| if draftpkg.HTMLContainsLargeAttachment(draftBody) { | ||
| // fall through — no quote heuristic; appending is safe. | ||
| } | ||
| merged := draftpkg.InsertBeforeQuoteOrAppend(draftBody, tplContent) | ||
| if merged != draftBody { | ||
| return merged | ||
| } | ||
| return tplContent + draftBody | ||
| } | ||
| return draftBody | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Lark/Feishu open mail API: do templates with inline cid images automatically render when applied to a draft, or must the draft carry inline MIME parts referencing the file_keys?
💡 Result:
Templates with inline cid images do not automatically render when applied to a draft in Lark/Feishu open mail API. The draft must carry inline MIME parts referencing the file_keys (via cid) for images to render properly. Official API supports constructing emails with inline attachments explicitly via the attachments array (with is_inline=true and cid), or raw EML with proper MIME structure. Lark CLI demonstrates that inline images require either auto-resolution from local paths in body HTML or explicit --inline specification with file_paths and cids; templates likely insert HTML with cid references but do not auto-embed MIME parts unless the API explicitly handles it, which evidence suggests it does not—drafts need MIME parts for rendering.
Citations:
- 1: https://github.com/larksuite/cli/blob/HEAD/skills/lark-mail/references/lark-mail-draft-create.md
- 2: https://open.larkoffice.com/document/uAjLw4CM/ukTMukTMukTM/reference/mail-v1/user_mailbox-message/send
- 3: https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/reference/mail-v1/user_mailbox-message/get
- 4: feat(mail): auto-resolve local image paths in all draft entry points #205
- 5: https://github.com/larksuite/cli/blob/main/skills/lark-mail/SKILL.md
🏁 Script executed:
# Check the template_compose.go file around the function in question
cat -n shortcuts/mail/template_compose.go | head -80 | tail -50Repository: larksuite/cli
Length of output: 2127
🏁 Script executed:
# Search for AddFileInline usage and where it's called from template-related code
rg -n "AddFileInline" --type go | head -20Repository: larksuite/cli
Length of output: 1620
🏁 Script executed:
# Search for template-related code that might handle inline images
rg -n "TemplateContent|template.*[Ii]nline" --type go -B 2 -A 2 | head -50Repository: larksuite/cli
Length of output: 3923
🏁 Script executed:
# Check how mail_send, mail_reply, mail_forward use mergeTemplateBody result
rg -n "mergeTemplateBody" --type go -B 3 -A 10Repository: larksuite/cli
Length of output: 3024
🏁 Script executed:
# Look for test cases related to templates with inline images
fd -e go "*test*" | xargs rg -l "template.*[Ii]nline|Template.*[Ii]nline" | head -10Repository: larksuite/cli
Length of output: 459
🏁 Script executed:
# Search for X-Lms-Large-Attachment-Ids or template attachment handling
rg -n "X-Lms-Large-Attachment-Ids|TemplateAttachments|template.*attach" --type go -B 2 -A 2 | head -40Repository: larksuite/cli
Length of output: 3131
🏁 Script executed:
# Get context around AddFileInline calls in mail_send.go
sed -n '175,205p' shortcuts/mail/mail_send.goRepository: larksuite/cli
Length of output: 1154
🏁 Script executed:
# Look at the full applyTemplate function and what it returns
rg -n "func applyTemplate|type.*applyTemplate" --type go -A 30 | head -80Repository: larksuite/cli
Length of output: 4147
🏁 Script executed:
# Check if template attachments are added to the builder anywhere
rg -n "\.Attachments|template.*[Aa]ttach" shortcuts/mail/mail_send.go -B 3 -A 3 | head -60Repository: larksuite/cli
Length of output: 2976
🏁 Script executed:
# Get the full context of how applyTemplate result is used in mail_send
sed -n '140,250p' shortcuts/mail/mail_send.goRepository: larksuite/cli
Length of output: 3816
🏁 Script executed:
# Find the templateApplyResult struct definition
rg -n "type templateApplyResult" --type go -A 15Repository: larksuite/cli
Length of output: 1282
🏁 Script executed:
# Check what happens to template attachments in the compose flow
rg -n "tpl\.Attachments" --type go -B 2 -A 5Repository: larksuite/cli
Length of output: 9319
🏁 Script executed:
# Check if template inline attachments are ever added to the builder
rg -n "templateAttachment|template.*[Ii]nline.*part|AddFileInline.*template" --type go -B 3 -A 3 | head -80Repository: larksuite/cli
Length of output: 6132
🏁 Script executed:
# Get the entire applyTemplate function to see what it does with attachments
sed -n '504,600p' shortcuts/mail/template_compose.goRepository: larksuite/cli
Length of output: 3002
🏁 Script executed:
# Look for tests related to template inline images
fd -type f -name "*test.go" | xargs rg -l "template.*inline|Template.*[Ii]nline" 2>/dev/nullRepository: larksuite/cli
Length of output: 344
🏁 Script executed:
# Check for any documentation or comments about the template + inline mechanism
rg -n "inline.*template|template.*inline|cid.*file_key|file_key.*cid" --type go -B 3 -A 3 | head -100Repository: larksuite/cli
Length of output: 6098
🏁 Script executed:
# Check the large attachment header mechanism and file_key reuse
rg -n "reuse.*file_key|file_key.*reuse|SMALL.*reused" --type go -B 2 -A 2Repository: larksuite/cli
Length of output: 1963
🏁 Script executed:
# Look for any test files specifically for template compose
fd -type f -name "*template*test.go"Repository: larksuite/cli
Length of output: 227
🏁 Script executed:
# Check buildTemplatePayloadFromFlags to understand how inline cids are set
sed -n '350,450p' shortcuts/mail/template_compose.goRepository: larksuite/cli
Length of output: 3624
🏁 Script executed:
# Check the SKILL.md documentation for templates
cat shortcuts/mail/skills.go 2>/dev/null || grep -r "template.*inline\|inline.*template" skills/ 2>/dev/null | head -20Repository: larksuite/cli
Length of output: 354
🏁 Script executed:
# Look for any tests related to template compose or inline images
find . -name "*.go" -path "*/test*" -o -name "*_test.go" | xargs grep -l "template" 2>/dev/null | head -5Repository: larksuite/cli
Length of output: 206
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to template inline images
rg -n "TODO|FIXME|BUG|XXX" shortcuts/mail/template_compose.go | grep -i "inline\|cid\|attach"Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Verify if the server actually supports materializing inline images from template file_keys
rg -n "server.*reuse\|reuse.*server\|materialize" shortcuts/mail/ -B 2 -A 2Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check the mail API documentation files to understand template behavior
find . -name "*.md" -path "*/lark-mail/*" | xargs grep -l "template" 2>/dev/nullRepository: larksuite/cli
Length of output: 244
🏁 Script executed:
# Read the templates documentation to understand the expected behavior
cat skills/lark-mail/references/lark-mail-templates.md | head -200Repository: larksuite/cli
Length of output: 7210
🏁 Script executed:
# Check specifically for any documentation about inline image rendering when templates are applied
cat skills/lark-mail/references/lark-mail-templates.md | grep -A 10 -B 5 "inline\|cid\|image"Repository: larksuite/cli
Length of output: 3313
🏁 Script executed:
# Check how the draft is created and whether template attachment metadata is sent
sed -n '200,230p' shortcuts/mail/mail_send.goRepository: larksuite/cli
Length of output: 1300
🏁 Script executed:
# Look at what data is sent to drafts.create or drafts.send
rg -n "CreateWithRaw\|draftpkg.Send\|drafts.create\|drafts.send" shortcuts/mail/ -B 5 -A 5 | head -80Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check the draft package to see what metadata might be sent
find . -name "*.go" -path "*/draft/*" | xargs grep -l "attachment\|Attachment" | head -3Repository: larksuite/cli
Length of output: 201
🏁 Script executed:
# Verify whether template SMALL attachments are included in any request
rg -n "SMALL\|Small" shortcuts/mail/template_compose.go -B 3 -A 3Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check if there's any mechanism to pass template attachment metadata to the server in the draft payload
rg -n "CreateWithRaw\|Send" shortcuts/mail/draft/ -lRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check applyTemplate result and how it's used - specifically for all attachments, not just LARGE
sed -n '555,595p' shortcuts/mail/template_compose.goRepository: larksuite/cli
Length of output: 1529
🏁 Script executed:
# Check if there's any code that processes template attachments separately for inline vs non-inline
rg -n "\.IsInline\|isInline" shortcuts/mail/ --type go -B 2 -A 2 | head -60Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check the full context of mergeTemplateBody and what it receives
sed -n '470,490p' shortcuts/mail/template_compose.goRepository: larksuite/cli
Length of output: 882
🏁 Script executed:
# Verify: do any of the mail_send/reply/forward callers iterate over tpl.Attachments to add them?
rg -n "for.*tpl\.Attachments\|for.*Attachments.*range" shortcuts/mail/ --type goRepository: larksuite/cli
Length of output: 39
Template inline images (cid:) will render as broken images in sent/draft messages
mergeTemplateBody embeds tpl.TemplateContent (containing <img src="cid:..."> references) directly into the draft, but applyTemplate only returns LargeAttachmentIDs—it discards the IsInline and CID metadata from template attachments. Neither LARGE nor SMALL template inline attachments are added to the EML builder: LARGE attachments are propagated via the X-Lms-Large-Attachment-Ids header, while SMALL attachments rely on undocumented "server-side reuse by file_key" semantics, which is insufficient for inline images. The Lark/Feishu API does not auto-materialize cid: → file_key bindings; the draft must carry inline MIME parts via AddFileInline to render correctly.
To fix, templateApplyResult must carry both large IDs and inline attachment metadata (path, CID), and callers must call AddFileInline for each inline template attachment before sending/drafting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/mail/template_compose.go` around lines 604 - 658, mergeTemplateBody
correctly injects tpl.TemplateContent (with <img src="cid:...">) but
applyTemplate and templateApplyResult only propagate LargeAttachmentIDs and drop
inline attachment metadata, so inline images never get added via AddFileInline
and render broken; to fix, extend templateApplyResult to include inline
attachment info (e.g., struct fields for IsInline, CID, and file path/file_key),
update applyTemplate to populate those inline entries for both LARGE and SMALL
template attachments, and then update the downstream send/draft code paths
(where EML is built and headers like X-Lms-Large-Attachment-Ids are set) to
iterate over templateApplyResult.inlineAttachments and call AddFileInline for
each before constructing/sending the EML so cid: references are satisfied.
| 3. 原 HTML 改写为 `<img src="cid:<uuid>">` | ||
| 4. 在 `attachments[]` 追加 `{id: <file_key>, cid, is_inline: true, filename, attachment_type}` | ||
|
|
||
| **LARGE 切换**：本地单文件 ≤ 20 MB 走单次上传、> 20 MB 走分块上传；`attachment_type` 则看累计 EML 投影（subject / to / cc / bcc / template_content + base64 附件体积），到 25 MB 后同批次剩余附件标 `LARGE`。两套判定相互独立。 |
There was a problem hiding this comment.
LARGE 阈值与代码不一致：应为 20 MB，不是 25 MB。
templateAttachmentBuilder.append 中判定逻辑为 if b.projectedSize > 20*1024*1024 { b.largeBucket = true }（见 shortcuts/mail/template_compose.go:326-328）。本行和下文 Q4（Line 108）都写成 25 MB，会让用户对切换时机判断有误。
✏️ Proposed fix
-**LARGE 切换**：本地单文件 ≤ 20 MB 走单次上传、> 20 MB 走分块上传；`attachment_type` 则看累计 EML 投影（subject / to / cc / bcc / template_content + base64 附件体积），到 25 MB 后同批次剩余附件标 `LARGE`。两套判定相互独立。
+**LARGE 切换**：本地单文件 ≤ 20 MB 走单次上传、> 20 MB 走分块上传；`attachment_type` 则看累计 EML 投影（subject / to / cc / bcc / template_content + base64 附件体积），累计超过 20 MB 后同批次剩余附件标 `LARGE`。两套判定相互独立。And in Line 108:
-| Q4 附件 | 所有 5 个 Shortcut | 过期 / 封禁超大附件丢弃；`emlProjectedSize` 累计 > 25MB 则模板小附件改标 LARGE；去重键 = `Attachment.id`（Drive file_key）；顺序 = 草稿在前、模板在后 |
+| Q4 附件 | 所有 5 个 Shortcut | 过期 / 封禁超大附件丢弃；`emlProjectedSize` 累计 > 20MB 则模板小附件改标 LARGE；去重键 = `Attachment.id`（Drive file_key）；顺序 = 草稿在前、模板在后 |📝 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.
| **LARGE 切换**：本地单文件 ≤ 20 MB 走单次上传、> 20 MB 走分块上传；`attachment_type` 则看累计 EML 投影（subject / to / cc / bcc / template_content + base64 附件体积），到 25 MB 后同批次剩余附件标 `LARGE`。两套判定相互独立。 | |
| **LARGE 切换**：本地单文件 ≤ 20 MB 走单次上传、> 20 MB 走分块上传；`attachment_type` 则看累计 EML 投影（subject / to / cc / bcc / template_content + base64 附件体积），累计超过 20 MB 后同批次剩余附件标 `LARGE`。两套判定相互独立。 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-mail/references/lark-mail-templates.md` at line 39, The doc
states the LARGE threshold as 25 MB but the implementation in
templateAttachmentBuilder.append uses 20*1024*1024; update the markdown to match
the code by changing both occurrences of "25 MB" to "20 MB" (so the description
of switching and the Q4 line read 20 MB) to reflect the logic in
templateAttachmentBuilder.append in template_compose.go where b.projectedSize >
20*1024*1024 sets b.largeBucket = true.
| ``` | ||
| warning: template to/cc/bcc are appended without de-duplication; you may see repeated recipients. Use --to/--cc/--bcc to override, or run +template-update to clear template addresses. | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block.
markdownlint MD040 flags this; use text (or an appropriate language) so renderers pick sensible defaults.
✏️ Proposed fix
-```
+```text
warning: template to/cc/bcc are appended without de-duplication; you may see repeated recipients. Use --to/--cc/--bcc to override, or run +template-update to clear template addresses.</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-mail/references/lark-mail-templates.md` around lines 115 - 117,
The fenced code block containing the warning string "warning: template to/cc/bcc
are appended without de-duplication; you may see repeated recipients. Use
--to/--cc/--bcc to override, or run +template-update to clear template
addresses." needs a language tag to satisfy markdownlint MD040; update the
triple-backtick fence that surrounds that warning to use a language like text
(e.g., ```text) so renderers and linters pick sensible defaults.
1 participant
Summary
Adds email template management to lark-cli's
+maildomain, rebased on top of latestmain.mail +template-create,mail +template-update--template-idon+send/+draft-create/+reply/+reply-all/+forward<img src>local paths → Drive upload → rewrite ascid:refs, emit viaTemplateAttachment{id=file_key, is_inline=true, cid, ...}IsSendSeparately— per-recipient separate-send is a draft-level concern and is intentionally not persisted on the template record.mail_addressfield only (noaddress_aliasfallback).--template-idapply paths (mailbox_id, template_id, counts, name_len — never content / subject / recipient plaintext).Relationship to earlier PR
Supersedes the earlier branch
harness/01kpmfk36nw9fhn08mab2sbjkb— that branch was based on an oldmainand mixed unrelated in-flight mail work. This branch rebuilds only the template work on top of currentupstream/main.Test plan
go build ./...passes locallymail +template-create --helpsurfaces the new flagsmail +send --template-id <id>DryRun shows extra GET step + merged fieldstemplate_contentgets uploaded to Drive and CID-substitutedSummary by CodeRabbit
Release Notes
--template-idsupport across send, draft, reply, reply-all, and forward operations--subjectoverride option for reply, reply-all, forward, and draft-create operations