feat: add mail sender service commands - #2081
Conversation
|
|
📝 WalkthroughWalkthroughThe PR introduces typed registry-driven service commands, built-in mail sender metadata, identity-aware request and response handling, structured pagination output, mail sender response projections, and expanded tests for command discovery, pagination, errors, content safety, dry-run output, and file uploads. ChangesService command pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ServiceCommand
participant APIClient
participant OutputEmitter
User->>ServiceCommand: invoke typed service command
ServiceCommand->>APIClient: build and dispatch request
APIClient->>ServiceCommand: return API response
ServiceCommand->>OutputEmitter: classify, transform, and emit output
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![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 (1)
internal/client/response.go (1)
161-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
classifySaveErrhardcodes--outputas the param even for the auto-save (derived filename) path.
classifySaveErris shared betweensaveAndPrint(user-supplied--output, correct) and the "no--output: auto-save with derived filename" branch at line 163-166, where the path comes fromResolveFilename(resp), not the user. A path-validation failure on that internally-derived filename (e.g. an unsafeContent-Dispositionfilename from the server) would incorrectly surface asValidationErroron--output, misleading users who never passed that flag.🔧 Proposed fix
-func classifySaveErr(err error) error { +func classifySaveErr(err error, param string) error { if errors.Is(err, fileio.ErrPathValidation) { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", err).WithParam("--output") + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", err).WithParam(param) } return errs.NewInternalError(errs.SubtypeFileIO, "save response: %v", err).WithCause(err) }Then pass
"--output"fromsaveAndPrint's caller sites and a different identifier (or empty) from the derived-filename auto-save call at line 165.Also applies to: 181-190
🤖 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 `@internal/client/response.go` around lines 161 - 166, Update the shared classifySaveErr call sites so the user-supplied --output identifier is passed only from saveAndPrint, while the no-output auto-save branches using ResolveFilename(resp) pass a distinct internal identifier or empty value. Ensure path-validation errors for derived filenames are not reported as validation failures on --output, including the additional affected branch around lines 181-190.
🧹 Nitpick comments (5)
cmd/service/service.go (2)
566-568: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePath join can produce a double slash.
opts.ServicePath + "/" + method.Pathassumesmethod.Pathnever carries a leading/. Registry-generated metadata that does (or a future overlay) yields/open-apis/mail/v1//user_mailboxes/..., which some gateways reject.strings.TrimPrefix(method.Path, "/")makes it defensive at no cost.🤖 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 `@cmd/service/service.go` around lines 566 - 568, Update the URL construction near method.Params() to remove any leading slash from method.Path before joining it with opts.ServicePath, using strings.TrimPrefix while preserving the existing service path and method path components.
686-690: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider bundling
servicePaginate's parameters.Eleven positional parameters, four of them adjacent strings/writers (
out, errOut, commandPath, schemaPath), make call-site swaps easy and silent. A smallservicePaginateInputstruct (or reusing the existing options-struct style ofclient.ResponseOptions) would make the call at Lines 434-435 self-documenting.🤖 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 `@cmd/service/service.go` around lines 686 - 690, Bundle servicePaginate’s eleven positional arguments into a dedicated input/options struct, grouping the adjacent output writers and path values together. Update servicePaginate and all call sites, including the invocation around the referenced command flow, to pass named fields so argument swaps are prevented while preserving existing pagination behavior.internal/registry/loader.go (1)
39-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEmbedded parse error is discarded silently.
reg, _ := meta.Parse(embeddedMetaJSON)swallows a malformed/absent embedded payload:embeddedVersionstays empty (so the cache-freshness comparison at Line 97 always treats the cache as newer) and the whole command tree silently degrades to zero services with no diagnostic. Keep the error in a package-level var and surface it (stderr warning or a diagnostics accessor next toConfiguredBrand).🤖 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 `@internal/registry/loader.go` around lines 39 - 50, The parse error in parseEmbedded is discarded, leaving malformed embedded metadata indistinguishable from valid empty metadata. Store the error in a package-level variable alongside the embedded metadata state, and expose or report it through the existing diagnostics path near ConfiguredBrand (or stderr) while preserving the current successful parsing behavior.cmd/service/service_test.go (1)
338-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the envelope drill-down assertions.
data["api"].([]interface{})/api[0]panic (rather than fail with a diagnostic) whenever the dry-run envelope shape regresses orapicomes back empty — the exact failure these tests exist to describe. A, okcheck plust.Fatalfwith the raw stdout keeps the message actionable. Same pattern at Lines 312-314 and 1424-1428.🤖 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 `@cmd/service/service_test.go` around lines 338 - 432, The TestMailSenderCommands_DryRunRequestShape assertions should validate the dry-run envelope before indexing it: use comma-ok checks for data["api"] and api[0], and call t.Fatalf with the raw stdout when either shape is invalid or empty. Apply the same guarded drill-down pattern to the corresponding assertions near the other referenced test locations, preserving the existing URL, params, and body checks.cmd/service/service_paginate_test.go (1)
213-257: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
Subtype/ParamalongsideCategoryfor typed metadata coverage.The test verifies cause preservation (
errors.Is) andCategoryviaerrs.ProblemOf, but per the repo's test guideline, error-path tests should also assertSubtypeandParamwhere applicable, not justCategory.✅ Suggested addition
problem, ok := errs.ProblemOf(err) - if !ok || problem.Category != errs.CategoryInternal { - t.Fatalf("servicePaginate() problem = %#v, %v; want internal typed error", problem, ok) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeFileIO { + t.Fatalf("servicePaginate() problem = %#v, %v; want internal fileio typed error", problem, ok) }🤖 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 `@cmd/service/service_paginate_test.go` around lines 213 - 257, Extend TestServicePaginate_StreamingWriteFailureStopsFurtherPages to assert the typed error’s expected Subtype and Param fields alongside Category after errs.ProblemOf(err). Use the repository’s established values for this writer-failure path, while preserving the existing cause-preservation and pagination assertions.Source: Path instructions
🤖 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 `@cmd/build_test.go`:
- Around line 15-20: Update TestBuildWithoutPluginsStillBuildsBuiltinCommands to
call t.Setenv with LARKSUITE_CLI_CONFIG_DIR and t.TempDir() before invoking
Build, ensuring the command-tree construction uses isolated temporary
configuration state.
In `@internal/registry/builtin_mail_sender_overlay.go`:
- Around line 41-47: Update mergeServiceOverlay and its mergeResourceOverlay
path to clone base.Resources before writing overlay entries, and clone each
existing resource’s Methods map before merging methods. Preserve the original
embedded service and resource maps so EmbeddedServicesTyped remains
overlay-free, while retaining existing merge behavior in mergedServices.
---
Outside diff comments:
In `@internal/client/response.go`:
- Around line 161-166: Update the shared classifySaveErr call sites so the
user-supplied --output identifier is passed only from saveAndPrint, while the
no-output auto-save branches using ResolveFilename(resp) pass a distinct
internal identifier or empty value. Ensure path-validation errors for derived
filenames are not reported as validation failures on --output, including the
additional affected branch around lines 181-190.
---
Nitpick comments:
In `@cmd/service/service_paginate_test.go`:
- Around line 213-257: Extend
TestServicePaginate_StreamingWriteFailureStopsFurtherPages to assert the typed
error’s expected Subtype and Param fields alongside Category after
errs.ProblemOf(err). Use the repository’s established values for this
writer-failure path, while preserving the existing cause-preservation and
pagination assertions.
In `@cmd/service/service_test.go`:
- Around line 338-432: The TestMailSenderCommands_DryRunRequestShape assertions
should validate the dry-run envelope before indexing it: use comma-ok checks for
data["api"] and api[0], and call t.Fatalf with the raw stdout when either shape
is invalid or empty. Apply the same guarded drill-down pattern to the
corresponding assertions near the other referenced test locations, preserving
the existing URL, params, and body checks.
In `@cmd/service/service.go`:
- Around line 566-568: Update the URL construction near method.Params() to
remove any leading slash from method.Path before joining it with
opts.ServicePath, using strings.TrimPrefix while preserving the existing service
path and method path components.
- Around line 686-690: Bundle servicePaginate’s eleven positional arguments into
a dedicated input/options struct, grouping the adjacent output writers and path
values together. Update servicePaginate and all call sites, including the
invocation around the referenced command flow, to pass named fields so argument
swaps are prevented while preserving existing pagination behavior.
In `@internal/registry/loader.go`:
- Around line 39-50: The parse error in parseEmbedded is discarded, leaving
malformed embedded metadata indistinguishable from valid empty metadata. Store
the error in a package-level variable alongside the embedded metadata state, and
expose or report it through the existing diagnostics path near ConfiguredBrand
(or stderr) while preserving the current successful parsing behavior.
🪄 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 Plus
Run ID: 08a3bafe-d530-4bcb-ac19-e056420c4466
📒 Files selected for processing (8)
cmd/build_test.gocmd/service/mail_sender_output.gocmd/service/service.gocmd/service/service_paginate_test.gocmd/service/service_test.gointernal/client/response.gointernal/registry/builtin_mail_sender_overlay.gointernal/registry/loader.go
| func TestBuildWithoutPluginsStillBuildsBuiltinCommands(t *testing.T) { | ||
| root := Build(context.Background(), cmdutil.InvocationContext{}, WithoutPlugins()) | ||
|
|
||
| if root == nil { | ||
| t.Fatal("Build returned nil root") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Isolate config state before building the full command tree.
Build walks the real registry/config stack, so without an isolated config dir this test can read (or seed) the developer's/CI's actual configuration and cache, making it order-dependent.
As per coding guidelines, "set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv to isolate configuration state."
🧪 Proposed fix
func TestBuildWithoutPluginsStillBuildsBuiltinCommands(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
root := Build(context.Background(), cmdutil.InvocationContext{}, WithoutPlugins())📝 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.
| func TestBuildWithoutPluginsStillBuildsBuiltinCommands(t *testing.T) { | |
| root := Build(context.Background(), cmdutil.InvocationContext{}, WithoutPlugins()) | |
| if root == nil { | |
| t.Fatal("Build returned nil root") | |
| } | |
| func TestBuildWithoutPluginsStillBuildsBuiltinCommands(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| root := Build(context.Background(), cmdutil.InvocationContext{}, WithoutPlugins()) | |
| if root == nil { | |
| t.Fatal("Build returned nil root") | |
| } |
🤖 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 `@cmd/build_test.go` around lines 15 - 20, Update
TestBuildWithoutPluginsStillBuildsBuiltinCommands to call t.Setenv with
LARKSUITE_CLI_CONFIG_DIR and t.TempDir() before invoking Build, ensuring the
command-tree construction uses isolated temporary configuration state.
Source: Coding guidelines
| if base.Resources == nil { | ||
| base.Resources = map[string]meta.Resource{} | ||
| } | ||
| for name, resource := range overlay.Resources { | ||
| base.Resources[name] = mergeResourceOverlay(base.Resources[name], resource) | ||
| } | ||
| mergedServices[overlay.Name] = base |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Overlay mutates maps shared with the "overlay-free" embedded parse.
loadEmbeddedIntoMerged (internal/registry/loader.go Lines 119-123) copies meta.Service values from embeddedServicesByName into mergedServices; the struct copy is shallow, so base.Resources (and each resource's Methods map) is the same map instance held by embeddedServices. When mail already exists in the embedded registry, mergeServiceOverlay/mergeResourceOverlay write overlay resources/methods straight into it, so EmbeddedServicesTyped() — documented as the overlay-free parse boundary for the schema envelope (loader.go Lines 52-54) — starts reporting overlay content.
Clone the maps before mutating.
🛠️ Sketch: copy-on-write the resource/method maps
- if base.Resources == nil {
- base.Resources = map[string]meta.Resource{}
- }
+ resources := make(map[string]meta.Resource, len(base.Resources)+len(overlay.Resources))
+ for name, r := range base.Resources {
+ resources[name] = r
+ }
+ base.Resources = resources
for name, resource := range overlay.Resources {
base.Resources[name] = mergeResourceOverlay(base.Resources[name], resource)
} func mergeResourceOverlay(base, overlay meta.Resource) meta.Resource {
- if base.Methods == nil {
- base.Methods = map[string]meta.Method{}
- }
+ methods := make(map[string]meta.Method, len(base.Methods)+len(overlay.Methods))
+ for name, m := range base.Methods {
+ methods[name] = m
+ }
+ base.Methods = methods
for name, method := range overlay.Methods {📝 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.
| if base.Resources == nil { | |
| base.Resources = map[string]meta.Resource{} | |
| } | |
| for name, resource := range overlay.Resources { | |
| base.Resources[name] = mergeResourceOverlay(base.Resources[name], resource) | |
| } | |
| mergedServices[overlay.Name] = base | |
| resources := make(map[string]meta.Resource, len(base.Resources)+len(overlay.Resources)) | |
| for name, r := range base.Resources { | |
| resources[name] = r | |
| } | |
| base.Resources = resources | |
| for name, resource := range overlay.Resources { | |
| base.Resources[name] = mergeResourceOverlay(base.Resources[name], resource) | |
| } | |
| mergedServices[overlay.Name] = base |
🤖 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 `@internal/registry/builtin_mail_sender_overlay.go` around lines 41 - 47,
Update mergeServiceOverlay and its mergeResourceOverlay path to clone
base.Resources before writing overlay entries, and clone each existing
resource’s Methods map before merging methods. Preserve the original embedded
service and resource maps so EmbeddedServicesTyped remains overlay-free, while
retaining existing merge behavior in mergedServices.
2 participants
Summary
Review resolution
Meego: https://meego.larkoffice.com/larksuite/story/detail/6930175032
Summary by CodeRabbit
New Features
Bug Fixes