Add mail signature write shortcuts#1217
Open
alphonseiwnl wants to merge 1 commit into
Open
Conversation
Add create, update, and delete signature shortcuts with local validation, dry-run coverage, and mail skill references. sprint: S4
📝 WalkthroughWalkthroughThis PR adds complete support for personal email signature management via three new CLI commands: ChangesMail Signature Management Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
root seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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: 5
🤖 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/mail_signature_create.go`:
- Around line 112-115: The follow-up hint is being printed to stdout via
runtime.OutFormat's writer; keep the created signature output on stdout and move
the usage hint to stderr by removing the fmt.Fprintln(w, ...) call inside the
runtime.OutFormat callback and instead write the hint to runtime.IO().ErrOut
(e.g. fmt.Fprintln(runtime.IO().ErrOut, "Use this signature_id with mail +send /
+reply / +forward --signature-id.")) after or outside the OutFormat call; leave
formatSignatureSummary(...) and the OutFormat invocation unchanged so only the
hint is redirected to stderr.
In `@shortcuts/mail/mail_signature_update.go`:
- Around line 241-246: Remove the duplicate concurrency warning from the data
output path: in the block that calls runtime.OutFormat(..., func(w io.Writer) {
formatSignatureSummary(...); fmt.Fprintln(w, "...") }), drop the fmt.Fprintln(w,
"warning: no optimistic locking; concurrent updates are last-write-wins.") so
the warning is not written into the OutFormat envelope; keep the existing
fmt.Fprintln(runtime.IO().ErrOut, "warning: signature endpoints have no
optimistic locking; concurrent updates are last-write-wins.") to ensure the
warning is emitted only to stderr. Ensure calls to formatSignatureSummary(...)
and the OutFormat invocation remain unchanged otherwise.
In `@shortcuts/mail/signature_content.go`:
- Around line 160-175: The current rejectSignatureLocalImages allows any
non-empty URL scheme which lets file:, data:, blob:, etc. through; update
rejectSignatureLocalImages to allow only an explicit whitelist of remote/CID
schemes (http, https, cid) and keep protocol-relative sources
(strings.HasPrefix(src, "//")) allowed, but treat all other schemes (including
file, data, blob) and schemeless local paths as invalid and return
output.ErrValidation; use the existing extractSignatureImageSources and
url.Parse(src) logic to inspect the scheme and enforce this allowlist.
In `@skills/lark-mail/references/lark-mail-signature-create.md`:
- Line 32: The table cell containing the parameter text `--device pc|mobile` is
breaking the Markdown table due to the unescaped `|`; update that cell to escape
the pipe (e.g., change `--device pc|mobile` to `--device pc\|mobile` or
otherwise ensure the pipe is escaped/encoded) so the table renders correctly
while keeping the inline code formatting intact.
In `@skills/lark-mail/references/lark-mail-signature-update.md`:
- Line 37: Update the Markdown table row that currently shows the option
"--set-device pc|mobile" so the pipe character inside the option is escaped:
insert a backslash before the '|' (i.e., pc\|mobile) within the existing inline
code span for --set-device to prevent the '|' from being interpreted as a table
column separator and thus keep the table columns valid.
🪄 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: 5e7b87e9-9f42-440b-81a0-311427b0a431
📒 Files selected for processing (14)
shortcuts/mail/mail_signature_create.goshortcuts/mail/mail_signature_delete.goshortcuts/mail/mail_signature_update.goshortcuts/mail/mail_signature_write_test.goshortcuts/mail/shortcuts.goshortcuts/mail/signature/model.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_content.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-signature-create.mdskills/lark-mail/references/lark-mail-signature-delete.mdskills/lark-mail/references/lark-mail-signature-update.mdtests/cli_e2e/mail/mail_signature_dryrun_test.go
Comment on lines
+112
to
+115
| runtime.OutFormat(out, nil, func(w io.Writer) { | ||
| formatSignatureSummary(w, "created", created, resolveLang(runtime)) | ||
| fmt.Fprintln(w, "Use this signature_id with mail +send / +reply / +forward --signature-id.") | ||
| }) |
There was a problem hiding this comment.
Move the follow-up hint to stderr.
Line 114 writes a usage hint through OutFormat, so non-result text is mixed into the command’s stdout stream. Keep the created signature data on stdout and send the hint to runtime.IO().ErrOut instead.
Proposed fix
runtime.OutFormat(out, nil, func(w io.Writer) {
formatSignatureSummary(w, "created", created, resolveLang(runtime))
- fmt.Fprintln(w, "Use this signature_id with mail +send / +reply / +forward --signature-id.")
})
+ fmt.Fprintln(runtime.IO().ErrOut, "hint: use this signature_id with mail +send / +reply / +forward --signature-id.")
return nil
},As per coding guidelines, stdout is data (JSON envelopes), stderr is everything else (progress, warnings, hints).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| runtime.OutFormat(out, nil, func(w io.Writer) { | |
| formatSignatureSummary(w, "created", created, resolveLang(runtime)) | |
| fmt.Fprintln(w, "Use this signature_id with mail +send / +reply / +forward --signature-id.") | |
| }) | |
| runtime.OutFormat(out, nil, func(w io.Writer) { | |
| formatSignatureSummary(w, "created", created, resolveLang(runtime)) | |
| }) | |
| fmt.Fprintln(runtime.IO().ErrOut, "hint: use this signature_id with mail +send / +reply / +forward --signature-id.") |
🤖 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_signature_create.go` around lines 112 - 115, The
follow-up hint is being printed to stdout via runtime.OutFormat's writer; keep
the created signature output on stdout and move the usage hint to stderr by
removing the fmt.Fprintln(w, ...) call inside the runtime.OutFormat callback and
instead write the hint to runtime.IO().ErrOut (e.g.
fmt.Fprintln(runtime.IO().ErrOut, "Use this signature_id with mail +send /
+reply / +forward --signature-id.")) after or outside the OutFormat call; leave
formatSignatureSummary(...) and the OutFormat invocation unchanged so only the
hint is redirected to stderr.
Comment on lines
+241
to
+246
| runtime.OutFormat(out, nil, func(w io.Writer) { | ||
| formatSignatureSummary(w, "updated", updated, resolveLang(runtime)) | ||
| fmt.Fprintln(w, "warning: no optimistic locking; concurrent updates are last-write-wins.") | ||
| }) | ||
| fmt.Fprintln(runtime.IO().ErrOut, | ||
| "warning: signature endpoints have no optimistic locking; concurrent updates are last-write-wins.") |
There was a problem hiding this comment.
Send the concurrency warning to stderr only.
Line 243 currently pushes a warning through OutFormat and then lines 245-246 print the same warning to stderr again. Drop the stdout copy so the result stream stays data-only and the warning remains a single stderr message.
Proposed fix
runtime.OutFormat(out, nil, func(w io.Writer) {
formatSignatureSummary(w, "updated", updated, resolveLang(runtime))
- fmt.Fprintln(w, "warning: no optimistic locking; concurrent updates are last-write-wins.")
})
fmt.Fprintln(runtime.IO().ErrOut,
"warning: signature endpoints have no optimistic locking; concurrent updates are last-write-wins.")As per coding guidelines, stdout is data (JSON envelopes), stderr is everything else (progress, warnings, hints).
🤖 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_signature_update.go` around lines 241 - 246, Remove the
duplicate concurrency warning from the data output path: in the block that calls
runtime.OutFormat(..., func(w io.Writer) { formatSignatureSummary(...);
fmt.Fprintln(w, "...") }), drop the fmt.Fprintln(w, "warning: no optimistic
locking; concurrent updates are last-write-wins.") so the warning is not written
into the OutFormat envelope; keep the existing fmt.Fprintln(runtime.IO().ErrOut,
"warning: signature endpoints have no optimistic locking; concurrent updates are
last-write-wins.") to ensure the warning is emitted only to stderr. Ensure calls
to formatSignatureSummary(...) and the OutFormat invocation remain unchanged
otherwise.
Comment on lines
+160
to
+175
| func rejectSignatureLocalImages(content string) error { | ||
| for _, src := range extractSignatureImageSources(content) { | ||
| if src == "" { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(src, "//") { | ||
| continue | ||
| } | ||
| u, err := url.Parse(src) | ||
| if err == nil && u.Scheme != "" { | ||
| continue | ||
| } | ||
| return output.ErrValidation("local image paths are not supported for signature content; use --images-json with cid/file_key") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
file:/data: image sources bypass local-image rejection.
The guard only rejects schemeless sources, treating any non-empty scheme as remote/allowed. That lets <img src="file:///home/user/secret.png"> and data: URIs through, which are exactly the local/inline references this validator is meant to block in favor of --images-json with cid/file_key. Prefer an explicit allowlist of remote/CID schemes.
🛡️ Proposed fix: allowlist http/https/cid
func rejectSignatureLocalImages(content string) error {
for _, src := range extractSignatureImageSources(content) {
if src == "" {
continue
}
if strings.HasPrefix(src, "//") {
continue
}
- u, err := url.Parse(src)
- if err == nil && u.Scheme != "" {
- continue
- }
+ if u, err := url.Parse(src); err == nil {
+ switch strings.ToLower(u.Scheme) {
+ case "http", "https", "cid":
+ continue
+ }
+ }
return output.ErrValidation("local image paths are not supported for signature content; use --images-json with cid/file_key")
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func rejectSignatureLocalImages(content string) error { | |
| for _, src := range extractSignatureImageSources(content) { | |
| if src == "" { | |
| continue | |
| } | |
| if strings.HasPrefix(src, "//") { | |
| continue | |
| } | |
| u, err := url.Parse(src) | |
| if err == nil && u.Scheme != "" { | |
| continue | |
| } | |
| return output.ErrValidation("local image paths are not supported for signature content; use --images-json with cid/file_key") | |
| } | |
| return nil | |
| } | |
| func rejectSignatureLocalImages(content string) error { | |
| for _, src := range extractSignatureImageSources(content) { | |
| if src == "" { | |
| continue | |
| } | |
| if strings.HasPrefix(src, "//") { | |
| continue | |
| } | |
| if u, err := url.Parse(src); err == nil { | |
| switch strings.ToLower(u.Scheme) { | |
| case "http", "https", "cid": | |
| continue | |
| } | |
| } | |
| return output.ErrValidation("local image paths are not supported for signature content; use --images-json with cid/file_key") | |
| } | |
| return nil | |
| } |
🤖 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/signature_content.go` around lines 160 - 175, The current
rejectSignatureLocalImages allows any non-empty URL scheme which lets file:,
data:, blob:, etc. through; update rejectSignatureLocalImages to allow only an
explicit whitelist of remote/CID schemes (http, https, cid) and keep
protocol-relative sources (strings.HasPrefix(src, "//")) allowed, but treat all
other schemes (including file, data, blob) and schemeless local paths as invalid
and return output.ErrValidation; use the existing extractSignatureImageSources
and url.Parse(src) logic to inspect the scheme and enforce this allowlist.
| | `--name <text>` | 是 | 签名名称，trim 后非空且 ≤100 字符 | | ||
| | `--content <html-or-text>` | 否 | 签名内容；与 `--content-file` 互斥 | | ||
| | `--content-file <path>` | 否 | 从相对路径读取签名内容 | | ||
| | `--device pc|mobile` | 否 | 签名设备，默认 `pc` | |
There was a problem hiding this comment.
Escape pipe in parameter cell to prevent table breakage.
Line 32 uses pc|mobile inside a Markdown table cell; the raw | is parsed as a new column and corrupts rendering.
Suggested fix
-| `--device pc|mobile` | 否 | 签名设备，默认 `pc` |
+| `--device pc\|mobile` | 否 | 签名设备，默认 `pc` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `--device pc|mobile` | 否 | 签名设备，默认 `pc` | | |
| | `--device pc\|mobile` | 否 | 签名设备，默认 `pc` | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 32-32: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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-signature-create.md` at line 32, The
table cell containing the parameter text `--device pc|mobile` is breaking the
Markdown table due to the unescaped `|`; update that cell to escape the pipe
(e.g., change `--device pc|mobile` to `--device pc\|mobile` or otherwise ensure
the pipe is escaped/encoded) so the table renders correctly while keeping the
inline code formatting intact.
| | `--set-name <text>` | 否 | 替换名称，trim 后非空且 ≤100 字符 | | ||
| | `--set-content <html-or-text>` | 否 | 替换内容；与 `--set-content-file` 互斥 | | ||
| | `--set-content-file <path>` | 否 | 从相对路径读取替换内容 | | ||
| | `--set-device pc|mobile` | 否 | 替换设备 | |
There was a problem hiding this comment.
Escape | in option syntax to keep table columns valid.
Line 37 currently creates an extra Markdown table column due to pc|mobile.
Suggested fix
-| `--set-device pc|mobile` | 否 | 替换设备 |
+| `--set-device pc\|mobile` | 否 | 替换设备 |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `--set-device pc|mobile` | 否 | 替换设备 | | |
| | `--set-device pc\|mobile` | 否 | 替换设备 | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 37-37: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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-signature-update.md` at line 37, Update
the Markdown table row that currently shows the option "--set-device pc|mobile"
so the pipe character inside the option is escaped: insert a backslash before
the '|' (i.e., pc\|mobile) within the existing inline code span for --set-device
to prevent the '|' from being interpreted as a table column separator and thus
keep the table columns valid.
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
Release Notes
New Features
+signature-createfor creating personal email signatures,+signature-updatefor editing existing signatures, and+signature-deletefor removing signatures.--dry-runmode for validation.Documentation