Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions shortcuts/task/shortcuts.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"strings"
"time"

"github.com/larksuite/cli/internal/output"
"github.com/larksuite/cli/shortcuts/common"
larkcore "github.com/larksuite/oapi-sdk-go/v3/core"
)
Expand Down Expand Up @@ -98,7 +99,7 @@ func buildTaskCreateBody(runtime *common.RuntimeContext) (map[string]interface{}
// Handle generic JSON payload if provided
if dataStr := runtime.Str("data"); dataStr != "" {
if err := json.Unmarshal([]byte(dataStr), &body); 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)
}
}

Expand Down Expand Up @@ -134,7 +135,7 @@ func buildTaskCreateBody(runtime *common.RuntimeContext) (map[string]interface{}
if dueStr := runtime.Str("due"); dueStr != "" {
dueObj, err := parseTaskTime(dueStr)
if err != nil {
return nil, fmt.Errorf("failed to parse due time: %v", err)
return nil, output.ErrValidation("failed to parse due time: %v", err)
}
body["due"] = dueObj
}
Expand All @@ -145,7 +146,7 @@ func buildTaskCreateBody(runtime *common.RuntimeContext) (map[string]interface{}

summary, _ := body["summary"].(string)
if strings.TrimSpace(summary) == "" {
return nil, fmt.Errorf("task summary is required")
return nil, output.ErrValidation("task summary is required")
}

return body, nil
Expand Down Expand Up @@ -201,7 +202,7 @@ var CreateTask = common.Shortcut{
var result map[string]interface{}
if err == nil {
if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil {
return fmt.Errorf("failed to parse response: %v", parseErr)
return output.Errorf(output.ExitAPI, "api_error", "failed to parse response: %v", parseErr)
}
}

Expand Down
134 changes: 134 additions & 0 deletions shortcuts/task/task_body_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
// Copyright (c) 2026 Lark Technologies Pte. Ltd.
// SPDX-License-Identifier: MIT

package task

import (
"errors"
"testing"

"github.com/larksuite/cli/internal/output"
"github.com/spf13/cobra"

"github.com/larksuite/cli/shortcuts/common"
)

func TestBuildTaskCreateBody_StructuredErrors(t *testing.T) {
tests := []struct {
name string
data string
summary string
wantCode int
wantType string
wantSubstr string
}{
Comment thread
Zhang-986 marked this conversation as resolved.
{
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)
}
})
}
}
Comment on lines +16 to +76
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


func TestBuildTaskUpdateBody_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: "",
wantCode: output.ExitValidation,
wantType: "validation",
wantSubstr: "--data must be a valid JSON object",
},
{
name: "no fields to update returns ErrValidation",
data: "",
summary: "",
wantCode: output.ExitValidation,
wantType: "validation",
wantSubstr: "no fields to update",
},
}

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("due", "", "")

runtime := &common.RuntimeContext{Cmd: cmd}
_, err := buildTaskUpdateBody(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)
}
})
}
}
14 changes: 8 additions & 6 deletions shortcuts/task/task_query_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"strconv"
"strings"
"time"

"github.com/larksuite/cli/internal/output"
)

func splitAndTrimCSV(input string) []string {
Expand Down Expand Up @@ -44,7 +46,7 @@ func parseTimeRangeMillis(input string) (string, string, error) {
}
startSecInt, err = strconv.ParseInt(startSec, 10, 64)
if err != nil {
return "", "", fmt.Errorf("invalid start timestamp: %w", err)
return "", "", output.ErrValidation("invalid start timestamp: %v", err)
}
hasStart = true
startMillis = startSec + "000"
Expand All @@ -56,13 +58,13 @@ func parseTimeRangeMillis(input string) (string, string, error) {
}
endSecInt, err = strconv.ParseInt(endSec, 10, 64)
if err != nil {
return "", "", fmt.Errorf("invalid end timestamp: %w", err)
return "", "", output.ErrValidation("invalid end timestamp: %v", err)
}
hasEnd = true
endMillis = endSec + "000"
}
if hasStart && hasEnd && startSecInt > endSecInt {
return "", "", fmt.Errorf("start time must be earlier than or equal to end time")
return "", "", output.ErrValidation("start time must be earlier than or equal to end time")
}
return startMillis, endMillis, nil
}
Expand All @@ -89,7 +91,7 @@ func parseTimeRangeRFC3339(input string) (string, string, error) {
}
startSecInt, err = strconv.ParseInt(startSec, 10, 64)
if err != nil {
return "", "", fmt.Errorf("invalid start timestamp: %w", err)
return "", "", output.ErrValidation("invalid start timestamp: %v", err)
}
hasStart = true
startTime = time.Unix(startSecInt, 0).Local().Format(time.RFC3339)
Expand All @@ -101,13 +103,13 @@ func parseTimeRangeRFC3339(input string) (string, string, error) {
}
endSecInt, err = strconv.ParseInt(endSec, 10, 64)
if err != nil {
return "", "", fmt.Errorf("invalid end timestamp: %w", err)
return "", "", output.ErrValidation("invalid end timestamp: %v", err)
}
hasEnd = true
endTime = time.Unix(endSecInt, 0).Local().Format(time.RFC3339)
}
if hasStart && hasEnd && startSecInt > endSecInt {
return "", "", fmt.Errorf("start time must be earlier than or equal to end time")
return "", "", output.ErrValidation("start time must be earlier than or equal to end time")
}
return startTime, endTime, nil
}
Expand Down
24 changes: 24 additions & 0 deletions shortcuts/task/task_query_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
package task

import (
"errors"
"strings"
"testing"

"github.com/larksuite/cli/internal/output"
)

func TestSplitAndTrimCSV(t *testing.T) {
Expand Down Expand Up @@ -95,6 +98,18 @@ func TestParseTimeRangeMillisAndRequireSearchFilter(t *testing.T) {
if err == nil {
t.Fatalf("parseTimeRangeMillis(%q) expected error, got nil", tt.input)
}
if tt.name == "reversed range fails fast" {
var exitErr *output.ExitError
if !errors.As(err, &exitErr) {
t.Fatalf("error type = %T, want *output.ExitError; error = %v", err, err)
}
if exitErr.Code != output.ExitValidation {
t.Errorf("exit code = %d, want %d", exitErr.Code, output.ExitValidation)
}
if exitErr.Detail == nil || exitErr.Detail.Type != "validation" {
t.Errorf("error detail type = %q, want %q", exitErr.Detail.Type, "validation")
}
}
return
}
if err != nil {
Expand Down Expand Up @@ -260,6 +275,15 @@ func TestRenderRelatedTasksPretty(t *testing.T) {
if err == nil {
t.Fatal("expected error, got nil")
}
if tt.name == "reversed range fails fast" {
var exitErr *output.ExitError
if !errors.As(err, &exitErr) {
t.Fatalf("error type = %T, want *output.ExitError; error = %v", err, err)
}
if exitErr.Code != output.ExitValidation {
t.Errorf("exit code = %d, want %d", exitErr.Code, output.ExitValidation)
}
}
return
}
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions shortcuts/task/task_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ var UpdateTask = common.Shortcut{
var result map[string]interface{}
if err == nil {
if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil {
return fmt.Errorf("failed to parse response for task %s: %v", taskId, parseErr)
return output.Errorf(output.ExitAPI, "api_error", "failed to parse response for task %s: %v", taskId, parseErr)
}
}

Expand Down Expand Up @@ -133,7 +133,7 @@ func buildTaskUpdateBody(runtime *common.RuntimeContext) (map[string]interface{}

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)
}
Comment on lines 134 to 137
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, locate the file and understand its structure
find . -type f -name "task_update.go" | head -5

Repository: larksuite/cli

Length of output: 89


🏁 Script executed:

# Check file size to determine how to read it
wc -l shortcuts/task/task_update.go

Repository: 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 -180

Repository: 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.

// If data is provided, assume keys are update fields
for k := range taskObj {
Expand All @@ -158,7 +158,7 @@ func buildTaskUpdateBody(runtime *common.RuntimeContext) (map[string]interface{}
if dueStr := runtime.Str("due"); dueStr != "" {
dueObj, err := parseTaskTime(dueStr)
if err != nil {
return nil, fmt.Errorf("failed to parse due time: %v", err)
return nil, output.ErrValidation("failed to parse due time: %v", err)
}
taskObj["due"] = dueObj
if !contains(updateFields, "due") {
Expand All @@ -167,7 +167,7 @@ func buildTaskUpdateBody(runtime *common.RuntimeContext) (map[string]interface{}
}

if len(updateFields) == 0 {
return nil, fmt.Errorf("no fields to update")
return nil, output.ErrValidation("no fields to update")
}

return map[string]interface{}{
Expand Down
4 changes: 2 additions & 2 deletions shortcuts/task/task_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func isRelativeTime(s string) bool {
func parseRelativeTime(s string) (time.Time, error) {
matches := relativeTimeRe.FindStringSubmatch(s)
if len(matches) == 0 {
return time.Time{}, fmt.Errorf("invalid relative time format: %s", s)
return time.Time{}, output.ErrValidation("invalid relative time format: %s", s)
}

sign := matches[1]
Expand All @@ -51,7 +51,7 @@ func parseRelativeTime(s string) (time.Time, error) {
case "h":
return now.Add(time.Duration(amount) * time.Hour), nil
default:
return time.Time{}, fmt.Errorf("unknown unit: %s", unit)
return time.Time{}, output.ErrValidation("unknown unit: %s", unit)
}
}

Expand Down
43 changes: 43 additions & 0 deletions shortcuts/task/task_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
package task

import (
"errors"
"testing"

"github.com/larksuite/cli/internal/output"
"github.com/smartystreets/goconvey/convey"
)

Expand All @@ -17,3 +19,44 @@ func TestContains(t *testing.T) {
convey.So(contains([]string{}, "a"), convey.ShouldBeFalse)
})
}

func TestParseRelativeTime_StructuredErrors(t *testing.T) {
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",
},
Comment on lines +24 to +37
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := parseRelativeTime(tt.input)
if err == nil {
t.Fatalf("parseRelativeTime(%q) expected error, got nil", tt.input)
}

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)
}
})
}
}
Comment thread
Zhang-986 marked this conversation as resolved.
Loading