diff --git a/doc/changes/client-robustness-fixes.md b/doc/changes/client-robustness-fixes.md new file mode 100644 index 0000000..9fe3e5c --- /dev/null +++ b/doc/changes/client-robustness-fixes.md @@ -0,0 +1,30 @@ +# Client Robustness Fixes + +## Summary + +This change improves shared client/context behavior used by all shortcuts and raw API calls: + +- send `Accept: application/json` on every request +- send `Content-Type: application/json` when a JSON body is present +- decode GitLink responses whose `data` field is itself a JSON string into structured data +- avoid mutating caller-provided pagination query parameters in `PaginateAll` +- normalize remote URL path segments more defensively when resolving `owner/repo` + +## Why + +These fixes make CLI behavior more predictable for both human users and Agents: + +- API gateways can correctly classify request and response formats. +- JSON-string `data` payloads become usable structured output instead of opaque strings. +- Pagination helpers no longer leak `page`/default `limit` mutations back to callers. +- Repository auto-detection works better with remote URLs that include redundant slashes or trailing path separators. + +## Validation + +```bash +git diff --check +GOPROXY=https://goproxy.cn,direct go test ./internal/client ./internal/context +go vet ./internal/client ./internal/context +GOPROXY=https://goproxy.cn,direct go test ./... +go vet ./... +``` diff --git a/internal/client/client.go b/internal/client/client.go index 1fb9d80..8331e22 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -78,6 +78,10 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o if err != nil { return nil, err } + req.Header.Set("Accept", "application/json") + if bodyReader != nil { + req.Header.Set("Content-Type", "application/json") + } if c.Debug { fmt.Printf("→ %s %s\n", method, fullURL) @@ -151,7 +155,7 @@ func (c *Client) Do(method, path string, body interface{}, query url.Values) (*o if dataStr, ok := raw["data"].(string); ok { var parsedData interface{} if err := json.Unmarshal([]byte(dataStr), &parsedData); err == nil { - raw["data"] = json.RawMessage(dataStr) + raw["data"] = parsedData } } diff --git a/internal/client/client_test.go b/internal/client/client_test.go index c46226b..d531c6b 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -226,6 +226,35 @@ func TestClientDoStatusZero(t *testing.T) { } } +func TestClientDoParsesJSONStringData(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"status":0,"data":"[{\"id\":1,\"name\":\"demo\"}]"}`)) + })) + defer server.Close() + + c := &Client{HTTP: server.Client(), BaseURL: server.URL} + env, err := c.Do("GET", "/api/test", nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + raw, ok := env.Data.(map[string]interface{}) + if !ok { + t.Fatalf("Data = %T, want map[string]interface{}", env.Data) + } + items, ok := raw["data"].([]interface{}) + if !ok { + t.Fatalf("nested data = %T, want []interface{}", raw["data"]) + } + if len(items) != 1 { + t.Fatalf("expected one parsed item, got %d", len(items)) + } + item, ok := items[0].(map[string]interface{}) + if !ok || item["name"] != "demo" { + t.Fatalf("unexpected parsed item: %#v", items[0]) + } +} + func TestClientDoPaginationMeta(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -297,6 +326,12 @@ func TestClientDoWithBody(t *testing.T) { if r.Method != "POST" { t.Fatalf("expected POST, got %s", r.Method) } + if got := r.Header.Get("Accept"); got != "application/json" { + t.Fatalf("Accept = %q, want application/json", got) + } + if got := r.Header.Get("Content-Type"); got != "application/json" { + t.Fatalf("Content-Type = %q, want application/json", got) + } w.Header().Set("Content-Type", "application/json") w.Write([]byte(`{"id":123}`)) })) @@ -312,6 +347,25 @@ func TestClientDoWithBody(t *testing.T) { } } +func TestClientDoWithoutBodySetsAcceptOnly(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Accept"); got != "application/json" { + t.Fatalf("Accept = %q, want application/json", got) + } + if got := r.Header.Get("Content-Type"); got != "" { + t.Fatalf("Content-Type = %q, want empty", got) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{}`)) + })) + defer server.Close() + + c := &Client{HTTP: server.Client(), BaseURL: server.URL} + if _, err := c.Do("GET", "/api/test", nil, nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + func TestClientGet(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { @@ -498,6 +552,30 @@ func TestPaginateAllWrappedData(t *testing.T) { } } +func TestPaginateAllDoesNotMutateParams(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"data":[{"id":1}]}`)) + })) + defer server.Close() + + params := url.Values{} + params.Set("state", "open") + c := &Client{HTTP: server.Client(), BaseURL: server.URL} + if _, err := c.PaginateAll("/test", params); err != nil { + t.Fatalf("PaginateAll error: %v", err) + } + if got := params.Get("state"); got != "open" { + t.Fatalf("state param mutated to %q", got) + } + if got := params.Get("page"); got != "" { + t.Fatalf("page param leaked into caller params: %q", got) + } + if got := params.Get("limit"); got != "" { + t.Fatalf("default limit leaked into caller params: %q", got) + } +} + func TestPaginateAllSingleObject(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") diff --git a/internal/client/pagination.go b/internal/client/pagination.go index 6eafc99..2f4534a 100644 --- a/internal/client/pagination.go +++ b/internal/client/pagination.go @@ -9,19 +9,20 @@ import ( // PaginateAll fetches all pages and returns combined results. func (c *Client) PaginateAll(path string, params url.Values) ([]json.RawMessage, error) { - if params == nil { - params = url.Values{} + pageParams := cloneValues(params) + if pageParams == nil { + pageParams = url.Values{} } - if params.Get("limit") == "" { - params.Set("limit", "50") + if pageParams.Get("limit") == "" { + pageParams.Set("limit", "50") } var all []json.RawMessage page := 1 for { - params.Set("page", strconv.Itoa(page)) - env, err := c.Get(path, params) + pageParams.Set("page", strconv.Itoa(page)) + env, err := c.Get(path, pageParams) if err != nil { return nil, err } @@ -61,7 +62,7 @@ func (c *Client) PaginateAll(path string, params url.Values) ([]json.RawMessage, all = append(all, items...) // Check if we got fewer items than limit - limit, _ := strconv.Atoi(params.Get("limit")) + limit, _ := strconv.Atoi(pageParams.Get("limit")) if len(items) < limit { break } @@ -71,3 +72,14 @@ func (c *Client) PaginateAll(path string, params url.Values) ([]json.RawMessage, return all, nil } + +func cloneValues(values url.Values) url.Values { + if values == nil { + return nil + } + cloned := make(url.Values, len(values)) + for key, vals := range values { + cloned[key] = append([]string(nil), vals...) + } + return cloned +} diff --git a/internal/context/repo.go b/internal/context/repo.go index f3a7085..ef4dc4e 100644 --- a/internal/context/repo.go +++ b/internal/context/repo.go @@ -59,10 +59,10 @@ func parseRemoteURL(remote string) (string, string, error) { } func parsePathSegments(path string) (string, string, error) { - path = strings.TrimPrefix(path, "/") + path = strings.Trim(path, "/") path = strings.TrimSuffix(path, ".git") parts := strings.SplitN(path, "/", 3) - if len(parts) < 2 { + if len(parts) < 2 || parts[0] == "" || parts[1] == "" { return "", "", fmt.Errorf("cannot extract owner/repo from path: %s", path) } return parts[0], parts[1], nil diff --git a/internal/context/repo_test.go b/internal/context/repo_test.go index 00f8daa..9b84295 100644 --- a/internal/context/repo_test.go +++ b/internal/context/repo_test.go @@ -76,8 +76,10 @@ func TestParsePathSegments(t *testing.T) { {"with git", "owner/repo.git", "owner", "repo", false}, {"leading slash", "/owner/repo", "owner", "repo", false}, {"both", "/owner/repo.git", "owner", "repo", false}, + {"trailing slash", "/owner/repo.git/", "owner", "repo", false}, {"with subpath", "owner/repo/sub", "owner", "repo", false}, {"single segment", "onlyowner", "", "", true}, + {"missing repo", "onlyowner/", "", "", true}, {"empty", "", "", "", true}, } for _, tt := range tests {