feat(mail): add HTML lint lib + +lint-html shortcut + write-path enforcement#756
Closed
bubbmon233 wants to merge 2 commits into
Closed
feat(mail): add HTML lint lib + +lint-html shortcut + write-path enforcement#756bubbmon233 wants to merge 2 commits into
bubbmon233 wants to merge 2 commits into
Conversation
…rcement
The new mail/lint package classifies HTML tags / attrs / inline styles into
three tiers (allow / warn-and-autofix / error-delete) aligned with the
mail-editor editor-kit branch DOMPurify config but stricter — <script> /
<style> / <iframe> / on*-handlers / javascript:/vbscript: URLs are removed
unconditionally. The lib is consumed by:
- A new read-only +lint-html shortcut (--body / --body-file / --auto-fix /
--strict). Returns {warnings, errors, cleaned_html}.
- The compose-5 write paths (+send / +draft-create / +reply / +reply-all /
+forward) and +draft-edit body ops (set_body / set_reply_body), which
call the lib BEFORE emlbuilder; the stdout envelope always carries
lint_applied[] and original_blocked[] arrays so callers can rely on them
unconditionally. There is no --no-lint opt-out — that's the safety
contract per spec §4.3.
For reply / reply-all / forward the lint runs on the user-authored body +
signature only; the <blockquote> derived from the original message is left
intact (server-side RemoteSanitizer already vetted it, double-sanitising
would risk dropping legitimate Lark quote-block class markup).
Adds three reference docs (HTML allowlist, Feishu native writing + 3
complete reusable templates, +lint-html usage) and updates the
skill-template main doc to link them and forbid hand-rolled raw EML.
No new third-party deps — reuses golang.org/x/net/html which is already in
go.mod via mail/draft. Lint package coverage 97.0%.
sprint: S4
|
fengzhihao.infeng 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
|
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)
📝 WalkthroughWalkthroughAdds a new Go HTML linting package for mail (types, rules, linter) and integrates write‑path linting into mail flows (draft create/edit/forward/reply/send) plus a read‑only ChangesHTML Linting Implementation & Integration
Documentation & Reference Guides
Sequence Diagram(s)sequenceDiagram
participant User
participant LintCmd as +lint-html
participant Reader as HTML Reader
participant Linter as lint.Run()
participant Parser as HTML Parser
participant Walker as DOM Walker
participant Classifier as Rule Classifier
participant Output as JSON Output
User->>LintCmd: invoke with --body / --body-file
LintCmd->>Reader: readLintHTMLBody()
Reader-->>LintCmd: rawHTML
LintCmd->>Linter: Run(rawHTML, Options{AutoFix,Strict})
Linter->>Parser: parse fragment
Parser-->>Linter: DOM nodes
Linter->>Walker: walk nodes
Walker->>Classifier: classifyTag / classifyURLValue / classifyStyleProperty
Classifier-->>Walker: kind, ruleID / allowed?
Walker->>Linter: append Finding (applied/blocked)
Linter-->>LintCmd: Report{Applied, Blocked, CleanedHTML}
LintCmd->>Output: render envelope JSON / pretty
Output-->>User: result
sequenceDiagram
participant User
participant MailSend as mail_send.go
participant Resolver as Image Resolver
participant WriteLint as runWritePathLint()
participant Linter as lint.Run()
participant Envelope as Output Envelope
User->>MailSend: Execute send with body HTML
MailSend->>Resolver: resolveLocalImages()
Resolver-->>MailSend: resolvedHTML
MailSend->>WriteLint: runWritePathLint(resolvedHTML)
WriteLint->>Linter: Run(html, Options{AutoFix:true})
Linter-->>WriteLint: Report{CleanedHTML, Applied, Blocked}
WriteLint-->>MailSend: cleanedHTML, report
MailSend->>MailSend: replace body with cleanedHTML
MailSend->>Envelope: applyLintToEnvelope(report)
Envelope-->>User: send / saved-draft output with lint_applied and original_blocked
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #756 +/- ##
==========================================
+ Coverage 65.08% 65.64% +0.55%
==========================================
Files 503 513 +10
Lines 46581 47293 +712
==========================================
+ Hits 30318 31044 +726
+ Misses 13623 13593 -30
- Partials 2640 2656 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@520b9dbaf187494a4a83a5ff089c93006e42d4b4🧩 Skill updatenpx skills add bubbmon233/cli#harness/01kqy6y49nzgktyd0abjgjthrv -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/mail/mail_forward.go (1)
245-278: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a
+forwardregression test for the authored-only lint boundary.This path now has forward-specific behavior: lint the user-authored prefix/signature, but leave the forwarded quote untouched, and emit
lint_applied/original_blockedin the output. I only see draft-create coverage in the added tests, so this contract can regress without detection.As per coding guidelines "Every behavior change must have an accompanying test".
🤖 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_forward.go` around lines 245 - 278, Add a regression test that exercises the HTML-forward path to ensure linting only touches the authored prefix/signature and leaves the forwarded quote untouched: create a test (e.g. TestForward_LintAuthoredOnly) that builds a forward request triggering the branch in mail_forward.go (set useHTML true, include an original message with a forward quote built via buildForwardQuoteHTML and a user-authored body that runWritePathLint will modify), call the forward handling code to produce composedHTMLBody, and assert the emitted lint flags/metrics include lint_applied and original_blocked (or that lintBlocked is set for the original part) so the contract around runWritePathLint + composedHTMLBody/regression for forward-specific behavior is covered.
🧹 Nitpick comments (6)
shortcuts/mail/lint/types.go (1)
84-87: 💤 Low valueDoc nit:
Blockedalso receives strict-promoted warnings.The comment states
Blockedonly surfaces error-tier findings, but inlinter.go(processElementand the URL-warning attribute branch) warnings get appended toBlockedwhenopts.Strictis true. Consider mentioning this for accuracy.📝 Suggested wording
- // Blocked surfaces error-tier findings that the lib removed unconditionally - // (writing-path safety floor: <script> / on* / javascript: URLs always go, - // regardless of AutoFix). + // Blocked surfaces error-tier findings that the lib removed unconditionally + // (writing-path safety floor: <script> / on* / javascript: URLs always go, + // regardless of AutoFix). Strict mode also promotes warning-tier findings + // into Blocked so callers can use a single field to drive non-zero exits.🤖 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/lint/types.go` around lines 84 - 87, Update the comment for the Blocked field to state that it contains not only error-tier findings removed unconditionally but also warnings that are promoted to blocked when opts.Strict is enabled; reference the logic in linter.go (processElement and the URL-warning attribute branch) which appends warnings to Blocked under strict mode so the comment accurately reflects both sources.shortcuts/mail/mail_lint_writepath.go (1)
62-82: 💤 Low valueTwo helpers that do the same thing — consider keeping just one.
emptyLintEnvelopeFieldsandemptyLintFindingsare byte-identical apart from the result-name labels and the doc-comment framing. Keeping both invites drift if one is later updated (e.g., to allocate a shared zero-value singleton). Since the file is new, picking one name now costs nothing.♻️ Suggested consolidation
-// emptyLintEnvelopeFields returns the writing-path stdout-envelope fields -// representing "no lint pass occurred" (e.g. plain-text body branch). Used by -// compose 5's plain-text path so the public envelope still carries the -// contract keys as empty arrays. -func emptyLintEnvelopeFields() (lintApplied, originalBlocked []lint.Finding) { - return []lint.Finding{}, []lint.Finding{} -} - // lintFinding aliases the lint package's Finding type for callers that don't // want to import shortcuts/mail/lint directly (e.g. function signatures in // existing mail_*.go files that want to keep their import set minimal). It is // purely a syntactic convenience — both names refer to the same struct. type lintFinding = lint.Finding -// emptyLintFindings returns two non-nil empty Finding slices, used by helpers -// that initialise their outputs before knowing whether the body is HTML. -// Equivalent to emptyLintEnvelopeFields but named to reflect "findings" rather -// than "envelope fields" so call-sites read consistently with their context. -func emptyLintFindings() (applied, blocked []lint.Finding) { +// emptyLintFindings returns two non-nil empty Finding slices used by both the +// plain-text write path (where no lint pass occurs) and helpers that +// initialise their outputs before knowing whether the body is HTML. +func emptyLintFindings() (applied, blocked []lint.Finding) { return []lint.Finding{}, []lint.Finding{} }Then update any callers of
emptyLintEnvelopeFieldsto useemptyLintFindings.🤖 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_lint_writepath.go` around lines 62 - 82, Remove the duplicate helper by keeping only one of the identical functions (preferably emptyLintFindings for naming clarity) and delete emptyLintEnvelopeFields; ensure you preserve the returned types (two non-nil empty []lint.Finding slices) and the doc-comment intent; then update all callers that reference emptyLintEnvelopeFields to call emptyLintFindings instead (search for usages and replace the symbol), and run tests/linters to confirm no references remain.shortcuts/mail/lint/rules.go (2)
127-156: 💤 Low valueDocstring claims
classifyTagreturns "unknown" but it never does.The doc on Line 129 lists
"unknown"as a possiblekind, but the function only ever returns"allow","warn", or"error"— the unknown-tag branch on Line 155 returns"allow"with an empty rule id. Either drop the mention from the docstring or actually return"unknown"so the caller inlinter.go:processElementcan distinguish niche tags.📝 Suggested wording fix
-// classifyTag returns the rule kind for the given lower-case tag name. -// -// kind is one of "allow", "warn", "error", "unknown". For "warn" / "error", -// ruleID names the firing rule; for "unknown", the caller falls back to -// allow-list-by-default but emits a hint via RuleTagUnknownStripped only when -// the tag is structurally suspect (e.g. <object>-like). The cli's existing +// classifyTag returns the rule kind for the given lower-case tag name. +// +// kind is one of "allow", "warn", "error". For "warn" / "error", ruleID +// names the firing rule. Unknown / niche tags fall through to "allow" with +// an empty ruleID — the cli's existing🤖 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/lint/rules.go` around lines 127 - 156, The docstring incorrectly lists "unknown" as a possible kind but classifyTag never returns it; update classifyTag to return "unknown" (with an empty ruleID) for niche/structurally-suspect tags instead of falling through to "allow" so callers like processElement in linter.go can distinguish them; specifically, change the final return in classifyTag (and adjust any comments) to return "unknown", "" and verify processElement handles the "unknown" branch appropriately.
304-318: 💤 Low valuePrefix match admits unknown
border-*/padding-*/margin-*properties.
classifyStylePropertyaccepts any property starting withborder-,padding-, ormargin-— including non-standard ones likeborder-fooorpadding-x-custom. In practice CSS doesn't define such names, but combined with the explicit allow-list philosophy this is a slightly looser policy than what the user-facing allowlist doc implies. Consider enumerating the eight validborder-*directional/property variants explicitly if you want a tight allow-list.🤖 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/lint/rules.go` around lines 304 - 318, The classifyStyleProperty function currently allows any property starting with border-, padding-, or margin-, which could admit invalid CSS properties. To fix this, replace the prefix checks for these groups with explicit enumeration of the valid variants, listing the eight accepted border-* properties and valid padding-* and margin-* properties explicitly in allowedStyleProps or a separate list. Update classifyStyleProperty to only return true for these exact known properties instead of any prefixed string. This tightening aligns the function with the explicit allow-list approach.shortcuts/mail/lint/linter.go (1)
391-431: 💤 Low valueThe naive semicolon split is theoretically problematic but not practically impactful for the current allowlist.
strings.Split(raw, ";")doesn't respect CSS quoted strings orurl(...)contexts. However, the allowlisted properties (color,font-size,padding,margin,border, etc.) don't typically contain quoted values with embedded semicolons. The examples given (font-familyandbackground-image) aren't allowlisted, so this edge case doesn't manifest in practice.If the allowlist expands to include properties with complex values (e.g.,
font-familywith multiple fallbacks), consider implementing proper CSS declaration parsing that respects quotes and parentheses, or document this limitation explicitly.🤖 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/lint/linter.go` around lines 391 - 431, The current sanitiseStyleAttr uses strings.Split(raw, ";") which wrongly splits semicolons inside quoted strings or url(...) values; update sanitiseStyleAttr to tokenise declarations with a small stateful parser that iterates runes and tracks being inside single/double quotes and parentheses so semicolons inside those contexts are ignored, then trim and process each declaration as before (use existing classifyStyleProperty(name) and preserve dropped-list behavior), or if you prefer not to implement parsing now, update the function comment to explicitly document this limitation and add a TODO referencing possible future allowlist entries (e.g., font-family) that would require proper CSS declaration parsing.shortcuts/mail/mail_draft_create.go (1)
188-195: ⚡ Quick winPretty output hides the new lint findings.
outnow carrieslint_applied/original_blocked, but--format prettystill prints only the draft ID/reference. That means this shortcut can silently rewrite or strip HTML without any user-visible hint in a supported output mode.Possible lightweight fix
fmt.Fprintln(w, "Draft created.") + if len(lintApplied) > 0 || len(lintBlocked) > 0 { + fmt.Fprintf(w, "lint_applied: %d, original_blocked: %d\n", len(lintApplied), len(lintBlocked)) + } // Intentionally keep +draft-create output minimal: unlike reply/forward/send // draft-save flows, it does not add a follow-up send tip. fmt.Fprintf(w, "draft_id: %s\n", draftResult.DraftID)🤖 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.go` around lines 188 - 195, The pretty output in mail_draft_create.go (inside the runtime.OutFormat callback that prints draftResult.DraftID and out["reference"]) omits the new out keys lint_applied and original_blocked, hiding lint rewrites; update the runtime.OutFormat writer lambda to check out["lint_applied"] and out["original_blocked"] (or their boolean/string forms) and print concise lines when present (e.g., "lint_applied: true" / "original_blocked: true" or the provided values) so --format pretty surfaces these new lint-related flags alongside draft_id and reference.
🤖 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/lint/linter.go`:
- Around line 235-265: The style-property branch in sanitisation (inside the if
name == "style" block using sanitiseStyleAttr and RuleStylePropertyDropped)
always appends Findings to rep.Applied with SeverityWarning and ignores
opts.Strict; update that branch so when opts.Strict is true any dropped property
Finding is created with SeverityError and appended to rep.Blocked (or otherwise
marked as an error) instead of rep.Applied, preserving the same
TagOrAttr/Excerpt/Hint; also ensure the code path that keeps/auto-fixes
attributes still behaves the same but promotes warnings to errors for strict
mode. Add a unit test in linter_test.go that calls Run(html, Options{Strict:
true}) with a template containing only a forbidden CSS property (e.g.
style="position:absolute") and assert rep.Blocked is non-empty /
rep.HasErrorFindings == true.
In `@shortcuts/mail/mail_lint_writepath_test.go`:
- Around line 154-160: The test currently does a type assertion like "las, _ :=
la.([]interface{})" which silently yields nil for non-array values and thus
misses regressions to null or other types; update the assertions to first verify
the value is actually a []interface{} (e.g., use the two-value form "las, ok :=
la.([]interface{}); if !ok { t.Fatalf(\"lint_applied must be an array, got %T\",
la) }") and then check its length, and do the same for "ob" / "original_blocked"
(e.g., "obs, ok := ob.([]interface{}); if !ok { t.Fatalf(\"original_blocked must
be an array, got %T\", ob) }" followed by the length check).
In `@skills/lark-mail/references/lark-mail-lint-html.md`:
- Around line 100-110: The scenario text incorrectly references original_blocked
(a write-path envelope key) instead of the actual +lint-html output shape;
update the description to state that +lint-html returns its problems under
data.errors (and/or data.warnings) and that the AI should inspect data.errors
for TAG_SCRIPT_BLOCKED, then use the cleaned_html to backfill the subsequent
+draft-create step; specifically replace mentions of original_blocked with
data.errors and reference TAG_SCRIPT_BLOCKED, +lint-html, and +draft-create so
readers can locate the relevant fields and flow.
---
Outside diff comments:
In `@shortcuts/mail/mail_forward.go`:
- Around line 245-278: Add a regression test that exercises the HTML-forward
path to ensure linting only touches the authored prefix/signature and leaves the
forwarded quote untouched: create a test (e.g. TestForward_LintAuthoredOnly)
that builds a forward request triggering the branch in mail_forward.go (set
useHTML true, include an original message with a forward quote built via
buildForwardQuoteHTML and a user-authored body that runWritePathLint will
modify), call the forward handling code to produce composedHTMLBody, and assert
the emitted lint flags/metrics include lint_applied and original_blocked (or
that lintBlocked is set for the original part) so the contract around
runWritePathLint + composedHTMLBody/regression for forward-specific behavior is
covered.
---
Nitpick comments:
In `@shortcuts/mail/lint/linter.go`:
- Around line 391-431: The current sanitiseStyleAttr uses strings.Split(raw,
";") which wrongly splits semicolons inside quoted strings or url(...) values;
update sanitiseStyleAttr to tokenise declarations with a small stateful parser
that iterates runes and tracks being inside single/double quotes and parentheses
so semicolons inside those contexts are ignored, then trim and process each
declaration as before (use existing classifyStyleProperty(name) and preserve
dropped-list behavior), or if you prefer not to implement parsing now, update
the function comment to explicitly document this limitation and add a TODO
referencing possible future allowlist entries (e.g., font-family) that would
require proper CSS declaration parsing.
In `@shortcuts/mail/lint/rules.go`:
- Around line 127-156: The docstring incorrectly lists "unknown" as a possible
kind but classifyTag never returns it; update classifyTag to return "unknown"
(with an empty ruleID) for niche/structurally-suspect tags instead of falling
through to "allow" so callers like processElement in linter.go can distinguish
them; specifically, change the final return in classifyTag (and adjust any
comments) to return "unknown", "" and verify processElement handles the
"unknown" branch appropriately.
- Around line 304-318: The classifyStyleProperty function currently allows any
property starting with border-, padding-, or margin-, which could admit invalid
CSS properties. To fix this, replace the prefix checks for these groups with
explicit enumeration of the valid variants, listing the eight accepted border-*
properties and valid padding-* and margin-* properties explicitly in
allowedStyleProps or a separate list. Update classifyStyleProperty to only
return true for these exact known properties instead of any prefixed string.
This tightening aligns the function with the explicit allow-list approach.
In `@shortcuts/mail/lint/types.go`:
- Around line 84-87: Update the comment for the Blocked field to state that it
contains not only error-tier findings removed unconditionally but also warnings
that are promoted to blocked when opts.Strict is enabled; reference the logic in
linter.go (processElement and the URL-warning attribute branch) which appends
warnings to Blocked under strict mode so the comment accurately reflects both
sources.
In `@shortcuts/mail/mail_draft_create.go`:
- Around line 188-195: The pretty output in mail_draft_create.go (inside the
runtime.OutFormat callback that prints draftResult.DraftID and out["reference"])
omits the new out keys lint_applied and original_blocked, hiding lint rewrites;
update the runtime.OutFormat writer lambda to check out["lint_applied"] and
out["original_blocked"] (or their boolean/string forms) and print concise lines
when present (e.g., "lint_applied: true" / "original_blocked: true" or the
provided values) so --format pretty surfaces these new lint-related flags
alongside draft_id and reference.
In `@shortcuts/mail/mail_lint_writepath.go`:
- Around line 62-82: Remove the duplicate helper by keeping only one of the
identical functions (preferably emptyLintFindings for naming clarity) and delete
emptyLintEnvelopeFields; ensure you preserve the returned types (two non-nil
empty []lint.Finding slices) and the doc-comment intent; then update all callers
that reference emptyLintEnvelopeFields to call emptyLintFindings instead (search
for usages and replace the symbol), and run tests/linters to confirm no
references remain.
🪄 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: a74e228d-0056-4a05-9c64-2247b9fc18e4
📒 Files selected for processing (21)
shortcuts/mail/lint/linter.goshortcuts/mail/lint/linter_test.goshortcuts/mail/lint/rules.goshortcuts/mail/lint/types.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_create_test.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_lint_html_test.goshortcuts/mail/mail_lint_writepath.goshortcuts/mail/mail_lint_writepath_test.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-feishu-native.mdskills/lark-mail/references/lark-mail-html-allowlist.mdskills/lark-mail/references/lark-mail-lint-html.md
Comment on lines
+235
to
+265
| // 3. `style` attribute → property-by-property allow-list. | ||
| if name == "style" { | ||
| cleaned, dropped := sanitiseStyleAttr(attr.Val) | ||
| for _, prop := range dropped { | ||
| rep.Applied = append(rep.Applied, Finding{ | ||
| RuleID: RuleStylePropertyDropped, | ||
| Severity: SeverityWarning, | ||
| TagOrAttr: "style." + prop, | ||
| Excerpt: truncateExcerpt(prop), | ||
| Hint: "已删除非白名单 CSS 属性（详见 references/lark-mail-html-allowlist.md）", | ||
| }) | ||
| } | ||
| if len(dropped) == 0 { | ||
| attr.Val = cleaned | ||
| keep = append(keep, attr) | ||
| continue | ||
| } | ||
| if !opts.AutoFix { | ||
| // AutoFix=false: keep the original property list so users see | ||
| // exactly what would change. | ||
| keep = append(keep, attr) | ||
| continue | ||
| } | ||
| if cleaned == "" { | ||
| // All properties dropped — remove the attribute entirely. | ||
| continue | ||
| } | ||
| attr.Val = cleaned | ||
| keep = append(keep, attr) | ||
| continue | ||
| } |
There was a problem hiding this comment.
STYLE_PROPERTY_DROPPED warnings bypass --strict.
In --strict mode every warning is supposed to be promoted to SeverityError and pushed onto Blocked so +lint-html --strict exits non-zero. The tag and URL branches honour this (see Lines 138–143 and 221–226), but the style-property branch always writes to Applied with SeverityWarning regardless of opts.Strict. As a result, a template containing only blacklisted CSS (e.g. style="position:absolute") silently passes +lint-html --strict even though the doc/spec states "任意 warning 也升级成 error".
🛡️ Proposed fix
if name == "style" {
cleaned, dropped := sanitiseStyleAttr(attr.Val)
for _, prop := range dropped {
- rep.Applied = append(rep.Applied, Finding{
- RuleID: RuleStylePropertyDropped,
- Severity: SeverityWarning,
- TagOrAttr: "style." + prop,
- Excerpt: truncateExcerpt(prop),
- Hint: "已删除非白名单 CSS 属性（详见 references/lark-mail-html-allowlist.md）",
- })
+ finding := Finding{
+ RuleID: RuleStylePropertyDropped,
+ Severity: SeverityWarning,
+ TagOrAttr: "style." + prop,
+ Excerpt: truncateExcerpt(prop),
+ Hint: "已删除非白名单 CSS 属性（详见 references/lark-mail-html-allowlist.md）",
+ }
+ if opts.Strict {
+ finding.Severity = SeverityError
+ rep.Blocked = append(rep.Blocked, finding)
+ } else {
+ rep.Applied = append(rep.Applied, finding)
+ }
}Add a corresponding test case in linter_test.go exercising Run(html, Options{Strict: true}) with a body that only contains a forbidden CSS property and assert len(rep.Blocked) > 0 / rep.HasErrorFindings == true.
As per coding guidelines, "Every behavior change must have an accompanying test".
🤖 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/lint/linter.go` around lines 235 - 265, The style-property
branch in sanitisation (inside the if name == "style" block using
sanitiseStyleAttr and RuleStylePropertyDropped) always appends Findings to
rep.Applied with SeverityWarning and ignores opts.Strict; update that branch so
when opts.Strict is true any dropped property Finding is created with
SeverityError and appended to rep.Blocked (or otherwise marked as an error)
instead of rep.Applied, preserving the same TagOrAttr/Excerpt/Hint; also ensure
the code path that keeps/auto-fixes attributes still behaves the same but
promotes warnings to errors for strict mode. Add a unit test in linter_test.go
that calls Run(html, Options{Strict: true}) with a template containing only a
forbidden CSS property (e.g. style="position:absolute") and assert rep.Blocked
is non-empty / rep.HasErrorFindings == true.
Comment on lines
+154
to
+160
| // Empty arrays are decoded as []interface{}{} (or could be nil; both | ||
| // are valid JSON []). | ||
| if las, _ := la.([]interface{}); len(las) != 0 { | ||
| t.Errorf("lint_applied should be empty, got %d", len(las)) | ||
| } | ||
| if obs, _ := ob.([]interface{}); len(obs) != 0 { | ||
| t.Errorf("original_blocked should be empty, got %d", len(obs)) |
There was a problem hiding this comment.
Fail on non-array values in the plain-text envelope test.
These assertions still pass if lint_applied or original_blocked regresses to null or any non-array type, because a failed []interface{} assertion produces a nil slice with length 0. Since the contract here is “always arrays”, the test should check the type first and then the length.
Suggested tightening
- if las, _ := la.([]interface{}); len(las) != 0 {
- t.Errorf("lint_applied should be empty, got %d", len(las))
- }
- if obs, _ := ob.([]interface{}); len(obs) != 0 {
- t.Errorf("original_blocked should be empty, got %d", len(obs))
- }
+ las, ok := la.([]interface{})
+ if !ok {
+ t.Fatalf("lint_applied should decode as an array, got %T", la)
+ }
+ if len(las) != 0 {
+ t.Errorf("lint_applied should be empty, got %d", len(las))
+ }
+ obs, ok := ob.([]interface{})
+ if !ok {
+ t.Fatalf("original_blocked should decode as an array, got %T", ob)
+ }
+ if len(obs) != 0 {
+ t.Errorf("original_blocked should be empty, got %d", len(obs))
+ }📝 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
| // Empty arrays are decoded as []interface{}{} (or could be nil; both | |
| // are valid JSON []). | |
| if las, _ := la.([]interface{}); len(las) != 0 { | |
| t.Errorf("lint_applied should be empty, got %d", len(las)) | |
| } | |
| if obs, _ := ob.([]interface{}); len(obs) != 0 { | |
| t.Errorf("original_blocked should be empty, got %d", len(obs)) | |
| // Empty arrays are decoded as []interface{}{} (or could be nil; both | |
| // are valid JSON []). | |
| las, ok := la.([]interface{}) | |
| if !ok { | |
| t.Fatalf("lint_applied should decode as an array, got %T", la) | |
| } | |
| if len(las) != 0 { | |
| t.Errorf("lint_applied should be empty, got %d", len(las)) | |
| } | |
| obs, ok := ob.([]interface{}) | |
| if !ok { | |
| t.Fatalf("original_blocked should decode as an array, got %T", ob) | |
| } | |
| if len(obs) != 0 { | |
| t.Errorf("original_blocked should be empty, got %d", len(obs)) | |
| } |
🤖 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_lint_writepath_test.go` around lines 154 - 160, The test
currently does a type assertion like "las, _ := la.([]interface{})" which
silently yields nil for non-array values and thus misses regressions to null or
other types; update the assertions to first verify the value is actually a
[]interface{} (e.g., use the two-value form "las, ok := la.([]interface{}); if
!ok { t.Fatalf(\"lint_applied must be an array, got %T\", la) }") and then check
its length, and do the same for "ob" / "original_blocked" (e.g., "obs, ok :=
ob.([]interface{}); if !ok { t.Fatalf(\"original_blocked must be an array, got
%T\", ob) }" followed by the length check).
Comment on lines
+100
to
+110
| ## 典型场景 | ||
|
|
||
| ### 场景 1：AI 在 +draft-create 之前自检 | ||
|
|
||
| ```bash | ||
| HTML='<p>本周进展：</p><font color="red">紧急</font><script>track()</script>' | ||
| lark-cli mail +lint-html --body "$HTML" | ||
| ``` | ||
|
|
||
| → AI 看到 `original_blocked` 含 `TAG_SCRIPT_BLOCKED`，把 cleaned_html 回填给后续 `+draft-create`，避免在写信路径才被静悄悄改动。 | ||
|
|
There was a problem hiding this comment.
Field name mismatch in scenario 1 description.
Line 109 tells readers that the AI will see original_blocked in +lint-html's output, but the documented return shape on lines 50–73 only contains data.warnings[] and data.errors[] (the keys lint_applied / original_blocked are reserved for the write‑path envelope per line 13). For consistency, this scenario should describe inspecting data.errors (containing TAG_SCRIPT_BLOCKED).
📝 Suggested wording
-→ AI 看到 `original_blocked` 含 `TAG_SCRIPT_BLOCKED`，把 cleaned_html 回填给后续 `+draft-create`，避免在写信路径才被静悄悄改动。
+→ AI 看到 `data.errors[]` 含 `TAG_SCRIPT_BLOCKED`，把 `cleaned_html` 回填给后续 `+draft-create`，避免在写信路径才被静悄悄改动。🤖 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-lint-html.md` around lines 100 - 110,
The scenario text incorrectly references original_blocked (a write-path envelope
key) instead of the actual +lint-html output shape; update the description to
state that +lint-html returns its problems under data.errors (and/or
data.warnings) and that the AI should inspect data.errors for
TAG_SCRIPT_BLOCKED, then use the cleaned_html to backfill the subsequent
+draft-create step; specifically replace mentions of original_blocked with
data.errors and reference TAG_SCRIPT_BLOCKED, +lint-html, and +draft-create so
readers can locate the relevant fields and flow.
…circuited bodyIsHTML used htmlTagRe as its only allow-list. The regex was missing center/marquee/blink, so single-tag bodies like '<center>x</center>' fell into the EmptyReport short-circuit in mail_lint_html.go and bypassed lint entirely, producing warnings=[] / errors=[] / cleaned_html=<input>. Sync htmlTagRe with lint.warnAutofixTags (font/center/marquee/blink) so the three new warn-autofix tags reach lint.Run and fire their respective TAG_*_TO_* warnings as the spec requires.
This was referenced May 7, 2026
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
Add HTML lint capability to
lark-cli mail: a new+lint-htmlshortcut and a reusable mail-domain lint library, plus mandatory sanitization on compose / draft-edit write paths so unsafe or malformed HTML can't reach drafts.Changes
shortcuts/mail/lintpackage (97% test coverage)+lint-htmlshortcut as the CLI entry pointcomposewrite paths and thedraft-editpatch pathreferences/SKILL.mdupdated to expose+lint-htmltrigger keywordsTest Plan
shortcuts/mail/lint~97% coverage)lark mail +lint-html <input>command works as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests