Address review: URL-escape page token, explicit parse errors, nil-safe constructor, connection error body

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

Author: Copilot

provider/gcp/deploy_test.go

@@ -340,7 +340,8 @@ func TestTestConnection_Success(t *testing.T) {
 	}
 }
 
-// TestTestConnection_Failure verifies that TestConnection returns Success=false on HTTP 403.
+// TestTestConnection_Failure verifies that TestConnection returns Success=false on HTTP 403
+// and includes the response body in the failure message.
 func TestTestConnection_Failure(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusForbidden)
@@ -359,6 +360,10 @@ func TestTestConnection_Failure(t *testing.T) {
 	if !strings.Contains(result.Message, "HTTP 403") {
 		t.Errorf("expected message to mention HTTP 403, got: %s", result.Message)
 	}
+	// The response body should be included in the failure message for easier diagnosis.
+	if !strings.Contains(result.Message, "permission denied") {
+		t.Errorf("expected message to include response body, got: %s", result.Message)
+	}
 }
 
 // TestGetMetrics_Success verifies that GetMetrics parses a Cloud Monitoring response correctly.

provider/gcp/plugin.go

@@ -56,13 +56,22 @@ func NewGCPProvider(config GCPConfig) *GCPProvider {
 }
 
 // NewGCPProviderWithClient creates a GCPProvider with an injectable HTTP client and token
-// function. This constructor is intended for testing.
-func NewGCPProviderWithClient(config GCPConfig, client HTTPDoer, tokenFunc func(ctx context.Context) (string, error)) *GCPProvider {
-	return &GCPProvider{
+// function. Nil arguments are replaced with safe defaults: a 30-second http.Client and
+// the provider's Application Default Credentials token function.
+func NewGCPProviderWithClient(config GCPConfig, client HTTPDoer, tokenFn func(ctx context.Context) (string, error)) *GCPProvider {
+	if client == nil {
+		client = &http.Client{Timeout: 30 * time.Second}
+	}
+	p := &GCPProvider{
 		config:     config,
 		httpClient: client,
-		tokenFunc:  tokenFunc,
 	}
+	if tokenFn == nil {
+		p.tokenFunc = p.defaultTokenFunc
+	} else {
+		p.tokenFunc = tokenFn
+	}
+	return p
 }
 
 // defaultTokenFunc retrieves a GCP OAuth2 access token using Application Default Credentials
@@ -286,9 +295,18 @@ func (p *GCPProvider) TestConnection(ctx context.Context, _ map[string]any) (*pr
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {
+		const maxBodyBytes = 1024
+		var bodySnippet string
+		if data, readErr := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)); readErr == nil && len(data) > 0 {
+			bodySnippet = strings.TrimSpace(string(data))
+		}
+		msg := fmt.Sprintf("gcp: project %q returned HTTP %d", p.config.ProjectID, resp.StatusCode)
+		if bodySnippet != "" {
+			msg = fmt.Sprintf("%s: %s", msg, bodySnippet)
+		}
 		return &provider.ConnectionResult{
 			Success: false,
-			Message: fmt.Sprintf("gcp: project %q returned HTTP %d", p.config.ProjectID, resp.StatusCode),
+			Message: msg,
 			Latency: latency,
 		}, nil
 	}

provider/gcp/registry.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -47,7 +48,7 @@ func (p *GCPProvider) ListImages(ctx context.Context, repo string) ([]provider.I
 	for {
 		reqURL := endpoint
 		if pageToken != "" {
-			reqURL += "?pageToken=" + pageToken
+			reqURL += "?pageToken=" + url.QueryEscape(pageToken)
 		}
 		resp, err := p.doRequest(ctx, http.MethodGet, reqURL, nil)
 		if err != nil {
@@ -67,8 +68,19 @@ func (p *GCPProvider) ListImages(ctx context.Context, repo string) ([]provider.I
 			return nil, fmt.Errorf("gcp: failed to parse ListImages response: %w", err)
 		}
 		for _, img := range result.DockerImages {
-			sizeBytes, _ := strconv.ParseInt(img.ImageSizeBytes, 10, 64)
-			pushedAt, _ := time.Parse(time.RFC3339, img.UploadTime)
+			sizeBytes, err := strconv.ParseInt(img.ImageSizeBytes, 10, 64)
+			if err != nil && img.ImageSizeBytes != "" {
+				return nil, fmt.Errorf("gcp: failed to parse image size %q for image %q: %w",
+					img.ImageSizeBytes, img.URI, err)
+			}
+			var pushedAt time.Time
+			if img.UploadTime != "" {
+				pushedAt, err = time.Parse(time.RFC3339, img.UploadTime)
+				if err != nil {
+					return nil, fmt.Errorf("gcp: failed to parse upload time %q for image %q: %w",
+						img.UploadTime, img.URI, err)
+				}
+			}
 			tags := img.Tags
 			if len(tags) == 0 {
 				tags = []string{""}

provider/gcp/registry_test.go

@@ -297,3 +297,67 @@ func TestPushImage_FallbackTokenFromProvider(t *testing.T) {
 		t.Fatalf("PushImage with fallback token: %v", err)
 	}
 }
+
+// TestListImages_InvalidSizeBytes verifies that ListImages returns an error when
+// the API returns a non-numeric imageSizeBytes value.
+func TestListImages_InvalidSizeBytes(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		resp := artifactRegistryDockerImagesResponse{
+			DockerImages: []artifactRegistryDockerImage{
+				{URI: "gcr.io/proj/img", Tags: []string{"v1"}, ImageSizeBytes: "not-a-number"},
+			},
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(resp)
+	}))
+	defer server.Close()
+
+	p := newTestProvider(server, GCPConfig{ProjectID: "proj", Region: "us"})
+	_, err := p.ListImages(context.Background(), "repo")
+	if err == nil {
+		t.Fatal("expected error for invalid imageSizeBytes")
+	}
+	if !strings.Contains(err.Error(), "parse image size") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+// TestListImages_InvalidUploadTime verifies that ListImages returns an error when
+// the API returns a non-RFC3339 uploadTime value.
+func TestListImages_InvalidUploadTime(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		resp := artifactRegistryDockerImagesResponse{
+			DockerImages: []artifactRegistryDockerImage{
+				{URI: "gcr.io/proj/img", Tags: []string{"v1"}, ImageSizeBytes: "100", UploadTime: "not-a-time"},
+			},
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(resp)
+	}))
+	defer server.Close()
+
+	p := newTestProvider(server, GCPConfig{ProjectID: "proj", Region: "us"})
+	_, err := p.ListImages(context.Background(), "repo")
+	if err == nil {
+		t.Fatal("expected error for invalid uploadTime")
+	}
+	if !strings.Contains(err.Error(), "parse upload time") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+// TestNewGCPProviderWithClient_NilSafety verifies that nil client and tokenFunc
+// arguments do not cause panics; the provider falls back to safe defaults.
+func TestNewGCPProviderWithClient_NilSafety(t *testing.T) {
+	// Should not panic.
+	p := NewGCPProviderWithClient(GCPConfig{ProjectID: "proj"}, nil, nil)
+	if p == nil {
+		t.Fatal("expected non-nil provider")
+	}
+	if p.httpClient == nil {
+		t.Error("expected non-nil httpClient after nil default")
+	}
+	if p.tokenFunc == nil {
+		t.Error("expected non-nil tokenFunc after nil default")
+	}
+}