feat(mail): auto-resolve local image paths in all draft entry points#205
feat(mail): auto-resolve local image paths in all draft entry points#205infeng wants to merge 6 commits intolarksuite:mainfrom
Conversation
…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.
|
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:
📝 WalkthroughWalkthroughAdds HTML-driven local image resolution: detects relative Changes
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@354c14d846019b68e56c8cc65e8aa784dd7bf692🧩 Skill updatenpx skills add infeng/cli#feat/auto-resolve-local-img-all-drafts -y -g |
There was a problem hiding this comment.
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 | 🟠 MajorFilter source inline parts before downloading them.
This helper eagerly downloads and embeds every
imagesentry. 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 reachcheckAttachmentSizeLimitbecause callers only pass local file paths there. That means an orphaned source inline can still block the command on a missingdownload_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_urland 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 checkingos.WriteFileerrors 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
📒 Files selected for processing (19)
shortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_inline_resolve_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_reply_forward_inline_test.goshortcuts/mail/mail_send.goskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/lark-mail/references/lark-mail-send.md
Greptile SummaryThis PR extends the auto-resolve local 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/5Safe 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
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
Reviews (5): Last reviewed commit: "test(mail): add CRLF case to newInlinePa..." | Re-trigger Greptile |
|
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
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.
There was a problem hiding this comment.
🧹 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
--inlineparts, and auto-resolved local images). While the UUID-basedgenerateCID()prevents collisions among auto-generated CIDs, a user-provided CID viaadd_inlinecould theoretically duplicate an existing Content-ID.The current
ValidateCIDReferencesandFindOrphanedCIDscheck reference completeness but not global uniqueness. Consider adding a uniqueness check when collectingcidPartsat 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
📒 Files selected for processing (8)
shortcuts/mail/draft/patch.goshortcuts/mail/mail_reply_forward_inline_test.goskills/lark-mail/references/lark-mail-draft-create.mdskills/lark-mail/references/lark-mail-draft-edit.mdskills/lark-mail/references/lark-mail-forward.mdskills/lark-mail/references/lark-mail-reply-all.mdskills/lark-mail/references/lark-mail-reply.mdskills/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
|
|
Good catch — you're right that In practice the risk is very low:
That said, it's a valid hardening point. Adding a uniqueness check across |
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.
There was a problem hiding this comment.
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.MkdirAllcalls ignore setup failures, so a temp-dir or write problem will show up later as a confusing parse or assertion failure. A tiny helper inhelpers_test.gowould 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
📒 Files selected for processing (2)
shortcuts/mail/draft/patch.goshortcuts/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.
There was a problem hiding this comment.
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
ResolveLocalImagePathstests only pin success and no-op paths. Please add one/absolute/path/logo.pngcase 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
📒 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.
There was a problem hiding this comment.
🧹 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_bodystopped preservingcid:a, or failed to rewrite./b.png/./logo.pngin the final body, they would still pass. Please also assert the resulting HTML contains the expectedcid: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
📒 Files selected for processing (1)
shortcuts/mail/draft/patch_inline_resolve_test.go
haidaodashushu
left a comment
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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<>()") { |
There was a problem hiding this comment.
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.
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
ResolveLocalImagePathsas an exported function fromdraft/patch.go, decoupling it from the snapshot-based patch flow so EML-builder paths can reuse itResolveLocalImagePathsin the HTML branch ofmail_draft_create,mail_send,mail_reply,mail_reply_all, andmail_forwardvalidateInlineCIDsfor bidirectional CID consistency checks (everycid:reference in HTML has a matching MIME part, every user-provided inline part is referenced in HTML)ValidateCIDReferencesandFindOrphanedCIDsas exported helpers from the existingpostProcessInlineImageslogiccheckAttachmentSizeLimitto enforce size limits across all inline sourcesaddInlineImagesToBuilderto returnsrcCIDsfor proper CID validation in reply/forward flowsmail_draft_create_test.go,mail_reply_forward_inline_test.go, expandedpatch_inline_resolve_test.goandhelpers_test.goTest Plan
make unit-testpassedlark mail +draft-create --body '<img src="./logo.png">' ...resolves image inlinelark mail +send --body '<img src="./pic.jpg">' ...resolves image inlinelark mail +reply --body '<img src="./sig.png">' ...resolves image inlinelark mail +forward --body '<img src="./chart.png">' ...resolves image inline--plain-textmode correctly ignores local image resolutionRelated Issues
Closes #81
Supersedes #139 (reverted in #199)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests