Skip to content

revert: undo auto-resolve local image paths in draft body HTML#199

Merged
infeng merged 3 commits intolarksuite:mainfrom
infeng:revert-relative-path-clarification
Apr 1, 2026
Merged

revert: undo auto-resolve local image paths in draft body HTML#199
infeng merged 3 commits intolarksuite:mainfrom
infeng:revert-relative-path-clarification

Conversation

@infeng
Copy link
Copy Markdown
Collaborator

@infeng infeng commented Apr 1, 2026

Summary

  • Reverts 70c72a2feat(mail): auto-resolve local image paths in draft body HTML (#81) (#139)

The auto-resolve feature (<img src="./local.png" /> → inline CID) was only wired into the +draft-edit patch path. All other EML-build paths (+draft-create, +send, +reply, +reply-all, +forward) did not support it, causing LLMs to misuse the pattern and produce broken inline images. Reverting until the feature can be implemented consistently across all entry points.

Note: 1ffe870 (fix(mail): clarify that file path flags only accept relative paths) is intentionally preserved.

Test plan

  • go build ./shortcuts/mail/... passes
  • go test ./shortcuts/mail/... passes
  • Verify +draft-edit inline images still work via explicit add_inline op with CID

Summary by CodeRabbit

  • Bug Fixes

    • Inline image references in email drafts now require explicit coordination: users must add <img src="cid:..."> tags via body edits and corresponding inline attachments via the add operation. Orphaned or unreferenced inline images are now rejected.
  • Documentation

    • Updated guidance on inline image handling to clarify the explicit attachment and referencing workflow required for email drafts.

@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 1, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 1, 2026

📝 Walkthrough

Walkthrough

Changed inline image handling in email draft patches from automatic local-path resolution and orphan cleanup to explicit post-apply validation. Removed auto-resolve logic for HTML <img src> references, refactored inline attachment helpers to use explicit addition, split validation into separate CID and orphan checks, and updated documentation to reflect the new explicit workflow.

Changes

Cohort / File(s) Summary
Core Inline Image Validation Refactor
shortcuts/mail/draft/patch.go
Replaced single postProcessInlineImages call with two-phase validation: validateInlineCIDAfterApply (checks HTML cid: references resolve) and validateOrphanedInlineCIDAfterApply (rejects unreferenced inline parts). Removed auto-resolve logic for local <img src> paths, regex-based CID generation, and orphan cleanup. Refactored loadAndAttachInline to simpler addInline that directly creates multipart/related container.
Test Coverage Removal
shortcuts/mail/draft/patch_inline_resolve_test.go
Entirely deleted 773-line test file covering auto-resolution of local image paths, orphan handling, CID generation/validation, and helper functions. Removed assertions for automatic <img src> → inline MIME part conversion and lifecycle management.
Test Behavior Updates
shortcuts/mail/draft/patch_test.go
Renamed TestApplySetBodyOrphanedInlineCIDIsAutoRemoved to TestApplySetBodyOrphanedInlineCIDIsRejected. Changed test expectations: Apply now returns an error containing "orphaned cids" instead of silently removing orphaned inline parts from the MIME tree.
Documentation Updates
shortcuts/mail/mail_draft_edit.go, skills/lark-mail/references/lark-mail-draft-edit.md
Updated JSON template and user documentation to clarify that add_inline only adds MIME binary parts without inserting <img> tags. Removed guidance on automatic local-path resolution. Added requirement that users must explicitly use set_body/set_reply_body to insert <img src="cid:..."> references alongside add_inline operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • chanthuang

Poem

🐰 No more magic, just be clear,
Inline images need to appear—
Add the part, then write the tag,
Let validation catch the lag,
Explicit beats automatic cheer! 📧

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% 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 accurately describes the main change: reverting the auto-resolve feature that converted local image paths in draft HTML to inline CIDs.
Description check ✅ Passed The pull request description is well-structured and covers all required sections from the template with substantive content.

✏️ 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 chanthuang April 1, 2026 14:26
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5051e259ba371df2e80a9119ffda186b6b123e63

🧩 Skill update

npx skills add infeng/cli#revert-relative-path-clarification -y -g

@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed size/L Large or sensitive change across domains or core paths labels Apr 1, 2026
@infeng infeng changed the title revert: undo auto-resolve local image paths and relative-path clarifications revert: undo auto-resolve local image paths in draft body HTML Apr 1, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 1, 2026

Greptile Summary

This PR reverts two commits that introduced auto-resolution of local <img src="./path"> references into inline CID MIME parts (70c72a2) and relative-path clarifications in flags (1ffe870). The motivation is correct: the auto-resolve feature was only wired into +draft-edit and was causing LLMs to produce broken inline images on all other entry points (+send, +reply, etc.).

The revert is broadly clean and the rationale is sound. Key changes:

  • patch.go: Removes resolveLocalImgSrc, removeOrphanedInlineParts, generateCID, isLocalFileSrc, loadAndAttachInline, and postProcessInlineImages. Replaces the unified post-process step with two separate validators: validateInlineCIDAfterApply (broken cid: references → error) and validateOrphanedInlineCIDAfterApply (unreferenced inline parts → error instead of silent removal).
  • patch_inline_resolve_test.go: Entire test file deleted (773 lines).
  • patch_test.go: One test renamed and updated to expect an error on orphaned CIDs.
  • mail_draft_edit.go and lark-mail-draft-edit.md: Documentation updated to require explicit two-step workflow for inline images.

Issues found:

  • validateOrphanedInlineCIDAfterApply iterates over flattenParts and flags every MIME part carrying a ContentID that is not referenced by an <img src="cid:..."> in the HTML body. However it does not filter by ContentDisposition: inline, so an HTML body part that carries its own Content-ID (valid per RFC 2387, emitted by Apple Mail and Outlook) is incorrectly flagged as "orphaned", breaking Apply on real-world drafts. The deleted TestOrphanCleanupPreservesHTMLBodyWithContentID test covered this exact scenario.
  • The strings.ContainsAny(cid, \" \ <>()\") guard was removed from newInlinePart along with the revert; this check is still relevant for manually-supplied add_inline CIDs and its absence permits malformed Content-ID headers.

Confidence Score: 4/5

Safe to merge after fixing the orphan validator's ContentDisposition filter; the P2 CID character check is a hardening improvement that can be addressed in a follow-up.

One clear P1 regression: validateOrphanedInlineCIDAfterApply flags HTML body parts that carry a Content-ID (valid RFC 2387 practice) as orphaned, returning an error on Apply for real-world drafts opened from emails generated by Apple Mail or Outlook. The fix is a one-line guard. The remaining finding (missing CID character validation) is a P2 input-hardening concern that does not block correctness for the common path.

shortcuts/mail/draft/patch.go — validateOrphanedInlineCIDAfterApply loop needs a ContentDisposition: inline filter

Important Files Changed

Filename Overview
shortcuts/mail/draft/patch.go Core logic reverted: removes auto-resolve and replaces with explicit CID validation; contains a P1 regression where validateOrphanedInlineCIDAfterApply checks ALL ContentID-bearing parts instead of only inline-disposition parts
shortcuts/mail/draft/patch_inline_resolve_test.go Entirely deleted (773 lines); removes TestOrphanCleanupPreservesHTMLBodyWithContentID which covered the HTML-body-with-ContentID edge case now regressed in the replacement validator
shortcuts/mail/draft/patch_test.go One test renamed/updated to expect an error on orphaned CIDs; consistent with the new reject-orphans policy
shortcuts/mail/mail_draft_edit.go Op shape strings and notes updated to remove auto-resolve hints and clarify explicit add_inline + set_body workflow
skills/lark-mail/references/lark-mail-draft-edit.md Documentation updated accurately to reflect the explicit two-step inline image workflow

Reviews (1): Last reviewed commit: "Reapply "fix(mail): clarify that file pa..." | Re-trigger Greptile

Comment thread shortcuts/mail/draft/patch.go
Comment thread shortcuts/mail/draft/patch.go
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: 4

🤖 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 892-915: The validator validateOrphanedInlineCIDAfterApply
currently treats any part with a ContentID as an orphan candidate and returns
early if no HTML part exists; change it to only consider parts where
part.ContentDisposition == "inline" and part.ContentID != "" when building the
orphan list (use flattenParts to iterate and the same strings.ToLower checks),
and if htmlPart is nil then reject (return an error) only when at least one such
inline part exists after apply; keep the existing error message/format but
ensure you reference the same symbols (validateOrphanedInlineCIDAfterApply,
findPart, extractCIDRefs, flattenParts) when making the change.

In `@shortcuts/mail/mail_draft_edit.go`:
- Line 366: Update the comment that explains add_inline behavior to explicitly
state that draftpkg.Apply will reject orphaned inline parts at edit time (during
the +draft-edit command) rather than allowing the edit to succeed and only
failing at send; mention the immediate validation error returned by
draftpkg.Apply and keep the advice that to display the image inline you must
also call set_body/set_reply_body with an <img src="cid:...">. Reference
add_inline, set_body/set_reply_body, draftpkg.Apply and +draft-edit so readers
know where the validation occurs and what commands to use to avoid orphaned CID
attachments.

In `@skills/lark-mail/references/lark-mail-draft-edit.md`:
- Around line 315-327: The patch example uses two different filenames causing a
mismatch: the JSON is written to "./patch.json" but the command references
"/tmp/patch.json"; update either the heredoc target or the lark-cli invocation
so both use the same path (e.g., change the heredoc to write to
"/tmp/patch.json" or change the command to use "./patch.json")—look for the
heredoc that creates "patch.json" and the lark-cli mail +draft-edit --patch-file
argument and make them identical.
- Around line 253-256: The examples use an absolute path (/tmp/patch.json) which
will be rejected by the --patch-file validation; update the examples to use a
relative path (e.g., ./patch.json) so loadPatchFile passes
validate.SafeInputPath. Locate the usage in the docs around the mail draft edit
example and change the CLI invocation to use a relative path for --patch-file;
mention or show ./patch.json instead of /tmp/patch.json so the runtime call to
shortcuts/mail/mail_draft_edit.go (safePath := validate.SafeInputPath(...))
succeeds.
🪄 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: 270ce1e0-43e0-4370-8ddc-776a84688a65

📥 Commits

Reviewing files that changed from the base of the PR and between a703202 and b21a90f.

📒 Files selected for processing (15)
  • shortcuts/mail/draft/patch.go
  • shortcuts/mail/draft/patch_inline_resolve_test.go
  • shortcuts/mail/draft/patch_test.go
  • shortcuts/mail/mail_draft_create.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_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
💤 Files with no reviewable changes (1)
  • shortcuts/mail/draft/patch_inline_resolve_test.go

Comment thread shortcuts/mail/draft/patch.go
Comment thread shortcuts/mail/mail_draft_edit.go
Comment thread skills/lark-mail/references/lark-mail-draft-edit.md Outdated
Comment thread skills/lark-mail/references/lark-mail-draft-edit.md Outdated
@infeng infeng merged commit eda2b9c into larksuite:main Apr 1, 2026
13 checks passed
infeng added a commit that referenced this pull request Apr 2, 2026
…ert (#199)

The revert of PR #81 (eda2b9c) also removed two independent bugfixes:

1. CID character validation in newInlinePart — reject spaces, tabs,
   angle brackets, and parentheses to prevent malformed MIME output.
2. Stale PartID lookup in validateInlineCIDAfterApply and
   validateOrphanedInlineCIDAfterApply — use findPrimaryBodyPart by
   media type instead of findPart by PrimaryHTMLPartID, which can go
   stale when ops restructure the MIME tree.
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.
infeng added a commit that referenced this pull request Apr 7, 2026
…ert (#230)

* fix(mail): restore CID validation and stale PartID lookup lost in revert (#199)

The revert of PR #81 (eda2b9c) also removed two independent bugfixes:

1. CID character validation in newInlinePart — reject spaces, tabs,
   angle brackets, and parentheses to prevent malformed MIME output.
2. Stale PartID lookup in validateInlineCIDAfterApply and
   validateOrphanedInlineCIDAfterApply — use findPrimaryBodyPart by
   media type instead of findPart by PrimaryHTMLPartID, which can go
   stale when ops restructure the MIME tree.

* test(mail): add tests for CID character validation and stale PartID lookup

- TestAddInlineRejectsInvalidCharactersInCID: verify spaces, tabs,
  embedded angle brackets, and parentheses in CID are rejected.
- TestValidateInlineCIDAfterSetBody: verify inline CID validation
  works correctly after set_body restructures the MIME tree (covers
  the findPrimaryBodyPart fix for stale PartID).

* fix(mail): add CID character validation to replaceInline and strengthen test assertions

Address CR feedback:
1. Add the same CID character validation (spaces, tabs, angle brackets,
   parentheses) to replaceInline, matching the check in newInlinePart.
   Previously replace_inline could bypass the restriction.
2. Strengthen orphaned CID test assertion to check for specific
   "orphaned cids" error message, not just non-nil error.
3. Add TestReplaceInlineRejectsInvalidCharactersInCID to cover the
   new validation in replace_inline.
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/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants