feat(mail): auto-attach default signature on send/reply/forward#1415
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA shared ChangesMail signature flag and composition refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/mail
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/mail/mail_draft_create_test.go (2)
274-296: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd coverage for the new plain-text signature path.
This test still only proves that CID resolution is skipped. The changed branch now also runs
injectPlainTextSignature(...), so a regression in plain-text signature rendering would go unnoticed. Add a variant with a non-nilsigResultand assert the emitted text body; ano-signaturevariant would lock down the flag behavior too.As per coding guidelines, every behavior change needs a test alongside the change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/mail/mail_draft_create_test.go` around lines 274 - 296, The test TestBuildRawEMLForDraftCreate_PlainTextSkipsResolve only checks that CID resolution is skipped but doesn't exercise the new plain-text signature path; update the test (or add a new variant) to call buildRawEMLForDraftCreate with a non-nil sigResult so injectPlainTextSignature(...) runs, then decode the produced raw EML and assert the emitted plain-text body contains the expected signature text; also add a complementary no-signature variant (sigResult nil) to lock down flag behavior and ensure both branches of injectPlainTextSignature are covered.Source: Coding guidelines
127-133: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert typed error contracts here, not message fragments.
These negative tests will still pass if the error category/subtype/param is downgraded or the cause chain is lost, because they only match substrings. Please switch them to typed assertions via
errs.ProblemOf(err), and useerrors.Aswith*errs.ValidationErrorwhen you need to verifyParam.As per coding guidelines, error-path tests must assert typed metadata and cause preservation, and based on learnings
errs.ProblemOfdoes not exposeParamdirectly.Also applies to: 148-154, 169-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/mail/mail_draft_create_test.go` around lines 127 - 133, Replace the substring-based assertions with typed error checks: call errs.ProblemOf(err) and assert the returned problem is non-nil, then use errors.As(err, &ve) to cast into *errs.ValidationError (or the appropriate error type) to assert the specific Param or subtype; apply this to the failing call to buildRawEMLForDraftCreate (and the similar cases around the other tests indicated) so the test verifies the typed error and Param rather than just matching "25 MB" or "large attachment" in the error message.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
skills/lark-mail/references/lark-mail-draft-create.md (1)
52-52: ⚡ Quick winConsider removing implementation detail
PlainTextFromHTMLfrom user-facing documentation.The parameter description exposes the internal function name
PlainTextFromHTML. User documentation typically describes behavior without referencing code internals. Consider revising to:"纯文本模式下也会自动追加纯文本签名（HTML 签名自动转换为纯文本，内联图片丢弃）"
This applies to all five command reference files at the same line describing
--plain-text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-mail/references/lark-mail-draft-create.md` at line 52, Update the user-facing docs to remove the internal function name PlainTextFromHTML from the `--plain-text` parameter description: replace the fragment "纯文本模式下也会自动追加纯文本签名（HTML 签名经 `PlainTextFromHTML` 转换，内联图片丢弃）" with "纯文本模式下也会自动追加纯文本签名（HTML 签名自动转换为纯文本，内联图片丢弃）" in the `--plain-text` descriptions across all five command reference files so the behavior is described without exposing implementation details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/signature_compose_test.go`:
- Around line 10-19: Update TestValidateNoSignatureConflictTypedError to assert
structured error metadata instead of only message text: call
validateNoSignatureConflict(true, "sig_123") then use errs.ProblemOf on the
returned error to assert the validation category/subtype produced by
errs.NewValidationError, and use errors.As to cast the error to the concrete
validation error type (e.g., *errs.ValidationError) and assert its Param field
equals the parameter name used by validateNoSignatureConflict (the signature id
param). Keep existing non-nil assertion but replace the message-only checks with
these structured assertions referencing validateNoSignatureConflict,
errs.NewValidationError, errs.ProblemOf and errors.As.
In `@shortcuts/mail/signature_compose.go`:
- Around line 36-45: In validateNoSignatureConflict replace the legacy
output.ErrValidation return with errs.NewValidationError and supply structured
metadata: call errs.NewValidationError with a category like "validation", a
subtype such as "mutually-exclusive", the param identifying the conflicting
flags (e.g. "--no-signature/--signature-id"), and the human message
"--no-signature and --signature-id are mutually exclusive"; also add the errs
import if missing and remove the output package usage for this error.
---
Outside diff comments:
In `@shortcuts/mail/mail_draft_create_test.go`:
- Around line 274-296: The test
TestBuildRawEMLForDraftCreate_PlainTextSkipsResolve only checks that CID
resolution is skipped but doesn't exercise the new plain-text signature path;
update the test (or add a new variant) to call buildRawEMLForDraftCreate with a
non-nil sigResult so injectPlainTextSignature(...) runs, then decode the
produced raw EML and assert the emitted plain-text body contains the expected
signature text; also add a complementary no-signature variant (sigResult nil) to
lock down flag behavior and ensure both branches of injectPlainTextSignature are
covered.
- Around line 127-133: Replace the substring-based assertions with typed error
checks: call errs.ProblemOf(err) and assert the returned problem is non-nil,
then use errors.As(err, &ve) to cast into *errs.ValidationError (or the
appropriate error type) to assert the specific Param or subtype; apply this to
the failing call to buildRawEMLForDraftCreate (and the similar cases around the
other tests indicated) so the test verifies the typed error and Param rather
than just matching "25 MB" or "large attachment" in the error message.
---
Nitpick comments:
In `@skills/lark-mail/references/lark-mail-draft-create.md`:
- Line 52: Update the user-facing docs to remove the internal function name
PlainTextFromHTML from the `--plain-text` parameter description: replace the
fragment "纯文本模式下也会自动追加纯文本签名（HTML 签名经 `PlainTextFromHTML` 转换，内联图片丢弃）" with
"纯文本模式下也会自动追加纯文本签名（HTML 签名自动转换为纯文本，内联图片丢弃）" in the `--plain-text` descriptions
across all five command reference files so the behavior is described without
exposing implementation details.
🪄 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: 2ee32e1b-c7ae-4d7a-9207-a1813039aa3e
📒 Files selected for processing (15)
shortcuts/mail/draft/htmltext.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/signature_compose_test.goskills/lark-mail/references/lark-mail-draft-create.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
xzcong0820
commented
Jun 12, 2026
xzcong0820
commented
Jun 12, 2026
xzcong0820
commented
Jun 12, 2026
xzcong0820
commented
Jun 12, 2026
Collaborator
Author
|
🤖 AI Review | CR 汇总 | 有风险（2 个 P2，2 个 P3） 增量审查：基于已有 resolved/withdrawn 评论，本次新增 4 条评论。主要风险是 plain-text 签名仍下载图片导致发信失败、自动默认签名 stale ID 阻断发信；另有 gofmt 门禁和核心行为测试覆盖缺口。 |
- Add exported PlainTextFromHTML wrapper in draft/htmltext.go - Add DefaultSendID/DefaultReplyID in signature/provider.go - Add noSignatureFlag, autoResolveSignatureID, validateNoSignatureConflict, injectPlainTextSignature in signature_compose.go; remove validateSignatureWithPlainText - mail_send, mail_draft_create: add --no-signature flag, auto-resolve default signature when no --signature-id given, inject plain-text sig in plain-text branch - mail_reply, mail_reply_all, mail_forward: same flag/validate changes + timing fix (resolveSignature moved to after senderEmail is finalized) - Update 5 reference docs: add --no-signature row, update --plain-text and --signature-id descriptions sprint: S1
- Add senderEmailHint param to buildRawEMLForDraftCreate to avoid duplicate profile API calls when Execute already resolved the sender - Update signature_compose_test: replace deleted validateSignatureWithPlainText test with validateNoSignatureConflict tests - Update mail_draft_create_test: add new senderEmailHint arg to all buildRawEMLForDraftCreate call sites sprint: S1
- Strip errs-typed error assertions from signature_compose_test.go (errs package not present on origin/main base; plain fmt.Errorf used) - Keep only validateNoSignatureConflict tests which don't need errs - Fix 4-return buildRawEMLForDraftCreate test calls to use 2-return form sprint: S1
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@57cd96a3d4519ea51d02270ae48f233a6216d79d🧩 Skill updatenpx skills add xzcong0820/larksuite-cli#feat/6dd7472 -y -g |
…efault signature - Add `userExplicit` and `includeImages` params to `resolveSignature`. Plain-text paths now pass `includeImages=false`, avoiding unnecessary HTTP downloads for images that are discarded in plain-text mode (also prevents send failures from expired pre-signed URLs / CDN errors). - When `userExplicit=false` (auto-resolved default) and `signature.Get` returns a ValidationError (stale ID pointing to deleted signature), degrade gracefully: emit a stderr warning and continue without signature instead of aborting the send/reply/forward operation. - Fix gofmt: remove trailing blank line in signature_compose.go. Change-Type: ci-fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1415 +/- ##
==========================================
+ Coverage 72.83% 73.38% +0.55%
==========================================
Files 732 750 +18
Lines 69140 69353 +213
==========================================
+ Hits 50356 50895 +539
+ Misses 15003 14709 -294
+ Partials 3781 3749 -32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…verage tests - Replace forbidden output.ErrValidation with mailValidationParamError (satisfies forbidigo lint rule [errs-typed-only]) - Update TestValidateNoSignatureConflictTypedError to assert *errs.ValidationError instead of *output.ExitError - Add unit tests for contentTypeFromFilename, signatureCIDs, injectSignatureIntoBody, and addSignatureImagesToBuilder (pure-function coverage) Change-Type: ci-fix
…Signature - TestAutoResolveSignatureID_APIFailureReturnsEmpty: API error degrades to "" - TestAutoResolveSignatureID_NoDefaultConfigured: no usage match returns "" - TestAutoResolveSignatureID_ReturnsSendID/ReturnsReplyID: correct default ID returned - TestResolveSignature_EmptyIDReturnsNil: empty signatureID short-circuits to nil - TestResolveSignature_StaleIDAutoDegradesGracefully: userExplicit=false + not-found yields nil/nil (no error) matching the graceful degradation path - TestResolveSignature_StaleIDUserExplicitFails: userExplicit=true propagates the ValidationError so caller can surface "signature not found" to the user Change-Type: ci-fix
…PlainTextFromHTML - shortcuts/mail/signature/provider_test.go: 10 tests covering DefaultSendID and DefaultReplyID (exact match, case-insensitive, no-match, ID=0, nil usages) - shortcuts/mail/draft/htmltext_test.go: TestPlainTextFromHTMLExported covering the exported PlainTextFromHTML wrapper Patch coverage was at 51.32% (target 60%); these tests cover 12 previously uncovered lines in signature/provider.go and 2 in draft/htmltext.go. Change-Type: ci-fix
xzcong0820
commented
Jun 12, 2026
xzcong0820
left a comment
xzcong0820
left a comment
Collaborator
Author
There was a problem hiding this comment.
🤖 AI Review | CR 汇总 | 有风险（1 个 P1，2 个 P3）
增量审查：基于已有评论，本次新增 3 条。重点问题是 reply/reply-all/forward 使用纯文本模板时仍可能先下载签名图片，导致本应丢弃的图片网络错误阻断发信。另有 CI 门禁被收窄和过期注释清理建议。
…omment - mail_reply.go: move resolveSignature call to after applyTemplate so the final plainText value (potentially set by template IsPlainTextMode) is used for the downloadImages decision. Prevents unnecessary image downloads when a template forces plain-text mode, avoiding CDN 403/5xx / timeout. - signature_compose.go: remove stale comment referencing output.ErrValidation; the function already uses mailValidationParamError consistently. Change-Type: ci-fix
恢复在 rebase 冲突解决中误删的 ./extension/... 单测范围 和 errs/ lint guard（go run -C lint . ..）步骤。 本 PR 目标为 mail 签名功能，不应缩小 CI 覆盖范围。
xzcong0820
commented
Jun 15, 2026
xzcong0820
commented
Jun 15, 2026
…ard and +reply-all In +forward and +reply-all, resolveSignature was called before applyTemplate, meaning it used the pre-template plainText value. A template can override plainText via IsPlainTextMode, so HTML signature images could be downloaded unnecessarily for ultimately plain-text mails, causing CDN 403/5xx/timeout failures. Move resolveSignature to after the template merge block, matching the existing pattern in +reply. Only signatureID resolution remains before the template merge (it depends on senderEmail, not on the final plainText). Change-Type: ci-fix
github-actions
Bot
added
size/L
xzcong0820
commented
Jun 15, 2026
infeng
approved these changes
Jun 15, 2026
infeng
left a comment
infeng
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. No new blocking issues found after the latest updates.
Verified the previous concerns against head 57cd96a3d4519ea51d02270ae48f233a6216d79d: the reply/reply-all/forward signature image download now happens after template plaintext mode is applied, the CI test/lint coverage was restored, and the stale signature comment was removed.
Checks run locally:
go test ./shortcuts/mail/...git diff --check 465c789f7c3da7048eb8835f15cfdc9d0760e8d8...HEADgofmt -l $(git diff --name-only 465c789f7c3da7048eb8835f15cfdc9d0760e8d8...HEAD -- '*.go')python3 scripts/fetch_meta.py && go test ./cmd/schema ./internal/schema
I also ran go test ./cmd/... ./internal/... ./shortcuts/... ./extension/...; it initially failed only because the detached review worktree had not generated API metadata yet, and the affected schema packages passed after running the same scripts/fetch_meta.py step used by CI.
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.
Generated by the harness-coding skill.
Sprints
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Documentation