fix: migrate task shortcut errors from bare fmt.Errorf to structured …#740
Open
Zhang-986 wants to merge 1 commit intolarksuite:mainfrom
Open
fix: migrate task shortcut errors from bare fmt.Errorf to structured …#740Zhang-986 wants to merge 1 commit intolarksuite:mainfrom
Zhang-986 wants to merge 1 commit intolarksuite:mainfrom
Conversation
|
|
github-actions
Bot
added
domain/task
📝 WalkthroughWalkthroughThe task shortcuts module standardizes error handling by converting parse/validation failures into structured Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR migrates error returns in the task shortcuts (shortcuts/task/) from bare fmt.Errorf to structured internal/output errors (output.Errorf / output.ErrValidation) to preserve the CLI’s JSON error envelope contract for AI agent consumers.
Changes:
- Replaced validation errors with
output.ErrValidationin task body/time parsing helpers. - Replaced API response unmarshal errors with
output.Errorf(output.ExitAPI, "api_error", ...). - Added/updated tests to assert returned errors are
*output.ExitErrorwith the expected exit codes and detail types.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shortcuts/task/task_util.go | Converts parseRelativeTime failures to structured validation errors. |
| shortcuts/task/shortcuts.go | Converts create-body validation errors + create response parse errors to structured output errors. |
| shortcuts/task/task_update.go | Converts update-body validation errors + update response parse errors to structured output errors. |
| shortcuts/task/task_query_helpers.go | Converts time-range parsing validation failures to structured validation errors. |
| shortcuts/task/task_util_test.go | Adds structured-error assertions for parseRelativeTime. |
| shortcuts/task/task_body_test.go | Adds new structured-error tests for create/update body builders. |
| shortcuts/task/task_query_helpers_test.go | Adds structured-error assertions for reversed time-range cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+37
| tests := []struct { | ||
| name string | ||
| input string | ||
| wantCode int | ||
| wantType string | ||
| wantSubstr string | ||
| }{ | ||
| { | ||
| name: "invalid format returns ErrValidation", | ||
| input: "not-relative", | ||
| wantCode: output.ExitValidation, | ||
| wantType: "validation", | ||
| wantSubstr: "invalid relative time format", | ||
| }, |
![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)
shortcuts/task/task_query_helpers_test.go (1)
257-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the structured validation assertion here.
The RFC3339 case only checks the exit code. Please also assert
Detail.Type == "validation"like the millis case so this path is covered by the same contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/task/task_query_helpers_test.go` around lines 257 - 286, In the parseTimeRangeRFC3339 test, extend the validation for the "reversed range fails fast" case to mirror the millis-case assertions: after asserting the error is an *output.ExitError and that exitErr.Code == output.ExitValidation (within the t.Run for parseTimeRangeRFC3339), also assert exitErr.Detail.Type == "validation" so the RFC3339 path enforces the same contract; reference parseTimeRangeRFC3339, output.ExitError, output.ExitValidation, and exitErr.Detail.Type when adding this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/task/task_body_test.go`:
- Around line 16-76: Add assertions in TestBuildTaskCreateBody_StructuredErrors
to verify the error message contains the expected substring (tt.wantSubstr).
After you obtain exitErr from buildTaskCreateBody, assert that
exitErr.Detail.Message (or exitErr.Error() if Message is empty) contains
tt.wantSubstr using strings.Contains; reference the test name
TestBuildTaskCreateBody_StructuredErrors, the table field wantSubstr, and the
error variable exitErr to locate where to add the check so the structured-error
tests fail on message regressions.
In `@shortcuts/task/task_util_test.go`:
- Around line 23-62: The test TestParseRelativeTime_StructuredErrors defines
wantSubstr but never asserts the human-readable message; update the test to
assert that exitErr.Detail.Message (the error text on the structured detail)
contains tt.wantSubstr using strings.Contains and t.Errorf when it does not, so
the validation message is checked for the expected substring after the existing
non-nil and Type assertions around parseRelativeTime.
---
Outside diff comments:
In `@shortcuts/task/task_query_helpers_test.go`:
- Around line 257-286: In the parseTimeRangeRFC3339 test, extend the validation
for the "reversed range fails fast" case to mirror the millis-case assertions:
after asserting the error is an *output.ExitError and that exitErr.Code ==
output.ExitValidation (within the t.Run for parseTimeRangeRFC3339), also assert
exitErr.Detail.Type == "validation" so the RFC3339 path enforces the same
contract; reference parseTimeRangeRFC3339, output.ExitError,
output.ExitValidation, and exitErr.Detail.Type when adding this assertion.
🪄 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: 2819e420-0d47-4916-ba99-fe6033c41dbd
📒 Files selected for processing (7)
shortcuts/task/shortcuts.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_update.goshortcuts/task/task_util.goshortcuts/task/task_util_test.go
Comment on lines
+16
to
+76
| func TestBuildTaskCreateBody_StructuredErrors(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| data string | ||
| summary string | ||
| wantCode int | ||
| wantType string | ||
| wantSubstr string | ||
| }{ | ||
| { | ||
| name: "invalid JSON data returns ErrValidation", | ||
| data: "not-json", | ||
| summary: "test", | ||
| wantCode: output.ExitValidation, | ||
| wantType: "validation", | ||
| wantSubstr: "--data must be a valid JSON object", | ||
| }, | ||
| { | ||
| name: "missing summary returns ErrValidation", | ||
| data: "", | ||
| summary: "", | ||
| wantCode: output.ExitValidation, | ||
| wantType: "validation", | ||
| wantSubstr: "task summary is required", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| cmd := &cobra.Command{} | ||
| cmd.Flags().String("data", tt.data, "") | ||
| cmd.Flags().String("summary", tt.summary, "") | ||
| cmd.Flags().String("description", "", "") | ||
| cmd.Flags().String("assignee", "", "") | ||
| cmd.Flags().String("follower", "", "") | ||
| cmd.Flags().String("due", "", "") | ||
| cmd.Flags().String("tasklist-id", "", "") | ||
| cmd.Flags().String("idempotency-key", "", "") | ||
|
|
||
| runtime := &common.RuntimeContext{Cmd: cmd} | ||
| _, err := buildTaskCreateBody(runtime) | ||
| if err == nil { | ||
| t.Fatal("expected error, got nil") | ||
| } | ||
|
|
||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("error type = %T, want *output.ExitError; error = %v", err, err) | ||
| } | ||
| if exitErr.Code != tt.wantCode { | ||
| t.Errorf("exit code = %d, want %d", exitErr.Code, tt.wantCode) | ||
| } | ||
| if exitErr.Detail == nil { | ||
| t.Fatal("expected non-nil error detail") | ||
| } | ||
| if exitErr.Detail.Type != tt.wantType { | ||
| t.Errorf("error type = %q, want %q", exitErr.Detail.Type, tt.wantType) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the expected message substrings in both structured-error tests.
The tables already carry wantSubstr, but the assertions only check type/code. Please assert the error text as well so a message regression doesn't pass unnoticed.
Suggested fix
import (
"errors"
+ "strings"
"testing"
@@
if exitErr.Detail.Type != tt.wantType {
t.Errorf("error type = %q, want %q", exitErr.Detail.Type, tt.wantType)
}
+ if !strings.Contains(err.Error(), tt.wantSubstr) {
+ t.Errorf("error = %q, want substring %q", err.Error(), tt.wantSubstr)
+ }
})
}
}Also applies to: 78-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/task/task_body_test.go` around lines 16 - 76, Add assertions in
TestBuildTaskCreateBody_StructuredErrors to verify the error message contains
the expected substring (tt.wantSubstr). After you obtain exitErr from
buildTaskCreateBody, assert that exitErr.Detail.Message (or exitErr.Error() if
Message is empty) contains tt.wantSubstr using strings.Contains; reference the
test name TestBuildTaskCreateBody_StructuredErrors, the table field wantSubstr,
and the error variable exitErr to locate where to add the check so the
structured-error tests fail on message regressions.
…output.Errorf/ErrValidation
Zhang-986
force-pushed
the
fix/task-structured-errors
branch
from
07765cd to
0f3be77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/task/task_update.go`:
- Around line 134-137: After unmarshalling dataStr into taskObj using
json.Unmarshal in the block that checks runtime.Str("data"), add a nil-check for
taskObj and either initialize it to an empty map (e.g., taskObj =
make(map[string]interface{})) or return a validation error if the caller passed
JSON null; this prevents panics when later code assigns taskObj["summary"],
taskObj["description"], and taskObj["due"]. Locate the json.Unmarshal call and
runtime.Str("data") check and ensure taskObj is non-nil before any field
assignments, preserving the existing output.ErrValidation error behavior on
malformed JSON.
🪄 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: c87f24c0-be2a-44a6-820a-f82f562fb80f
📒 Files selected for processing (7)
shortcuts/task/shortcuts.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_update.goshortcuts/task/task_util.goshortcuts/task/task_util_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/task/task_util_test.go
- shortcuts/task/task_query_helpers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/task/task_util.go
- shortcuts/task/task_body_test.go
- shortcuts/task/task_query_helpers_test.go
Comment on lines
134
to
137
| if dataStr := runtime.Str("data"); dataStr != "" { | ||
| if err := json.Unmarshal([]byte(dataStr), &taskObj); err != nil { | ||
| return nil, fmt.Errorf("--data must be a valid JSON object: %v", err) | ||
| return nil, output.ErrValidation("--data must be a valid JSON object: %v", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and understand its structure
find . -type f -name "task_update.go" | head -5Repository: larksuite/cli
Length of output: 89
🏁 Script executed:
# Check file size to determine how to read it
wc -l shortcuts/task/task_update.goRepository: larksuite/cli
Length of output: 91
🏁 Script executed:
# Read the file to see the context around lines 134-137
cat -n shortcuts/task/task_update.go | head -180Repository: larksuite/cli
Length of output: 6347
Add nil check after JSON unmarshal to prevent panic on --data null.
When --data null is provided, json.Unmarshal sets taskObj to nil. Subsequent field assignments at lines 145, 152, and 163 (taskObj["summary"], taskObj["description"], taskObj["due"]) will panic on a nil map.
Suggested fix
if dataStr := runtime.Str("data"); dataStr != "" {
if err := json.Unmarshal([]byte(dataStr), &taskObj); err != nil {
return nil, output.ErrValidation("--data must be a valid JSON object: %v", err)
}
+ if taskObj == nil {
+ return nil, output.ErrValidation("--data must be a non-null JSON object")
+ }
}📝 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
| if dataStr := runtime.Str("data"); dataStr != "" { | |
| if err := json.Unmarshal([]byte(dataStr), &taskObj); err != nil { | |
| return nil, fmt.Errorf("--data must be a valid JSON object: %v", err) | |
| return nil, output.ErrValidation("--data must be a valid JSON object: %v", err) | |
| } | |
| if dataStr := runtime.Str("data"); dataStr != "" { | |
| if err := json.Unmarshal([]byte(dataStr), &taskObj); err != nil { | |
| return nil, output.ErrValidation("--data must be a valid JSON object: %v", err) | |
| } | |
| if taskObj == nil { | |
| return nil, output.ErrValidation("--data must be a non-null JSON object") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/task/task_update.go` around lines 134 - 137, After unmarshalling
dataStr into taskObj using json.Unmarshal in the block that checks
runtime.Str("data"), add a nil-check for taskObj and either initialize it to an
empty map (e.g., taskObj = make(map[string]interface{})) or return a validation
error if the caller passed JSON null; this prevents panics when later code
assigns taskObj["summary"], taskObj["description"], and taskObj["due"]. Locate
the json.Unmarshal call and runtime.Str("data") check and ensure taskObj is
non-nil before any field assignments, preserving the existing
output.ErrValidation error behavior on malformed JSON.
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
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
Migrated 16 error paths in the task management shortcuts (
shortcuts/task/) from barefmt.Errorfto structuredoutput.Errorf/output.ErrValidation, enabling AI agent consumers (Claude Code, Cursor, Gemini CLI) to programmatically parse and recover from errors via the JSON error envelope contract.Per AGENTS.md: "RunE functions must return output.Errorf / output.ErrWithHint — never bare fmt.Errorf. AI agents parse stderr as JSON; bare errors break this contract."
Changes
Error migration (16 paths across 4 files)
task_util.go(2 paths — validation errors):parseRelativeTime: invalid format →output.ErrValidationparseRelativeTime: unknown unit →output.ErrValidationshortcuts.go(4 paths — 3 validation + 1 API):buildTaskCreateBody: invalid--dataJSON →output.ErrValidationbuildTaskCreateBody: invalid--duetime →output.ErrValidationbuildTaskCreateBody: missing summary →output.ErrValidationCreateTask.Execute: response parse failure →output.Errorf(ExitAPI, "api_error", ...)task_update.go(4 paths — 3 validation + 1 API):UpdateTask.Execute: response parse failure →output.Errorf(ExitAPI, "api_error", ...)buildTaskUpdateBody: invalid--dataJSON →output.ErrValidationbuildTaskUpdateBody: invalid--duetime →output.ErrValidationbuildTaskUpdateBody: no fields to update →output.ErrValidationtask_query_helpers.go(6 paths — all validation):parseTimeRangeMillis: invalid start/end timestamp →output.ErrValidationparseTimeRangeMillis: reversed time range →output.ErrValidationparseTimeRangeRFC3339: invalid start/end timestamp →output.ErrValidationparseTimeRangeRFC3339: reversed time range →output.ErrValidationTest additions
task_util_test.go: AddedTestParseRelativeTime_StructuredErrors— verifies*output.ExitErrortype, exit code, and error detail type.task_body_test.go(new): AddedTestBuildTaskCreateBody_StructuredErrorsandTestBuildTaskUpdateBody_StructuredErrors— validates structured error output for invalid JSON data, missing summary, and no-fields-to-update cases.task_query_helpers_test.go: Enhanced existing tests with*output.ExitErrortype assertions for reversed time range errors.Test plan
go test ./shortcuts/task/... -race— all passgo vet ./shortcuts/task/...— cleangofmt -l shortcuts/task/— no outputgo mod tidy— no changesgolangci-lint run --new-from-rev=origin/main ./shortcuts/task/...— 0 issuesSummary by CodeRabbit
Bug Fixes
Tests