refactor: migrate mail shortcuts to FileIO#356
Conversation
- DraftSnapshot.FIO: inject FileIO into draft snapshot for patch ops (addAttachment, loadAndAttachInline, replaceInline) - emlbuilder.Builder.fio: inject via WithFileIO(), readFile uses FileIO.Open - mail_draft_edit: loadPatchFile uses runtime.FileIO().Open - helpers: checkAttachmentSizeLimit takes fio param, uses FileIO.Stat - validateComposeInlineAndAttachments: pass fio through to size check - All mail entry points (send/reply/reply_all/forward/draft_create): pass runtime.FileIO() to builder and size limit checks Change-Id: I580b126a970c57b4ccfcf13dab4d6aacbe255de9
- addAttachment/replaceInline: distinguish path validation errors (wrapped with context) from stat failures (bare error), matching original SafeInputPath + vfs.Stat two-step behavior - checkAttachmentSizeLimit: distinguish "unsafe attachment path" (path validation) from "failed to stat attachment" (file not found/perm) Change-Id: I6e632c949aa9803a05ef63f529c3db9dfa8ab839
…ration - addAttachment: restore original order (path validation via Stat before extension check) so path traversal errors are reported before blocked extension errors - DraftSnapshot.FIO comment: remove "nil falls back" claim; nil panics - Builder.fio comment: same fix Change-Id: I21ff209987c72704f2302ed3481c58e2a4256764
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)
📝 WalkthroughWalkthroughIntroduce a runtime dependency container Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runtime
participant DraftPkg
participant FileIO
CLI->>Runtime: execute mail command (attach/inline/patch)
Runtime->>DraftPkg: create DraftCtx{FIO: Runtime.FileIO()}
CLI->>DraftPkg: Apply(dctx, snapshot, patch)
DraftPkg->>FileIO: Stat/Open(path) via dctx.FIO
FileIO-->>DraftPkg: file handle / bytes or ErrPathValidation / error
DraftPkg->>DraftPkg: process attachment/inline, create parts
DraftPkg-->>Runtime: return mutated snapshot / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR migrates all mail shortcut file I/O from the One new concern: the Confidence Score: 4/5Safe to merge once the Builder.fio nil-guard is addressed; all production call sites already use WithFileIO correctly. The refactor is clean and all existing callers are correctly wired. One P1 remains: AddFileAttachment/AddFileInline/AddFileOtherPart will panic rather than accumulate an error when WithFileIO is omitted, violating the builder's own contract. Concerns about DraftCtx.FIO nil guard and error-wrapping inconsistency were raised in prior review rounds and remain outstanding. shortcuts/mail/emlbuilder/builder.go — AddFileAttachment, AddFileInline, AddFileOtherPart need nil guards on b.fio
|
| Filename | Overview |
|---|---|
| shortcuts/mail/emlbuilder/builder.go | Migrated file reads from vfs to injected FileIO via WithFileIO; AddFile* methods call readFile(b.fio, …) but lack a nil guard on b.fio, causing a panic instead of an error when WithFileIO is not called. |
| shortcuts/mail/draft/patch.go | Core patch-apply logic migrated to DraftCtx/FileIO; checkBlockedExtension correctly precedes FIO.Stat in addAttachment; error-context wrapping is inconsistent between loadAndAttachInline (all errors wrapped) and addAttachment/replaceInline (bare errors). |
| shortcuts/mail/draft/model.go | Introduces DraftCtx to carry FIO dependency separately from DraftSnapshot; the FIO field is documented as required but not guarded in Apply or its callees. |
| shortcuts/mail/mail_draft_edit.go | DraftCtx correctly wired with runtime.FileIO(); loadPatchFile migrated to use FileIO.Open; no issues. |
| shortcuts/mail/helpers.go | checkAttachmentSizeLimit and validateComposeInlineAndAttachments correctly accept fileio.FileIO; all call sites pass runtime.FileIO(); clean migration. |
| shortcuts/mail/emlbuilder/builder_test.go | Test suite updated to use WithFileIO(testFIO) for all AddFile* paths; comprehensive coverage of blocked extensions and allowed formats. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Mail Command] --> B[runtime.FileIO]
B --> C[DraftCtx with FIO]
B --> D[emlbuilder.New with WithFileIO]
C --> E[draft.Apply dctx + snapshot + patch]
E --> F{op type}
F -->|add_attachment| G[addAttachment]
F -->|add_inline / replace_inline| H[loadAndAttachInline / replaceInline]
F -->|set_body / set_reply_body| I[postProcessInlineImages]
I --> H
G --> K[dctx.FIO.Stat + Open]
H --> K
D --> L[AddFileAttachment]
D --> M[AddFileInline / AddFileOtherPart]
L --> N[readFile via b.fio]
M --> N
style N fill:#fbb,stroke:#f00
style K fill:#bfb,stroke:#0a0
Comments Outside Diff (1)
-
shortcuts/mail/emlbuilder/builder.go, line 428-442 (link)Nil
fiopanics instead of accumulating an errorAddFileAttachment,AddFileInline, andAddFileOtherPartall forward toreadFile(b.fio, path). IfWithFileIOwas never called,b.fiois a nil interface, andfio.Open(path)insidereadFilewill panic at runtime rather than return a stored error. This breaks the builder's own error-accumulation contract —Build()is supposed to be the only call site that can panic-equivalent-fail.A nil guard at the top of each
AddFile*method would be consistent with the pattern already used forb.err:func (b Builder) AddFileAttachment(path string) Builder { if b.err != nil { return b } if b.fio == nil { b.err = fmt.Errorf("emlbuilder: FileIO not set; call WithFileIO before AddFileAttachment") return b } // ... }
The same guard is needed in
AddFileInline(line 487) andAddFileOtherPart(line 546).
Reviews (3): Last reviewed commit: "fix: restore original validation order i..." | Re-trigger Greptile
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1c3be9873808789e3b9feb6f92dcb28f253d0f9e🧩 Skill updatenpx skills add larksuite/cli#feat/fileio-migrate-mail -y -g |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Move fileio.FileIO out of DraftSnapshot (pure data model) into a separate DraftCtx struct, keeping data and runtime concerns decoupled. Apply and internal file-operation functions now receive *DraftCtx as an explicit parameter. Change-Id: Ibabb77c389f75db8cc92d3558a350774e90d1ce1
![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
🧹 Nitpick comments (1)
shortcuts/mail/emlbuilder/builder.go (1)
76-102: Consider adding a nil-guard or documenting the requirement forfio.The
fiofield comment states "must be set before AddFile* calls", but if a caller forgets to callWithFileIO(), the code will panic with a nil pointer dereference inreadFilewhenfio.Open(path)is called.Options:
- Return an explicit error in
AddFile*methods ifb.fio == nil- Accept current behavior since all call sites in this codebase use
runtime.FileIO()which is guaranteed non-nilGiven that retrieved learnings confirm
runtime.FileIO()is always non-nil in this package's execution context, this is acceptable. However, ifemlbuilderis intended for use outside this context, a defensive check would be safer.🛡️ Optional: Add explicit nil check for clearer error
func readFile(fio fileio.FileIO, path string) ([]byte, error) { + if fio == nil { + return nil, fmt.Errorf("attachment %q: FileIO not configured (call WithFileIO before AddFile*)", path) + } f, err := fio.Open(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/emlbuilder/builder.go` around lines 76 - 102, The Builder's fio field can be nil and will cause a panic when readFile calls fio.Open(path); add a defensive nil-guard: in each public AddFile* method (and/or readFile) check if b.fio == nil and return a clear error (or set b.err) instead of dereferencing, or alternately update the Builder docs/comment near fio/WithFileIO to explicitly state WithFileIO must be called and that fio is required; locate fio, WithFileIO, readFile and the AddFile* methods to implement the chosen fix so callers get an explicit error rather than a nil pointer panic.
🤖 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 497-504: When reading attachments/inlines using snapshot.FIO.Open
followed by io.ReadAll (the block with snapshot.FIO.Open and io.ReadAll),
preserve the path context on errors by wrapping the returned error with a
descriptive message that includes the variable path (e.g., use fmt.Errorf("read
attachment %s: %w", path, err) or errors.Wrapf). Apply the same pattern in the
other occurrence (the block around lines 619-626) and ensure both the Open error
and the ReadAll error include the path so callers see which attachment/inline
failed; reference the snapshot.FIO.Open + io.ReadAll sequence and
loadAndAttachInline behavior for consistency.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 266-275: The io.ReadAll error in loadPatchFile should preserve the
same "--patch-file" context as the Open error; replace the raw return of err
after io.ReadAll with a wrapped error like fmt.Errorf("--patch-file %q: %w",
path, err) so callers still see the flag/path in failure messages. Update the
error return in loadPatchFile (around the io.ReadAll handling) to wrap the read
error using the same format string and include the path variable.
---
Nitpick comments:
In `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 76-102: The Builder's fio field can be nil and will cause a panic
when readFile calls fio.Open(path); add a defensive nil-guard: in each public
AddFile* method (and/or readFile) check if b.fio == nil and return a clear error
(or set b.err) instead of dereferencing, or alternately update the Builder
docs/comment near fio/WithFileIO to explicitly state WithFileIO must be called
and that fio is required; locate fio, WithFileIO, readFile and the AddFile*
methods to implement the chosen fix so callers get an explicit error rather than
a nil pointer panic.
🪄 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: 0d253e5b-5926-4381-bdf0-4279e06c4f34
📒 Files selected for processing (14)
shortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_attachment_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/emlbuilder/builder_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/mail/draft/patch.go (1)
497-505:⚠️ Potential issue | 🟡 MinorPreserve path context for
Open/ReadAllerrors in attachment/inline updates.These branches still return raw I/O errors, which can hide which input path failed. Please wrap both
Openandio.ReadAllerrors with the file path, consistent with the surrounding contextual errors.🛠️ Suggested patch
func addAttachment(dctx *DraftCtx, snapshot *DraftSnapshot, path string) error { @@ f, err := dctx.FIO.Open(path) if err != nil { - return err + return fmt.Errorf("attachment %q: %w", path, err) } defer f.Close() content, err := io.ReadAll(f) if err != nil { - return err + return fmt.Errorf("attachment %q: %w", path, err) } @@ func replaceInline(dctx *DraftCtx, snapshot *DraftSnapshot, partID, path, cid, fileName, contentType string) error { @@ f, err := dctx.FIO.Open(path) if err != nil { - return err + return fmt.Errorf("inline image %q: %w", path, err) } defer f.Close() content, err := io.ReadAll(f) if err != nil { - return err + return fmt.Errorf("inline image %q: %w", path, err) }Also applies to: 619-627
🤖 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 497 - 505, The Open and ReadAll error returns in the attachment/inline update code should include the file path for context; locate the calls to dctx.FIO.Open(path) and io.ReadAll(f) and change their error returns to wrap the underlying error with the path (e.g., using fmt.Errorf or errors.Wrapf) so the returned error reads like "open/read <path>: <original error>" and do the same for the analogous block around the later occurrence (the block at the other reported lines handling attachments).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shortcuts/mail/draft/patch.go`:
- Around line 497-505: The Open and ReadAll error returns in the
attachment/inline update code should include the file path for context; locate
the calls to dctx.FIO.Open(path) and io.ReadAll(f) and change their error
returns to wrap the underlying error with the path (e.g., using fmt.Errorf or
errors.Wrapf) so the returned error reads like "open/read <path>: <original
error>" and do the same for the analogous block around the later occurrence (the
block at the other reported lines handling attachments).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3456c035-b4c4-4dd6-b2f5-af62fc277c1f
📒 Files selected for processing (12)
shortcuts/mail/draft/acceptance_test.goshortcuts/mail/draft/model.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_attachment_test.goshortcuts/mail/draft/patch_body_test.goshortcuts/mail/draft/patch_header_test.goshortcuts/mail/draft/patch_inline_resolve_test.goshortcuts/mail/draft/patch_recipient_test.goshortcuts/mail/draft/patch_test.goshortcuts/mail/draft/serialize_golden_test.goshortcuts/mail/draft/serialize_test.goshortcuts/mail/mail_draft_edit.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/mail/draft/patch_attachment_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/mail/draft/model.go
- shortcuts/mail/mail_draft_edit.go
Move checkBlockedExtension back before FIO.Stat to match the original SafeInputPath → checkBlockedExtension → Stat order. Also remove unused errors and fileio imports. Change-Id: I42e726be30409d03a16bb7306625732fd103d8b9
2 participants
Summary
vfs.*calls) to useruntime.FileIO()abstractionhelpers.go,emlbuilder/builder.go,draft/patch.go, and all mail command files (mail_draft_create.go,mail_draft_edit.go,mail_forward.go,mail_reply.go,mail_reply_all.go,mail_send.go)DraftCtxfromDraftSnapshotto separate runtime dependencies (FileIO) from pure data modelTest plan
Automated tests
go test ./shortcuts/mail/...passesgo test ./shortcuts/mail/draft/...passesgo test ./shortcuts/mail/emlbuilder/...passesgolangci-lint run ./shortcuts/mail/...clean (6 pre-existing issues, none introduced by this PR)Manual testing (CLI built from branch HEAD: ed1c3ee)
+draft-createwith--attach+draft-createwith inline image (<img src="local.png">)+draft-edit --inspect+draft-edit --patch-filewithadd_attachment(DraftCtx core path)+reply(draft only, no send)+forward+replywith--attach+forwardwith inline image+draft-editpatchset_subject+set_body(non-file ops)Not tested:
+send/--confirm-send(requiresmail:user_mailbox.message:sendscope, not available)Summary by CodeRabbit
Release Notes