Fix connection upsert bugs with rules, auth, and partial updates

.github/workflows/test.yml

@@ -10,6 +10,18 @@ on:
       - main
 
 jobs:
+  unit-test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Code checkout
+        uses: actions/checkout@v4
+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: 1.24.9
+      - name: Run unit tests
+        run: go test -short ./pkg/...
+
   build-mac:
     runs-on: macos-latest
     steps:

pkg/cmd/connection_common.go

@@ -191,7 +191,11 @@ func buildConnectionRules(f *connectionRuleFlags) ([]hookdeck.Rule, error) {
 			rule["interval"] = f.RuleRetryInterval
 		}
 		if f.RuleRetryResponseStatusCode != "" {
-			rule["response_status_codes"] = f.RuleRetryResponseStatusCode
+			codes := strings.Split(f.RuleRetryResponseStatusCode, ",")
+			for i := range codes {
+				codes[i] = strings.TrimSpace(codes[i])
+			}
+			rule["response_status_codes"] = codes
 		}
 		rules = append(rules, rule)
 	}

pkg/cmd/connection_upsert.go

@@ -94,7 +94,7 @@ func newConnectionUpsertCmd() *connectionUpsertCmd {
 	cu.cmd.Flags().StringVar(&cu.destinationType, "destination-type", "", "Destination type (CLI, HTTP, MOCK)")
 	cu.cmd.Flags().StringVar(&cu.destinationDescription, "destination-description", "", "Destination description")
 	cu.cmd.Flags().StringVar(&cu.destinationURL, "destination-url", "", "URL for HTTP destinations")
-	cu.cmd.Flags().StringVar(&cu.destinationCliPath, "destination-cli-path", "/", "CLI path for CLI destinations (default: /)")
+	cu.cmd.Flags().StringVar(&cu.destinationCliPath, "destination-cli-path", "", "CLI path for CLI destinations (default: / for new connections)")
 
 	// Use a string flag to allow explicit true/false values
 	var pathForwardingDisabledStr string
@@ -257,10 +257,10 @@ func (cu *connectionUpsertCmd) validateSourceFlags() error {
 		return fmt.Errorf("cannot use --source-id with --source-name or --source-type")
 	}
 
-	// If creating inline, require both name and type
-	if (cu.sourceName != "" || cu.sourceType != "") && (cu.sourceName == "" || cu.sourceType == "") {
-		return fmt.Errorf("both --source-name and --source-type are required for inline source creation")
-	}
+	// For upsert, we don't require both --source-name and --source-type.
+	// If the connection already exists, providing just --source-name is valid
+	// (the existing source type will be preserved). The API will reject
+	// incomplete data if this is actually a create.
 
 	return nil
 }
@@ -272,10 +272,10 @@ func (cu *connectionUpsertCmd) validateDestinationFlags() error {
 		return fmt.Errorf("cannot use --destination-id with --destination-name or --destination-type")
 	}
 
-	// If creating inline, require both name and type
-	if (cu.destinationName != "" || cu.destinationType != "") && (cu.destinationName == "" || cu.destinationType == "") {
-		return fmt.Errorf("both --destination-name and --destination-type are required for inline destination creation")
-	}
+	// For upsert, we don't require both --destination-name and --destination-type.
+	// If the connection already exists, providing just --destination-name is valid
+	// (the existing destination type will be preserved). The API will reject
+	// incomplete data if this is actually a create.
 
 	return nil
 }
@@ -304,7 +304,11 @@ func (cu *connectionUpsertCmd) runConnectionUpsertCmd(cmd *cobra.Command, args [
 		cu.DestinationRateLimit != 0 || cu.DestinationAuthMethod != "") &&
 		cu.destinationName == "" && cu.destinationType == "" && cu.destinationID == ""
 
-	needsExisting := cu.dryRun || (!cu.hasAnySourceFlag() && !cu.hasAnyDestinationFlag()) || hasSourceConfigOnly || hasDestinationConfigOnly
+	// Also need to fetch existing when name is provided without type (to fill in the type)
+	hasPartialSourceInline := (cu.sourceName != "" && cu.sourceType == "" && cu.sourceID == "")
+	hasPartialDestinationInline := (cu.destinationName != "" && cu.destinationType == "" && cu.destinationID == "")
+
+	needsExisting := cu.dryRun || (!cu.hasAnySourceFlag() && !cu.hasAnyDestinationFlag()) || hasSourceConfigOnly || hasDestinationConfigOnly || hasPartialSourceInline || hasPartialDestinationInline
 
 	var existing *hookdeck.Connection
 	var isUpdate bool
@@ -411,6 +415,10 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection,
 	if cu.sourceID != "" {
 		req.SourceID = &cu.sourceID
 	} else if cu.sourceName != "" || cu.sourceType != "" {
+		// For upsert updates, fill in missing source type from existing connection
+		if cu.sourceType == "" && isUpdate && existing != nil && existing.Source != nil {
+			cu.sourceType = existing.Source.Type
+		}
 		sourceInput, err := cu.buildSourceInput()
 		if err != nil {
 			return nil, err
@@ -442,6 +450,14 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection,
 	if cu.destinationID != "" {
 		req.DestinationID = &cu.destinationID
 	} else if cu.destinationName != "" || cu.destinationType != "" {
+		// For upsert updates, fill in missing destination type from existing connection
+		if cu.destinationType == "" && isUpdate && existing != nil && existing.Destination != nil {
+			cu.destinationType = existing.Destination.Type
+		}
+		// Default CLI path to "/" for new CLI destinations when not explicitly set
+		if strings.ToUpper(cu.destinationType) == "CLI" && cu.destinationCliPath == "" {
+			cu.destinationCliPath = "/"
+		}
 		destinationInput, err := cu.buildDestinationInput()
 		if err != nil {
 			return nil, err
@@ -469,10 +485,13 @@ func (cu *connectionUpsertCmd) buildUpsertRequest(existing *hookdeck.Connection,
 		}
 	}
 
-	// Also preserve source if not specified
+	// Preserve existing source/destination if not specified
 	if req.SourceID == nil && req.Source == nil && isUpdate && existing != nil && existing.Source != nil {
 		req.SourceID = &existing.Source.ID
 	}
+	if req.DestinationID == nil && req.Destination == nil && isUpdate && existing != nil && existing.Destination != nil {
+		req.DestinationID = &existing.Destination.ID
+	}
 
 	// Handle Rules
 	rules, err := buildConnectionRules(&cu.connectionCreateCmd.connectionRuleFlags)

pkg/cmd/connection_upsert_test.go

@@ -0,0 +1,239 @@
+package cmd
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func strPtr(s string) *string {
+	return &s
+}
+
+// TestBuildConnectionRulesRetryStatusCodesArray verifies that buildConnectionRules
+// produces response_status_codes as a []string array, not a single string.
+// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 3.
+func TestBuildConnectionRulesRetryStatusCodesArray(t *testing.T) {
+	tests := []struct {
+		name          string
+		flags         connectionRuleFlags
+		wantCodes     []string
+		wantCodeCount int
+		wantRuleCount int
+	}{
+		{
+			name: "comma-separated status codes should produce array",
+			flags: connectionRuleFlags{
+				RuleRetryStrategy:           "linear",
+				RuleRetryCount:              3,
+				RuleRetryInterval:           5000,
+				RuleRetryResponseStatusCode: "500,502,503,504",
+			},
+			wantCodes:     []string{"500", "502", "503", "504"},
+			wantCodeCount: 4,
+			wantRuleCount: 1,
+		},
+		{
+			name: "single status code should produce single-element array",
+			flags: connectionRuleFlags{
+				RuleRetryStrategy:           "exponential",
+				RuleRetryResponseStatusCode: "500",
+			},
+			wantCodes:     []string{"500"},
+			wantCodeCount: 1,
+			wantRuleCount: 1,
+		},
+		{
+			name: "status codes with spaces should be trimmed",
+			flags: connectionRuleFlags{
+				RuleRetryStrategy:           "linear",
+				RuleRetryResponseStatusCode: "500, 502, 503",
+			},
+			wantCodes:     []string{"500", "502", "503"},
+			wantCodeCount: 3,
+			wantRuleCount: 1,
+		},
+		{
+			name: "no status codes should not include response_status_codes",
+			flags: connectionRuleFlags{
+				RuleRetryStrategy: "linear",
+				RuleRetryCount:    3,
+			},
+			wantCodes:     nil,
+			wantCodeCount: 0,
+			wantRuleCount: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			rules, err := buildConnectionRules(&tt.flags)
+			require.NoError(t, err, "buildConnectionRules should not error")
+			require.Len(t, rules, tt.wantRuleCount, "Expected %d rule(s)", tt.wantRuleCount)
+
+			if tt.wantRuleCount == 0 {
+				return
+			}
+
+			retryRule := rules[len(rules)-1]
+			assert.Equal(t, "retry", retryRule["type"], "Last rule should be retry")
+
+			if tt.wantCodes == nil {
+				_, exists := retryRule["response_status_codes"]
+				assert.False(t, exists, "response_status_codes should not be present when not specified")
+				return
+			}
+
+			statusCodes, ok := retryRule["response_status_codes"]
+			require.True(t, ok, "response_status_codes should be present")
+
+			codesSlice, ok := statusCodes.([]string)
+			require.True(t, ok, "response_status_codes should be []string, got %T", statusCodes)
+			assert.Equal(t, tt.wantCodeCount, len(codesSlice))
+			assert.Equal(t, tt.wantCodes, codesSlice)
+
+			// Verify it serializes to a JSON array
+			jsonBytes, err := json.Marshal(retryRule)
+			require.NoError(t, err)
+
+			var parsed map[string]interface{}
+			err = json.Unmarshal(jsonBytes, &parsed)
+			require.NoError(t, err)
+
+			jsonCodes, ok := parsed["response_status_codes"].([]interface{})
+			require.True(t, ok, "JSON response_status_codes should be an array, got %T", parsed["response_status_codes"])
+			assert.Len(t, jsonCodes, tt.wantCodeCount)
+		})
+	}
+}
+
+// TestUpsertBuildRequestRulesOnlyPreservesDestinationByID verifies that when
+// upserting with only rule flags, the request uses destination_id (not a full
+// destination object that could include incomplete auth config).
+// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1.
+func TestUpsertBuildRequestRulesOnlyPreservesDestinationByID(t *testing.T) {
+	cu := &connectionUpsertCmd{
+		connectionCreateCmd: &connectionCreateCmd{},
+	}
+	cu.name = "test-conn"
+	// Only set rule flags, no source/destination flags
+	cu.RuleRetryStrategy = "linear"
+	cu.RuleRetryCount = 3
+
+	existing := &hookdeck.Connection{
+		ID:   "conn_123",
+		Name: strPtr("test-conn"),
+		Source: &hookdeck.Source{
+			ID:   "src_123",
+			Name: "test-source",
+			Type: "WEBHOOK",
+		},
+		Destination: &hookdeck.Destination{
+			ID:   "dst_123",
+			Name: "test-dest",
+			Type: "HTTP",
+			Config: map[string]interface{}{
+				"url":       "https://api.example.com",
+				"auth_type": "AWS_SIGNATURE",
+			},
+		},
+	}
+
+	req, err := cu.buildUpsertRequest(existing, true)
+	require.NoError(t, err)
+
+	// Should reference existing destination by ID, not recreate it
+	assert.NotNil(t, req.DestinationID, "Should use DestinationID")
+	assert.Equal(t, "dst_123", *req.DestinationID)
+	assert.Nil(t, req.Destination, "Should NOT send full Destination object")
+
+	// Source should also be preserved by ID
+	assert.NotNil(t, req.SourceID, "Should use SourceID")
+	assert.Equal(t, "src_123", *req.SourceID)
+	assert.Nil(t, req.Source, "Should NOT send full Source object")
+
+	// Rules should be present
+	assert.NotEmpty(t, req.Rules)
+}
+
+// TestUpsertHasAnyDestinationFlagIgnoresDefault verifies that hasAnyDestinationFlag
+// returns false when no destination flags are explicitly set (the cli-path default
+// of "/" was previously causing this to always return true).
+// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 1.
+func TestUpsertHasAnyDestinationFlagIgnoresDefault(t *testing.T) {
+	cu := &connectionUpsertCmd{
+		connectionCreateCmd: &connectionCreateCmd{},
+	}
+	// No destination flags set at all (cli-path default is "" for upsert)
+
+	result := cu.hasAnyDestinationFlag()
+	assert.False(t, result, "hasAnyDestinationFlag should be false with no destination flags set")
+}
+
+// TestUpsertValidateSourceFlagsAllowsNameOnly verifies that validateSourceFlags
+// allows --source-name without --source-type for the upsert command.
+// Regression test for https://github.com/hookdeck/hookdeck-cli/issues/209 Bug 2.
+func TestUpsertValidateSourceFlagsAllowsNameOnly(t *testing.T) {
+	cu := &connectionUpsertCmd{
+		connectionCreateCmd: &connectionCreateCmd{},
+	}
+	cu.sourceName = "my-source"
+	// sourceType intentionally empty
+
+	err := cu.validateSourceFlags()
+	assert.NoError(t, err, "validateSourceFlags should allow --source-name alone for upsert")
+}
+
+// TestUpsertValidateDestinationFlagsAllowsNameOnly verifies the same relaxation
+// for destination flags.
+func TestUpsertValidateDestinationFlagsAllowsNameOnly(t *testing.T) {
+	cu := &connectionUpsertCmd{
+		connectionCreateCmd: &connectionCreateCmd{},
+	}
+	cu.destinationName = "my-dest"
+	// destinationType intentionally empty
+
+	err := cu.validateDestinationFlags()
+	assert.NoError(t, err, "validateDestinationFlags should allow --destination-name alone for upsert")
+}
+
+// TestUpsertBuildRequestFillsSourceTypeFromExisting verifies that when
+// --source-name is provided without --source-type during an update,
+// the existing source type is used.
+func TestUpsertBuildRequestFillsSourceTypeFromExisting(t *testing.T) {
+	cu := &connectionUpsertCmd{
+		connectionCreateCmd: &connectionCreateCmd{},
+	}
+	cu.name = "test-conn"
+	cu.sourceName = "new-source-name"
+	// sourceType intentionally empty - should be filled from existing
+
+	existing := &hookdeck.Connection{
+		ID:   "conn_123",
+		Name: strPtr("test-conn"),
+		Source: &hookdeck.Source{
+			ID:   "src_123",
+			Name: "old-source-name",
+			Type: "WEBHOOK",
+		},
+		Destination: &hookdeck.Destination{
+			ID:   "dst_123",
+			Name: "test-dest",
+			Type: "HTTP",
+			Config: map[string]interface{}{
+				"url": "https://api.example.com",
+			},
+		},
+	}
+
+	req, err := cu.buildUpsertRequest(existing, true)
+	require.NoError(t, err)
+
+	// Source should be an inline input (not just ID) since name was changed
+	require.NotNil(t, req.Source, "Should have Source input for name change")
+	assert.Equal(t, "new-source-name", req.Source.Name)
+	assert.Equal(t, "WEBHOOK", req.Source.Type, "Should fill type from existing source")
+}