feat(task): emit typed error envelopes across the task domain#1231
Open
evandance wants to merge 1 commit into
Open
feat(task): emit typed error envelopes across the task domain#1231evandance wants to merge 1 commit into
evandance wants to merge 1 commit into
Conversation
evandance
added
enhancement
📝 WalkthroughWalkthroughMigrate shortcuts/task from legacy output/WrapTaskError to typed github.com/larksuite/cli/errs: add shared helpers and taskAPIHints, convert validation/internal/API/network/file errors across command handlers and builders, update partial-failure reporting and tests, and exempt shortcuts/task/ in lint rules. 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 docstrings
🧪 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@2e0369f74b7aeb0587bd49b0946c0df566020261🧩 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
Comment on lines
37
to
+39
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| if runtime.Str("add") == "" && runtime.Str("remove") == "" { | ||
| return WrapTaskError(ErrCodeTaskInvalidParams, "must specify either --add or --remove", "validate followers") | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "must specify either --add or --remove") |
There was a problem hiding this comment.
Reject comma/whitespace-only follower lists in Validate.
buildFollowersBody() strips blank entries, so inputs like --add "," still get through here and end up posting members: []. That should stay a local validation error rather than surfacing as a downstream API failure.
Proposed fix
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
- if runtime.Str("add") == "" && runtime.Str("remove") == "" {
+ hasFollowers := func(raw string) bool {
+ for _, id := range strings.Split(raw, ",") {
+ if strings.TrimSpace(id) != "" {
+ return true
+ }
+ }
+ return false
+ }
+ if !hasFollowers(runtime.Str("add")) && !hasFollowers(runtime.Str("remove")) {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "must specify either --add or --remove")
}
return nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| if runtime.Str("add") == "" && runtime.Str("remove") == "" { | |
| return WrapTaskError(ErrCodeTaskInvalidParams, "must specify either --add or --remove", "validate followers") | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "must specify either --add or --remove") | |
| Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { | |
| hasFollowers := func(raw string) bool { | |
| for _, id := range strings.Split(raw, ",") { | |
| if strings.TrimSpace(id) != "" { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| if !hasFollowers(runtime.Str("add")) && !hasFollowers(runtime.Str("remove")) { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "must specify either --add or --remove") | |
| } | |
| return nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/task/task_followers.go` around lines 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.
Comment on lines
92
to
95
| fio := runtime.FileIO() | ||
| if fio == nil { | ||
| return output.ErrValidation("file operations require a FileIO provider") | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "file operations require a FileIO provider") | ||
| } |
There was a problem hiding this comment.
Classify a missing FileIO provider as an internal failure.
A nil runtime.FileIO() means the runtime was wired incorrectly, not that the user passed a bad --file. Returning SubtypeInvalidArgument here sends exit 2, but the PR contract says local internal failures should stay on exit 5.
Suggested fix
fio := runtime.FileIO()
if fio == nil {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "file operations require a FileIO provider")
+ return errs.NewInternalError(errs.SubtypeFileIO, "file operations require a FileIO provider")
}📝 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
| fio := runtime.FileIO() | |
| if fio == nil { | |
| return output.ErrValidation("file operations require a FileIO provider") | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "file operations require a FileIO provider") | |
| } | |
| fio := runtime.FileIO() | |
| if fio == nil { | |
| return errs.NewInternalError(errs.SubtypeFileIO, "file operations require a FileIO provider") | |
| } |
🤖 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_upload_attachment.go` around lines 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.
Comment on lines
96
to
99
| stat, err := fio.Stat(filePath) | ||
| if err != nil { | ||
| return common.WrapInputStatError(err, "file not found") | ||
| return taskInputStatError(err, "file not found") | ||
| } |
There was a problem hiding this comment.
Use a neutral message for generic Stat failures.
fio.Stat can fail for more than “not found” (for example, permission-denied or other filesystem errors). With the current prefix, an existing but inaccessible path turns into a misleading file not found: ... envelope.
Suggested fix
stat, err := fio.Stat(filePath)
if err != nil {
- return taskInputStatError(err, "file not found")
+ return taskInputStatError(err, "cannot access file")
}📝 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
| stat, err := fio.Stat(filePath) | |
| if err != nil { | |
| return common.WrapInputStatError(err, "file not found") | |
| return taskInputStatError(err, "file not found") | |
| } | |
| stat, err := fio.Stat(filePath) | |
| if err != nil { | |
| return taskInputStatError(err, "cannot access file") | |
| } |
🤖 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_upload_attachment.go` around lines 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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1231 +/- ##
=======================================
Coverage 69.19% 69.19%
=======================================
Files 637 638 +1
Lines 59753 59738 -15
=======================================
- Hits 41345 41338 -7
+ Misses 15067 15057 -10
- Partials 3341 3343 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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
Comment on lines
115
to
116
| code, _ := util.ToFloat64(codeVal) | ||
| larkCode := int(code) |
There was a problem hiding this comment.
Reject non-numeric code fields as invalid responses.
The conversion error from util.ToFloat64(codeVal) is discarded, so a payload like {"code":"oops"} is indistinguishable from code == 0 in this function and can fall through as a successful response with nil data. Treat a failed conversion as SubtypeInvalidResponse before branching on larkCode.
Suggested fix
- code, _ := util.ToFloat64(codeVal)
+ code, convErr := util.ToFloat64(codeVal)
+ if convErr != nil {
+ return nil, errs.NewInternalError(
+ errs.SubtypeInvalidResponse,
+ "%s: invalid code in task API response",
+ action,
+ )
+ }
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 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.
Comment on lines
+173
to
175
| if len(failedTasks) > 0 { | ||
| return runtime.OutPartialFailure(outData, nil) | ||
| } |
There was a problem hiding this comment.
Partial failures now bypass the custom text formatter.
Returning here skips the runtime.OutFormat(..., writer) block at Lines 177-204, so the existing human-readable summary is never rendered when any sub-task fails. OutPartialFailure only emits the generic payload via ctx.emit in shortcuts/common/runner.go:693-708. If you want to preserve the current text-mode contract, emit the formatted output first and then return the partial-failure exit signal.
🤖 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 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.
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
1 participant
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 a localtask_errors.gohelper for file-input and transport errors.internal/errclass/codemeta_task.gowhile preserving per-code recovery hints; it now runs only on the genuine API-response path.shortcuts/task/via golangci-lint.Exit codes are now derived from the error category: input validation → 2, permission denied → 3, other API errors → 1, local internal failures → 5. Batch operations keep their current per-item reporting and exit behavior; typed partial-failure signaling is deferred.
Test Plan
go test ./shortcuts/task/): typed code→category/subtype/exit/hint mapping, validation/internal envelopes, batch partial-failure shape — all green.tests_e2e/task): validation envelopes and exit-2 consistency across update / create / assign / followers / reminder / search / get-my-tasks / tasklist-members / upload-attachment / tasklist-create — 3 pass / 1 fixture-gated skip / 0 fail.golangci-lint --new-from-rev=origin/main0 issues;gofmt/go vet/go build ./...clean.Related Issues
Part of the ongoing effort to migrate business domains to the typed
errs.*error contract (task domain).Summary by CodeRabbit
Refactor
Tests
Chores