feat(task): emit typed error envelopes across the task domain#1231
Merged
Conversation
evandance
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrate shortcuts/task from legacy output/WrapTaskError to typed github.com/larksuite/cli/errs across builders, command handlers, upload flows, tasklist flows, shared helpers, tests, and lint config. ChangesTask Shortcut Error Handling Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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/task
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@173289eb00eb7618943efd0c614f93a7d48b992f🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-task -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: 5
🧹 Nitpick comments (2)
shortcuts/task/tasklist_add_task_test.go (1)
92-111: ⚡ Quick winParse the JSON and assert the per-item failure fields directly.
Substring checks can still pass if
failed_tasks[].codeorfailed_tasks[].hintregress, or if the subtype string appears elsewhere in the payload.Suggested diff
import ( + "encoding/json" "strings" "testing" @@ out := stdout.String() + var payload struct { + FailedTasks []struct { + Guid string `json:"guid"` + Type string `json:"type"` + Code string `json:"code"` + Hint string `json:"hint"` + } `json:"failed_tasks"` + } + if err := json.Unmarshal([]byte(out), &payload); err != nil { + t.Fatalf("expected JSON output, got %v; body=%s", err, out) + } @@ - if !strings.Contains(out, string(errs.SubtypePermissionDenied)) { - t.Errorf("expected typed subtype %q in failed_tasks, got: %s", errs.SubtypePermissionDenied, out) - } - if !strings.Contains(out, string(errs.SubtypeNotFound)) { - t.Errorf("expected typed subtype %q in failed_tasks, got: %s", errs.SubtypeNotFound, out) - } + if len(payload.FailedTasks) != 2 { + t.Fatalf("expected 2 failed tasks, got %#v", payload.FailedTasks) + } + if payload.FailedTasks[0].Type != string(errs.SubtypePermissionDenied) && payload.FailedTasks[1].Type != string(errs.SubtypePermissionDenied) { + t.Errorf("expected a permission-denied failure, got %#v", payload.FailedTasks) + } + if payload.FailedTasks[0].Type != string(errs.SubtypeNotFound) && payload.FailedTasks[1].Type != string(errs.SubtypeNotFound) { + t.Errorf("expected a not-found failure, got %#v", payload.FailedTasks) + } + for _, f := range payload.FailedTasks { + if f.Code == "" || f.Hint == "" { + t.Errorf("expected typed code and hint for %#v", f) + } + }🤖 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/task/tasklist_add_task_test.go` around lines 92 - 111, Parse the stdout JSON in the task list test and assert the per-item failure fields directly instead of using substring checks. Update the assertions around the existing tasklist_add_task_test behavior to inspect the failed_tasks entries by field, verifying the expected code and hint values for each failed task while still checking the successful task entry and ensuring the legacy subtype/type shape does not appear.shortcuts/task/tasklist_create.go (1)
135-137: ⚡ Quick winKeep the JSON decode cause in the per-task typed error.
This path drops
parseErr, so batch failures lose the invalid-response reason while the other tasklist parse paths keep it.Suggested diff
- if tErr == nil { - if json.Unmarshal(tResp.RawBody, &tResult) != nil { - tErr = errs.NewInternalError(errs.SubtypeInvalidResponse, "failed to parse task response") - } - } + if tErr == nil { + if parseErr := json.Unmarshal(tResp.RawBody, &tResult); parseErr != nil { + tErr = errs.NewInternalError(errs.SubtypeInvalidResponse, "failed to parse task response: %v", parseErr) + } + }🤖 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/task/tasklist_create.go` around lines 135 - 137, The error handling in the JSON unmarshalling part of the per-task logic drops the original parse error, losing valuable debugging information. In the code block inside tasklist_create.go where json.Unmarshal is called on tResp.RawBody, capture the error from Unmarshal into a variable (e.g., parseErr) and pass this error as the cause when creating the new errs.NewInternalError. This keeps the original JSON decode cause wrapped inside the typed error for better traceability.
🤖 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/task/task_assign.go`:
- Around line 37-39: The Validate function currently only rejects completely
empty --add/--remove but allows comma/whitespace-only values; before accepting
flags, parse each of runtime.Str("add") and runtime.Str("remove") using the same
trimming/splitting/filtering logic as buildMembersBody (or call
buildMembersBody) and if the resulting member slice is empty while the original
flag was non-empty, return errs.NewValidationError(errs.SubtypeInvalidArgument,
"must specify either --add or --remove") (or a similar non-empty-members
message) to reject comma/whitespace-only inputs.
In `@shortcuts/task/task_followers.go`:
- Around line 37-39: The Validate function should reject comma/whitespace-only
follower lists: after checking runtime.Str("add") / runtime.Str("remove")
non-empty, run the same parsing/cleanup used by buildFollowersBody (split on
commas, trim whitespace, filter out empty entries) for each of the provided
flags and if the resultant slice is empty treat it as invalid and return a
validation error; update Validate (the Validate: func(ctx context.Context,
runtime *common.RuntimeContext) error block) to call that parsing logic (or call
buildFollowersBody if available) for "add" and "remove" and return
errs.NewValidationError(errs.SubtypeInvalidArgument, "must specify at least one
non-empty follower in --add/--remove") when a flag is present but yields no
non-blank followers.
In `@shortcuts/task/task_upload_attachment.go`:
- Around line 96-99: The Stat error path uses a misleading literal "file not
found" message; update the call site where fio.Stat(filePath) error is handled
to use a neutral, accurate prefix (e.g., "stat failed" or "unable to stat file")
when calling taskInputStatError(err, ...), and ensure the original err is
preserved so the real filesystem error (permission denied, etc.) is included in
the returned/enveloped message; change only the message string passed to
taskInputStatError at the fio.Stat error branch.
- Around line 92-95: The check that treats a nil runtime.FileIO() as an invalid
argument should be changed to an internal failure: in task_upload_attachment.go,
locate the runtime.FileIO() call and the branch that returns
errs.NewValidationError(errs.SubtypeInvalidArgument, ...), and instead return
the internal error subtype (e.g., errs.SubtypeInternal) or an appropriate
internal error constructor so that a missing FileIO provider (variable fio ==
nil) is classified as an internal/runtime wiring failure rather than a user
validation error.
In `@shortcuts/task/task_util.go`:
- Around line 160-165: The branch that handles !hasCode must not delegate to
common.HandleApiResult (which reintroduces legacy output errors); instead,
remove the call to common.HandleApiResult and return a task-domain typed
response/error for the malformed-response case using the local types: inspect
result and action, build and return the typed envelope (e.g., a nil data + a
typed "invalid/malformed response" error value or an ApiResult with an explicit
error Code) so the task domain contract remains enforced; update the !hasCode
branch in task_util.go (replace the common.HandleApiResult call) to construct
and return that typed error/envelope.
---
Nitpick comments:
In `@shortcuts/task/tasklist_add_task_test.go`:
- Around line 92-111: Parse the stdout JSON in the task list test and assert the
per-item failure fields directly instead of using substring checks. Update the
assertions around the existing tasklist_add_task_test behavior to inspect the
failed_tasks entries by field, verifying the expected code and hint values for
each failed task while still checking the successful task entry and ensuring the
legacy subtype/type shape does not appear.
In `@shortcuts/task/tasklist_create.go`:
- Around line 135-137: The error handling in the JSON unmarshalling part of the
per-task logic drops the original parse error, losing valuable debugging
information. In the code block inside tasklist_create.go where json.Unmarshal is
called on tResp.RawBody, capture the error from Unmarshal into a variable (e.g.,
parseErr) and pass this error as the cause when creating the new
errs.NewInternalError. This keeps the original JSON decode cause wrapped inside
the typed error for better traceability.
🪄 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: 5f007437-6664-4a1a-add4-20b7f4cdabfe
📒 Files selected for processing (27)
.golangci.ymlshortcuts/task/shortcuts.goshortcuts/task/task_assign.goshortcuts/task/task_body_test.goshortcuts/task/task_comment.goshortcuts/task/task_complete.goshortcuts/task/task_errors.goshortcuts/task/task_followers.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_related_tasks.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_reminder.goshortcuts/task/task_reopen.goshortcuts/task/task_search.goshortcuts/task/task_set_ancestor.goshortcuts/task/task_subscribe_event.goshortcuts/task/task_tasklist_search.goshortcuts/task/task_update.goshortcuts/task/task_upload_attachment.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util.goshortcuts/task/task_util_test.goshortcuts/task/tasklist_add_task.goshortcuts/task/tasklist_add_task_test.goshortcuts/task/tasklist_create.goshortcuts/task/tasklist_members.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1231 +/- ##
==========================================
+ Coverage 70.46% 70.97% +0.51%
==========================================
Files 679 680 +1
Lines 65611 65318 -293
==========================================
+ Hits 46231 46362 +131
+ Misses 15726 15316 -410
+ Partials 3654 3640 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
evandance
force-pushed
the
feat/errs-migrate-task
branch
from
360567e to
2e0369f
Compare
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 (3)
shortcuts/task/shortcuts.go (1)
183-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDry-run still hides these validation failures behind an exit-0 payload.
buildTaskCreateBodynow produces typed validation errors, butDryRunconverts them into{"error": ...}instead of letting the command fail through the validation path. That makes--data,--due, and missing--summarybehave differently in dry-run versus normal execution, and it loses the exit-2 typed envelope this migration is aiming for. Move these checks intoValidate(or otherwise surface the typed error beforeDryRunruns).Based on learnings: in
larksuite/cliE2E dry-run tests, validation failures from theValidatecallback must exit with code 2 and be asserted viaresult.Stdout + result.Stderr, while DryRun-callback errors stay exit 0.🤖 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/task/shortcuts.go` around lines 183 - 187, The DryRun implementation in DryRun: func(ctx context.Context, runtime *common.RuntimeContext) currently calls buildTaskCreateBody and swallows typed validation errors by returning common.NewDryRunAPI().Set("error", err.Error()); instead, move the input validation logic that buildTaskCreateBody now performs into the command's Validate callback (or ensure Validate calls the same validation helpers) so validation failures are produced before DryRun runs and surface as typed exit-2 validation errors; update DryRun to assume buildTaskCreateBody never returns validation errors (remove the Set("error", ...) conversion) and only handle non-validation runtime errors there, keeping function names DryRun, buildTaskCreateBody and Validate as the hooks to modify.shortcuts/task/task_util.go (1)
35-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep overflowed relative offsets on the validation path.
A huge value like
+999999999999999999hmatches the regex but makesstrconv.Atoifail here, and returning that raw error downgrades user input into an untyped internal failure. Map this branch to the same typed validation error shape as the format mismatch path.Suggested fix
amount, err := strconv.Atoi(amountStr) if err != nil { - return time.Time{}, err + return time.Time{}, errs.NewValidationError( + errs.SubtypeInvalidArgument, + "invalid relative time format: %s", + s, + ) }🤖 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/task/task_util.go` around lines 35 - 37, The atoi call at "amount, err := strconv.Atoi(amountStr)" can fail for huge offsets and currently returns the raw strconv error; change this branch to return the same typed validation error used for the format-mismatch path (the regex/format validation branch) so overflowed relative offsets are treated as a user validation error rather than an internal error. Locate the format-mismatch error construction in this file and mirror its error type/message/shape when handling the strconv.Atoi error (instead of returning err) so callers receive a consistent validation error for bad relative-offset input.shortcuts/task/tasklist_create.go (1)
97-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't swallow worker panics as silent success.
This
recover()only logs to stderr and exits the goroutine. It never records a failed item, so the newlen(failedTasks) > 0gate at Line 173 can still report overall success even though one requested task creation blew up. Please convert the recovered panic into a failed entry (or propagate a typed internal error afterwg.Wait) instead of only printing it.🤖 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/task/tasklist_create.go` around lines 97 - 103, The defer inside the anonymous goroutine currently calls recover() and only prints to stderr, which swallows the failure; change that defer (inside the goroutine defined at go func(idx int, tDef map[string]interface{})) to capture the recovered value and record it as a failedTasks entry (or send a typed error on the existing error/failure channel) before calling wg.Done so the outer logic that checks len(failedTasks) sees the failure; ensure you synchronize access to failedTasks (use the same mutex used elsewhere or create a failure channel and append after wg.Wait), include the panic value and contextual info (idx and tDef identifier) in the recorded error, and keep the existing stderr logging as supplemental.
♻️ Duplicate comments (1)
shortcuts/task/task_util.go (1)
107-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep malformed task responses typed instead of falling back to the legacy helper.
At this point
erris already nil, so!hasCodeis a malformed-response case, not a transport case. Delegating tocommon.HandleApiResultreintroduces the legacyoutputpath into a migrated package and breaks the typed-envelope contract the PR is trying to enforce.Suggested fix
if !hasCode { - // No code field (e.g. network/transport error): delegate to the shared - // helper. common.HandleApiResult still emits legacy errors; migrating the - // shared helper layer is out of scope for the task domain. - data, err := common.HandleApiResult(result, err, action) - return data, err + return nil, errs.NewInternalError( + errs.SubtypeInvalidResponse, + "%s: missing code in task API response", + action, + ) }🤖 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/task/task_util.go` around lines 107 - 112, The branch that checks !hasCode currently delegates to common.HandleApiResult, which reintroduces legacy output handling; instead, detect the malformed-response case (err is nil and !hasCode) and return a typed malformed-response error from this package (do not call common.HandleApiResult). Update the code inside the !hasCode block to construct and return the package's specific error type (e.g., ErrMalformedTaskResponse or a new typed error) along with a nil data value, including contextual info from result and action to aid debugging, so callers keep the typed-envelope contract.
🤖 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/task/task_util.go`:
- Around line 115-116: The code currently discards the error from
util.ToFloat64(codeVal) causing non-numeric "code" values to be treated as 0;
change the logic in the function that reads codeVal (the block that assigns
code, err := util.ToFloat64(codeVal)) to check the conversion error and treat a
failed conversion as an invalid response by returning SubtypeInvalidResponse (or
the function's equivalent invalid response path) before converting to larkCode
and branching; specifically, stop ignoring the error from util.ToFloat64, use
the error to detect bad payloads, and only set larkCode := int(code) if
conversion succeeded so malformed values like {"code":"oops"} are rejected.
In `@shortcuts/task/tasklist_create.go`:
- Around line 173-175: The partial-failure branch in tasklist_create.go returns
via runtime.OutPartialFailure when len(failedTasks) > 0, which skips the
subsequent runtime.OutFormat(..., writer) human-readable output; change the
control flow so you call runtime.OutFormat(outData, writer) (the same formatter
used for the success path) before invoking runtime.OutPartialFailure(outData,
nil) so the formatted text is emitted and then the partial-failure exit signal
is returned; reference the failedTasks check, outData, writer, runtime.OutFormat
and runtime.OutPartialFailure (and the ctx.emit behavior in
shortcuts/common/runner.go) to locate and update the code.
---
Outside diff comments:
In `@shortcuts/task/shortcuts.go`:
- Around line 183-187: The DryRun implementation in DryRun: func(ctx
context.Context, runtime *common.RuntimeContext) currently calls
buildTaskCreateBody and swallows typed validation errors by returning
common.NewDryRunAPI().Set("error", err.Error()); instead, move the input
validation logic that buildTaskCreateBody now performs into the command's
Validate callback (or ensure Validate calls the same validation helpers) so
validation failures are produced before DryRun runs and surface as typed exit-2
validation errors; update DryRun to assume buildTaskCreateBody never returns
validation errors (remove the Set("error", ...) conversion) and only handle
non-validation runtime errors there, keeping function names DryRun,
buildTaskCreateBody and Validate as the hooks to modify.
In `@shortcuts/task/task_util.go`:
- Around line 35-37: The atoi call at "amount, err := strconv.Atoi(amountStr)"
can fail for huge offsets and currently returns the raw strconv error; change
this branch to return the same typed validation error used for the
format-mismatch path (the regex/format validation branch) so overflowed relative
offsets are treated as a user validation error rather than an internal error.
Locate the format-mismatch error construction in this file and mirror its error
type/message/shape when handling the strconv.Atoi error (instead of returning
err) so callers receive a consistent validation error for bad relative-offset
input.
In `@shortcuts/task/tasklist_create.go`:
- Around line 97-103: The defer inside the anonymous goroutine currently calls
recover() and only prints to stderr, which swallows the failure; change that
defer (inside the goroutine defined at go func(idx int, tDef
map[string]interface{})) to capture the recovered value and record it as a
failedTasks entry (or send a typed error on the existing error/failure channel)
before calling wg.Done so the outer logic that checks len(failedTasks) sees the
failure; ensure you synchronize access to failedTasks (use the same mutex used
elsewhere or create a failure channel and append after wg.Wait), include the
panic value and contextual info (idx and tDef identifier) in the recorded error,
and keep the existing stderr logging as supplemental.
---
Duplicate comments:
In `@shortcuts/task/task_util.go`:
- Around line 107-112: The branch that checks !hasCode currently delegates to
common.HandleApiResult, which reintroduces legacy output handling; instead,
detect the malformed-response case (err is nil and !hasCode) and return a typed
malformed-response error from this package (do not call common.HandleApiResult).
Update the code inside the !hasCode block to construct and return the package's
specific error type (e.g., ErrMalformedTaskResponse or a new typed error) along
with a nil data value, including contextual info from result and action to aid
debugging, so callers keep the typed-envelope contract.
🪄 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: 76cf4480-f81b-4d64-b4c5-e1a00fd149d9
📒 Files selected for processing (27)
.golangci.ymlshortcuts/task/shortcuts.goshortcuts/task/task_assign.goshortcuts/task/task_body_test.goshortcuts/task/task_comment.goshortcuts/task/task_complete.goshortcuts/task/task_errors.goshortcuts/task/task_followers.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_related_tasks.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_reminder.goshortcuts/task/task_reopen.goshortcuts/task/task_search.goshortcuts/task/task_set_ancestor.goshortcuts/task/task_subscribe_event.goshortcuts/task/task_tasklist_search.goshortcuts/task/task_update.goshortcuts/task/task_upload_attachment.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util.goshortcuts/task/task_util_test.goshortcuts/task/tasklist_add_task.goshortcuts/task/tasklist_add_task_test.goshortcuts/task/tasklist_create.goshortcuts/task/tasklist_members.go
🚧 Files skipped from review as they are similar to previous changes (20)
- shortcuts/task/task_get_related_tasks.go
- .golangci.yml
- shortcuts/task/task_reopen.go
- shortcuts/task/task_set_ancestor.go
- shortcuts/task/task_complete.go
- shortcuts/task/task_assign.go
- shortcuts/task/task_upload_attachment_test.go
- shortcuts/task/task_subscribe_event.go
- shortcuts/task/task_query_helpers.go
- shortcuts/task/task_get_my_tasks.go
- shortcuts/task/task_tasklist_search.go
- shortcuts/task/task_query_helpers_test.go
- shortcuts/task/task_search.go
- shortcuts/task/task_upload_attachment.go
- shortcuts/task/task_errors.go
- shortcuts/task/tasklist_members.go
- shortcuts/task/task_update.go
- shortcuts/task/task_reminder.go
- shortcuts/task/task_followers.go
- shortcuts/task/task_body_test.go
evandance
force-pushed
the
feat/errs-migrate-task
branch
from
2e0369f to
1463d40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/task/task_reminder.go (1)
68-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse
--setbefore the initial task GET.A malformed
--setvalue still reachesruntime.DoAPI()before the localstrconv.Atoivalidation runs, so bad client input can currently surface auth/network/API errors instead of the typed validation error this migration is supposed to guarantee. Move the--setparsing above the first GET and reuse the parsed minutes later in the add-reminders branch.Suggested direction
Execute: func(ctx context.Context, runtime *common.RuntimeContext) error { taskId := url.PathEscape(runtime.Str("task-id")) queryParams := make(larkcore.QueryParams) queryParams.Set("user_id_type", "open_id") + var minutes int + if setStr := runtime.Str("set"); setStr != "" { + var parseErr error + switch { + case strings.HasSuffix(setStr, "m"): + minutes, parseErr = strconv.Atoi(strings.TrimSuffix(setStr, "m")) + case strings.HasSuffix(setStr, "h"): + var h int + h, parseErr = strconv.Atoi(strings.TrimSuffix(setStr, "h")) + if parseErr == nil { + minutes = h * 60 + } + case strings.HasSuffix(setStr, "d"): + var d int + d, parseErr = strconv.Atoi(strings.TrimSuffix(setStr, "d")) + if parseErr == nil { + minutes = d * 24 * 60 + } + default: + minutes, parseErr = strconv.Atoi(setStr) + } + if parseErr != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --set value %q: %v", setStr, parseErr) + } + } + // First, get the task to find existing reminders getResp, err := runtime.DoAPI(&larkcore.ApiReq{ HttpMethod: http.MethodGet, ApiPath: "/open-apis/task/v2/tasks/" + taskId, QueryParams: queryParams, }) @@ - } else if setStr := runtime.Str("set"); setStr != "" { - // Parse relative time string (e.g. 15m, 1h, 1d, or plain 30) - var minutes int - var parseErr error - ... - if parseErr != nil { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", parseErr) - } + } else if runtime.Str("set") != "" { // If any reminders exist, remove them first🤖 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/task/task_reminder.go` around lines 68 - 160, Move the parsing/validation of runtime.Str("set") to the top of Execute so malformed values are handled before any API calls: compute and validate the minutes integer (currently done with variables minutes and parseErr and strconv.Atoi logic) right after reading setStr and before the initial runtime.DoAPI GET, returning the validation error if parseErr != nil; then remove the duplicate parsing in the add-reminders branch and reuse the previously computed minutes when building the add-reminders request payload. Ensure the existing flow for remove and no-set cases remains unchanged and reference the Execute function, setStr/runtime.Str("set"), minutes, parseErr, and the add-reminders branch where minutes is used.
♻️ Duplicate comments (1)
shortcuts/task/task_util.go (1)
113-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject fractional API
codevalues before truncating.
util.ToFloat64accepts numeric payloads like0.5, andint(code)then truncates that to0, which makes a malformed response look like success. This path should treat any non-integer numericcodeaserrs.SubtypeInvalidResponsebefore branching onlarkCode.Suggested fix
+// add `math` to the imports above code, ok := util.ToFloat64(codeVal) if !ok { return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "%s: malformed response (non-numeric code %v)", action, codeVal) } + if code != math.Trunc(code) { + return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "%s: malformed response (non-integer code %v)", action, codeVal) + } larkCode := int(code)🤖 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/task/task_util.go` around lines 113 - 117, The code currently uses util.ToFloat64 to parse codeVal and then casts to int, which silently truncates fractional values; update the validation in the parsing block (around util.ToFloat64, codeVal, larkCode) to reject any numeric value that is not an exact integer: after obtaining code (float64) and ok, check that code == math.Trunc(code) (or use math.Modf) and if it is fractional return errs.NewInternalError(errs.SubtypeInvalidResponse, "... malformed response (non-integer code %v)", codeVal); only then safely convert to int and proceed.
🤖 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/task/tasklist_create_test.go`:
- Around line 101-123: The test currently registers an HTTP POST stub before
invoking runMountedTaskShortcut so a client-side invalid --data JSON error can
occur after the POST; change the test so the invalid JSON is validated before
any remote call: in TestCreateTasklist_InvalidDataJSON remove or defer
reg.Register(...) and instead call runMountedTaskShortcut with args and assert
it returns the typed *errs.ValidationError; alternatively keep the stub but
assert reg received zero POSTs after runMountedTaskShortcut to ensure no request
was sent; reference CreateTasklist, runMountedTaskShortcut, reg.Register, and
TestCreateTasklist_InvalidDataJSON when making the change.
---
Outside diff comments:
In `@shortcuts/task/task_reminder.go`:
- Around line 68-160: Move the parsing/validation of runtime.Str("set") to the
top of Execute so malformed values are handled before any API calls: compute and
validate the minutes integer (currently done with variables minutes and parseErr
and strconv.Atoi logic) right after reading setStr and before the initial
runtime.DoAPI GET, returning the validation error if parseErr != nil; then
remove the duplicate parsing in the add-reminders branch and reuse the
previously computed minutes when building the add-reminders request payload.
Ensure the existing flow for remove and no-set cases remains unchanged and
reference the Execute function, setStr/runtime.Str("set"), minutes, parseErr,
and the add-reminders branch where minutes is used.
---
Duplicate comments:
In `@shortcuts/task/task_util.go`:
- Around line 113-117: The code currently uses util.ToFloat64 to parse codeVal
and then casts to int, which silently truncates fractional values; update the
validation in the parsing block (around util.ToFloat64, codeVal, larkCode) to
reject any numeric value that is not an exact integer: after obtaining code
(float64) and ok, check that code == math.Trunc(code) (or use math.Modf) and if
it is fractional return errs.NewInternalError(errs.SubtypeInvalidResponse, "...
malformed response (non-integer code %v)", codeVal); only then safely convert to
int and proceed.
🪄 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: eb37c3b3-a13a-402e-bc2c-7e34a15b023f
📒 Files selected for processing (35)
.golangci.ymlshortcuts/task/shortcuts.goshortcuts/task/task_assign.goshortcuts/task/task_assign_test.goshortcuts/task/task_body_test.goshortcuts/task/task_comment.goshortcuts/task/task_complete.goshortcuts/task/task_errors.goshortcuts/task/task_errors_test.goshortcuts/task/task_followers.goshortcuts/task/task_followers_test.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_my_tasks_test.goshortcuts/task/task_get_related_tasks.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_reminder.goshortcuts/task/task_reminder_test.goshortcuts/task/task_reopen.goshortcuts/task/task_search.goshortcuts/task/task_search_test.goshortcuts/task/task_set_ancestor.goshortcuts/task/task_subscribe_event.goshortcuts/task/task_tasklist_search.goshortcuts/task/task_update.goshortcuts/task/task_upload_attachment.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util.goshortcuts/task/task_util_test.goshortcuts/task/tasklist_add_task.goshortcuts/task/tasklist_add_task_test.goshortcuts/task/tasklist_create.goshortcuts/task/tasklist_create_test.goshortcuts/task/tasklist_members.goshortcuts/task/tasklist_members_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- shortcuts/task/task_get_related_tasks.go
- shortcuts/task/task_set_ancestor.go
- shortcuts/task/task_comment.go
- shortcuts/task/task_complete.go
- shortcuts/task/task_assign.go
- shortcuts/task/task_query_helpers_test.go
- shortcuts/task/task_body_test.go
- shortcuts/task/task_subscribe_event.go
- shortcuts/task/task_followers.go
- shortcuts/task/tasklist_add_task.go
- shortcuts/task/task_update.go
- shortcuts/task/task_tasklist_search.go
- shortcuts/task/task_query_helpers.go
- shortcuts/task/task_errors.go
- shortcuts/task/task_get_my_tasks.go
- shortcuts/task/tasklist_create.go
- shortcuts/task/tasklist_add_task_test.go
- shortcuts/task/tasklist_members.go
- shortcuts/task/shortcuts.go
- .golangci.yml
- shortcuts/task/task_search.go
evandance
force-pushed
the
feat/errs-migrate-task
branch
from
1463d40 to
c42cfe9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/task/tasklist_create.go (1)
143-145:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffPartial failures still bypass the text formatter.
Returning here skips the
runtime.OutFormat(..., writer)block at lines 147-174, so the human-readable summary (including the failed tasks list) is never rendered in text mode when any sub-task fails. Consider emitting the formatted output first, then returning the partial-failure signal:if len(failedTasks) > 0 { // Emit formatted output first runtime.OutFormat(outData, nil, func(w io.Writer) { fmt.Fprintf(w, "✅ Tasklist created successfully!\n") fmt.Fprintf(w, "Tasklist Name: %s\n", tasklistName) fmt.Fprintf(w, "Tasklist ID: %s\n", tasklistGuid) if tasklistUrl != "" { fmt.Fprintf(w, "Tasklist URL: %s\n", tasklistUrl) } if len(tasks) > 0 { fmt.Fprintln(w, strings.Repeat("-", 20)) fmt.Fprintf(w, "Tasks created: %d/%d\n", len(createdTasks), len(tasks)) for _, t := range createdTasks { guid, _ := t["guid"].(string) urlVal, _ := t["url"].(string) fmt.Fprintf(w, " - ID: %s", guid) if urlVal != "" { fmt.Fprintf(w, ", URL: %s", urlVal) } fmt.Fprintln(w) } if len(failedTasks) > 0 { fmt.Fprintf(w, "\nFailed tasks:\n") for _, f := range failedTasks { fmt.Fprintf(w, " - %s\n", f) } } } }) // Then signal partial failure return runtime.OutPartialFailure(outData, 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/task/tasklist_create.go` around lines 143 - 145, The partial-failure early return (when len(failedTasks) > 0) bypasses the human-readable text formatter; change the flow in the block that checks failedTasks so you call runtime.OutFormat(outData, nil, func(w io.Writer) { ... }) to emit the formatted summary (include tasklistName, tasklistGuid, tasklistUrl, count and details from createdTasks, and list failedTasks) before returning; after emitting formatted output, return runtime.OutPartialFailure(outData, nil) so the text output is shown and the partial-failure signal is still returned.
🤖 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.
Duplicate comments:
In `@shortcuts/task/tasklist_create.go`:
- Around line 143-145: The partial-failure early return (when len(failedTasks) >
0) bypasses the human-readable text formatter; change the flow in the block that
checks failedTasks so you call runtime.OutFormat(outData, nil, func(w io.Writer)
{ ... }) to emit the formatted summary (include tasklistName, tasklistGuid,
tasklistUrl, count and details from createdTasks, and list failedTasks) before
returning; after emitting formatted output, return
runtime.OutPartialFailure(outData, nil) so the text output is shown and the
partial-failure signal is still returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0374f6b-b76d-49f8-89e1-2e6d8e0203cb
📒 Files selected for processing (37)
.golangci.ymlshortcuts/task/shortcuts.goshortcuts/task/task_assign.goshortcuts/task/task_assign_test.goshortcuts/task/task_body_test.goshortcuts/task/task_comment.goshortcuts/task/task_complete.goshortcuts/task/task_errors.goshortcuts/task/task_errors_test.goshortcuts/task/task_followers.goshortcuts/task/task_followers_test.goshortcuts/task/task_get_my_tasks.goshortcuts/task/task_get_my_tasks_test.goshortcuts/task/task_get_related_tasks.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_reminder.goshortcuts/task/task_reminder_test.goshortcuts/task/task_reopen.goshortcuts/task/task_search.goshortcuts/task/task_search_test.goshortcuts/task/task_set_ancestor.goshortcuts/task/task_subscribe_event.goshortcuts/task/task_subscribe_event_test.goshortcuts/task/task_tasklist_search.goshortcuts/task/task_tasklist_search_test.goshortcuts/task/task_update.goshortcuts/task/task_upload_attachment.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util.goshortcuts/task/task_util_test.goshortcuts/task/tasklist_add_task.goshortcuts/task/tasklist_add_task_test.goshortcuts/task/tasklist_create.goshortcuts/task/tasklist_create_test.goshortcuts/task/tasklist_members.goshortcuts/task/tasklist_members_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- shortcuts/task/task_complete.go
- shortcuts/task/task_update.go
- shortcuts/task/task_errors.go
- shortcuts/task/task_upload_attachment.go
- shortcuts/task/tasklist_add_task_test.go
- shortcuts/task/task_errors_test.go
- shortcuts/task/task_assign.go
- shortcuts/task/shortcuts.go
- shortcuts/task/task_query_helpers.go
- .golangci.yml
- shortcuts/task/task_upload_attachment_test.go
- shortcuts/task/task_tasklist_search.go
- shortcuts/task/task_reopen.go
- shortcuts/task/task_util_test.go
- shortcuts/task/tasklist_add_task.go
- shortcuts/task/task_search_test.go
- shortcuts/task/tasklist_create_test.go
- shortcuts/task/task_util.go
- shortcuts/task/task_body_test.go
- shortcuts/task/task_query_helpers_test.go
- shortcuts/task/task_reminder.go
evandance
force-pushed
the
feat/errs-migrate-task
branch
6 times, most recently
from
122ca57 to
5ee283f
Compare
tengchengwei
previously approved these changes
Jun 5, 2026
evandance
force-pushed
the
feat/errs-migrate-task
branch
2 times, most recently
from
a28b974 to
c5cd8b9
Compare
Task commands now return structured, typed errors instead of the legacy exit-code envelope: every failure carries a stable category, subtype, and recovery hint, so callers can branch on the error class instead of parsing messages. Exit codes derive from the error category — input validation exits 2, a permission denial exits 3, other API errors exit 1. Batch operations (adding tasks to a tasklist, creating a tasklist with tasks) now report partial failure honestly: the per-item successes and failures stay on stdout and the command exits non-zero instead of masking failures as a success.
evandance
force-pushed
the
feat/errs-migrate-task
branch
from
c5cd8b9 to
173289e
Compare
zero-my
approved these changes
Jun 5, 2026
8 tasks
SunPeiYang996
pushed a commit
that referenced
this pull request
Jun 8, 2026
Task commands now return structured, typed errors instead of the legacy exit-code envelope: every failure carries a stable category, subtype, and recovery hint, so callers can branch on the error class instead of parsing messages. Exit codes derive from the error category — input validation exits 2, a permission denial exits 3, other API errors exit 1. Batch operations (adding tasks to a tasklist, creating a tasklist with tasks) now report partial failure honestly: the per-item successes and failures stay on stdout and the command exits non-zero instead of masking failures as a success.
This was referenced Jun 9, 2026
6 tasks
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
3 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
Migrate the task domain's error output from the legacy exit-code envelope to typed
errs.*errors. Every task command failure now carries a stable category, subtype, and recovery hint on a structured stderr envelope, so callers (AI agents, scripts) can branch on the error class instead of parsing free-text messages.Changes
output.Err*/output.Exit*/&output.ExitError{}producers inshortcuts/task/with typederrs.*builders; add local task helpers for file-input and transport errors.internal/errclass/codemeta_task.gowhile preserving per-code recovery hints; the upload-attachment stream path now passes runtime classify context too.ok:false+ non-zero exit while preserving per-item successes/failures; pretty output keeps the tasklist-create human summary.shortcuts/task/via golangci-lint and errscontract guards.Exit codes are derived from the error category: input validation → 2, permission denied → 3, other API errors → 1, local internal failures → 5, partial batch failures → 1.
Test Plan
make unit-testgo vet ./...gofmt -l .go mod tidyin root andlint/go test -C lint ./...go run -C lint . ..go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainRelated Issues
Part of the ongoing effort to migrate business domains to the typed
errs.*error contract (task domain).Summary by CodeRabbit
Refactor
Tests
Chores