Add UNSPECIFIED zero value for ActorSnapshotTagScope

cmd/ateapi/internal/controlapi/actor_snapshot.go

@@ -19,6 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"slices"
+	"strings"
 
 	"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
 	"github.com/agent-substrate/substrate/internal/fieldmask"
@@ -30,6 +31,8 @@ import (
 )
 
 // actorSnapshotTagScopes lists the scopes a client may set on an ActorSnapshotTag.
+// ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED is deliberately absent: scope is required
+// on the wire, not defaulted. See validateActorSnapshotTagScope.
 var actorSnapshotTagScopes = []ateapipb.ActorSnapshotTagScope{
 	ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
 	ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED,
@@ -161,9 +164,7 @@ func validateUpdateActorSnapshotTagRequest(req *ateapipb.UpdateActorSnapshotTagR
 
 	errs = append(errs, fieldmask.Validate(req.GetUpdateMask(), actorSnapshotTagMutableFields, field.NewPath("update_mask"))...)
 
-	if scope, p := tag.GetScope(), tagPath.Child("scope"); validateActorSnapshotTagScope(scope) != nil {
-		errs = append(errs, field.NotSupported(p, scope.String(), actorSnapshotTagScopeNames))
-	}
+	errs = append(errs, validateActorSnapshotTagScope(tag.GetScope(), tagPath.Child("scope"))...)
 
 	return errs
 }
@@ -267,12 +268,19 @@ func validateActorSnapshotTag(tag *ateapipb.ActorSnapshotTag, name string) error
 	if errs := resources.ValidateObjectRef(&ateapipb.ObjectRef{Atespace: tag.GetMetadata().GetAtespace(), Name: tag.GetMetadata().GetName()}, p.Child("metadata")); len(errs) > 0 {
 		return status.Error(codes.InvalidArgument, errs.ToAggregate().Error())
 	}
-	return validateActorSnapshotTagScope(tag.GetScope())
+	if errs := validateActorSnapshotTagScope(tag.GetScope(), p.Child("scope")); len(errs) > 0 {
+		return status.Error(codes.InvalidArgument, errs.ToAggregate().Error())
+	}
+	return nil
 }
 
-func validateActorSnapshotTagScope(scope ateapipb.ActorSnapshotTagScope) error {
-	if slices.Contains(actorSnapshotTagScopes, scope) {
-		return nil
+// validateActorSnapshotTagScope checks that scope is one a client may set.
+func validateActorSnapshotTagScope(scope ateapipb.ActorSnapshotTagScope, p *field.Path) field.ErrorList {
+	switch {
+	case scope == ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED:
+		return field.ErrorList{field.Required(p, "must be one of: "+strings.Join(actorSnapshotTagScopeNames, ", "))}
+	case !slices.Contains(actorSnapshotTagScopes, scope):
+		return field.ErrorList{field.NotSupported(p, scope.String(), actorSnapshotTagScopeNames)}
 	}
-	return status.Error(codes.InvalidArgument, "invalid ActorSnapshot tag scope")
+	return nil
 }

cmd/ateapi/internal/controlapi/actor_snapshot_test.go

@@ -202,14 +202,35 @@ func TestValidateUpdateActorSnapshotTagRequest(t *testing.T) {
 			wantError: field.ErrorList{field.NotSupported(field.NewPath("update_mask"), "snapshot", mutableFields)},
 		},
 		{
-			// The zero value is ATESPACE, so leaving scope unset unpublishes the tag.
 			name: "unset tag.scope",
 			req: &ateapipb.UpdateActorSnapshotTagRequest{
 				Tag: &ateapipb.ActorSnapshotTag{
 					Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"},
 				},
 				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}},
 			},
+			wantError: field.ErrorList{field.Required(field.NewPath("tag", "scope"), "")},
+		},
+		{
+			name: "explicit tag.scope UNSPECIFIED",
+			req: &ateapipb.UpdateActorSnapshotTagRequest{
+				Tag: &ateapipb.ActorSnapshotTag{
+					Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"},
+					Scope:    ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED,
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}},
+			},
+			wantError: field.ErrorList{field.Required(field.NewPath("tag", "scope"), "")},
+		},
+		{
+			name: "tag.scope ATESPACE explicitly unpublishes",
+			req: &ateapipb.UpdateActorSnapshotTagRequest{
+				Tag: &ateapipb.ActorSnapshotTag{
+					Metadata: &ateapipb.ResourceMetadata{Atespace: "ns1", Name: "tag1"},
+					Scope:    ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}},
+			},
 			wantError: nil,
 		},
 		{
@@ -240,18 +261,18 @@ func TestUpdateActorSnapshotTag_FieldMasks(t *testing.T) {
 		want      *ateapipb.ActorSnapshotTag
 	}{
 		{
-			name:      "mask sets scope",
-			stored:    &ateapipb.ActorSnapshotTag{},
+			name:      "mask publishes an atespace-scoped tag",
+			stored:    &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE},
 			req:       &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED},
 			maskPaths: []string{"scope"},
 			want:      &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED},
 		},
 		{
-			name:      "mask clears scope left unset on request, resetting to the zero value",
+			name:      "mask unpublishes a published tag",
 			stored:    &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED},
-			req:       &ateapipb.ActorSnapshotTag{},
+			req:       &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE},
 			maskPaths: []string{"scope"},
-			want:      &ateapipb.ActorSnapshotTag{},
+			want:      &ateapipb.ActorSnapshotTag{Scope: ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE},
 		},
 	}
 	for _, tt := range tests {
@@ -280,6 +301,59 @@ func TestUpdateActorSnapshotTag_FieldMasks(t *testing.T) {
 	}
 }
 
+// TestUpdateActorSnapshotTag_UnsetScopeDoesNotUnpublish checks that masking
+// scope without populating it is rejected.
+func TestUpdateActorSnapshotTag_UnsetScopeDoesNotUnpublish(t *testing.T) {
+	ctx := context.Background()
+	svc, stored := serviceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{
+		Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag1"},
+		Scope:    ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED,
+	})
+
+	_, err := svc.UpdateActorSnapshotTag(ctx, &ateapipb.UpdateActorSnapshotTagRequest{
+		Tag: &ateapipb.ActorSnapshotTag{
+			Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag1"},
+		},
+		UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"scope"}},
+	})
+	if code := status.Code(err); code != codes.InvalidArgument {
+		t.Errorf("UpdateActorSnapshotTag error = %v (code %v), want code InvalidArgument", err, code)
+	}
+
+	_, current, err := svc.persistence.GetActorSnapshotByTag(ctx, testAtespace, "tag1")
+	if err != nil {
+		t.Fatalf("GetActorSnapshotByTag: %v", err)
+	}
+	if got, want := current.GetScope(), ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED; got != want {
+		t.Errorf("stored scope = %v, want %v: the rejected update must not have unpublished the tag", got, want)
+	}
+	if got, want := current.GetMetadata().GetVersion(), stored.GetMetadata().GetVersion(); got != want {
+		t.Errorf("stored version = %d, want %d: the rejected update must not have written", got, want)
+	}
+}
+
+// TestTagActorSnapshot_RejectsUnsetScope checks that scope is required at
+// creation.
+func TestTagActorSnapshot_RejectsUnsetScope(t *testing.T) {
+	ctx := context.Background()
+	svc, stored := serviceWithActorSnapshotTag(t, &ateapipb.ActorSnapshotTag{
+		Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag1"},
+		Scope:    ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE,
+	})
+
+	_, err := svc.TagActorSnapshot(ctx, &ateapipb.TagActorSnapshotRequest{
+		Snapshot: &ateapipb.ActorSnapshotRef{
+			Reference: &ateapipb.ActorSnapshotRef_Snapshot{Snapshot: stored.GetSnapshot()},
+		},
+		Tag: &ateapipb.ActorSnapshotTag{
+			Metadata: &ateapipb.ResourceMetadata{Atespace: testAtespace, Name: "tag2"},
+		},
+	})
+	if code := status.Code(err); code != codes.InvalidArgument {
+		t.Errorf("TagActorSnapshot error = %v (code %v), want code InvalidArgument", err, code)
+	}
+}
+
 // serviceWithActorSnapshotTag seeds an ActorSnapshot and a tag pointing at it
 // in a miniredis-backed store, and returns a Service over it.
 func serviceWithActorSnapshotTag(t *testing.T, tag *ateapipb.ActorSnapshotTag) (*Service, *ateapipb.ActorSnapshotTag) {

cmd/kubectl-ate/internal/cmd/actor_snapshots.go

@@ -187,7 +187,7 @@ func parseActorSnapshotTagScope(value string) (ateapipb.ActorSnapshotTagScope, e
 	case "published":
 		return ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED, nil
 	default:
-		return 0, fmt.Errorf("invalid scope %q; must be atespace or published", value)
+		return ateapipb.ActorSnapshotTagScope_ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED, fmt.Errorf("invalid scope %q; must be atespace or published", value)
 	}
 }
 

pkg/proto/ateapipb/ateapi.proto

@@ -132,11 +132,13 @@ enum SnapshotContentScope {
 }
 
 enum ActorSnapshotTagScope {
+  // Not set and rejected wherever a client supplies a scope
+  ACTOR_SNAPSHOT_TAG_SCOPE_UNSPECIFIED = 0;
   // May initialize Actors only in the tag's owning Atespace.
-  ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE = 0;
+  ACTOR_SNAPSHOT_TAG_SCOPE_ATESPACE = 1;
   // Published for use by Actors in any Atespace. The tag remains addressed
   // through its owning Atespace.
-  ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED = 1;
+  ACTOR_SNAPSHOT_TAG_SCOPE_PUBLISHED = 2;
 }
 
 // Selector matches worker pools by label.