fix: remove index-based immutability check that blocked demo deploy (#2091)

* fix: remove index-based immutability check that blocked cross-manifest applies

The validateImmutability function matched instruments and account types by
array index position, producing false "code changed" errors when applying
a manifest with different composition than the previously applied one
(e.g. energy manifest after banking manifest on demo deploy).

Codes are primary keys - identity is code-based, not position-based. A
code appearing in the old manifest but not the new is a removal, already
caught by validateDestructiveChanges. The index-based check is removed
and the function made a no-op.

* fix: update applier test expecting IMMUTABLE_FIELD_CHANGED

The applier integration test also expected IMMUTABLE_FIELD_CHANGED errors
from the now-removed index-based immutability check. Updated to verify no
such errors are produced.

---------

Co-authored-by: Ben Coombs <bjcoombs@users.noreply.github.com>

Author: bjcoombs

services/control-plane/internal/applier/grpc_handler_test.go

@@ -535,11 +535,12 @@ func TestApplyManifest_ExpectedSequenceNumber_NoHistoryService_SkipsCheck(t *tes
 	assert.Equal(t, controlplanev1.ApplyManifestStatus_APPLY_MANIFEST_STATUS_DRY_RUN, resp.Status)
 }
 
-func TestApplyManifest_SkipImmutabilityChecks_NotDryRun_StillEnforces(t *testing.T) {
+func TestApplyManifest_SkipImmutabilityChecks_NotDryRun_NoImmutabilityErrors(t *testing.T) {
 	prev := newTestManifest()
 	handler := newTestHandlerWithVersionStore(t, prev)
 
-	// Change instrument code — triggers IMMUTABLE_FIELD_CHANGED
+	// Change instrument code - validateImmutability is now a no-op, so this
+	// should not produce IMMUTABLE_FIELD_CHANGED errors regardless of flags.
 	curr := newTestManifest()
 	curr.Instruments[0].Code = "USD"
 	curr.AccountTypes[0].AllowedInstruments = []string{"USD"}
@@ -548,24 +549,17 @@ func TestApplyManifest_SkipImmutabilityChecks_NotDryRun_StillEnforces(t *testing
 		Manifest:               curr,
 		DryRun:                 false,
 		AppliedBy:              "test-user",
-		SkipImmutabilityChecks: true, // should be ignored because dry_run=false
+		SkipImmutabilityChecks: true,
 	})
 
 	require.NoError(t, err)
 	require.NotNil(t, resp)
 
-	// Should fail validation because skip_immutability_checks is ignored when dry_run=false
-	assert.Equal(t, controlplanev1.ApplyManifestStatus_APPLY_MANIFEST_STATUS_VALIDATION_FAILED, resp.Status,
-		"expected VALIDATION_FAILED when skip_immutability_checks is set but dry_run=false")
-
-	found := false
+	// No IMMUTABLE_FIELD_CHANGED errors since the check is a no-op
 	for _, ve := range resp.ValidationErrors {
-		if ve.Code == "IMMUTABLE_FIELD_CHANGED" {
-			found = true
-			break
-		}
+		assert.NotEqual(t, "IMMUTABLE_FIELD_CHANGED", ve.Code,
+			"unexpected IMMUTABLE_FIELD_CHANGED error: %s", ve.Message)
 	}
-	assert.True(t, found, "expected IMMUTABLE_FIELD_CHANGED error when dry_run=false, regardless of skip flag")
 }
 
 // --- Phase Status Tests ---

services/control-plane/internal/validator/lifecycle_validator.go

@@ -97,50 +97,16 @@ func (v *ManifestValidator) detectOrphanInstructionRoutes(
 	}
 }
 
-// validateImmutability checks that immutable code fields have not been changed.
+// validateImmutability is intentionally a no-op. The previous implementation matched
+// instruments and account types by array index position, which produced false positives
+// whenever manifests had different compositions (e.g. applying an energy manifest after
+// a banking manifest). Codes are primary keys and identity is code-based, so a "rename"
+// is actually a removal + addition - already caught by validateDestructiveChanges.
 func (v *ManifestValidator) validateImmutability(
-	current *controlplanev1.Manifest,
-	previous *controlplanev1.Manifest,
-	result *ValidationResult,
+	_ *controlplanev1.Manifest,
+	_ *controlplanev1.Manifest,
+	_ *ValidationResult,
 ) {
-	// Build maps of previous codes by index position to detect renames
-	prevInstrumentsByIdx := make(map[int]string)
-	for i, inst := range previous.GetInstruments() {
-		prevInstrumentsByIdx[i] = inst.GetCode()
-	}
-
-	prevAccountTypesByIdx := make(map[int]string)
-	for i, acct := range previous.GetAccountTypes() {
-		prevAccountTypesByIdx[i] = acct.GetCode()
-	}
-
-	// Check instruments: detect code changes at same index position
-	for i, inst := range current.GetInstruments() {
-		if prevCode, existed := prevInstrumentsByIdx[i]; existed {
-			if inst.GetCode() != prevCode {
-				addError(result, ValidationError{
-					Severity: SeverityError,
-					Path:     fmt.Sprintf("instruments[%d].code", i),
-					Code:     "IMMUTABLE_FIELD_CHANGED",
-					Message:  fmt.Sprintf("instrument code changed from %q to %q; codes are immutable primary keys", prevCode, inst.GetCode()),
-				})
-			}
-		}
-	}
-
-	// Check account types: detect code changes at same index position
-	for i, acct := range current.GetAccountTypes() {
-		if prevCode, existed := prevAccountTypesByIdx[i]; existed {
-			if acct.GetCode() != prevCode {
-				addError(result, ValidationError{
-					Severity: SeverityError,
-					Path:     fmt.Sprintf("account_types[%d].code", i),
-					Code:     "IMMUTABLE_FIELD_CHANGED",
-					Message:  fmt.Sprintf("account type code changed from %q to %q; codes are immutable primary keys", prevCode, acct.GetCode()),
-				})
-			}
-		}
-	}
 }
 
 // validateDestructiveChanges detects removal of resources that have dependencies in the

services/control-plane/internal/validator/lifecycle_validator_test.go

@@ -164,91 +164,37 @@ func TestDetectOrphanInstructionRoutes_NotUsed(t *testing.T) {
 	assert.Contains(t, orphanRoutes[0].Message, "REFUND")
 }
 
-// ─── Immutability checks ─────────────────────────────────────────────────────
+// ─── Immutability checks (now a no-op; removals caught by destructive changes) ─
 
-func TestValidateImmutability_NoChange(t *testing.T) {
-	v, err := New()
-	require.NoError(t, err)
-
-	manifest := &controlplanev1.Manifest{
-		Instruments: []*controlplanev1.InstrumentDefinition{
-			{Code: "GBP", Name: "British Pound"},
-		},
-		AccountTypes: []*controlplanev1.AccountTypeDefinition{
-			{Code: "SETTLEMENT", Name: "Settlement"},
-		},
-	}
-
-	result := &ValidationResult{Valid: true}
-	v.validateImmutability(manifest, manifest, result)
-	assert.Empty(t, result.Errors)
-}
-
-func TestValidateImmutability_InstrumentCodeChanged_Direct(t *testing.T) {
+func TestValidateImmutability_IsNoOp(t *testing.T) {
 	v, err := New()
 	require.NoError(t, err)
 
+	// validateImmutability is intentionally a no-op. Different instrument/account
+	// compositions between manifests should not produce false "code changed" errors.
+	// Removals are caught by validateDestructiveChanges instead.
 	previous := &controlplanev1.Manifest{
 		Instruments: []*controlplanev1.InstrumentDefinition{
 			{Code: "GBP", Name: "British Pound"},
+			{Code: "EUR", Name: "Euro"},
 		},
-	}
-	current := &controlplanev1.Manifest{
-		Instruments: []*controlplanev1.InstrumentDefinition{
-			{Code: "USD", Name: "US Dollar"},
-		},
-	}
-
-	result := &ValidationResult{Valid: true}
-	v.validateImmutability(current, previous, result)
-
-	require.Len(t, result.Errors, 1)
-	assert.Equal(t, "IMMUTABLE_FIELD_CHANGED", result.Errors[0].Code)
-	assert.Contains(t, result.Errors[0].Message, "GBP")
-	assert.Contains(t, result.Errors[0].Message, "USD")
-}
-
-func TestValidateImmutability_AccountTypeCodeChanged_Direct(t *testing.T) {
-	v, err := New()
-	require.NoError(t, err)
-
-	previous := &controlplanev1.Manifest{
 		AccountTypes: []*controlplanev1.AccountTypeDefinition{
 			{Code: "SETTLEMENT", Name: "Settlement"},
 		},
 	}
 	current := &controlplanev1.Manifest{
-		AccountTypes: []*controlplanev1.AccountTypeDefinition{
-			{Code: "CLEARING", Name: "Clearing"},
-		},
-	}
-
-	result := &ValidationResult{Valid: true}
-	v.validateImmutability(current, previous, result)
-
-	require.Len(t, result.Errors, 1)
-	assert.Equal(t, "IMMUTABLE_FIELD_CHANGED", result.Errors[0].Code)
-}
-
-func TestValidateImmutability_AddingNewInstrument(t *testing.T) {
-	v, err := New()
-	require.NoError(t, err)
-
-	previous := &controlplanev1.Manifest{
 		Instruments: []*controlplanev1.InstrumentDefinition{
 			{Code: "GBP", Name: "British Pound"},
+			{Code: "KWH", Name: "Kilowatt Hour"},
 		},
-	}
-	current := &controlplanev1.Manifest{
-		Instruments: []*controlplanev1.InstrumentDefinition{
-			{Code: "GBP", Name: "British Pound"},
-			{Code: "USD", Name: "US Dollar"},
+		AccountTypes: []*controlplanev1.AccountTypeDefinition{
+			{Code: "ENERGY_TRADING", Name: "Energy Trading"},
 		},
 	}
 
 	result := &ValidationResult{Valid: true}
 	v.validateImmutability(current, previous, result)
-	assert.Empty(t, result.Errors)
+	assert.Empty(t, result.Errors, "validateImmutability should be a no-op")
 }
 
 // ─── dispatchInstructionRegex ────────────────────────────────────────────────

services/control-plane/internal/validator/manifest_validator_test.go

@@ -797,7 +797,7 @@ func TestValidateCrossRef_WithSuggestion(t *testing.T) {
 	}
 }
 
-func TestValidateImmutability_InstrumentCodeChanged(t *testing.T) {
+func TestValidateImmutability_InstrumentCodeChanged_NoImmutabilityError(t *testing.T) {
 	v, err := New()
 	if err != nil {
 		t.Fatalf("New() error: %v", err)
@@ -808,26 +808,16 @@ func TestValidateImmutability_InstrumentCodeChanged(t *testing.T) {
 	curr.Instruments[0].Code = "USD" // Changed from GBP
 
 	result := v.Validate(curr, prev)
-	if result.Valid {
-		t.Error("expected invalid manifest for changed instrument code")
-	}
-
-	found := false
+	// validateImmutability is now a no-op - removals are caught by destructive changes.
+	// There should be no IMMUTABLE_FIELD_CHANGED errors.
 	for _, e := range result.Errors {
-		if e.Code == "IMMUTABLE_FIELD_CHANGED" && strings.Contains(e.Path, "instruments") {
-			found = true
-			if !strings.Contains(e.Message, "GBP") || !strings.Contains(e.Message, "USD") {
-				t.Errorf("expected message to mention old and new codes, got: %s", e.Message)
-			}
-			break
+		if e.Code == "IMMUTABLE_FIELD_CHANGED" {
+			t.Errorf("unexpected IMMUTABLE_FIELD_CHANGED error: %s", e.Message)
 		}
 	}
-	if !found {
-		t.Error("expected IMMUTABLE_FIELD_CHANGED error for instruments")
-	}
 }
 
-func TestValidateImmutability_AccountTypeCodeChanged(t *testing.T) {
+func TestValidateImmutability_AccountTypeCodeChanged_NoImmutabilityError(t *testing.T) {
 	v, err := New()
 	if err != nil {
 		t.Fatalf("New() error: %v", err)
@@ -838,20 +828,12 @@ func TestValidateImmutability_AccountTypeCodeChanged(t *testing.T) {
 	curr.AccountTypes[0].Code = "SAVINGS" // Changed from SETTLEMENT
 
 	result := v.Validate(curr, prev)
-	if result.Valid {
-		t.Error("expected invalid manifest for changed account type code")
-	}
-
-	found := false
+	// validateImmutability is now a no-op - removals are caught by destructive changes.
 	for _, e := range result.Errors {
-		if e.Code == "IMMUTABLE_FIELD_CHANGED" && strings.Contains(e.Path, "account_types") {
-			found = true
-			break
+		if e.Code == "IMMUTABLE_FIELD_CHANGED" {
+			t.Errorf("unexpected IMMUTABLE_FIELD_CHANGED error: %s", e.Message)
 		}
 	}
-	if !found {
-		t.Error("expected IMMUTABLE_FIELD_CHANGED error for account_types")
-	}
 }
 
 func TestValidateImmutability_DisplayNameChangeAllowed(t *testing.T) {
@@ -2966,7 +2948,7 @@ func TestWithSkipImmutabilityChecks_SkipsDestructiveRemoval(t *testing.T) {
 	}
 }
 
-func TestWithoutSkipImmutabilityChecks_StillEnforcesImmutability(t *testing.T) {
+func TestWithoutSkipImmutabilityChecks_NoImmutabilityErrors(t *testing.T) {
 	v, err := New()
 	require.NoError(t, err)
 
@@ -2975,16 +2957,10 @@ func TestWithoutSkipImmutabilityChecks_StillEnforcesImmutability(t *testing.T) {
 	curr.Instruments[0].Code = "USD" // Changed from GBP
 
 	result := v.Validate(curr, prev)
-	assert.False(t, result.Valid)
-
-	found := false
+	// validateImmutability is now a no-op - no IMMUTABLE_FIELD_CHANGED errors expected.
 	for _, e := range result.Errors {
-		if e.Code == "IMMUTABLE_FIELD_CHANGED" {
-			found = true
-			break
-		}
+		assert.NotEqual(t, "IMMUTABLE_FIELD_CHANGED", e.Code, "unexpected IMMUTABLE_FIELD_CHANGED error: %s", e.Message)
 	}
-	assert.True(t, found, "expected IMMUTABLE_FIELD_CHANGED error without skip option")
 }
 
 // --- Market Data validation tests ---