Skip to content

feat(mail): add +template-create and +template-update shortcuts#677

Open
infeng wants to merge 1 commit intolarksuite:mainfrom
infeng:harness/smoke-cli-mail-template-write-20260427-165654
Open

feat(mail): add +template-create and +template-update shortcuts#677
infeng wants to merge 1 commit intolarksuite:mainfrom
infeng:harness/smoke-cli-mail-template-write-20260427-165654

Conversation

@infeng
Copy link
Copy Markdown
Collaborator

@infeng infeng commented Apr 27, 2026

Generated by the harness-coding skill.

  • Task ID: smoke-cli-mail-template-write-20260427-165654
  • Branch: harness/smoke-cli-mail-template-write-20260427-165654
  • Target: main

Sprints

ID Title Status Commit
S1 Synthesize transport contract for larksuite/cli passed
S2 Implement mail +template-create / +template-update shortcuts passed 7cb8c8b

Summary

Adds two new write-side mail shortcuts to lark-cli:

  • mail +template-create — POST /open-apis/mail/v1/user_mailboxes/:uid/templates
  • mail +template-update — PUT full-replacement with --inspect / --print-patch-template / --patch-file / flat --set-* flags (Get → apply patch → PUT)

Shared template_compose.go covers HTML inline <img> scan + Drive upload (chunked >20MB / upload_all ≤20MB based on file size only) + cid rewrite, attachment SMALL/LARGE/inline classifier honoring inline-always-SMALL and largeBucket-once-set-stays-set invariants, pre-flight 3MB template_content cap and 25MB cumulative cap mirroring server-side limits, and plain-text buildBodyDiv reuse from existing +send / +draft-create. AuthTypes=["user","bot"], scope=mail:user_mailbox.message:modify. +template-update emits a last-write-wins concurrency warning.

Source specs

  • /tmp/harness-test/smoke-cli-mail-template-write-20260427-165654/input/spec.md

This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.

Summary by CodeRabbit

Release Notes

  • New Features
    • Added email template creation via +template-create shortcut with support for HTML/plain-text body, inline images, and file attachments
    • Added email template updates via +template-update shortcut with inspect mode, JSON patch editing, and preview functionality

Add two new write-side mail shortcuts that wrap the user_mailbox.templates
POST/PUT endpoints, plus a shared compose helper covering HTML inline-image
upload, cid rewrite, attachment SMALL/LARGE/inline classification, and the
3 MB body / 25 MB cumulative server-mirrored caps. Templates reject the
LARGE branch entirely so apply-into-draft never silently degrades.

sprint: S2
@github-actions github-actions Bot added domain/mail PR touches the mail domain size/L Large or sensitive change across domains or core paths labels Apr 27, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

📝 Walkthrough

Walkthrough

This PR introduces two new mail shortcuts for managing email templates: +template-create for creating templates via POST and +template-update for updating templates via PUT. Both shortcuts include validation, composition logic with Drive file upload integration, inline image CID rewriting, attachment size tracking, and comprehensive test coverage. Reference documentation and shortcut registry updates are included.

Changes

Cohort / File(s) Summary
Template Create Shortcut
shortcuts/mail/mail_template_create.go
Implements +template-create shortcut with input validation, flag parsing, template composition, inline image/attachment upload, and response formatting including template ID extraction and attachment count reporting.
Template Update Shortcut
shortcuts/mail/mail_template_update.go
Implements +template-update shortcut supporting inspect-only, patch template printing, and patch-mode updates with --patch-file JSON merge and flat --set-* overrides; includes full-replacement PUT logic and last-write-wins warnings.
Shared Composition Logic
shortcuts/mail/template_compose.go
Provides template composition module enforcing 3MB content and 25MB cumulative size limits, inline image CID rewriting, Drive upload dispatch logic (single vs multipart based on file size), and OAPI request payload builders for POST/PUT operations.
Test Suites
shortcuts/mail/mail_template_create_test.go, shortcuts/mail/mail_template_update_test.go
Comprehensive test coverage validating attachment classification, content size enforcement, body wrapping, Drive upload dispatch, HTML composition with image rewriting, patch precedence, response parsing, and dry-run step enumeration.
Registry & Documentation
shortcuts/mail/shortcuts.go, skills/lark-mail/references/lark-mail-template-*.md
Updates shortcut registry to expose new templates; adds reference docs defining template constraints, CLI examples, parameters, and response structures for both create and update operations.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI Input
    participant Validate as Validator
    participant Compose as Composer
    participant Drive as Drive API
    participant Mail as Mail API
    participant Resp as Response

    CLI->>Validate: Provide flags (name, subject, content)
    Validate->>Validate: Check required fields & size limits
    Validate-->>Compose: ✓ Valid
    
    Compose->>Compose: Wrap body, resolve inline images
    Compose->>Compose: Build attachment list, classify types
    Compose->>Drive: Upload inline images & files
    Drive-->>Compose: File tokens
    Compose->>Compose: Rewrite img src to cid:, validate CID
    Compose->>Compose: Build OAPI request payload
    Compose->>Mail: POST template
    Mail-->>Resp: Response with template_id
    Resp-->>CLI: Format & output (template_id, attachments)
Loading
sequenceDiagram
    participant CLI as CLI Input
    participant Validate as Validator
    participant Route as Mode Router
    participant Inspect as Inspect Path
    participant Patch as Patch Path
    participant Compose as Composer
    participant Drive as Drive API
    participant Mail as Mail API
    participant Resp as Response

    CLI->>Validate: Provide --template-id, mode flags, inputs
    Validate->>Validate: Validate template-id format, mode exclusivity
    Validate-->>Route: ✓ Valid

    alt Inspect Mode
        Route->>Inspect: --inspect flag
        Inspect->>Mail: GET template
        Mail-->>Inspect: Existing template
        Inspect-->>CLI: Print projection
    else Patch Print Mode
        Route->>Patch: --print-patch-template flag
        Patch-->>CLI: Print JSON skeleton
    else Patch Update Mode
        Route->>Patch: --patch-file and/or --set-* overrides
        Patch->>Mail: GET existing template
        Mail-->>Patch: Existing template
        Patch->>Patch: Merge patch file + apply overrides
        Patch->>Compose: Composed template payload
        Compose->>Drive: Upload attachments
        Drive-->>Compose: File tokens
        Compose->>Mail: PUT (full replacement)
        Mail-->>Resp: Response
        Resp-->>CLI: Format & output
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

enhancement, domain/mail, size/XL

Suggested reviewers

  • chanthuang

Poem

🐰 Templates spring forth with +create and +update,
Inline images dance with CIDs to permeate,
Drive uploads dispatch with size-conscious care,
While validations guard each gigabyte fair! 📧✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete. It lacks a structured Summary section, detailed Changes list, Test Plan checklist, and Related Issues section as specified in the template. Reformat the description to match the template structure: add a concise Summary, expand Changes into a bulleted list, include the Test Plan checklist, and explicitly state Related Issues.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding two new mail shortcuts for template creation and updating.
Docstring Coverage ✅ Passed Docstring coverage is 91.11% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7cb8c8be56902f12a7e2256f6ef2708613214293

🧩 Skill update

npx skills add infeng/cli#harness/smoke-cli-mail-template-write-20260427-165654 -y -g

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
shortcuts/mail/mail_template_update_test.go (3)

329-331: Dead compile-time anchor — drop it (and possibly the draftpkg import).

draftpkg.ResolveLocalImagePaths is already imported and used by template_compose.go; if it were renamed, the production code would fail to compile long before this test does. The var _ line is redundant and the draftpkg import is unused otherwise — goimports/go vet would flag it once the anchor is removed.

♻️ Proposed cleanup
 import (
 	...
-	draftpkg "github.com/larksuite/cli/shortcuts/mail/draft"
 	...
 )

-// Ensure ResolveLocalImagePaths is referenced (compile-time anchor for the
-// rewritten path the body test depends on).
-var _ = draftpkg.ResolveLocalImagePaths
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_template_update_test.go` around lines 329 - 331, Remove
the redundant compile-time anchor referencing draftpkg.ResolveLocalImagePaths in
mail_template_update_test.go: delete the line "var _ =
draftpkg.ResolveLocalImagePaths" and, if that makes the draftpkg import unused,
remove the draftpkg import as well so goimports/go vet won't flag an unused
import; keep all other test logic unchanged.

39-52: Replace itoaSeq with strconv.FormatInt.

This is a roll-your-own decimal converter for what strconv.FormatInt(i, 10) already does, and it doesn't handle negative values (the loop assumes i > 0).

♻️ Proposed simplification
+import "strconv"

-func itoaSeq(i int64) string {
-	const digits = "0123456789"
-	if i == 0 {
-		return "0"
-	}
-	var b [20]byte
-	pos := len(b)
-	for i > 0 {
-		pos--
-		b[pos] = digits[i%10]
-		i /= 10
-	}
-	return string(b[pos:])
-}
+// (use strconv.FormatInt(i, 10) at the call site)

Then update line 28:

-		AppID:      "template-test-" + itoaSeq(templateTestSeq.Add(1)),
+		AppID:      "template-test-" + strconv.FormatInt(templateTestSeq.Add(1), 10),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_template_update_test.go` around lines 39 - 52, The custom
itoaSeq function should be removed and replaced with the standard library call
strconv.FormatInt(i, 10); update all uses of itoaSeq to call
strconv.FormatInt(seq, 10) (or strconv.FormatInt(i, 10)) so negative values are
handled correctly, and add the strconv import to the test file; ensure no other
custom itoaSeq references remain.

80-113: Test approach: chdir per test is process-global.

The os.Chdir(dir) + cleanup pattern (here and in lines 117-168, 174-219, 248-273, 278-309) works only because tests don't call t.Parallel(). If a future test in this package calls t.Parallel() it will race with these working-directory changes. Consider asking the upload helper to accept an explicit base path (or use runtime.FileIO().ResolvePath) so tests can stay parallel-safe. Not blocking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_template_update_test.go` around lines 80 - 113, The test
mutates process-wide working directory via os.Chdir in
TestUploadAttachmentToDrive_SinglePartDispatch which is unsafe for parallel
tests; change the test to avoid chdir by passing an explicit base path into the
helper used to open files (modify uploadAttachmentToDrive signature to accept a
baseDir or an absolute path) and update writeTempFile to return an absolute path
(or resolve using runtime.FileIO().ResolvePath inside uploadAttachmentToDrive);
update all other tests that call os.Chdir similarly to instead construct
absolute file paths and call the new uploadAttachmentToDrive(baseDir, ...) API
so the working directory is not changed.
shortcuts/mail/template_compose.go (2)

229-232: composedTemplate.driveSteps is set but never read.

The steps slice is collected in composeTemplate (line 281, appended to throughout), assigned to driveSteps (line 351), but neither caller (mail_template_create.go Execute nor mail_template_update.go Execute) reads composed.driveSteps. DryRun uses buildTemplateDryRunSteps which builds its own list. Either drop the field (and the steps accumulator inside composeTemplate) or wire it into a debug log.

Also applies to: 281-281, 343-352

🤖 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 229 - 232, The
composedTemplate.driveSteps field is populated in composeTemplate (the steps
slice is accumulated and assigned to composed.driveSteps) but never read by
callers (mail_template_create.go Execute and mail_template_update.go Execute)
because DryRun uses buildTemplateDryRunSteps; fix by either removing the unused
driveSteps field and the internal steps accumulator in composeTemplate, or wire
the populated composedTemplate.driveSteps into the callers (have Execute in
mail_template_create.go and mail_template_update.go read composed.driveSteps and
log or include it in DryRun output), updating composeTemplate, the
composedTemplate type, and the two Execute methods accordingly so the data is
actually consumed (or eliminate all related accumulation code to avoid dead
state).

119-152: Unreachable LARGE branch — consider dropping or asserting.

AppendAttachment's LARGE-classification branch (lines 134-141) is structurally unreachable in this PR: every constructor (newTemplateAttachmentBuilder) seeds rejectLarge = true, and the field is never flipped in production code (only in mail_template_create_test.go line 22 to test the invariant). If the design intent is "LARGE is permanently rejected for templates", an assert / panic("unreachable: rejectLarge invariant violated") would document that intent more clearly than dead branches; alternatively, drop the branch and the AttachmentTypeLARGE constant from this file.

🤖 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 119 - 152, AppendAttachment
contains a LARGE branch that's never taken because newTemplateAttachmentBuilder
initializes templateAttachmentBuilder.rejectLarge = true and production code
never flips it; either make that invariant explicit by adding a runtime
assertion/panic when rejectLarge is false (e.g., assert in
newTemplateAttachmentBuilder or at the top of AppendAttachment) to document
"LARGE not supported", or remove the unreachable branch and the
AttachmentTypeLARGE usage from templateAttachmentBuilder and related code;
locate symbols AppendAttachment, templateAttachmentBuilder,
newTemplateAttachmentBuilder, rejectLarge, and AttachmentTypeLARGE to apply the
change and update tests that flip rejectLarge in mail_template_create_test.go
accordingly.
🤖 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_update.go`:
- Line 47: The help text is misleading because the merge logic checks if
runtime.Str("set-to") != "" which cannot detect an explicit empty value; update
the merge logic to detect explicit assignment using
runtime.Cmd.Flags().Changed("set-to") and then treat an empty
runtime.Str("set-to") as a clear (replace recipients with empty list) — apply
the same pattern for "set-cc" and "set-bcc" where advertised; adjust the branch
that currently reads v := runtime.Str("set-to") (and the similar blocks) to
first check Flags().Changed(...) and then use the value (possibly empty) to set
or clear the corresponding To/Cc/Bcc fields, or alternatively remove the
parenthetical from the Desc if you prefer the simpler text-only fix.
- Around line 214-224: The patch template currently emits an "attachments" array
via patchTemplateSkeleton but applyPatchOverrides (function applyPatchOverrides)
ignores that key, so user edits to attachments are discarded; fix by extending
applyPatchOverrides to detect and validate the "attachments" entry from the
parsed patch map (same place it handles
"name","subject","content","to","cc","bcc"), convert each attachment object into
the same internal representation used by the projection/update flow and include
it in the returned overrides (or merge into the projection that is later saved)
so edited attachments round-trip; alternatively remove the "attachments" field
from patchTemplateSkeleton if you prefer not to support edits—pick one approach
and update patchTemplateSkeleton or applyPatchOverrides accordingly.

In `@skills/lark-mail/references/lark-mail-template-create.md`:
- Around line 19-22: The example command for "lark-cli mail +template-create"
uses the nonexistent flag "--body"; change that flag to the correct "--content"
flag (the one defined in mail_template_create.go) so the snippet reads "lark-cli
mail +template-create ... --content '<...>'" and will not fail with an
unknown-flag error.

In `@skills/lark-mail/references/lark-mail-template-update.md`:
- Line 50: Update the table row for `--set-attach` (and similarly
`--set-inline`) to document attachment-preservation semantics: state that when
omitted during a template update (patch mode), existing attachments will be
preserved unless the flag is provided to explicitly replace them; reference the
implementation location by mentioning mail_template_update.go and the flags
`--set-attach` / `--set-inline` so readers know this aligns with the code change
that preserves attachments on update.

---

Nitpick comments:
In `@shortcuts/mail/mail_template_update_test.go`:
- Around line 329-331: Remove the redundant compile-time anchor referencing
draftpkg.ResolveLocalImagePaths in mail_template_update_test.go: delete the line
"var _ = draftpkg.ResolveLocalImagePaths" and, if that makes the draftpkg import
unused, remove the draftpkg import as well so goimports/go vet won't flag an
unused import; keep all other test logic unchanged.
- Around line 39-52: The custom itoaSeq function should be removed and replaced
with the standard library call strconv.FormatInt(i, 10); update all uses of
itoaSeq to call strconv.FormatInt(seq, 10) (or strconv.FormatInt(i, 10)) so
negative values are handled correctly, and add the strconv import to the test
file; ensure no other custom itoaSeq references remain.
- Around line 80-113: The test mutates process-wide working directory via
os.Chdir in TestUploadAttachmentToDrive_SinglePartDispatch which is unsafe for
parallel tests; change the test to avoid chdir by passing an explicit base path
into the helper used to open files (modify uploadAttachmentToDrive signature to
accept a baseDir or an absolute path) and update writeTempFile to return an
absolute path (or resolve using runtime.FileIO().ResolvePath inside
uploadAttachmentToDrive); update all other tests that call os.Chdir similarly to
instead construct absolute file paths and call the new
uploadAttachmentToDrive(baseDir, ...) API so the working directory is not
changed.

In `@shortcuts/mail/template_compose.go`:
- Around line 229-232: The composedTemplate.driveSteps field is populated in
composeTemplate (the steps slice is accumulated and assigned to
composed.driveSteps) but never read by callers (mail_template_create.go Execute
and mail_template_update.go Execute) because DryRun uses
buildTemplateDryRunSteps; fix by either removing the unused driveSteps field and
the internal steps accumulator in composeTemplate, or wire the populated
composedTemplate.driveSteps into the callers (have Execute in
mail_template_create.go and mail_template_update.go read composed.driveSteps and
log or include it in DryRun output), updating composeTemplate, the
composedTemplate type, and the two Execute methods accordingly so the data is
actually consumed (or eliminate all related accumulation code to avoid dead
state).
- Around line 119-152: AppendAttachment contains a LARGE branch that's never
taken because newTemplateAttachmentBuilder initializes
templateAttachmentBuilder.rejectLarge = true and production code never flips it;
either make that invariant explicit by adding a runtime assertion/panic when
rejectLarge is false (e.g., assert in newTemplateAttachmentBuilder or at the top
of AppendAttachment) to document "LARGE not supported", or remove the
unreachable branch and the AttachmentTypeLARGE usage from
templateAttachmentBuilder and related code; locate symbols AppendAttachment,
templateAttachmentBuilder, newTemplateAttachmentBuilder, rejectLarge, and
AttachmentTypeLARGE to apply the change and update tests that flip rejectLarge
in mail_template_create_test.go accordingly.
🪄 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: ae0ab2db-5001-4a94-9c78-1a5226af6802

📥 Commits

Reviewing files that changed from the base of the PR and between 8ec95a4 and 7cb8c8b.

📒 Files selected for processing (8)
  • shortcuts/mail/mail_template_create.go
  • shortcuts/mail/mail_template_create_test.go
  • shortcuts/mail/mail_template_update.go
  • shortcuts/mail/mail_template_update_test.go
  • shortcuts/mail/shortcuts.go
  • shortcuts/mail/template_compose.go
  • skills/lark-mail/references/lark-mail-template-create.md
  • skills/lark-mail/references/lark-mail-template-update.md

{Name: "set-name", Desc: "Mode 3 flat override: replace template name."},
{Name: "set-subject", Desc: "Mode 3 flat override: replace template subject."},
{Name: "set-content", Desc: "Mode 3 flat override: replace template HTML/plain body. Same 3 MB cap; --plain-text wraps via buildBodyDiv before storage."},
{Name: "set-to", Desc: "Mode 3 flat override: replace To recipients (comma-separated; empty string clears)."},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Misleading description: --set-to cannot detect "empty string clears".

The description claims --set-to "" clears recipients, but the merge logic at line 300-302 uses if v := runtime.Str("set-to"); v != "", which can't distinguish "flag unset" from "flag set to empty". Passing --set-to "" is therefore a no-op, leaving the existing to recipients intact.

Either drop the misleading parenthetical from the help text, or detect explicit assignment via runtime.Cmd.Flags().Changed("set-to") and treat the empty-string case as a clear. (Same comment applies to set-cc/set-bcc if "clear via empty" is the intended UX — currently only set-to advertises it.)

📝 Quick text-only fix (defers behavior change)
-		{Name: "set-to", Desc: "Mode 3 flat override: replace To recipients (comma-separated; empty string clears)."},
+		{Name: "set-to", Desc: "Mode 3 flat override: replace To recipients (comma-separated). To clear, pass via --patch-file with \"to\": []."},
📝 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
{Name: "set-to", Desc: "Mode 3 flat override: replace To recipients (comma-separated; empty string clears)."},
{Name: "set-to", Desc: "Mode 3 flat override: replace To recipients (comma-separated). To clear, pass via --patch-file with \"to\": []."},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_template_update.go` at line 47, The help text is
misleading because the merge logic checks if runtime.Str("set-to") != "" which
cannot detect an explicit empty value; update the merge logic to detect explicit
assignment using runtime.Cmd.Flags().Changed("set-to") and then treat an empty
runtime.Str("set-to") as a clear (replace recipients with empty list) — apply
the same pattern for "set-cc" and "set-bcc" where advertised; adjust the branch
that currently reads v := runtime.Str("set-to") (and the similar blocks) to
first check Flags().Changed(...) and then use the value (possibly empty) to set
or clear the corresponding To/Cc/Bcc fields, or alternatively remove the
parenthetical from the Desc if you prefer the simpler text-only fix.

Comment on lines +214 to +224
"attachments": []map[string]interface{}{
{
"id": "<file_key>",
"filename": "<name>",
"cid": "<empty for non-inline>",
"is_inline": false,
"attachment_type": "SMALL",
},
},
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

patchTemplateSkeleton emits attachments but applyPatchOverrides ignores it.

The skeleton printed by --print-patch-template includes an attachments array intended for round-trip editing (lines 214-222), but applyPatchOverrides (lines 321-340) only handles name/subject/content/to/cc/bcc. A user who edits the skeleton's attachments and pipes it back via --patch-file will see those edits silently discarded.

This compounds the attachment-preservation issue flagged on lines 226-281. Either drop attachments from the skeleton or wire it into the apply path (and into the projection so existing attachments round-trip).

Also applies to: 321-340

🤖 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 214 - 224, The patch
template currently emits an "attachments" array via patchTemplateSkeleton but
applyPatchOverrides (function applyPatchOverrides) ignores that key, so user
edits to attachments are discarded; fix by extending applyPatchOverrides to
detect and validate the "attachments" entry from the parsed patch map (same
place it handles "name","subject","content","to","cc","bcc"), convert each
attachment object into the same internal representation used by the
projection/update flow and include it in the returned overrides (or merge into
the projection that is later saved) so edited attachments round-trip;
alternatively remove the "attachments" field from patchTemplateSkeleton if you
prefer not to support edits—pick one approach and update patchTemplateSkeleton
or applyPatchOverrides accordingly.

Comment on lines +226 to +281
// mergeExistingWithFlags applies --patch-file then flat --set-* flags
// (precedence: flag > patch-file > existing) to the GET projection, then
// projects the merged document into a templateComposeInput so compose can
// re-run uniformly with the create path.
func mergeExistingWithFlags(existing map[string]interface{}, runtime *common.RuntimeContext) (templateComposeInput, error) {
current := projectExistingTemplate(existing)

if pf := runtime.Str("patch-file"); pf != "" {
f, err := runtime.FileIO().Open(pf)
if err != nil {
return templateComposeInput{}, output.ErrValidation("read --patch-file %s: %v", pf, err)
}
raw, err := io.ReadAll(f)
_ = f.Close()
if err != nil {
return templateComposeInput{}, output.ErrValidation("read --patch-file %s: %v", pf, err)
}
var patch map[string]interface{}
if err := json.Unmarshal(raw, &patch); err != nil {
return templateComposeInput{}, output.ErrValidation("parse --patch-file %s as JSON: %v", pf, err)
}
applyPatchOverrides(&current, patch)
}

return buildTemplateUpdateComposeInput(runtime, &current)
}

// projectExistingTemplate flattens the GET response into the same string
// fields composeTemplate expects.
func projectExistingTemplate(resp map[string]interface{}) templateComposeInput {
body := resp
if data, ok := resp["data"].(map[string]interface{}); ok {
body = data
}
if tmpl, ok := body["template"].(map[string]interface{}); ok {
body = tmpl
}
return templateComposeInput{
Name: asString(body["name"]),
Subject: asString(body["subject"]),
Content: asString(body["content"]),
To: joinStringList(body["to"]),
CC: joinStringList(body["cc"]),
BCC: joinStringList(body["bcc"]),
// Note: attachments are not round-tripped through --set-attach
// because the existing entries are already file_keys, not local
// paths. Patch mode replaces them when --set-attach is provided;
// otherwise the existing entries are preserved by feeding them
// directly into the PUT body via composeTemplate's recipients-only
// shape (handled by the caller setting Attach=""). Templates with
// preserved attachments require server-side preservation (PUT is
// full-replace, so cli MUST resend the existing attachment
// id/filename/is_inline tuple — this is intentionally tracked as
// a known divergence in the §3.4 ledger if it bites).
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: existing attachments are silently dropped on partial updates.

Trace for lark-cli mail +template-update --template-id X --set-name new:

  1. projectExistingTemplate (lines 263-280) intentionally does not populate attachments — templateComposeInput has only Attach/Inline (local paths), and existing attachments are file_keys.
  2. applyPatchOverrides (lines 321-340) does not handle attachments either.
  3. buildTemplateUpdateComposeInput only sets Attach/Inline when --set-attach/--set-inline are given.
  4. composeTemplate builds Attachments exclusively from these local paths → empty when neither flag is set.
  5. templateBodyFromBuild marshals Attachments with omitempty → empty array on the wire.
  6. The endpoint is full-replacement PUT → server wipes all existing attachments.

The comment on lines 270-280 acknowledges this as a "known divergence" and even states "cli MUST resend the existing attachment id/filename/is_inline tuple" — but the code doesn't. For a write-side update of a template that has any inline images or attachments, this is silent data loss; users updating only the subject lose their banner image. The doc on lark-mail-template-update.md line 5 promises "全量替换" semantics without warning the user that omitting --set-attach is destructive.

Recommend one of:

  • Pass the existing attachments[] (already file_keys with id/filename/cid/is_inline/attachment_type) through projection→merge→PUT body, replacing only when --set-attach/--set-inline is provided.
  • Or, at minimum, fail-closed in patch mode when the existing template has attachments and no --set-attach/--set-inline was given, requiring an explicit --set-attach="" (or similar) to opt into wiping.

Want me to sketch the projection→PUT pass-through implementation (extending templateComposeInput or composedTemplate with a passthrough []TemplateAttachment and skipping it in composeTemplate when no new flags are set)?

Comment on lines +19 to +22
lark-cli mail +template-create \
--name '周报模板' --subject '本周进展' \
--body '<p>本周完成:</p><ul><li>...</li></ul>'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo: --body should be --content.

The shortcut defines a --content flag (per mail_template_create.go line 35); there is no --body. Other examples in this same file (lines 26, 31) correctly use --content. Running this snippet as written would fail with an unknown-flag error.

📝 Proposed fix
 lark-cli mail +template-create \
   --name '周报模板' --subject '本周进展' \
-  --body '<p>本周完成:</p><ul><li>...</li></ul>'
+  --content '<p>本周完成:</p><ul><li>...</li></ul>'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/lark-mail/references/lark-mail-template-create.md` around lines 19 -
22, The example command for "lark-cli mail +template-create" uses the
nonexistent flag "--body"; change that flag to the correct "--content" flag (the
one defined in mail_template_create.go) so the snippet reads "lark-cli mail
+template-create ... --content '<...>'" and will not fail with an unknown-flag
error.

| `--patch-file <path>` | 否 | 模式 3:JSON 补丁文件 |
| `--set-name/--set-subject/--set-content` | 否 | 模式 3 扁平覆盖 |
| `--set-to/--set-cc/--set-bcc` | 否 | 模式 3 收件人替换(PUT 全量替换语义) |
| `--set-attach` | 否 | 模式 3:附件本地路径 csv 替换;LARGE 分支拒绝 |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Document attachment-preservation behavior on update.

The --set-attach row says "替换" but doesn't tell users what happens when the flag is omitted in patch mode. Per the implementation in mail_template_update.go, omitting --set-attach and --set-inline currently causes the PUT full-replace to drop existing attachments (see the linked review comment on mail_template_update.go lines 226-281). Once that root-cause is fixed (preserve existing attachments unless explicitly replaced), please add a one-line note here so users understand the contract.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@skills/lark-mail/references/lark-mail-template-update.md` at line 50, Update
the table row for `--set-attach` (and similarly `--set-inline`) to document
attachment-preservation semantics: state that when omitted during a template
update (patch mode), existing attachments will be preserved unless the flag is
provided to explicitly replace them; reference the implementation location by
mentioning mail_template_update.go and the flags `--set-attach` / `--set-inline`
so readers know this aligns with the code change that preserves attachments on
update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/mail PR touches the mail domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant