feat(mail): auto-resolve local image paths in draft body HTML (#81)#139
Merged
infeng merged 8 commits intolarksuite:mainfrom Apr 1, 2026
Merged
Conversation
Greptile SummaryThis PR implements automatic resolution of local Key changes:
Confidence Score: 4/5Safe to merge; all prior blocking concerns are resolved and the implementation is correct across the tested scenarios. The regex shortcuts/mail/draft/patch.go — specifically Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Apply called] --> B[Execute each PatchOp in order]
B --> C{op type?}
C -->|set_body / set_reply_body| D[Write HTML to htmlPart.Body]
C -->|add_inline| E[loadAndAttachInline → append to multipart/related]
C -->|remove_inline| F[Remove MIME part from tree]
C -->|other ops| G[Header / recipient changes]
D & E & F & G --> H[postProcessInlineImages]
H --> I[findPrimaryBodyPart → htmlPart]
I --> J{htmlPart found?}
J -->|No| K[return nil]
J -->|Yes| L[resolveLocalImgSrc]
L --> M{Any img matches?}
M -->|No| N[return html unchanged]
M -->|Yes| O[Iterate matches in reverse]
O --> P{isLocalFileSrc?}
P -->|No - cid: http: data: //| Q[skip]
P -->|Yes - relative or absolute path| R{pathToCID cache hit?}
R -->|Hit| S[reuse existing CID]
R -->|Miss| T[generateCID UUID]
T --> U[loadAndAttachInline]
U --> V[SafeInputPath + CheckInlineImageFormat + newInlinePart]
V --> W[append to multipart/related container]
S & W --> X[Replace src value with cid:UUID in html]
X --> Q
Q --> O
N & X --> Y[Update htmlPart.Body if changed]
Y --> Z[Build refSet from extractCIDRefs]
Z --> AA[Validate all cid: refs have matching MIME parts]
AA -->|missing ref| BB[return error]
AA -->|ok| CC[removeOrphanedInlineParts]
CC --> DD[Walk tree; remove unreferenced inline children from multipart/related]
DD --> EE[refreshSnapshot]
EE --> FF[return nil]
Reviews (4): Last reviewed commit: "fix(mail): revert orphan cleanup to Cont..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughApply now auto-resolves local HTML Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Apply as Apply
participant Parser as HTML Parser
participant Loader as loadAndAttachInline
participant MIME as MIME Container
participant Post as postProcessInlineImages
participant Snapshot as Snapshot
User->>Apply: submit patch with set_body (HTML with local <img src="./img.png">)
Apply->>Parser: scan HTML for <img src> values
Parser-->>Apply: returns local file paths
Apply->>Loader: loadAndAttachInline(path)
Loader->>MIME: read file, validate image, create/reuse multipart/related part and inline Part (generate CID)
MIME-->>Loader: attached Part with CID
Loader-->>Apply: return CID
Apply->>Post: postProcessInlineImages(snapshot)
Post->>Post: rewrite local src -> cid:, validate referenced CIDs, remove orphan inline parts
Post-->>Apply: post-processing complete
Apply->>Snapshot: refreshSnapshot(snapshot)
Snapshot-->>User: updated draft with cid: URIs and inline parts
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/mail/draft/patch.go (1)
913-926:uniqueCIDhas an unbounded loop but is practically safe.The
for i := 2; ; i++loop theoretically runs forever if all CIDs are taken. In practice, this is impossible since the suffix space is unbounded (integers), and the set of existing CIDs is finite.Consider adding a reasonable upper bound as defensive coding, though this is low priority.
♻️ Optional: Add upper bound for defensive coding
func uniqueCID(baseCID string, usedCIDs map[string]bool) string { if !usedCIDs[strings.ToLower(baseCID)] { return baseCID } - for i := 2; ; i++ { + for i := 2; i < 10000; i++ { candidate := fmt.Sprintf("%s-%d", baseCID, i) if !usedCIDs[strings.ToLower(candidate)] { return candidate } } + // Fallback: use UUID-style suffix (extremely unlikely to reach here) + return fmt.Sprintf("%s-%d", baseCID, time.Now().UnixNano()) }🤖 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 913 - 926, The uniqueCID function currently uses an unbounded for loop; add a defensive upper bound by introducing a const maxAttempts (e.g., 10000) and change the loop to for i := 2; i <= maxAttempts; i++ checking candidates via strings.ToLower against usedCIDs; if no unique candidate is found after maxAttempts, generate a guaranteed-unique fallback (for example append a time-based suffix like fmt.Sprintf("%s-%d", baseCID, time.Now().UnixNano()) or a random/UUID suffix) and return that; update imports (time or crypto/rand/uuid) as needed and keep the function signature unchanged.
🤖 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 913-926: The uniqueCID function currently uses an unbounded for
loop; add a defensive upper bound by introducing a const maxAttempts (e.g.,
10000) and change the loop to for i := 2; i <= maxAttempts; i++ checking
candidates via strings.ToLower against usedCIDs; if no unique candidate is found
after maxAttempts, generate a guaranteed-unique fallback (for example append a
time-based suffix like fmt.Sprintf("%s-%d", baseCID, time.Now().UnixNano()) or a
random/UUID suffix) and return that; update imports (time or crypto/rand/uuid)
as needed and keep the function signature unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96129ab7-b34f-4618-905d-31fc4ff6f750
📒 Files selected for processing (5)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_inline_resolve_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/mail_draft_edit.goskills/lark-mail/references/lark-mail-draft-edit.md
chanthuang
previously approved these changes
Mar 31, 2026
Collaborator
chanthuang
left a comment
chanthuang
left a comment
There was a problem hiding this comment.
LGTM. Clean design — collapsing the two-step add_inline + set_body workflow into a single <img src="./path"> auto-resolve is a big UX win for both agents and humans.
A few notes for follow-up (non-blocking):
-
Orphan cleanup is now silent — the old behavior errored on orphaned CIDs, the new behavior auto-removes them. Consider adding a stderr warning when parts are actually removed, so users aren't surprised if an image disappears from a draft.
-
Regex edge cases — might be worth adding test coverage for multi-space / newline between
<imgandsrc(e.g.<img src="./a.png">). -
Skill reference — consider marking
add_inlineas a low-level op in the docs, nudging agents toward the simpler local-path approach.
Test coverage is solid (20 new cases, 82.5% package coverage). The CID hardening in the second commit (whitespace → hyphens, validation for spaces/tabs/brackets) is a good catch.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/mail/draft/patch_inline_resolve_test.go (1)
651-695: Consider adding a test case for single-quotedsrcattributes.The
imgSrcRegexpsupports both single and double quotes, but the tests only use double quotes. A test case like<img src='./logo.png' />would verify this path works correctly.🤖 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 651 - 695, Add a test case to TestImgSrcRegexpSkipsDataSrc that verifies imgSrcRegexp matches single-quoted src attributes (e.g. `<img src='./logo.png' />`) by adding an entry in the tests slice with name like "single-quoted src", html set to a single-quoted src string, and want set to the expected captured value "./logo.png"; this ensures imgSrcRegexp (used in TestImgSrcRegexpSkipsDataSrc) is exercised for single-quote attribute syntax.shortcuts/mail/draft/patch.go (1)
952-978: Nestedmultipart/relatedcontainers are not recursively cleaned.When the root is
multipart/related, kept children are not recursively processed. If a draft contains nestedmultipart/relatedstructures, orphaned inline parts inside the nested container would not be removed.This is likely acceptable since nested
multipart/relatedis uncommon in email drafts. Consider adding a comment noting this limitation if you want to be explicit.🤖 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 952 - 978, The function removeOrphanedInlineParts currently skips recursive processing when root is multipart/related, so nested multipart/related containers are not cleaned; update it so that after you build the kept slice for a multipart/related root you iterate over kept children and call removeOrphanedInlineParts(child, referencedCIDs) for each kept child and if any child.Dirty is true set root.Dirty = true; keep existing behavior for non-related roots. Reference: function removeOrphanedInlineParts, root.Children, referencedCIDs, and root.Dirty.
🤖 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 651-695: Add a test case to TestImgSrcRegexpSkipsDataSrc that
verifies imgSrcRegexp matches single-quoted src attributes (e.g. `<img
src='./logo.png' />`) by adding an entry in the tests slice with name like
"single-quoted src", html set to a single-quoted src string, and want set to the
expected captured value "./logo.png"; this ensures imgSrcRegexp (used in
TestImgSrcRegexpSkipsDataSrc) is exercised for single-quote attribute syntax.
In `@shortcuts/mail/draft/patch.go`:
- Around line 952-978: The function removeOrphanedInlineParts currently skips
recursive processing when root is multipart/related, so nested multipart/related
containers are not cleaned; update it so that after you build the kept slice for
a multipart/related root you iterate over kept children and call
removeOrphanedInlineParts(child, referencedCIDs) for each kept child and if any
child.Dirty is true set root.Dirty = true; keep existing behavior for
non-related roots. Reference: function removeOrphanedInlineParts, root.Children,
referencedCIDs, and root.Dirty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57227bbf-d674-48a1-8554-e313ae1eb62c
📒 Files selected for processing (2)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_inline_resolve_test.go
chanthuang
previously approved these changes
Mar 31, 2026
…ite#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
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.
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.
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.
infeng
force-pushed
the
fix/mail-add-inline-insert-html
branch
from
a11111b to
7f1ac95
Compare
…ption 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.
chanthuang
previously approved these changes
Mar 31, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 984-988: The HTML post-processing currently uses
findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) which can pick the wrong
node when PrimaryHTMLPartID is empty/stale; change postProcessInlineImages to
locate the HTML node with findPrimaryBodyPart(snapshot.Body, "text/html") (the
same approach refreshSnapshot uses) so it reliably finds the actual HTML part in
DraftSnapshot regardless of PrimaryHTMLPartID, and then proceed with the
existing inline src rewriting/cleanup logic using that returned part instead of
findPart.
- Around line 883-891: The isLocalFileSrc function incorrectly treats Windows
absolute paths like "C:\..." or "C:/..." as URIs because uriSchemeRegexp matches
the "C:" prefix; update isLocalFileSrc to special-case Windows drive-letter
absolute paths before the scheme check by detecting a leading drive-letter
pattern (e.g., /^[A-Za-z]:[\\/]/) on the trimmed src and returning true for
those, while keeping the existing empty, double-slash, and uriSchemeRegexp
checks (function name: isLocalFileSrc; symbol: uriSchemeRegexp).
- Around line 964-977: The cleanup loop only removes children that are both
Content-Disposition:inline and have a Content-ID, but elsewhere any Part with a
ContentID is treated as inline; update the loop to treat parts with a non-empty
ContentID as inline (regardless of ContentDisposition) and remove them if their
CID is not referenced, while still removing parts that are
Content-Disposition:inline even if they lack a CID. Concretely, in the loop over
root.Children (the Part instances), first check if child.ContentID != "" and if
so consult referencedCIDs[strings.ToLower(child.ContentID)] and skip (and mark
root.Dirty) when unreferenced; otherwise if
strings.EqualFold(child.ContentDisposition, "inline") then mark root.Dirty and
skip; append all other children as before.
🪄 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: 2d401403-d812-452d-b6d0-d4897f1c20c8
📒 Files selected for processing (5)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_inline_resolve_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/mail_draft_edit.goskills/lark-mail/references/lark-mail-draft-edit.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-mail/references/lark-mail-draft-edit.md
- shortcuts/mail/mail_draft_edit.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_test.go
- 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.
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.
infeng
added
domain/mail
There was a problem hiding this comment.
♻️ Duplicate comments (3)
shortcuts/mail/draft/patch.go (3)
883-891:⚠️ Potential issue | 🟠 MajorHandle Windows drive-letter paths before the scheme check.
uriSchemeRegexpmatches theC:prefix, soC:\foo.pngandC:/foo.pngare treated as URI schemes and never go through local-path resolution. That breaks the new workflow on Windows whenever callers use an absolute path.Suggested fix
func isLocalFileSrc(src string) bool { trimmed := strings.TrimSpace(src) if trimmed == "" { return false } + if len(trimmed) >= 3 && + ((trimmed[0] >= 'A' && trimmed[0] <= 'Z') || (trimmed[0] >= 'a' && trimmed[0] <= 'z')) && + trimmed[1] == ':' && + (trimmed[2] == '\\' || trimmed[2] == '/') { + return true + } if strings.HasPrefix(trimmed, "//") { return false } return !uriSchemeRegexp.MatchString(trimmed) }🤖 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 883 - 891, The function isLocalFileSrc incorrectly treats Windows absolute paths like "C:\foo" as URIs because uriSchemeRegexp matches the "C:" prefix; update isLocalFileSrc to detect Windows drive-letter absolute paths before the scheme check (e.g., test trimmed against a drive-letter pattern like /^[A-Za-z]:[\\/]/) and return true for those cases so they are treated as local files, then proceed with the existing uriSchemeRegexp.MatchString(trimmed) check for other schemes.
964-977:⚠️ Potential issue | 🟠 MajorUse the same inline predicate during orphan cleanup.
This sweep only drops children that are both
Content-Disposition:inlineandContent-ID-bearing. Elsewhere in this file, any part with a CID is already treated as inline, so CID-only related images from existing drafts survive cleanup after their<img>disappears. Inline parts without a CID also stay around here even though they can’t be referenced from HTML.Suggested fix
kept := make([]*Part, 0, len(root.Children)) for _, child := range root.Children { if child == nil { continue } - if strings.EqualFold(child.ContentDisposition, "inline") && child.ContentID != "" { - if !referencedCIDs[strings.ToLower(child.ContentID)] { + cid := strings.ToLower(strings.Trim(strings.TrimSpace(child.ContentID), "<>")) + if cid != "" { + if !referencedCIDs[cid] { + root.Dirty = true + continue + } + } else if strings.EqualFold(child.ContentDisposition, "inline") { root.Dirty = true continue - } } kept = append(kept, child) }🤖 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 964 - 977, The orphan cleanup loop over root.Children currently only treats parts as inline when ContentDisposition == "inline" and ContentID != "", which mismatches the rest of the file where any part with a CID is treated as inline; update the loop in shortcuts/mail/draft/patch.go to use the same inline predicate (i.e., consider a part inline if ContentID != "" OR ContentDisposition equals "inline") and then: if the part is inline but either has no ContentID or its lowercased ContentID is not in referencedCIDs, mark root.Dirty and skip it; otherwise keep the part (update references to ContentID, ContentDisposition, referencedCIDs, and root.Children accordingly).
984-988:⚠️ Potential issue | 🟠 MajorFind the HTML part from the current tree, not
PrimaryHTMLPartID.
postProcessInlineImagesnow runs beforerefreshSnapshot, sosnapshot.PrimaryHTMLPartIDcan still be empty or stale when this apply pass creates or rewraps the HTML subtree. In that state,findPart(snapshot.Body, snapshot.PrimaryHTMLPartID)can pick the wrong unindexed node or none at all, which skips localsrcrewriting and can orphan-clean the wrong related parts.Suggested fix
func postProcessInlineImages(snapshot *DraftSnapshot) error { - htmlPart := findPart(snapshot.Body, snapshot.PrimaryHTMLPartID) + htmlPart := findPrimaryBodyPart(snapshot.Body, "text/html") if htmlPart == nil { return nil }🤖 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 984 - 988, postProcessInlineImages should not rely on snapshot.PrimaryHTMLPartID since it can be empty/stale before refreshSnapshot; update postProcessInlineImages to locate the HTML part by searching the current tree (snapshot.Body) for a part whose ContentType/IsHTML indicates "text/html" (e.g., a helper like findPartByContentType or a traversal that checks part.MIME/ContentType) instead of calling findPart(snapshot.Body, snapshot.PrimaryHTMLPartID); use that discovered part for inline image rewriting and related-part cleanup so the function operates on the actual HTML node present in the snapshot tree.
🧹 Nitpick comments (1)
shortcuts/mail/draft/patch.go (1)
763-781: Share the stricter CID validator withreplaceInline.The new character check only runs through
newInlinePart;replaceInlinestill trims<>and rejects only CR/LF before assigningpart.ContentID. That leavesreplace_inlineable to emit invalidContent-IDvalues whileadd_inlineand local-path resolution reject the same input.🤖 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 763 - 781, The CID validation logic in newInlinePart is stricter than replaceInline, allowing replaceInline to set invalid Content-ID values; extract the CID trimming and validation into a shared helper (e.g., validateInlineCID or normalizeInlineCID) and call it from both newInlinePart and replaceInline before assigning part.ContentID; the helper should perform trimming of "<>", check for empty, call validate.RejectCRLF, and enforce the strings.ContainsAny(... " \t<>()") rule so both newInlinePart and replaceInline use identical validation and reject invalid CIDs consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 883-891: The function isLocalFileSrc incorrectly treats Windows
absolute paths like "C:\foo" as URIs because uriSchemeRegexp matches the "C:"
prefix; update isLocalFileSrc to detect Windows drive-letter absolute paths
before the scheme check (e.g., test trimmed against a drive-letter pattern like
/^[A-Za-z]:[\\/]/) and return true for those cases so they are treated as local
files, then proceed with the existing uriSchemeRegexp.MatchString(trimmed) check
for other schemes.
- Around line 964-977: The orphan cleanup loop over root.Children currently only
treats parts as inline when ContentDisposition == "inline" and ContentID != "",
which mismatches the rest of the file where any part with a CID is treated as
inline; update the loop in shortcuts/mail/draft/patch.go to use the same inline
predicate (i.e., consider a part inline if ContentID != "" OR ContentDisposition
equals "inline") and then: if the part is inline but either has no ContentID or
its lowercased ContentID is not in referencedCIDs, mark root.Dirty and skip it;
otherwise keep the part (update references to ContentID, ContentDisposition,
referencedCIDs, and root.Children accordingly).
- Around line 984-988: postProcessInlineImages should not rely on
snapshot.PrimaryHTMLPartID since it can be empty/stale before refreshSnapshot;
update postProcessInlineImages to locate the HTML part by searching the current
tree (snapshot.Body) for a part whose ContentType/IsHTML indicates "text/html"
(e.g., a helper like findPartByContentType or a traversal that checks
part.MIME/ContentType) instead of calling findPart(snapshot.Body,
snapshot.PrimaryHTMLPartID); use that discovered part for inline image rewriting
and related-part cleanup so the function operates on the actual HTML node
present in the snapshot tree.
---
Nitpick comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 763-781: The CID validation logic in newInlinePart is stricter
than replaceInline, allowing replaceInline to set invalid Content-ID values;
extract the CID trimming and validation into a shared helper (e.g.,
validateInlineCID or normalizeInlineCID) and call it from both newInlinePart and
replaceInline before assigning part.ContentID; the helper should perform
trimming of "<>", check for empty, call validate.RejectCRLF, and enforce the
strings.ContainsAny(... " \t<>()") rule so both newInlinePart and replaceInline
use identical validation and reject invalid CIDs consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6db1e3a3-c5ac-4603-92ee-7dbb0e601765
📒 Files selected for processing (2)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_inline_resolve_test.go
…ct 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.
chanthuang
approved these changes
Apr 1, 2026
infeng
added a commit
to infeng/cli
that referenced
this pull request
Apr 1, 2026
…larksuite#81) (larksuite#139)" This reverts commit 70c72a2.
3 tasks
infeng
added a commit
that referenced
this pull request
Apr 1, 2026
* Revert "fix(mail): clarify that file path flags only accept relative paths (#141)" This reverts commit 1ffe870. * Revert "feat(mail): auto-resolve local image paths in draft body HTML (#81) (#139)" This reverts commit 70c72a2. * Reapply "fix(mail): clarify that file path flags only accept relative paths (#141)" This reverts commit d465e08.
infeng
added a commit
to infeng/cli
that referenced
this pull request
Apr 2, 2026
…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.
7 tasks
4 tasks
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
…139) * feat(mail): auto-resolve local image paths in draft body HTML (#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 #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.
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
* Revert "fix(mail): clarify that file path flags only accept relative paths (#141)" This reverts commit eac6fa9. * Revert "feat(mail): auto-resolve local image paths in draft body HTML (#81) (#139)" This reverts commit 22cc882. * Reapply "fix(mail): clarify that file path flags only accept relative paths (#141)" This reverts commit d465e08.
This was referenced Apr 7, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Allow
<img src="./local/path.png" />inset_body/set_reply_bodyHTML — local filepaths
are automatically resolved into inline MIME parts with generated CIDs. Inline image
lifecycle
(add/delete/replace) is now fully managed through body HTML, eliminating the need to
manually
pair
add_inlinewithset_body.Changes
postProcessInlineImagesto unify local path resolution, CID validation, andorphan
cleanup into a single post-processing step, replacing the separate
validateInlineCIDAfterApplyand
validateOrphanedInlineCIDAfterApplyfunctionsloadAndAttachInlineshared helper to deduplicate file-read/validate/attachlogic
between
addInlineandresolveLocalImgSrcpart
uriSchemeRegexp) instead of blacklist,correctly
handling
blob:,ftp:,file:,//protocol-relative URLs<img>tags are removed from body HTML--print-patch-templatenotes andset_body/set_reply_bodyshapedescriptions to
guide AI agents toward the simpler local-path approach
lark-mail-draft-edit.mdwith new inline image workflowTest Plan
make unit-testpassedpatch_inline_resolve_test.gocovering:logo,logo-2)cid:,http://,data:,blob:,ftp:,//)set_reply_bodywith local path (quote block preserved)add_inline+ local path usageadd_inlinesame file as local pathremove_inlinebut body still references CIDremove_inline+ replace with local pathisLocalFileSrc,cidFromFileName,uniqueCID--print-patch-templateconfirms AI agent usessingle-step workflow
Related Issues
Closes #81
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation