fix(simulation): a control test result was whatever string the client sent

internal/app/compliance/simulation.go

@@ -270,7 +270,9 @@ func (s *SimulationService) RecordControlTestResult(ctx context.Context, input R
 		return nil, err
 	}
 
-	ct.RecordResult(simulation.ControlTestStatus(input.Status), input.Evidence, input.Notes, testerID)
+	if err := ct.RecordResult(simulation.ControlTestStatus(input.Status), input.Evidence, input.Notes, testerID); err != nil {
+		return nil, err
+	}
 
 	if err := s.controlRepo.Update(ctx, ct); err != nil {
 		return nil, fmt.Errorf("failed to record control test result: %w", err)

pkg/domain/simulation/control.go

@@ -19,6 +19,27 @@ const (
 	ControlTestStatusNotApplicable ControlTestStatus = "not_applicable"
 )
 
+// AllControlTestStatuses is the complete accepted vocabulary.
+//
+// There is no CHECK constraint on control_tests.status, so this list is the
+// only thing standing between a client-supplied string and the column. Adding
+// a constant above without adding it here makes it unusable; adding it here
+// without the constant will not compile.
+func AllControlTestStatuses() []ControlTestStatus {
+	return []ControlTestStatus{
+		ControlTestStatusUntested,
+		ControlTestStatusPass,
+		ControlTestStatusFail,
+		ControlTestStatusPartial,
+		ControlTestStatusNotApplicable,
+	}
+}
+
+// IsValid reports whether the status is one the platform understands.
+func (s ControlTestStatus) IsValid() bool {
+	return slices.Contains(AllControlTestStatuses(), s)
+}
+
 // ControlTest represents a security control effectiveness test.
 type ControlTest struct {
 	id                  shared.ID
@@ -84,7 +105,7 @@ func ReconstituteControlTest(
 		framework: framework, controlID: controlID,
 		controlName: controlName, category: category,
 		testProcedure: testProcedure, expectedResult: expectedResult,
-		status: status,
+		status:       status,
 		lastTestedAt: lastTestedAt, lastTestedBy: lastTestedBy,
 		evidence: evidence, notes: notes, riskLevel: riskLevel,
 		linkedSimulationIDs: linkedSimulationIDs, tags: tags,
@@ -94,25 +115,25 @@ func ReconstituteControlTest(
 
 // Getters
 func (c *ControlTest) ID() shared.ID                 { return c.id }
-func (c *ControlTest) TenantID() shared.ID            { return c.tenantID }
-func (c *ControlTest) Name() string                   { return c.name }
-func (c *ControlTest) Description() string             { return c.description }
-func (c *ControlTest) Framework() string               { return c.framework }
-func (c *ControlTest) ControlID() string               { return c.controlID }
-func (c *ControlTest) ControlName() string             { return c.controlName }
-func (c *ControlTest) Category() string                { return c.category }
-func (c *ControlTest) TestProcedure() string           { return c.testProcedure }
-func (c *ControlTest) ExpectedResult() string          { return c.expectedResult }
-func (c *ControlTest) Status() ControlTestStatus       { return c.status }
-func (c *ControlTest) LastTestedAt() *time.Time        { return c.lastTestedAt }
-func (c *ControlTest) LastTestedBy() *shared.ID        { return c.lastTestedBy }
-func (c *ControlTest) Evidence() string                { return c.evidence }
-func (c *ControlTest) Notes() string                   { return c.notes }
-func (c *ControlTest) RiskLevel() string               { return c.riskLevel }
-func (c *ControlTest) LinkedSimulationIDs() []string   { return c.linkedSimulationIDs }
-func (c *ControlTest) Tags() []string                  { return c.tags }
-func (c *ControlTest) CreatedAt() time.Time            { return c.createdAt }
-func (c *ControlTest) UpdatedAt() time.Time            { return c.updatedAt }
+func (c *ControlTest) TenantID() shared.ID           { return c.tenantID }
+func (c *ControlTest) Name() string                  { return c.name }
+func (c *ControlTest) Description() string           { return c.description }
+func (c *ControlTest) Framework() string             { return c.framework }
+func (c *ControlTest) ControlID() string             { return c.controlID }
+func (c *ControlTest) ControlName() string           { return c.controlName }
+func (c *ControlTest) Category() string              { return c.category }
+func (c *ControlTest) TestProcedure() string         { return c.testProcedure }
+func (c *ControlTest) ExpectedResult() string        { return c.expectedResult }
+func (c *ControlTest) Status() ControlTestStatus     { return c.status }
+func (c *ControlTest) LastTestedAt() *time.Time      { return c.lastTestedAt }
+func (c *ControlTest) LastTestedBy() *shared.ID      { return c.lastTestedBy }
+func (c *ControlTest) Evidence() string              { return c.evidence }
+func (c *ControlTest) Notes() string                 { return c.notes }
+func (c *ControlTest) RiskLevel() string             { return c.riskLevel }
+func (c *ControlTest) LinkedSimulationIDs() []string { return c.linkedSimulationIDs }
+func (c *ControlTest) Tags() []string                { return c.tags }
+func (c *ControlTest) CreatedAt() time.Time          { return c.createdAt }
+func (c *ControlTest) UpdatedAt() time.Time          { return c.updatedAt }
 
 // Update sets mutable fields.
 func (c *ControlTest) Update(name, description, controlName, category string) {
@@ -133,14 +154,24 @@ func (c *ControlTest) SetTestDetails(procedure, expected string) {
 }
 
 // RecordResult records a test result.
-func (c *ControlTest) RecordResult(status ControlTestStatus, evidence, notes string, testedBy shared.ID) {
+// RecordResult stores a test outcome. An unknown status is rejected rather than
+// stored: nothing else validates it — RecordControlTestResult casts the request
+// string straight through and control_tests has no CHECK constraint — so an
+// arbitrary value would persist and then match none of the status filters,
+// leaving a control that reads as neither passed nor failed.
+func (c *ControlTest) RecordResult(status ControlTestStatus, evidence, notes string, testedBy shared.ID) error {
+	if !status.IsValid() {
+		return fmt.Errorf("%w: unknown control test status %q", shared.ErrValidation, status)
+	}
+
 	now := time.Now()
 	c.status = status
 	c.evidence = evidence
 	c.notes = notes
 	c.lastTestedAt = &now
 	c.lastTestedBy = &testedBy
 	c.updatedAt = now
+	return nil
 }
 
 // LinkSimulation links a simulation to this control test.

pkg/domain/simulation/control_status_test.go

@@ -0,0 +1,99 @@
+package simulation
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/openctemio/api/pkg/domain/shared"
+)
+
+// RecordResult used to cast whatever string arrived straight into the entity
+// and persist it. Nothing else checked: RecordControlTestResult does
+// `simulation.ControlTestStatus(input.Status)` on the raw request field, and
+// control_tests has no CHECK constraint on status — unlike compensating_controls,
+// which does.
+//
+// The consequence is quiet. A control recorded as "Pass" or "passed" is stored
+// happily and then matches neither `status === 'pass'` nor `'fail'` in the
+// Control Testing page's own summary, so it reads as neither passed nor failed —
+// a tested control that looks untested.
+
+func TestRecordResult_RejectsUnknownStatus(t *testing.T) {
+	bad := []string{
+		"Pass",           // capitalised
+		"passed",         // near-miss
+		"PARTIAL",        //
+		"not-applicable", // hyphen instead of underscore
+		"",               // empty
+		"untested ",      // trailing space
+	}
+
+	for _, in := range bad {
+		t.Run(in, func(t *testing.T) {
+			ct := &ControlTest{status: ControlTestStatusUntested}
+
+			err := ct.RecordResult(ControlTestStatus(in), "evidence", "notes", shared.NewID())
+			if err == nil {
+				t.Fatalf("status %q was accepted; it will persist and then match none "+
+					"of the status filters, so the control reads as neither passed nor "+
+					"failed", in)
+			}
+			if !errors.Is(err, shared.ErrValidation) {
+				t.Errorf("error should be a validation error, got %v", err)
+			}
+			if ct.status != ControlTestStatusUntested {
+				t.Errorf("the entity was mutated despite the rejection: status = %q", ct.status)
+			}
+			if ct.lastTestedAt != nil {
+				t.Error("lastTestedAt was set despite the rejection — the control would " +
+					"show a test date for a test that was refused")
+			}
+		})
+	}
+}
+
+func TestRecordResult_AcceptsEveryKnownStatus(t *testing.T) {
+	for _, status := range AllControlTestStatuses() {
+		t.Run(string(status), func(t *testing.T) {
+			ct := &ControlTest{status: ControlTestStatusUntested}
+			tester := shared.NewID()
+
+			if err := ct.RecordResult(status, "evidence", "notes", tester); err != nil {
+				t.Fatalf("a status from AllControlTestStatuses was rejected: %v", err)
+			}
+			if ct.status != status {
+				t.Errorf("status = %q, want %q", ct.status, status)
+			}
+			if ct.lastTestedAt == nil {
+				t.Error("lastTestedAt was not set on a successful record")
+			}
+			if ct.lastTestedBy == nil || *ct.lastTestedBy != tester {
+				t.Error("lastTestedBy was not recorded")
+			}
+		})
+	}
+}
+
+// The vocabulary must stay closed. A constant added above without being listed
+// in AllControlTestStatuses is silently unusable — IsValid would reject it and
+// the failure would look like a client bug.
+func TestAllControlTestStatuses_CoversEveryConstant(t *testing.T) {
+	declared := []ControlTestStatus{
+		ControlTestStatusUntested,
+		ControlTestStatusPass,
+		ControlTestStatusFail,
+		ControlTestStatusPartial,
+		ControlTestStatusNotApplicable,
+	}
+
+	for _, c := range declared {
+		if !c.IsValid() {
+			t.Errorf("%q is a declared constant but AllControlTestStatuses omits it, "+
+				"so RecordResult would reject a value the platform defines", c)
+		}
+	}
+	if got, want := len(AllControlTestStatuses()), len(declared); got != want {
+		t.Errorf("AllControlTestStatuses has %d entries, %d constants are declared "+
+			"in this test — add the new one to both", got, want)
+	}
+}