Fix CI migration secret handling and error parsing

Author: paulbalaji

pkg/api/ci.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"strings"
 )
 
 var baseURLFunc = getBaseURL
@@ -62,9 +63,18 @@ func ciRequest[T any](ctx context.Context, token, orgID, path string, payload in
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		var errResp ErrorResponse
-		if err := json.Unmarshal(body, &errResp); err == nil && !errResp.OK {
+		if err := json.Unmarshal(body, &errResp); err == nil && strings.TrimSpace(errResp.Error) != "" {
 			return nil, fmt.Errorf("%s", errResp.Error)
 		}
+
+		var connectErr struct {
+			Code    string `json:"code"`
+			Message string `json:"message"`
+		}
+		if err := json.Unmarshal(body, &connectErr); err == nil && strings.TrimSpace(connectErr.Message) != "" {
+			return nil, fmt.Errorf("%s", connectErr.Message)
+		}
+
 		return nil, fmt.Errorf("API error: status %d", resp.StatusCode)
 	}
 

pkg/api/ci_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 )
 
@@ -302,6 +303,29 @@ func TestCIErrorHandling(t *testing.T) {
 	}
 }
 
+func TestCIErrorHandlingConnectMessage(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadRequest)
+		json.NewEncoder(w).Encode(map[string]string{
+			"code":    "invalid_argument",
+			"message": "invalid secret value",
+		})
+	}))
+	defer server.Close()
+
+	oldBaseURLFunc := baseURLFunc
+	baseURLFunc = func() string { return server.URL }
+	defer func() { baseURLFunc = oldBaseURLFunc }()
+
+	err := CIAddSecret(context.Background(), "test-token", "test-org", "SECRET", "value")
+	if err == nil {
+		t.Fatal("expected error for non-200 response")
+	}
+	if !strings.Contains(err.Error(), "invalid secret value") {
+		t.Fatalf("expected connect message in error, got %q", err.Error())
+	}
+}
+
 func TestCIDepotOrgHeader(t *testing.T) {
 	headerReceived := ""
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

pkg/ci/migrate/secrets.go

@@ -9,8 +9,8 @@ import (
 )
 
 var (
-	secretRegex   = regexp.MustCompile(`\$\{\{\s*secrets\.([A-Za-z_][A-Za-z0-9_]*)\s*[}\s|]`)
-	variableRegex = regexp.MustCompile(`\$\{\{\s*vars\.([A-Za-z_][A-Za-z0-9_]*)\s*[}\s|]`)
+	secretRegex   = regexp.MustCompile(`\$\{\{\s*secrets\.([A-Za-z_][A-Za-z0-9_]*)(?:\s|[^A-Za-z0-9_.]|$)`)
+	variableRegex = regexp.MustCompile(`\$\{\{\s*vars\.([A-Za-z_][A-Za-z0-9_]*)(?:\s|[^A-Za-z0-9_.]|$)`)
 )
 
 // DetectSecretsFromFile scans a file's raw content for ${{ secrets.X }} references.

pkg/ci/migrate/secrets_test.go

@@ -28,6 +28,17 @@ func TestDetectSecretsFromFileWithDefaultExpression(t *testing.T) {
 	assertStringSliceEqual(t, secrets, []string{"X"})
 }
 
+func TestDetectSecretsFromFileWithOperatorNoSpaces(t *testing.T) {
+	path := writeTempTextFile(t, t.TempDir(), "operator.yml", "if: ${{ secrets.DEPLOY_KEY!='' }}\n")
+
+	secrets, err := DetectSecretsFromFile(path)
+	if err != nil {
+		t.Fatalf("DetectSecretsFromFile failed: %v", err)
+	}
+
+	assertStringSliceEqual(t, secrets, []string{"DEPLOY_KEY"})
+}
+
 func TestDetectSecretsFromFileNoFalsePositive(t *testing.T) {
 	path := writeTempTextFile(t, t.TempDir(), "env.yml", "value: ${{ env.SOME_VAR }}\n")
 
@@ -52,6 +63,17 @@ func TestDetectVariablesFromFile(t *testing.T) {
 	assertStringSliceEqual(t, variables, []string{"MY_VAR"})
 }
 
+func TestDetectVariablesFromFileWithOperatorNoSpaces(t *testing.T) {
+	path := writeTempTextFile(t, t.TempDir(), "vars-operator.yml", "if: ${{ vars.ENV&&true }}\n")
+
+	variables, err := DetectVariablesFromFile(path)
+	if err != nil {
+		t.Fatalf("DetectVariablesFromFile failed: %v", err)
+	}
+
+	assertStringSliceEqual(t, variables, []string{"ENV"})
+}
+
 func TestDetectSecretsFromFileMultipleDeduplicated(t *testing.T) {
 	path := writeTempTextFile(t, t.TempDir(), "multi.yml", "a: ${{ secrets.A }}\nb: ${{ secrets.B }}\nc: ${{ secrets.A }}\n")
 

pkg/cmd/ci/migrate.go

@@ -223,27 +223,50 @@ func runMigrate(ctx context.Context, opts migrateOptions) error {
 		return err
 	}
 
-	if opts.yes {
-		if len(secretAssignments) > 0 {
-			orgID, token, err := resolveMigrationAuth(ctx, opts)
-			if err != nil {
-				return err
-			}
+	authResolved := false
+	var orgID, token string
+	resolveAuth := func() (string, string, error) {
+		if authResolved {
+			return orgID, token, nil
+		}
 
-			secretNames := make([]string, 0, len(secretAssignments))
-			for name := range secretAssignments {
-				secretNames = append(secretNames, name)
-			}
-			sort.Strings(secretNames)
+		var err error
+		orgID, token, err = resolveMigrationAuth(ctx, opts)
+		if err != nil {
+			return "", "", err
+		}
+		authResolved = true
+		return orgID, token, nil
+	}
 
-			for _, name := range secretNames {
-				if err := api.CIAddSecret(ctx, token, orgID, name, secretAssignments[name]); err != nil {
-					return fmt.Errorf("failed to configure secret %s: %w", name, err)
-				}
-				configuredSecrets = append(configuredSecrets, name)
+	configureSecret := func(name, value string) error {
+		orgID, token, err := resolveAuth()
+		if err != nil {
+			return err
+		}
+
+		if err := api.CIAddSecret(ctx, token, orgID, name, value); err != nil {
+			return fmt.Errorf("failed to configure secret %s: %w", name, err)
+		}
+		configuredSecrets = append(configuredSecrets, name)
+		return nil
+	}
+
+	if len(secretAssignments) > 0 {
+		secretNames := make([]string, 0, len(secretAssignments))
+		for name := range secretAssignments {
+			secretNames = append(secretNames, name)
+		}
+		sort.Strings(secretNames)
+
+		for _, name := range secretNames {
+			if err := configureSecret(name, secretAssignments[name]); err != nil {
+				return err
 			}
 		}
+	}
 
+	if opts.yes {
 		missingSecrets := make([]string, 0)
 		for _, name := range detectedSecrets {
 			if _, ok := secretAssignments[name]; !ok {
@@ -256,12 +279,11 @@ func runMigrate(ctx context.Context, opts migrateOptions) error {
 			warnings = append(warnings, fmt.Sprintf("configure missing secrets with `depot ci secrets add <NAME> --value <VALUE>` (missing: %s)", strings.Join(missingSecrets, ", ")))
 		}
 	} else if len(detectedSecrets) > 0 {
-		orgID, token, err := resolveMigrationAuth(ctx, opts)
-		if err != nil {
-			return err
-		}
-
 		for _, name := range detectedSecrets {
+			if _, ok := secretAssignments[name]; ok {
+				continue
+			}
+
 			value, err := promptForCISecret(fmt.Sprintf("Enter value for secret '%s' (leave empty to skip): ", name))
 			if err != nil {
 				return fmt.Errorf("failed to read value for secret %s: %w", name, err)
@@ -271,10 +293,9 @@ func runMigrate(ctx context.Context, opts migrateOptions) error {
 				continue
 			}
 
-			if err := api.CIAddSecret(ctx, token, orgID, name, value); err != nil {
-				return fmt.Errorf("failed to configure secret %s: %w", name, err)
+			if err := configureSecret(name, value); err != nil {
+				return err
 			}
-			configuredSecrets = append(configuredSecrets, name)
 		}
 	}