Skip to content

feat(mail): auto-resolve local image paths in all draft entry points#205

Open
infeng wants to merge 6 commits intolarksuite:mainfrom
infeng:feat/auto-resolve-local-img-all-drafts
Open

feat(mail): auto-resolve local image paths in all draft entry points#205
infeng wants to merge 6 commits intolarksuite:mainfrom
infeng:feat/auto-resolve-local-img-all-drafts

Conversation

@infeng
Copy link
Copy Markdown
Collaborator

@infeng infeng commented Apr 2, 2026

Summary

Extend the auto-resolve local image paths feature (<img src="./local.png">cid: inline MIME) from draft-edit (patch) to all draft entry points: +draft-create, +send, +reply, +reply-all, and +forward. This supersedes and extends the reverted #139 by covering every code path that builds an EML with HTML body.

Changes

  • Extract ResolveLocalImagePaths as an exported function from draft/patch.go, decoupling it from the snapshot-based patch flow so EML-builder paths can reuse it
  • Apply ResolveLocalImagePaths in the HTML branch of mail_draft_create, mail_send, mail_reply, mail_reply_all, and mail_forward
  • Add validateInlineCIDs for bidirectional CID consistency checks (every cid: reference in HTML has a matching MIME part, every user-provided inline part is referenced in HTML)
  • Extract ValidateCIDReferences and FindOrphanedCIDs as exported helpers from the existing postProcessInlineImages logic
  • Include auto-resolved file paths in checkAttachmentSizeLimit to enforce size limits across all inline sources
  • Update addInlineImagesToBuilder to return srcCIDs for proper CID validation in reply/forward flows
  • Update skill reference docs to document local image path support
  • Add tests: mail_draft_create_test.go, mail_reply_forward_inline_test.go, expanded patch_inline_resolve_test.go and helpers_test.go

Test Plan

  • make unit-test passed
  • Manual: lark mail +draft-create --body '<img src="./logo.png">' ... resolves image inline
  • Manual: lark mail +send --body '<img src="./pic.jpg">' ... resolves image inline
  • Manual: lark mail +reply --body '<img src="./sig.png">' ... resolves image inline
  • Manual: lark mail +forward --body '<img src="./chart.png">' ... resolves image inline
  • Manual: orphaned/missing CID references produce clear validation errors
  • Manual: --plain-text mode correctly ignores local image resolution

Related Issues

Closes #81
Supersedes #139 (reverted in #199)

Summary by CodeRabbit

  • New Features

    • HTML bodies may embed local images via relative ; these are auto-resolved into inline MIME parts, deduplicated, assigned generated CIDs, and included in size checks. Remote/data/cid: and absolute/protocol-relative src values are ignored/rejected.
  • Bug Fixes

    • Orphaned inline parts are auto-removed when no longer referenced; inline CID validation rejects invalid/malformed CIDs and mismatched references.
  • Documentation

    • Updated draft/reply/forward/send examples to recommend relative-path embedding.
  • Tests

    • Extensive tests for resolution, reuse, validation, cleanup, and size-limit behavior.

infeng and others added 2 commits April 2, 2026 00:23
…ite#81) (larksuite#139)

* feat(mail): auto-resolve local image paths in draft body HTML (larksuite#81)

Allow <img src="./local/path.png" /> in set_body/set_reply_body HTML.
Local file paths are automatically resolved into inline MIME parts with
generated CIDs, eliminating the need to manually pair add_inline with
set_body. Removing or replacing an <img> tag in the body automatically
cleans up or replaces the corresponding MIME inline part.

- Add postProcessInlineImages to unify resolve, validate, and orphan
  cleanup into a single post-processing step
- Extract loadAndAttachInline shared helper to deduplicate addInline
  and resolveLocalImgSrc logic
- Cache resolved paths so the same file is only attached once
- Use whitelist URI scheme detection instead of blacklist
- Remove dead validateInlineCIDAfterApply and
  validateOrphanedInlineCIDAfterApply functions

Closes larksuite#81

* fix(mail): harden inline image CID handling

1. Fix imgSrcRegexp to skip attribute names like data-src/x-src that
   contain "src" as a suffix — only match the real src attribute.
2. Sanitize cidFromFileName to replace whitespace with hyphens,
   producing RFC-safe CID tokens (e.g. "my logo.png" → "my-logo").
3. Add CID validation in newInlinePart to reject spaces, tabs, angle
   brackets, and parentheses — fail fast instead of silently producing
   broken inline images in the sent email.

* refactor(mail): use UUID for auto-generated inline CIDs

Replace filename-derived CID generation (cidFromFileName + uniqueCID)
with UUID-based generation. UUIDs contain only [0-9a-f-] characters,
eliminating all RFC compliance risks from special characters, Unicode,
or filename collisions. Same-file deduplication via pathToCID cache
is preserved — multiple <img> tags referencing the same file still
share one MIME part and one CID.

* fix(mail): avoid panic in generateCID by using uuid.NewRandom

uuid.New() calls Must(NewRandom()) which panics if the random source
fails. Replace with uuid.NewRandom() and propagate the error through
resolveLocalImgSrc, so the CLI returns a clear error instead of
crashing in extreme environments.

* fix(mail): restore quote block hint in set_reply_body template description

The auto-resolve PR accidentally dropped "the quote block is
re-appended automatically" from the set_reply_body shape description.
Restore it alongside the new local-path support note.

* fix(mail): add orphan invariant comment and expand regex test coverage

- Add comment in postProcessInlineImages explaining that partially
  attached inline parts on error are cleaned up by the next Apply.
- Add regex test cases: single-quoted src, multiple spaces before src,
  and newline before src.

* fix(mail): use consistent inline predicate and safer HTML part lookup

1. removeOrphanedInlineParts: change condition from
   ContentDisposition=="inline" && ContentID!="" to
   isInlinePart(child) && ContentID!="", matching the predicate used
   elsewhere — parts with only a ContentID (no Content-Disposition)
   are now correctly cleaned up.
2. postProcessInlineImages: use findPrimaryBodyPart instead of
   findPart(snapshot.Body, PrimaryHTMLPartID) to avoid stale PartID
   after ops restructure the MIME tree.

* fix(mail): revert orphan cleanup to ContentDisposition check to protect HTML body

The previous change (d3d1982) broadened the orphan cleanup predicate to
isInlinePart(), which treats any part with a ContentID as inline. This
deletes the primary HTML body when it carries a Content-ID header
(valid in multipart/related), even on metadata-only edits like
set_subject.

Revert to the original ContentDisposition=="inline" && ContentID!=""
condition — only parts explicitly marked as inline attachments are
candidates for orphan removal. Add regression test covering
multipart/related with a Content-ID-bearing HTML body.
…points

Extract ResolveLocalImagePaths as exported function from draft/patch.go
and apply it uniformly across draft-create, send, reply, reply-all, and
forward commands. Add bidirectional CID validation via validateInlineCIDs
to ensure HTML references and MIME inline parts stay consistent. Update
skill references and add tests for the new inline resolution paths.
@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 2, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds HTML-driven local image resolution: detects relative <img src="./..."> in HTML, rewrites to cid:..., embeds matching inline MIME parts, validates CID references, and auto-removes orphaned inline parts. Exposes helpers for resolving, validating, and finding orphaned CIDs and updates mail flows, tests, and docs.

Changes

Cohort / File(s) Summary
Core inline image resolution pipeline
shortcuts/mail/draft/patch.go, shortcuts/mail/draft/patch_inline_resolve_test.go
Introduce ResolveLocalImagePaths, LocalImageRef, CID generation, ValidateCIDReferences, FindOrphanedCIDs, removeOrphanedInlineParts; tighten CID validation; refactor inline-attachment flow (loadAndAttachInline / addInline); comprehensive tests for resolution, dedupe, reuse, errors, and orphan lifecycle.
Patch behavior test update
shortcuts/mail/draft/patch_test.go
Adjust tests to expect auto-removal of orphaned inline MIME parts (no longer an error).
Helpers & builder adjustments
shortcuts/mail/helpers.go, shortcuts/mail/helpers_test.go
Add validateInlineCIDs (uses draftpkg helpers), change addInlineImagesToBuilder to also return normalized CIDs, update tests to new signature, and adjust attachment size-check flow.
Draft create flow & tests
shortcuts/mail/mail_draft_create.go, shortcuts/mail/mail_draft_create_test.go
Resolve local images early in draft creation, embed auto-resolved inline files via AddFileInline, include auto-resolved paths in size checks, validate combined CIDs, and add tests covering resolution, size accounting, and error cases.
Reply / Reply-All / Forward / Send flows
shortcuts/mail/mail_forward.go, shortcuts/mail/mail_reply.go, shortcuts/mail/mail_reply_all.go, shortcuts/mail/mail_send.go
Parse --inline early, call ResolveLocalImagePaths for HTML bodies, add auto-resolved inline files, track autoResolvedPaths, validate CIDs via validateInlineCIDs, and include all file paths in pre-attachment size checks. Removed unconditional post-attachment inline embedding.
Reply/Forward integration tests
shortcuts/mail/mail_reply_forward_inline_test.go
Add HTTP-stubbing helper and end-to-end tests for source inline preservation, local-path auto-resolution, orphan handling, and plaintext behavior.
Docs and CLI examples
skills/lark-mail/references/... (6 files)
Update examples and parameter docs to recommend relative-path <img src="./..."> embedding, reframe --inline as advanced/manual CID mapping, and update patch-template docs for set_body/set_reply_body.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller
    participant Flow as Apply/Build Flow
    participant HTML as HTML Parser
    participant FS as FileSystem
    participant MIME as MIME Builder
    participant Validator as CID Validator

    Caller->>Flow: submit HTML body / patch
    Flow->>HTML: find <img src="./..."> refs
    HTML-->>Flow: list of local refs
    Flow->>FS: read image files
    FS-->>Flow: file bytes / error
    Flow->>MIME: generate CID & attach inline part
    MIME-->>Flow: Content-ID
    Flow->>HTML: rewrite src -> cid:ID
    HTML-->>Flow: updated HTML
    Flow->>Validator: ValidateCIDReferences(updated HTML, available CIDs)
    Validator-->>Flow: ok / error
    Flow->>MIME: remove orphaned inline parts (if any)
    MIME-->>Flow: cleaned MIME tree
    Flow-->>Caller: updated draft/eml or error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement, size/M

Suggested reviewers

  • chanthuang

Poem

🐰 I nibble at ./logo, find its tiny thread,
I wrap it up in cid and tuck it in bed,
Old lonely parts hop off, new CIDs snugly glide,
The draft hums with pictures, MIME parts side by side,
A rabbit cheers — the message hops out with pride! 🥕📮

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: extending auto-resolution of local image paths to all draft entry points in the mail feature.
Description check ✅ Passed The description covers all required template sections: a clear summary explaining the scope and motivation, a detailed list of main changes, a comprehensive test plan with manual verification items, and related issue references.

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

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

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

@infeng infeng requested a review from haidaodashushu April 2, 2026 02:52
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@354c14d846019b68e56c8cc65e8aa784dd7bf692

🧩 Skill update

npx skills add infeng/cli#feat/auto-resolve-local-img-all-drafts -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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
shortcuts/mail/helpers.go (1)

1788-1806: ⚠️ Potential issue | 🟠 Major

Filter source inline parts before downloading them.

This helper eagerly downloads and embeds every images entry. In reply/reply-all/forward, any source CID that is no longer referenced by the quoted HTML still gets fetched and attached, and its bytes/count never reach checkAttachmentSizeLimit because callers only pass local file paths there. That means an orphaned source inline can still block the command on a missing download_url, and large quoted inlines can bypass the 25 MB limit. Please pass the referenced-source CID set into this helper and return the downloaded byte/count totals so callers only add/count the parts that still appear in the quote.

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

In `@shortcuts/mail/helpers.go` around lines 1788 - 1806, The
addInlineImagesToBuilder helper currently downloads every inlineSourcePart
indiscriminately; change its signature (addInlineImagesToBuilder) to accept a
set/map of referenced CIDs (e.g., referencedCIDs map[string]struct{}) and filter
the images slice by normalized CID (use normalizeInlineCID) before calling
downloadAttachmentContent so only referenced sources are fetched; while
building, accumulate and return totalDownloadedBytes and totalDownloadedCount
alongside the modified emlbuilder.Builder and cids so callers can pass those
totals into checkAttachmentSizeLimit and avoid counting/unblocking orphaned or
oversized inlines.
🧹 Nitpick comments (3)
shortcuts/mail/helpers_test.go (1)

742-763: Assert the new CID-return contract instead of discarding it.

These tests were updated for the extra return value, but they still don't verify it. The reply/forward/send paths now rely on that slice for CID validation, so this is the part most likely to regress silently.

🧪 Suggested assertions
-	_, _, err := addInlineImagesToBuilder(rt, bld, images)
+	_, cids, err := addInlineImagesToBuilder(rt, bld, images)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	if len(cids) != 0 {
+		t.Fatalf("returned CIDs = %v, want none", cids)
+	}
@@
-	result, _, err := addInlineImagesToBuilder(rt, bld, images)
+	result, cids, err := addInlineImagesToBuilder(rt, bld, images)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
+	if len(cids) != 1 || cids[0] != "banner" {
+		t.Fatalf("returned CIDs = %v, want [banner]", cids)
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/helpers_test.go` around lines 742 - 763, The tests call
addInlineImagesToBuilder but don't assert the new returned CID slice; update
both test cases to capture that return (e.g., result or cids) and assert its
length equals len(images) and that each element equals the corresponding
inlineSourcePart.CID (for the example test expect []string{"cid:banner"}).
Ensure you assert the CID slice in the earlier call that currently ignores
returns (replace the leading underscores) and in
TestAddInlineImagesToBuilder_Success (check variable result contains the
expected CID values).
shortcuts/mail/mail_reply_forward_inline_test.go (1)

129-147: These orphan-source tests still mask eager inline downloads.

Each case gives the unreferenced source image a valid download_url and a registered download stub, so the test still passes if reply/reply-all/forward downloads every source inline part. Please make the orphan image lack a URL or assert that its download endpoint is never hit, so this regression is actually covered.

Also applies to: 174-201, 208-226

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

In `@shortcuts/mail/mail_reply_forward_inline_test.go` around lines 129 - 147, The
orphan-source inline-image tests (e.g., TestReply_SourceOrphanCIDNotBlocked)
currently provide a valid download_url and a registered download stub for the
unreferenced image which masks eager downloads; update the stub setup in
stubSourceMessageWithInlineImages (used by TestReply_SourceOrphanCIDNotBlocked
and the similar tests around 174-201 and 208-226) so the orphan inline part has
no download_url (or remove its download stub), or alternatively add an assertion
on the HTTP mock layer that the orphan image's download endpoint is never called
after runMountedMailShortcut(MailReply, ...) to ensure reply/reply-all/forward
do not attempt to download unreferenced source inline parts.
shortcuts/mail/mail_draft_create_test.go (1)

14-14: Consider checking os.WriteFile errors for test robustness.

While tests will fail anyway if files aren't created, explicitly checking errors improves debuggability. This is a minor suggestion.

Example fix
-	os.WriteFile("test_image.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644)
+	if err := os.WriteFile("test_image.png", []byte{0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A}, 0o644); err != nil {
+		t.Fatalf("WriteFile() error = %v", err)
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_draft_create_test.go` at line 14, The os.WriteFile call
in mail_draft_create_test.go should check and handle the returned error to
improve test debuggability: capture the error from
os.WriteFile("test_image.png", ...) and fail the test if non-nil (e.g., t.Fatalf
or require.NoError) so failures report the write error immediately; update the
test helper or the test function that contains the os.WriteFile invocation to
perform this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 1043-1049: The comment indicates a gofmt formatting problem in the
block around the postProcessInlineImages function; run gofmt -w on the file
(shortcuts/mail/draft/patch.go) or reformat the function declaration and its
comment to comply with gofmt rules, ensuring the comment block and the function
name postProcessInlineImages (and the related note about
processInlineImagesForEML) are properly formatted and indented so static
analysis passes.

In `@shortcuts/mail/mail_forward.go`:
- Around line 129-157: This branch is downloading/validating all
sourceMsg.InlineImages before knowing which CIDs appear in the final HTML;
instead, build fullHTML first (use buildBodyDiv + buildForwardQuoteHTML +
draftpkg.ResolveLocalImagePaths to get resolved + refs and inlineSpecs), collect
the set of referenced CIDs/filepaths (refs' CIDs, inlineSpecs' CIDs, and any
CIDs found in resolved HTML), then filter sourceMsg.InlineImages to only those
whose CID is in that referenced set and pass that filtered slice into
addInlineImagesToBuilder (and only validate/download/count those items before
calling validateInlineCIDs); apply the same change to the other similar branch
mentioned (the branch around the other occurrence at lines ~197-198).

In `@shortcuts/mail/mail_reply_forward_inline_test.go`:
- Around line 1-276: The file fails gofmt; run gofmt (e.g. gofmt -w) on the test
file to fix spacing/indentation and import formatting so the file compiles in
CI; specifically reformat the stubSourceMessageWithInlineImages function and the
Test* functions (TestReply_SourceInlineImagesPreserved,
TestReply_SourceOrphanCIDNotBlocked, TestReply_WithAutoResolveLocalImage,
TestReplyAll_SourceOrphanCIDNotBlocked, TestForward_SourceOrphanCIDNotBlocked,
TestForward_WithAutoResolveLocalImage, TestReply_QuotedContentNotAutoResolved)
to match gofmt output and commit the formatted file.

In `@skills/lark-mail/references/lark-mail-draft-create.md`:
- Around line 46-52: Update the documentation text for the auto-resolved inline
image feature to explicitly state that only relative paths are supported (e.g.,
"./logo.png") and absolute paths like "/tmp/logo.png" or "C:\logo.png" will be
rejected; modify the `--body` description that currently shows `<img
src="./local.png" />`, the `--inline` parameter explanation, and the other
occurrence mentioned (lines 86–95) to replace vague "本地路径" with "相对路径,例如
`./logo.png`" and add a short note that absolute paths are not supported.

In `@skills/lark-mail/references/lark-mail-reply.md`:
- Around line 67-74: Update the docs for the `--body <text>` option to state
that embedded images must use relative paths (e.g., ./logo.png) rather than
“local path”; explicitly note that absolute paths like /tmp/logo.png or Windows
absolute paths (C:\... or C:/...) are not accepted because the resolver (see
isLocalFileSrc) intentionally returns false for those forms. Make the wording
concise and add a short example of a supported relative path.

---

Outside diff comments:
In `@shortcuts/mail/helpers.go`:
- Around line 1788-1806: The addInlineImagesToBuilder helper currently downloads
every inlineSourcePart indiscriminately; change its signature
(addInlineImagesToBuilder) to accept a set/map of referenced CIDs (e.g.,
referencedCIDs map[string]struct{}) and filter the images slice by normalized
CID (use normalizeInlineCID) before calling downloadAttachmentContent so only
referenced sources are fetched; while building, accumulate and return
totalDownloadedBytes and totalDownloadedCount alongside the modified
emlbuilder.Builder and cids so callers can pass those totals into
checkAttachmentSizeLimit and avoid counting/unblocking orphaned or oversized
inlines.

---

Nitpick comments:
In `@shortcuts/mail/helpers_test.go`:
- Around line 742-763: The tests call addInlineImagesToBuilder but don't assert
the new returned CID slice; update both test cases to capture that return (e.g.,
result or cids) and assert its length equals len(images) and that each element
equals the corresponding inlineSourcePart.CID (for the example test expect
[]string{"cid:banner"}). Ensure you assert the CID slice in the earlier call
that currently ignores returns (replace the leading underscores) and in
TestAddInlineImagesToBuilder_Success (check variable result contains the
expected CID values).

In `@shortcuts/mail/mail_draft_create_test.go`:
- Line 14: The os.WriteFile call in mail_draft_create_test.go should check and
handle the returned error to improve test debuggability: capture the error from
os.WriteFile("test_image.png", ...) and fail the test if non-nil (e.g., t.Fatalf
or require.NoError) so failures report the write error immediately; update the
test helper or the test function that contains the os.WriteFile invocation to
perform this check.

In `@shortcuts/mail/mail_reply_forward_inline_test.go`:
- Around line 129-147: The orphan-source inline-image tests (e.g.,
TestReply_SourceOrphanCIDNotBlocked) currently provide a valid download_url and
a registered download stub for the unreferenced image which masks eager
downloads; update the stub setup in stubSourceMessageWithInlineImages (used by
TestReply_SourceOrphanCIDNotBlocked and the similar tests around 174-201 and
208-226) so the orphan inline part has no download_url (or remove its download
stub), or alternatively add an assertion on the HTTP mock layer that the orphan
image's download endpoint is never called after
runMountedMailShortcut(MailReply, ...) to ensure reply/reply-all/forward do not
attempt to download unreferenced source inline parts.
🪄 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: 27bed34a-b2f5-41a1-9698-97bf0da5cc0e

📥 Commits

Reviewing files that changed from the base of the PR and between eda2b9c and 1fac59a.

📒 Files selected for processing (19)
  • shortcuts/mail/draft/patch.go
  • shortcuts/mail/draft/patch_inline_resolve_test.go
  • shortcuts/mail/draft/patch_test.go
  • shortcuts/mail/helpers.go
  • shortcuts/mail/helpers_test.go
  • shortcuts/mail/mail_draft_create.go
  • shortcuts/mail/mail_draft_create_test.go
  • shortcuts/mail/mail_draft_edit.go
  • shortcuts/mail/mail_forward.go
  • shortcuts/mail/mail_reply.go
  • shortcuts/mail/mail_reply_all.go
  • shortcuts/mail/mail_reply_forward_inline_test.go
  • shortcuts/mail/mail_send.go
  • skills/lark-mail/references/lark-mail-draft-create.md
  • skills/lark-mail/references/lark-mail-draft-edit.md
  • skills/lark-mail/references/lark-mail-forward.md
  • skills/lark-mail/references/lark-mail-reply-all.md
  • skills/lark-mail/references/lark-mail-reply.md
  • skills/lark-mail/references/lark-mail-send.md

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR extends the auto-resolve local <img src="./path">cid: inline MIME feature from the snapshot/patch (+draft-edit) path to all five EML-builder entry points (+draft-create, +send, +reply, +reply-all, +forward). It extracts ResolveLocalImagePaths and ValidateCIDReferences/FindOrphanedCIDs as shared exported helpers, and unifies post-processing (resolve → validate → orphan-sweep) into postProcessInlineImages for the snapshot path and the equivalent inline validateInlineCIDs calls for the EML builder path.

The implementation is consistent across all entry points, deduplication and size-limit accounting for auto-resolved paths are correctly handled, and the test coverage is thorough.

Confidence Score: 5/5

Safe to merge; all findings are P2 style/maintenance suggestions with no runtime impact on the happy path.

All three findings are P2: a stale comment referencing a non-existent helper function, a mildly confusing error message for absolute-path img src values, and an edge-case risk (hard error on replies to malformed third-party emails with inconsistent CID/attachment lists). None of these affect normal usage and the core implementation is correct, consistent, and well-tested across all five entry points.

shortcuts/mail/draft/patch.go (stale processInlineImagesForEML comment); shortcuts/mail/helpers.go (validateInlineCIDs quoted-CID edge case)

Important Files Changed

Filename Overview
shortcuts/mail/draft/patch.go Major refactor: extracts ResolveLocalImagePaths, ValidateCIDReferences, FindOrphanedCIDs as exported helpers; unifies post-processing into postProcessInlineImages; adds newInlinePart CID character validation. One stale comment references non-existent processInlineImagesForEML.
shortcuts/mail/helpers.go Adds validateInlineCIDs and updated addInlineImagesToBuilder (returns srcCIDs); logic is correct but ValidateCIDReferences on fullHTML could fail for replies to emails with inconsistent CID/attachment lists.
shortcuts/mail/mail_reply.go Adds ResolveLocalImagePaths for local img src in reply body, with size-limit accounting for auto-resolved paths and bidirectional CID validation; implementation consistent with other entry points.
shortcuts/mail/mail_forward.go Mirrors reply changes: local image resolution in the prepended body (not the quote), auto-resolved paths included in size limit, correct validateInlineCIDs call with srcCIDs from source message.
shortcuts/mail/mail_send.go Local image resolution added to HTML body path; auto-resolved paths included in size check; validateInlineCIDs called with both auto-resolved and user-provided CIDs.
shortcuts/mail/mail_draft_create.go Same pattern as mail_send.go — correct auto-resolve integration in the HTML branch; size-limit accounting and CID validation are consistent.
shortcuts/mail/mail_reply_all.go Mirrors mail_reply.go changes identically for the reply-all flow; correct and consistent.
shortcuts/mail/draft/patch_inline_resolve_test.go New test file with ~950 lines covering basic resolution, multi-image, deduplication, same-file reuse, non-image rejection, orphan cleanup, mixed add_inline+local, regex edge cases, and plain-text no-op; thorough coverage.
shortcuts/mail/mail_reply_forward_inline_test.go New integration tests for reply/forward inline resolution paths; covers happy path, size-limit enforcement, and plain-text bypass.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User HTML body with local img src] --> B{bodyIsHTML?}
    B -- No --> C[TextBody - no resolution]
    B -- Yes --> D[ResolveLocalImagePaths]
    D --> E[Replace local paths with cid:uuid in HTML]
    E --> F[Collect LocalImageRefs]
    F --> G{Entry point}

    G -- draft-edit patch --> H[resolveLocalImgSrc\nloadAndAttachInline\nadds MIME parts to snapshot]
    H --> I[postProcessInlineImages\nvalidate CIDs\nremove orphaned parts]
    I --> J[refreshSnapshot]

    G -- send / draft-create --> K[bld.AddFileInline per ref\ncheckAttachmentSizeLimit\nvalidateInlineCIDs]
    G -- reply / reply-all --> L[addInlineImagesToBuilder\nsrc message images\nAddFileInline per ref\nvalidateInlineCIDs with srcCIDs]
    G -- forward --> M[addInlineImagesToBuilder\nforwardQuote\nAddFileInline per ref\nvalidateInlineCIDs with srcCIDs]

    K --> N[bld.BuildBase64URL]
    L --> N
    M --> N
Loading

Reviews (5): Last reviewed commit: "test(mail): add CRLF case to newInlinePa..." | Re-trigger Greptile

@haidaodashushu
Copy link
Copy Markdown
Collaborator

haidaodashushu commented Apr 2, 2026

One additional edge case I did not see covered in the existing comments: CID uniqueness across source inline parts and user-provided inline parts.

In +reply / +reply-all / +forward, the final message can contain:

  • inline parts copied from the source message (srcCIDs)
  • user-provided inline parts from --inline
  • auto-resolved local images from <img src="./...">

validateInlineCIDs() currently checks reference completeness and orphaned user CIDs, but it does not reject duplicate Content-IDs across those groups. That means a user can provide --inline with a CID that already exists in the quoted source message, and the built EML may end up with two MIME parts sharing the same Content-ID. In that situation, <img src="cid:..."> resolution becomes ambiguous and client behavior is not guaranteed.

I do not think this is a blocker for this PR, but since this change is already expanding inline-CID validation, it may be worth adding one more validation step to enforce global CID uniqueness in the final message, plus a regression test for reply/forward flows.

Fix gofmt formatting issues in patch.go (missing blank comment line)
and mail_reply_forward_inline_test.go (extra blank comment line).
Update all skill reference docs to say "relative path" instead of
"local path" since absolute paths are rejected by design.
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.

🧹 Nitpick comments (1)
shortcuts/mail/draft/patch.go (1)

1067-1084: Consider global CID uniqueness validation for robustness.

As noted in the PR discussion, there's a potential edge case where Content-IDs could collide across multiple sources (source message inline parts in reply/forward, user-provided --inline parts, and auto-resolved local images). While the UUID-based generateCID() prevents collisions among auto-generated CIDs, a user-provided CID via add_inline could theoretically duplicate an existing Content-ID.

The current ValidateCIDReferences and FindOrphanedCIDs check reference completeness but not global uniqueness. Consider adding a uniqueness check when collecting cidParts at line 1069, or documenting that duplicate CIDs are undefined behavior.

🔧 Optional: Add duplicate CID detection
 	// Collect all CIDs present as MIME parts.
 	var cidParts []string
+	cidSeen := make(map[string]bool)
 	for _, part := range flattenParts(snapshot.Body) {
 		if part != nil && part.ContentID != "" {
+			lowerCID := strings.ToLower(part.ContentID)
+			if cidSeen[lowerCID] {
+				return fmt.Errorf("duplicate Content-ID %q found in MIME parts", part.ContentID)
+			}
+			cidSeen[lowerCID] = true
 			cidParts = append(cidParts, part.ContentID)
 		}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/draft/patch.go` around lines 1067 - 1084, The code collects
CIDs from flattenParts(snapshot.Body) but doesn't validate global uniqueness, so
add a duplicate-detection step after building cidParts: iterate cidParts (use
strings.ToLower to normalize) into a seen map and if any CID already exists
return a clear error (or wrap into the existing ValidateCIDReferences error
flow); update ValidateCIDReferences or call a new ValidateUniqueCIDs helper to
ensure user-provided or source CIDs (e.g., from add_inline or generateCID)
cannot collide before calling extractCIDRefs and removeOrphanedInlineParts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 1067-1084: The code collects CIDs from flattenParts(snapshot.Body)
but doesn't validate global uniqueness, so add a duplicate-detection step after
building cidParts: iterate cidParts (use strings.ToLower to normalize) into a
seen map and if any CID already exists return a clear error (or wrap into the
existing ValidateCIDReferences error flow); update ValidateCIDReferences or call
a new ValidateUniqueCIDs helper to ensure user-provided or source CIDs (e.g.,
from add_inline or generateCID) cannot collide before calling extractCIDRefs and
removeOrphanedInlineParts.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 18ce47f6-cd73-472f-bee6-f7eb558d9bc7

📥 Commits

Reviewing files that changed from the base of the PR and between 1fac59a and 610fb51.

📒 Files selected for processing (8)
  • shortcuts/mail/draft/patch.go
  • shortcuts/mail/mail_reply_forward_inline_test.go
  • skills/lark-mail/references/lark-mail-draft-create.md
  • skills/lark-mail/references/lark-mail-draft-edit.md
  • skills/lark-mail/references/lark-mail-forward.md
  • skills/lark-mail/references/lark-mail-reply-all.md
  • skills/lark-mail/references/lark-mail-reply.md
  • skills/lark-mail/references/lark-mail-send.md
✅ Files skipped from review due to trivial changes (3)
  • skills/lark-mail/references/lark-mail-reply-all.md
  • skills/lark-mail/references/lark-mail-send.md
  • shortcuts/mail/mail_reply_forward_inline_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • skills/lark-mail/references/lark-mail-forward.md
  • skills/lark-mail/references/lark-mail-draft-create.md
  • skills/lark-mail/references/lark-mail-reply.md
  • skills/lark-mail/references/lark-mail-draft-edit.md

@infeng
Copy link
Copy Markdown
Collaborator Author

infeng commented Apr 2, 2026

Good catch — you're right that validateInlineCIDs does not enforce cross-group CID uniqueness today.

In practice the risk is very low:

  • Auto-resolved CIDs use uuid.NewRandom(), so collisions with source CIDs are effectively impossible.
  • Only a manual --inline where the user happens to pick a CID identical to a source message's existing CID would trigger this. That's an unusual and unlikely scenario.

That said, it's a valid hardening point. Adding a uniqueness check across srcCIDs and userCIDs before building the EML would be straightforward. I'll track this as a follow-up improvement — agree it's not a blocker for this PR. Thanks for the review!

@infeng infeng closed this Apr 2, 2026
@infeng
Copy link
Copy Markdown
Collaborator Author

infeng commented Apr 2, 2026

Good catch — you're right that validateInlineCIDs does not enforce cross-group CID uniqueness today.

In practice the risk is very low:

  • Auto-resolved CIDs use uuid.NewRandom(), so collisions with source CIDs are effectively impossible.
  • Only a manual --inline where the user happens to pick a CID identical to a source message's existing CID would trigger this. That's an unusual and unlikely scenario.

That said, it's a valid hardening point. Adding a uniqueness check across srcCIDs and userCIDs before building the EML would be straightforward. I'll track this as a follow-up improvement — agree it's not a blocker for this PR. Thanks for the review!

@infeng infeng reopened this Apr 2, 2026
haidaodashushu
haidaodashushu previously approved these changes Apr 2, 2026
removeOrphanedInlineParts only searched multipart/related containers,
but some servers return drafts with inline parts as direct children of
multipart/mixed. When set_body replaced HTML with a local <img src>,
the old inline part survived under multipart/mixed while a new one was
created under a freshly wrapped multipart/related, causing CID mismatch.

Extend the cleanup to also search multipart/mixed containers and recurse
into remaining children after filtering. Add regression test with a
multipart/mixed draft containing a direct inline child.
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: 2

🧹 Nitpick comments (1)
shortcuts/mail/draft/patch_inline_resolve_test.go (1)

19-19: Fail fast on fixture setup errors.

These os.WriteFile / os.MkdirAll calls ignore setup failures, so a temp-dir or write problem will show up later as a confusing parse or assertion failure. A tiny helper in helpers_test.go would make the failing step explicit and remove most of the duplicated setup code.

Also applies to: 71-72, 147-149, 183-183, 228-228, 290-290, 345-345, 394-395, 440-440, 523-523, 760-760, 797-797, 855-855

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

In `@shortcuts/mail/draft/patch_inline_resolve_test.go` at line 19, Replace the
silent os.WriteFile/os.MkdirAll calls with a fail-fast test helper: add
helpers_test.go with functions like mustWriteFile(t testing.TB, path string,
data []byte, perm fs.FileMode) and mustMkdirAll(t testing.TB, path string, perm
fs.FileMode) that call t.Helper(), invoke os.WriteFile/os.MkdirAll, check the
error and call t.Fatalf with a clear message including the path and err; then
update each occurrence in shortcuts/mail/draft/patch_inline_resolve_test.go (the
os.WriteFile and os.MkdirAll calls listed) to use mustWriteFile/mustMkdirAll so
setup failures fail immediately and explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/mail/draft/patch_inline_resolve_test.go`:
- Around line 641-667: Add test cases for Windows absolute paths to
TestIsLocalFileSrc so the helper's intentional exclusion of Windows-style paths
is locked in; update the tests slice in TestIsLocalFileSrc to include examples
like "C:\\Windows\\file.txt" and "C:/Windows/file.txt" (and optionally lowercase
drive letters such as "c:\\path") with want set to false, ensuring
isLocalFileSrc continues to return false for Windows absolute paths.
- Around line 578-593: The test TestResolveLocalImgSrcNoHTMLBody currently only
checks Apply didn't return an error; update it to assert the final text/plain
body content equals "Updated plain text." and that no inline parts were added.
After calling Apply(snapshot, ...), retrieve the text/plain part from the
snapshot produced by mustParseFixtureDraft (e.g. via the draft/snapshot API used
elsewhere: look for methods like GetBodyPart/GetPartByContentType or a
BodyParts/snapshot.Parts slice) and assert its content equals "Updated plain
text."; also verify there are no parts with Content-Type image/* or with
Content-Disposition "inline" (or that the snapshot's inline/attachments
collection length is zero) to ensure no inline parts were injected.

---

Nitpick comments:
In `@shortcuts/mail/draft/patch_inline_resolve_test.go`:
- Line 19: Replace the silent os.WriteFile/os.MkdirAll calls with a fail-fast
test helper: add helpers_test.go with functions like mustWriteFile(t testing.TB,
path string, data []byte, perm fs.FileMode) and mustMkdirAll(t testing.TB, path
string, perm fs.FileMode) that call t.Helper(), invoke os.WriteFile/os.MkdirAll,
check the error and call t.Fatalf with a clear message including the path and
err; then update each occurrence in
shortcuts/mail/draft/patch_inline_resolve_test.go (the os.WriteFile and
os.MkdirAll calls listed) to use mustWriteFile/mustMkdirAll so setup failures
fail immediately and explicitly.
🪄 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: d8eb9ff0-e24f-484c-8ac6-579480821c84

📥 Commits

Reviewing files that changed from the base of the PR and between 610fb51 and b91dabf.

📒 Files selected for processing (2)
  • shortcuts/mail/draft/patch.go
  • shortcuts/mail/draft/patch_inline_resolve_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • shortcuts/mail/draft/patch.go

Assert text/plain body content and absence of inline parts in
TestResolveLocalImgSrcNoHTMLBody. Add Windows absolute path cases
(C:\, C:/, c:\) to TestIsLocalFileSrc to lock in the intentional
rejection behavior.
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: 1

🧹 Nitpick comments (1)
shortcuts/mail/draft/patch_inline_resolve_test.go (1)

773-836: Add the absolute-path failure case for the exported resolver.

These ResolveLocalImagePaths tests only pin success and no-op paths. Please add one /absolute/path/logo.png case that expects an error, so the relative-path-only contract is enforced at the exported API boundary too.

🧪 Suggested coverage
+func TestResolveLocalImagePathsRejectsAbsolutePath(t *testing.T) {
+	_, _, err := ResolveLocalImagePaths(`<img src="/absolute/path/logo.png" />`)
+	if err == nil {
+		t.Fatal("expected error for absolute path")
+	}
+}

Based on learnings, auto-resolution is intended only for relative paths, and absolute paths should be rejected by validate.SafeInputPath.

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

In `@shortcuts/mail/draft/patch_inline_resolve_test.go` around lines 773 - 836,
Add a test case to TestResolveLocalImagePaths* that passes an absolute path like
"/absolute/path/logo.png" into ResolveLocalImagePaths and asserts it returns an
error (and zero refs / unchanged or specific behavior per validate.SafeInputPath
contract); this enforces the exported API rejects absolute paths — reference the
ResolveLocalImagePaths function and the validate.SafeInputPath behavior when
writing the assertion so the test expects failure for absolute file paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/mail/draft/patch_inline_resolve_test.go`:
- Around line 842-856: The test currently only checks spaces/tabs/brackets and
misses CRLF header-injection vectors; update
TestNewInlinePartRejectsInvalidCIDChars to include CID examples containing '\r',
'\n', and CRLF sequences (e.g. "bad\r\nInject: x" and variants like "a\r", "\n",
"\r\n", "foo\r\nbar") so it will fail if the validate.RejectCRLF guard in
newInlinePart is removed or broken; ensure these new bad CIDs are asserted to
produce an error (while keeping the existing valid-CID cases).

---

Nitpick comments:
In `@shortcuts/mail/draft/patch_inline_resolve_test.go`:
- Around line 773-836: Add a test case to TestResolveLocalImagePaths* that
passes an absolute path like "/absolute/path/logo.png" into
ResolveLocalImagePaths and asserts it returns an error (and zero refs /
unchanged or specific behavior per validate.SafeInputPath contract); this
enforces the exported API rejects absolute paths — reference the
ResolveLocalImagePaths function and the validate.SafeInputPath behavior when
writing the assertion so the test expects failure for absolute file paths.
🪄 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: c200fd46-d03a-4167-bd51-4fea3de3d9ef

📥 Commits

Reviewing files that changed from the base of the PR and between b91dabf and e72433d.

📒 Files selected for processing (1)
  • shortcuts/mail/draft/patch_inline_resolve_test.go

Pin the validate.RejectCRLF guard so header-injection protection
cannot silently regress.
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.

🧹 Nitpick comments (1)
shortcuts/mail/draft/patch_inline_resolve_test.go (1)

405-431: Add HTML postconditions to these mixed inline/local-path tests.

Both tests only count MIME parts. If set_body stopped preserving cid:a, or failed to rewrite ./b.png / ./logo.png in the final body, they would still pass. Please also assert the resulting HTML contains the expected cid: references and no remaining local file paths.

Also applies to: 452-479

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

In `@shortcuts/mail/draft/patch_inline_resolve_test.go` around lines 405 - 431,
The test currently only checks MIME parts but not the rendered HTML, so update
the test that calls Apply(...) with Patch ops (including add_inline and
set_body) to also assert snapshot.Body (or the string returned by flattenParts
if appropriate) contains the expected cid references and no unresolved local
paths: verify the body includes "cid:a" (preserved from add_inline) and that the
local "./b.png" was rewritten to a cid reference (no occurrences of "./b.png" or
"./logo.png" remain), and mirror the same additional HTML content assertions in
the other mixed inline/local-path test (the similar test block around lines
452-479) so both validate final HTML postconditions as well as MIME part counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@shortcuts/mail/draft/patch_inline_resolve_test.go`:
- Around line 405-431: The test currently only checks MIME parts but not the
rendered HTML, so update the test that calls Apply(...) with Patch ops
(including add_inline and set_body) to also assert snapshot.Body (or the string
returned by flattenParts if appropriate) contains the expected cid references
and no unresolved local paths: verify the body includes "cid:a" (preserved from
add_inline) and that the local "./b.png" was rewritten to a cid reference (no
occurrences of "./b.png" or "./logo.png" remain), and mirror the same additional
HTML content assertions in the other mixed inline/local-path test (the similar
test block around lines 452-479) so both validate final HTML postconditions as
well as MIME part counts.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 16176ae9-1bf4-4a21-9967-6856681aa82d

📥 Commits

Reviewing files that changed from the base of the PR and between e72433d and 354c14d.

📒 Files selected for processing (1)
  • shortcuts/mail/draft/patch_inline_resolve_test.go

Copy link
Copy Markdown
Collaborator

@haidaodashushu haidaodashushu left a comment

Choose a reason for hiding this comment

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

Found two blocking issues in the inline-image auto-resolve change.

return err
}
if err := validateInlineCIDAfterApply(snapshot); err != nil {
if err := postProcessInlineImages(snapshot); err != nil {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apply() now runs postProcessInlineImages() for every patch, not just body edits. That means a metadata-only edit like set_subject or recipient/header changes will scan the existing HTML for local <img src="./..."> paths, try to read files from disk, and potentially mutate the MIME tree or fail if the file is missing. This is a behavior regression for unrelated edits. The auto-resolve step needs to be gated to body-changing ops (set_body, set_reply_body, maybe explicit inline ops), otherwise an externally-created draft containing local paths can no longer be safely edited.

if err := validate.RejectCRLF(cid, "inline cid"); err != nil {
return nil, err
}
if strings.ContainsAny(cid, " \t<>()") {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This new CID restriction is only enforced on the newInlinePart / add_inline path. replace_inline still validates only CR/LF, so it can still write values like "my logo" or "img(1)" and bypass the new rule entirely. Please share the same CID validator between both paths and add a replace_inline test for the new invalid-character cases.

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.

mail +draft-edit: add_inline does not actually insert inline image into HTML body

2 participants