Skip to content

Commit dcb533e

Browse files
authored
[TW-4734] refactor(cli): reduce code duplication across commands and adapters (#42)
1 parent 723f3db commit dcb533e

23 files changed

Lines changed: 1566 additions & 346 deletions

CLAUDE.md

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ Quick reference for AI assistants working on this codebase.
77
## ⛔ CRITICAL RULES - YOU MUST FOLLOW THESE
88

99
### NEVER DO (IMPORTANT - violations will break the workflow):
10-
- **NEVER run `git commit`** - User commits manually
11-
- **NEVER run `git push`** - User pushes manually
1210
- **NEVER commit secrets** - No API keys, tokens, passwords, .env files
1311
- **NEVER skip tests** - All changes require passing tests
1412
- **NEVER skip security scans** - Run `make security` before commits
@@ -139,31 +137,17 @@ Credentials stored in system keyring (service: `"nylas"`) via `nylas auth config
139137

140138
## Testing
141139

142-
**Command:** `make ci-full` (complete CI: quality + tests + cleanup)
143-
144-
**Quick checks:** `make ci` (no integration tests)
145-
146140
**Details:** `.claude/rules/testing.md`
147141

148142
---
149143

150-
## Hooks & Commands
151-
152-
**Hooks:** Auto-enforce quality (blocks bad code, auto-formats). See `.claude/HOOKS-CONFIG.md`
153-
154-
**Skills:** `/session-start`, `/run-tests`, `/add-command`, `/generate-tests`, `/security-scan`
155-
156-
**Agents:** See `.claude/agents/README.md` for parallelization guide
157-
158-
---
159-
160-
## Context & Session
144+
## Hooks, Skills & Agents
161145

162-
**Token tips:** Use `/compact` mid-session, `/clear` for new tasks, `/mcp` to disable unused servers
146+
**Hooks:** Auto-enforce quality. See `.claude/HOOKS-CONFIG.md`
163147

164-
**On-demand docs:** `docs/COMMANDS.md`, `docs/ARCHITECTURE.md`, `.claude/shared/patterns/*.md`
148+
**Skills:** `/run-tests`, `/add-command`, `/generate-tests`, `/security-scan`
165149

166-
**Session handoff:** Use `/diary` skill to record progress after major tasks
150+
**Agents:** See `.claude/agents/README.md`
167151

168152
---
169153

@@ -206,6 +190,10 @@ This section captures lessons learned from mistakes. Claude updates this section
206190
- AI clients: Use shared helpers in `adapters/ai/base_client.go` (ConvertMessagesToMaps, ConvertToolsOpenAIFormat, FallbackStreamChat)
207191
- Output formatting: Use `common.GetOutputWriter(cmd)` for JSON/YAML/quiet support, NEVER create custom --format flags
208192
- Client helpers: Use `common.WithClient()` and `WithClientNoGrant()` to reduce boilerplate, NEVER duplicate setup code
193+
- Status colors: Use `common.StatusColor()`/`StatusIcon()`/`ColorSprint()`, NEVER create package-local status color functions
194+
- Pagination: Use `common.SetupPagination()` for limit/maxItems resolution, NEVER duplicate auto-pagination logic in list commands
195+
- Pagination helpers: Use `common.NormalizePageSize()` and `FetchCursorPages()` for cursor-based pagination boilerplate
196+
- Test JSON responses: Use `testutil.WriteJSONResponse()` in httptest handlers, NEVER repeat the Content-Type/WriteHeader/Encode triplet
209197

210198
### Non-Obvious Workflows
211199
- Progressive disclosure: Keep main skill files under 100 lines, use references/ for details

internal/adapters/dashboard/http.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ func (c *AccountClient) doPost(ctx context.Context, path string, body any, extra
4646
return nil
4747
}
4848

49+
// setDPoPProof generates and sets the DPoP proof header on the request.
50+
func (c *AccountClient) setDPoPProof(req *http.Request, method, fullURL, accessToken string) error {
51+
proof, err := c.dpop.GenerateProof(method, fullURL, accessToken)
52+
if err != nil {
53+
return err
54+
}
55+
req.Header.Set("DPoP", proof)
56+
return nil
57+
}
58+
4959
// doPostRaw sends a JSON POST request and returns the raw response body.
5060
func (c *AccountClient) doPostRaw(ctx context.Context, path string, body any, extraHeaders map[string]string, accessToken string) ([]byte, error) {
5161
fullURL := c.baseURL + path
@@ -68,12 +78,9 @@ func (c *AccountClient) doPostRaw(ctx context.Context, path string, body any, ex
6878
req.Header.Set("Content-Type", "application/json")
6979
}
7080

71-
// Add DPoP proof
72-
proof, err := c.dpop.GenerateProof(http.MethodPost, fullURL, accessToken)
73-
if err != nil {
81+
if err := c.setDPoPProof(req, http.MethodPost, fullURL, accessToken); err != nil {
7482
return nil, err
7583
}
76-
req.Header.Set("DPoP", proof)
7784

7885
// Add extra headers (Authorization, X-Nylas-Org)
7986
for k, v := range extraHeaders {
@@ -135,12 +142,9 @@ func (c *AccountClient) doGetRaw(ctx context.Context, path string, extraHeaders
135142
return nil, fmt.Errorf("failed to create request: %w", err)
136143
}
137144

138-
// Add DPoP proof
139-
proof, err := c.dpop.GenerateProof(http.MethodGet, fullURL, accessToken)
140-
if err != nil {
145+
if err := c.setDPoPProof(req, http.MethodGet, fullURL, accessToken); err != nil {
141146
return nil, err
142147
}
143-
req.Header.Set("DPoP", proof)
144148

145149
for k, v := range extraHeaders {
146150
req.Header.Set(k, v)
Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
package dashboard
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
8+
"testing"
9+
)
10+
11+
// mockDPoP implements ports.DPoP for testing.
12+
type mockDPoP struct {
13+
proof string
14+
err error
15+
}
16+
17+
func (m *mockDPoP) GenerateProof(method, url, accessToken string) (string, error) {
18+
return m.proof, m.err
19+
}
20+
21+
func (m *mockDPoP) Thumbprint() string {
22+
return "test-thumbprint"
23+
}
24+
25+
func TestSetDPoPProof(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
proof string
29+
proofErr error
30+
wantHeader string
31+
wantErr bool
32+
}{
33+
{
34+
name: "sets DPoP header on success",
35+
proof: "test-proof-jwt",
36+
wantHeader: "test-proof-jwt",
37+
},
38+
{
39+
name: "returns error on failure",
40+
proofErr: errTestDPoP,
41+
wantErr: true,
42+
},
43+
}
44+
45+
for _, tt := range tests {
46+
t.Run(tt.name, func(t *testing.T) {
47+
client := &AccountClient{
48+
dpop: &mockDPoP{proof: tt.proof, err: tt.proofErr},
49+
}
50+
51+
req := httptest.NewRequest(http.MethodPost, "https://example.com/test", nil)
52+
err := client.setDPoPProof(req, http.MethodPost, "https://example.com/test", "token")
53+
54+
if tt.wantErr {
55+
if err == nil {
56+
t.Fatal("expected error, got nil")
57+
}
58+
return
59+
}
60+
61+
if err != nil {
62+
t.Fatalf("unexpected error: %v", err)
63+
}
64+
65+
if got := req.Header.Get("DPoP"); got != tt.wantHeader {
66+
t.Errorf("DPoP header = %q, want %q", got, tt.wantHeader)
67+
}
68+
})
69+
}
70+
}
71+
72+
var errTestDPoP = &testError{msg: "dpop generation failed"}
73+
74+
type testError struct{ msg string }
75+
76+
func (e *testError) Error() string { return e.msg }
77+
78+
func TestParseErrorResponse(t *testing.T) {
79+
tests := []struct {
80+
name string
81+
statusCode int
82+
body string
83+
wantMsg string
84+
}{
85+
{
86+
name: "parses error with code and message",
87+
statusCode: 400,
88+
body: `{"error":{"code":"invalid_request","message":"bad input"}}`,
89+
wantMsg: "invalid_request: bad input",
90+
},
91+
{
92+
name: "parses error with message only",
93+
statusCode: 500,
94+
body: `{"error":{"message":"internal error"}}`,
95+
wantMsg: "internal error",
96+
},
97+
{
98+
name: "falls back to raw body",
99+
statusCode: 502,
100+
body: "Bad Gateway",
101+
wantMsg: "Bad Gateway",
102+
},
103+
{
104+
name: "truncates long body",
105+
statusCode: 500,
106+
body: string(make([]byte, 300)),
107+
wantMsg: "", // truncated to 200 chars
108+
},
109+
}
110+
111+
for _, tt := range tests {
112+
t.Run(tt.name, func(t *testing.T) {
113+
err := parseErrorResponse(tt.statusCode, []byte(tt.body))
114+
dashErr, ok := err.(*DashboardAPIError)
115+
if !ok {
116+
t.Fatalf("expected *DashboardAPIError, got %T", err)
117+
}
118+
if dashErr.StatusCode != tt.statusCode {
119+
t.Errorf("StatusCode = %d, want %d", dashErr.StatusCode, tt.statusCode)
120+
}
121+
if tt.wantMsg != "" && dashErr.ServerMsg != tt.wantMsg {
122+
t.Errorf("ServerMsg = %q, want %q", dashErr.ServerMsg, tt.wantMsg)
123+
}
124+
})
125+
}
126+
}
127+
128+
func TestUnwrapEnvelope(t *testing.T) {
129+
tests := []struct {
130+
name string
131+
body string
132+
wantKey string
133+
wantErr bool
134+
}{
135+
{
136+
name: "unwraps data field",
137+
body: `{"request_id":"abc","success":true,"data":{"name":"test"}}`,
138+
wantKey: "name",
139+
},
140+
{
141+
name: "returns body as-is when no data field",
142+
body: `{"name":"test"}`,
143+
wantKey: "name",
144+
},
145+
{
146+
name: "returns error on invalid JSON",
147+
body: "not json",
148+
wantErr: true,
149+
},
150+
}
151+
152+
for _, tt := range tests {
153+
t.Run(tt.name, func(t *testing.T) {
154+
result, err := unwrapEnvelope([]byte(tt.body))
155+
if tt.wantErr {
156+
if err == nil {
157+
t.Fatal("expected error, got nil")
158+
}
159+
return
160+
}
161+
if err != nil {
162+
t.Fatalf("unexpected error: %v", err)
163+
}
164+
165+
var parsed map[string]any
166+
if jsonErr := json.Unmarshal(result, &parsed); jsonErr != nil {
167+
t.Fatalf("result is not valid JSON: %v", jsonErr)
168+
}
169+
if _, ok := parsed[tt.wantKey]; !ok {
170+
t.Errorf("result missing key %q: %s", tt.wantKey, string(result))
171+
}
172+
})
173+
}
174+
}
175+
176+
func TestDashboardAPIError_Error(t *testing.T) {
177+
tests := []struct {
178+
name string
179+
err DashboardAPIError
180+
wantStr string
181+
}{
182+
{
183+
name: "with message",
184+
err: DashboardAPIError{StatusCode: 400, ServerMsg: "bad request"},
185+
wantStr: "dashboard API error (HTTP 400): bad request",
186+
},
187+
{
188+
name: "without message",
189+
err: DashboardAPIError{StatusCode: 500},
190+
wantStr: "dashboard API error (HTTP 500)",
191+
},
192+
}
193+
194+
for _, tt := range tests {
195+
t.Run(tt.name, func(t *testing.T) {
196+
if got := tt.err.Error(); got != tt.wantStr {
197+
t.Errorf("Error() = %q, want %q", got, tt.wantStr)
198+
}
199+
})
200+
}
201+
}
202+
203+
func TestDoPostAndGet_Integration(t *testing.T) {
204+
// Set up a test server that returns a valid envelope response
205+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
206+
// Verify DPoP header was set
207+
if dpop := r.Header.Get("DPoP"); dpop == "" {
208+
t.Error("DPoP header not set")
209+
}
210+
211+
w.Header().Set("Content-Type", "application/json")
212+
resp := map[string]any{
213+
"request_id": "test-123",
214+
"success": true,
215+
"data": map[string]string{"id": "app-1", "name": "Test App"},
216+
}
217+
_ = json.NewEncoder(w).Encode(resp)
218+
}))
219+
defer server.Close()
220+
221+
client := &AccountClient{
222+
baseURL: server.URL,
223+
httpClient: server.Client(),
224+
dpop: &mockDPoP{proof: "test-proof"},
225+
}
226+
227+
t.Run("doPost decodes response", func(t *testing.T) {
228+
var result map[string]string
229+
err := client.doPost(context.Background(), "/test", nil, nil, "token", &result)
230+
if err != nil {
231+
t.Fatalf("unexpected error: %v", err)
232+
}
233+
if result["id"] != "app-1" {
234+
t.Errorf("result[id] = %q, want %q", result["id"], "app-1")
235+
}
236+
})
237+
238+
t.Run("doGet decodes response", func(t *testing.T) {
239+
var result map[string]string
240+
err := client.doGet(context.Background(), "/test", nil, "token", &result)
241+
if err != nil {
242+
t.Fatalf("unexpected error: %v", err)
243+
}
244+
if result["name"] != "Test App" {
245+
t.Errorf("result[name] = %q, want %q", result["name"], "Test App")
246+
}
247+
})
248+
}

0 commit comments

Comments
 (0)