Skip to content

Commit 0957a6d

Browse files
salmonumbrellaclaude
salmonumbrella
and
claude
committed
fix(cli): address code review findings
- Fix nil pointer dereference in confirmDestructive when flags is nil - Deduplicate dry-run logic by delegating to dryRunExit - Remove deprecated net.Error.Temporary() call (dead since Go 1.18) - Add unit tests for resolveTasklistID and resolveCalendarID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 969135e commit 0957a6d

3 files changed

Lines changed: 210 additions & 20 deletions

File tree

‎internal/cmd/confirm.go‎

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,29 +11,13 @@ import (
1111
"golang.org/x/term"
1212

1313
"github.com/steipete/gogcli/internal/input"
14-
"github.com/steipete/gogcli/internal/outfmt"
15-
"github.com/steipete/gogcli/internal/ui"
1614
)
1715

1816
func confirmDestructive(ctx context.Context, flags *RootFlags, action string) error {
19-
if flags != nil && flags.DryRun {
20-
// Dry-run exits successfully after printing the intended action.
21-
if outfmt.IsJSON(ctx) {
22-
_ = outfmt.WriteJSON(ctx, os.Stdout, map[string]any{
23-
"dry_run": true,
24-
"action": action,
25-
})
26-
} else if outfmt.IsPlain(ctx) {
27-
fmt.Fprintf(os.Stdout, "dry_run\ttrue\n")
28-
fmt.Fprintf(os.Stdout, "action\t%s\n", action)
29-
} else if u := ui.FromContext(ctx); u != nil {
30-
u.Out().Printf("Dry run: would %s", action)
31-
} else {
32-
fmt.Printf("Dry run: would %s\n", action)
33-
}
34-
return &ExitError{Code: 0, Err: nil}
17+
if err := dryRunExit(ctx, flags, action, nil); err != nil {
18+
return err
3519
}
36-
if flags.Force {
20+
if flags == nil || flags.Force {
3721
return nil
3822
}
3923

‎internal/cmd/exit_codes.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func stableExitCode(err error) error {
7474

7575
var ne net.Error
7676
if errors.As(err, &ne) {
77-
if ne.Timeout() || ne.Temporary() {
77+
if ne.Timeout() {
7878
return &ExitError{Code: exitCodeRetryable, Err: err}
7979
}
8080
}

‎internal/cmd/name_resolution_test.go‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
package cmd
2+
3+
import (
4+
"context"
5+
"testing"
6+
)
7+
8+
// --- resolveTasklistID bypass/short-circuit cases ---
9+
10+
func TestResolveTasklistID_EmptyString(t *testing.T) {
11+
got, err := resolveTasklistID(context.TODO(), nil, "")
12+
if err != nil {
13+
t.Fatalf("expected no error, got %v", err)
14+
}
15+
if got != "" {
16+
t.Fatalf("expected empty string, got %q", got)
17+
}
18+
}
19+
20+
func TestResolveTasklistID_WhitespaceOnly(t *testing.T) {
21+
got, err := resolveTasklistID(context.TODO(), nil, " ")
22+
if err != nil {
23+
t.Fatalf("expected no error, got %v", err)
24+
}
25+
if got != "" {
26+
t.Fatalf("expected empty string, got %q", got)
27+
}
28+
}
29+
30+
func TestResolveTasklistID_DefaultLowercase(t *testing.T) {
31+
got, err := resolveTasklistID(context.TODO(), nil, "default")
32+
if err != nil {
33+
t.Fatalf("expected no error, got %v", err)
34+
}
35+
if got != "@default" {
36+
t.Fatalf("expected %q, got %q", "@default", got)
37+
}
38+
}
39+
40+
func TestResolveTasklistID_DefaultMixedCase(t *testing.T) {
41+
got, err := resolveTasklistID(context.TODO(), nil, "Default")
42+
if err != nil {
43+
t.Fatalf("expected no error, got %v", err)
44+
}
45+
if got != "@default" {
46+
t.Fatalf("expected %q, got %q", "@default", got)
47+
}
48+
}
49+
50+
func TestResolveTasklistID_DefaultUppercase(t *testing.T) {
51+
got, err := resolveTasklistID(context.TODO(), nil, "DEFAULT")
52+
if err != nil {
53+
t.Fatalf("expected no error, got %v", err)
54+
}
55+
if got != "@default" {
56+
t.Fatalf("expected %q, got %q", "@default", got)
57+
}
58+
}
59+
60+
func TestResolveTasklistID_AtDefault(t *testing.T) {
61+
got, err := resolveTasklistID(context.TODO(), nil, "@default")
62+
if err != nil {
63+
t.Fatalf("expected no error, got %v", err)
64+
}
65+
if got != "@default" {
66+
t.Fatalf("expected %q, got %q", "@default", got)
67+
}
68+
}
69+
70+
func TestResolveTasklistID_DefaultWithLeadingWhitespace(t *testing.T) {
71+
got, err := resolveTasklistID(context.TODO(), nil, " default ")
72+
if err != nil {
73+
t.Fatalf("expected no error, got %v", err)
74+
}
75+
if got != "@default" {
76+
t.Fatalf("expected %q, got %q", "@default", got)
77+
}
78+
}
79+
80+
func TestResolveTasklistID_LongOpaqueID(t *testing.T) {
81+
id := "MDQ2NTI3MjEwMzA0NjUyOTM1NzA6MDow"
82+
got, err := resolveTasklistID(context.TODO(), nil, id)
83+
if err != nil {
84+
t.Fatalf("expected no error, got %v", err)
85+
}
86+
if got != id {
87+
t.Fatalf("expected %q, got %q", id, got)
88+
}
89+
}
90+
91+
func TestResolveTasklistID_Exactly16Chars(t *testing.T) {
92+
id := "abcdefghij012345"
93+
got, err := resolveTasklistID(context.TODO(), nil, id)
94+
if err != nil {
95+
t.Fatalf("expected no error, got %v", err)
96+
}
97+
if got != id {
98+
t.Fatalf("expected %q, got %q", id, got)
99+
}
100+
}
101+
102+
func TestResolveTasklistID_LongStringWithSpacesNotBypassed(t *testing.T) {
103+
// A long string that contains spaces should NOT be treated as an ID.
104+
// This would require an API call, so passing nil svc should panic.
105+
defer func() {
106+
if r := recover(); r == nil {
107+
t.Fatalf("expected panic due to nil service for API resolution path")
108+
}
109+
}()
110+
_, _ = resolveTasklistID(context.TODO(), nil, "my very long tasklist name here")
111+
}
112+
113+
// --- resolveCalendarID bypass/short-circuit cases ---
114+
115+
func TestResolveCalendarID_EmptyString(t *testing.T) {
116+
got, err := resolveCalendarID(context.TODO(), nil, "")
117+
if err != nil {
118+
t.Fatalf("expected no error, got %v", err)
119+
}
120+
if got != "" {
121+
t.Fatalf("expected empty string, got %q", got)
122+
}
123+
}
124+
125+
func TestResolveCalendarID_WhitespaceOnly(t *testing.T) {
126+
got, err := resolveCalendarID(context.TODO(), nil, " ")
127+
if err != nil {
128+
t.Fatalf("expected no error, got %v", err)
129+
}
130+
if got != "" {
131+
t.Fatalf("expected empty string, got %q", got)
132+
}
133+
}
134+
135+
func TestResolveCalendarID_PrimaryLowercase(t *testing.T) {
136+
got, err := resolveCalendarID(context.TODO(), nil, "primary")
137+
if err != nil {
138+
t.Fatalf("expected no error, got %v", err)
139+
}
140+
if got != "primary" {
141+
t.Fatalf("expected %q, got %q", "primary", got)
142+
}
143+
}
144+
145+
func TestResolveCalendarID_PrimaryMixedCase(t *testing.T) {
146+
got, err := resolveCalendarID(context.TODO(), nil, "Primary")
147+
if err != nil {
148+
t.Fatalf("expected no error, got %v", err)
149+
}
150+
if got != "primary" {
151+
t.Fatalf("expected %q, got %q", "primary", got)
152+
}
153+
}
154+
155+
func TestResolveCalendarID_PrimaryUppercase(t *testing.T) {
156+
got, err := resolveCalendarID(context.TODO(), nil, "PRIMARY")
157+
if err != nil {
158+
t.Fatalf("expected no error, got %v", err)
159+
}
160+
if got != "primary" {
161+
t.Fatalf("expected %q, got %q", "primary", got)
162+
}
163+
}
164+
165+
func TestResolveCalendarID_PrimaryWithWhitespace(t *testing.T) {
166+
got, err := resolveCalendarID(context.TODO(), nil, " primary ")
167+
if err != nil {
168+
t.Fatalf("expected no error, got %v", err)
169+
}
170+
if got != "primary" {
171+
t.Fatalf("expected %q, got %q", "primary", got)
172+
}
173+
}
174+
175+
func TestResolveCalendarID_EmailAddress(t *testing.T) {
176+
id := "user@example.com"
177+
got, err := resolveCalendarID(context.TODO(), nil, id)
178+
if err != nil {
179+
t.Fatalf("expected no error, got %v", err)
180+
}
181+
if got != id {
182+
t.Fatalf("expected %q, got %q", id, got)
183+
}
184+
}
185+
186+
func TestResolveCalendarID_GroupCalendarID(t *testing.T) {
187+
id := "company.com_abcdef1234@group.calendar.google.com"
188+
got, err := resolveCalendarID(context.TODO(), nil, id)
189+
if err != nil {
190+
t.Fatalf("expected no error, got %v", err)
191+
}
192+
if got != id {
193+
t.Fatalf("expected %q, got %q", id, got)
194+
}
195+
}
196+
197+
func TestResolveCalendarID_NameWithoutAtTriggeresAPI(t *testing.T) {
198+
// A plain name without "@" would trigger API resolution.
199+
// Passing nil svc should panic.
200+
defer func() {
201+
if r := recover(); r == nil {
202+
t.Fatalf("expected panic due to nil service for API resolution path")
203+
}
204+
}()
205+
_, _ = resolveCalendarID(context.TODO(), nil, "Work Calendar")
206+
}

0 commit comments

Comments
 (0)