[AI-FSSDK] [FSSDK-12760] add localHoldouts to datafile for backward compatibility (#453)

* [AI-FSSDK] [FSSDK-12760] add localHoldouts to datafile for backward compatibility

* [FSSDK-12760] Fix duplicate ID collision, log level, and brittle test

- Log a warning when the same holdout ID appears in both datafile
  sections (the later entry overwrites the earlier one in the ID map)
- Downgrade missing-includedRules log from Error to Warning — this is
  a data quality issue, not a runtime failure
- Replace brittle TestMapHoldoutsErrorMessageWording (asserted on
  specific words) with TestMapHoldoutsDuplicateIDsAcrossSectionsLogsWarning

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* [FSSDK-12760] Empty commit to re-trigger CI

* chore: trigger CI rerun

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: use rule Key instead of ID for local holdout lookup (#454)

GetLocalDecisionForRule was called with experiment.ID, but ruleHoldoutsMap
is keyed by rule Keys (from datafile includedRules). The mismatch caused
local holdouts to silently never match.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* [FSSDK-12670] Retrigger CI

* [FSSDK-12670] retrigger CI

* Trigger CI

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: revert to using experiment ID for local holdout lookup

The datafile `includedRules` contains experiment IDs, not keys.
PR #454 incorrectly changed the lookup to use Key, causing
ruleHoldoutsMap lookups to never match.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: update ODP test to expect no identify events for single identifier

TestOdpEventsEarlyEventsDispatched was waiting for identify events
that are now correctly blocked by the single-identifier guard.
Updated to verify no events are sent.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Matjaz Pirnovar <Mat001@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: 3 people

pkg/client/optimizely_user_context_odp_test.go

@@ -210,26 +210,21 @@ func (o *OptimizelyUserContextODPTestSuite) TestFetchQualifiedSegmentsParameters
 	segmentManager.AssertExpectations(o.T())
 }
 
-func (o *OptimizelyUserContextODPTestSuite) TestOdpEventsEarlyEventsDispatched() {
+func (o *OptimizelyUserContextODPTestSuite) TestOdpIdentifySkippedForSingleIdentifier() {
 	eventAPIManager := &MockEventAPIManager{}
 	eventManager := event.NewBatchEventManager(event.WithAPIManager(eventAPIManager), event.WithFlushInterval(0))
 	odpManager := odp.NewOdpManager("", false, odp.WithEventManager(eventManager))
 	factory := OptimizelyFactory{Datafile: o.datafile, odpManager: odpManager}
 	optimizelyClient, _ := factory.Client()
-	eventAPIManager.wg.Add(1)
-	// identified event will be sent
-	_ = optimizelyClient.CreateUserContext(o.userID, nil)
-	eventAPIManager.wg.Wait()
-
-	o.Equal(1, len(eventAPIManager.eventsSent))
 
-	expectedEvents := 100
-	eventAPIManager.wg.Add(expectedEvents)
-	for i := 0; i < expectedEvents; i++ {
+	// Server-side SDKs only have fs_user_id (single identifier),
+	// so CreateUserContext should NOT dispatch an identify event.
+	_ = optimizelyClient.CreateUserContext(o.userID, nil)
+	for i := 0; i < 100; i++ {
 		_ = optimizelyClient.CreateUserContext(fmt.Sprintf("%d", i), nil)
 	}
-	eventAPIManager.wg.Wait()
-	o.Equal(expectedEvents+1, len(eventAPIManager.eventsSent))
+
+	o.Equal(0, len(eventAPIManager.eventsSent))
 }
 
 // Tests with live ODP server

pkg/config/datafileprojectconfig/config.go

@@ -62,9 +62,9 @@ type DatafileProjectConfig struct {
 	flagVariationsMap map[string][]entities.Variation
 	holdouts          []entities.Holdout
 	holdoutIDMap      map[string]entities.Holdout
-	// ruleHoldoutsMap maps rule IDs to local holdouts targeting those rules
+	// ruleHoldoutsMap maps rule IDs to local holdouts (from the `localHoldouts` datafile section) targeting those rules.
 	ruleHoldoutsMap map[string][]entities.Holdout
-	// globalHoldouts holds only global holdouts (IncludedRules == nil)
+	// globalHoldouts holds running entries from the `holdouts` datafile section.
 	globalHoldouts []entities.Holdout
 }
 
@@ -287,14 +287,14 @@ func (c DatafileProjectConfig) GetRegion() string {
 	return c.region
 }
 
-// GetGlobalHoldouts returns all global holdouts (those with IncludedRules == nil).
-// These are evaluated at flag level, before any per-rule evaluation.
+// GetGlobalHoldouts returns all running global holdouts from the `holdouts` datafile section.
+// Evaluated at flag level before any per-rule evaluation; section membership is the sole signal for scope.
 func (c DatafileProjectConfig) GetGlobalHoldouts() []entities.Holdout {
 	return c.globalHoldouts
 }
 
-// GetHoldoutsForRule returns all local holdouts that target the given rule ID.
-// These are evaluated per-rule, after forced decisions, before audience/traffic checks.
+// GetHoldoutsForRule returns running local holdouts (from the `localHoldouts` datafile section)
+// that target the given rule ID. Evaluated per-rule, after forced decisions, before audience/traffic checks.
 func (c DatafileProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout {
 	if holdouts, exists := c.ruleHoldoutsMap[ruleID]; exists {
 		return holdouts
@@ -348,7 +348,11 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP
 
 	audienceMap, audienceSegmentList := mappers.MapAudiences(append(datafile.TypedAudiences, datafile.Audiences...))
 	flagVariationsMap := mappers.MapFlagVariations(featureMap)
-	holdouts, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts)
+	// Two top-level holdout sections drive scope (FSSDK-12760):
+	//   `holdouts`      → global holdouts (every entry; `includedRules` stripped)
+	//   `localHoldouts` → local holdouts (must carry `includedRules`; otherwise logged and skipped)
+	// Older datafiles without `localHoldouts` continue to work unchanged.
+	holdouts, holdoutIDMap, globalHoldouts, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, datafile.LocalHoldouts, logger)
 
 	attributeKeyMap := make(map[string]entities.Attribute)
 	attributeIDToKeyMap := make(map[string]string)

pkg/config/datafileprojectconfig/config_test.go

@@ -767,3 +767,131 @@ func TestGetHoldoutsForRuleWithNoHoldouts(t *testing.T) {
 	assert.Len(t, actual, 0)
 	assert.Equal(t, []entities.Holdout{}, actual)
 }
+
+// ---------------------------------------------------------------------------
+// FSSDK-12760 — `localHoldouts` JSON section end-to-end parsing
+// ---------------------------------------------------------------------------
+
+// TestNewDatafileProjectConfigPartitionsHoldoutsByDatafileSection exercises the
+// full datafile → entity pipeline to confirm the `holdouts` and `localHoldouts`
+// top-level keys are partitioned strictly by section membership.
+func TestNewDatafileProjectConfigPartitionsHoldoutsByDatafileSection(t *testing.T) {
+	datafile := []byte(`{
+		"version": "4",
+		"accountId": "123",
+		"projectId": "456",
+		"revision": "1",
+		"holdouts": [
+			{
+				"id": "g1",
+				"key": "global_holdout",
+				"status": "Running",
+				"audienceIds": [],
+				"variations": [{"id": "vg1", "key": "v"}],
+				"trafficAllocation": [{"entityId": "vg1", "endOfRange": 10000}]
+			}
+		],
+		"localHoldouts": [
+			{
+				"id": "l1",
+				"key": "local_holdout_rule_x",
+				"status": "Running",
+				"audienceIds": [],
+				"includedRules": ["rule_x"],
+				"variations": [{"id": "vl1", "key": "v"}],
+				"trafficAllocation": [{"entityId": "vl1", "endOfRange": 5000}]
+			}
+		]
+	}`)
+
+	config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig"))
+	assert.NoError(t, err)
+	assert.NotNil(t, config)
+
+	// Global section → global holdouts; never registers against any rule.
+	globals := config.GetGlobalHoldouts()
+	assert.Len(t, globals, 1)
+	assert.Equal(t, "g1", globals[0].ID)
+	assert.True(t, globals[0].IsGlobal())
+
+	// Local section → per-rule map; never appears in global list.
+	ruleX := config.GetHoldoutsForRule("rule_x")
+	assert.Len(t, ruleX, 1)
+	assert.Equal(t, "l1", ruleX[0].ID)
+	assert.False(t, ruleX[0].IsGlobal())
+
+	// Global id must not have leaked into the per-rule map.
+	for _, h := range ruleX {
+		assert.NotEqual(t, "g1", h.ID)
+	}
+}
+
+// TestNewDatafileProjectConfigBackwardCompatNoLocalHoldoutsSection verifies that
+// older datafiles emitted before FSSDK-12760 (no `localHoldouts` key) are parsed
+// exactly like before — every entry in `holdouts` is global, no errors.
+func TestNewDatafileProjectConfigBackwardCompatNoLocalHoldoutsSection(t *testing.T) {
+	datafile := []byte(`{
+		"version": "4",
+		"accountId": "123",
+		"projectId": "456",
+		"revision": "1",
+		"holdouts": [
+			{
+				"id": "old1",
+				"key": "old_global_holdout",
+				"status": "Running",
+				"audienceIds": [],
+				"variations": [{"id": "v1", "key": "v"}],
+				"trafficAllocation": [{"entityId": "v1", "endOfRange": 10000}]
+			}
+		]
+	}`)
+
+	config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig"))
+	assert.NoError(t, err)
+	assert.NotNil(t, config)
+
+	globals := config.GetGlobalHoldouts()
+	assert.Len(t, globals, 1)
+	assert.Equal(t, "old1", globals[0].ID)
+	assert.True(t, globals[0].IsGlobal())
+
+	// No localHoldouts section means no per-rule entries.
+	assert.Empty(t, config.GetHoldoutsForRule("any_rule"))
+}
+
+// TestNewDatafileProjectConfigGlobalSectionStripsIncludedRules verifies that any
+// `includedRules` field accidentally present on an entry in the `holdouts` (global)
+// section is stripped during parsing — section membership is the sole signal for scope.
+func TestNewDatafileProjectConfigGlobalSectionStripsIncludedRules(t *testing.T) {
+	datafile := []byte(`{
+		"version": "4",
+		"accountId": "123",
+		"projectId": "456",
+		"revision": "1",
+		"holdouts": [
+			{
+				"id": "g_with_rules",
+				"key": "global_with_stray_rules",
+				"status": "Running",
+				"audienceIds": [],
+				"includedRules": ["rule_should_be_ignored"],
+				"variations": [{"id": "v1", "key": "v"}],
+				"trafficAllocation": [{"entityId": "v1", "endOfRange": 10000}]
+			}
+		]
+	}`)
+
+	config, err := NewDatafileProjectConfig(datafile, logging.GetLogger("", "DatafileProjectConfig"))
+	assert.NoError(t, err)
+	assert.NotNil(t, config)
+
+	// Still classified as global; IncludedRules was stripped on the entity.
+	globals := config.GetGlobalHoldouts()
+	assert.Len(t, globals, 1)
+	assert.True(t, globals[0].IsGlobal())
+	assert.Nil(t, globals[0].IncludedRules)
+
+	// The stray rule id must NOT show up in any per-rule lookup.
+	assert.Empty(t, config.GetHoldoutsForRule("rule_should_be_ignored"))
+}

pkg/config/datafileprojectconfig/entities/entities.go

@@ -114,7 +114,10 @@ type Rollout struct {
 	Experiments []Experiment `json:"experiments"`
 }
 
-// Holdout represents a holdout from the Optimizely datafile
+// Holdout represents a holdout from the Optimizely datafile.
+// Scope (global vs local) is set by the datafile section the entry came from
+// (`holdouts` vs `localHoldouts`); `IncludedRules` is stripped on `holdouts` entries
+// and required on `localHoldouts` entries.
 type Holdout struct {
 	ID                 string              `json:"id"`
 	Key                string              `json:"key"`
@@ -123,9 +126,8 @@ type Holdout struct {
 	AudienceConditions interface{}         `json:"audienceConditions"`
 	Variations         []Variation         `json:"variations"`
 	TrafficAllocation  []TrafficAllocation `json:"trafficAllocation"`
-	// IncludedRules is optional. nil = global holdout (applies to all rules across all flags).
-	// Non-nil array = local holdout (applies only to the specified rule IDs).
-	// An empty non-nil array means a local holdout that targets no rules.
+	// IncludedRules carries per-rule targeting for local holdouts. Required on
+	// `localHoldouts` entries; ignored/stripped on `holdouts` entries at parse time.
 	IncludedRules *[]string `json:"includedRules,omitempty"`
 }
 
@@ -146,6 +148,7 @@ type Datafile struct {
 	Events            []Event       `json:"events"`
 	Rollouts          []Rollout     `json:"rollouts"`
 	Holdouts          []Holdout     `json:"holdouts,omitempty"`
+	LocalHoldouts     []Holdout     `json:"localHoldouts,omitempty"`
 	Integrations      []Integration `json:"integrations"`
 	TypedAudiences    []Audience    `json:"typedAudiences"`
 	Variables         []string      `json:"variables"`

pkg/config/datafileprojectconfig/mappers/holdout.go

@@ -18,14 +18,27 @@
 package mappers
 
 import (
+	"fmt"
+
 	datafileEntities "github.com/optimizely/go-sdk/v2/pkg/config/datafileprojectconfig/entities"
 	"github.com/optimizely/go-sdk/v2/pkg/entities"
+	"github.com/optimizely/go-sdk/v2/pkg/logging"
 )
 
-// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities.
-// Global holdouts (IncludedRules == nil) are returned in globalHoldouts for flag-level evaluation.
-// Local holdouts (IncludedRules != nil) are indexed by rule ID in ruleHoldoutsMap.
-func MapHoldouts(holdouts []datafileEntities.Holdout) (
+// MapHoldouts maps the two top-level datafile holdout sections to SDK Holdout entities.
+//
+// Section membership is the sole signal for scope (FSSDK-12760):
+//   - `holdouts` entries are global; any `includedRules` field on them is stripped.
+//   - `localHoldouts` entries are local and MUST carry a non-nil `includedRules` list;
+//     entries missing it are logged via `logger` and excluded (no fallback to global).
+//
+// Returns the combined entity list, an id->entity map, the global-only list, and a
+// per-rule map for local holdouts.
+func MapHoldouts(
+	globalHoldoutsRaw []datafileEntities.Holdout,
+	localHoldoutsRaw []datafileEntities.Holdout,
+	logger logging.OptimizelyLogProducer,
+) (
 	holdoutList []entities.Holdout,
 	holdoutIDMap map[string]entities.Holdout,
 	globalHoldouts []entities.Holdout,
@@ -36,29 +49,83 @@ func MapHoldouts(holdouts []datafileEntities.Holdout) (
 	globalHoldouts = []entities.Holdout{}
 	ruleHoldoutsMap = make(map[string][]entities.Holdout)
 
-	for _, holdout := range holdouts {
-		// Only process running holdouts
+	// Process global holdouts: drop any `IncludedRules` so section membership alone
+	// determines scope, even if the datafile incorrectly includes one.
+	for _, holdout := range globalHoldoutsRaw {
 		if holdout.Status != string(entities.HoldoutStatusRunning) {
 			continue
 		}
 
-		mappedHoldout := mapHoldout(holdout)
+		sanitized := holdout
+		sanitized.IncludedRules = nil
+
+		mappedHoldout := mapHoldout(sanitized)
 		holdoutList = append(holdoutList, mappedHoldout)
-		holdoutIDMap[holdout.ID] = mappedHoldout
-
-		if mappedHoldout.IsGlobal() {
-			globalHoldouts = append(globalHoldouts, mappedHoldout)
-		} else {
-			// Local holdout: applies only to the specified rule IDs
-			for _, ruleID := range *mappedHoldout.IncludedRules {
-				ruleHoldoutsMap[ruleID] = append(ruleHoldoutsMap[ruleID], mappedHoldout)
+		if _, exists := holdoutIDMap[mappedHoldout.ID]; exists && logger != nil {
+			logger.Warning(
+				fmt.Sprintf(
+					"Duplicate holdout ID %q encountered across datafile sections; later entry overwrites earlier one.",
+					mappedHoldout.ID,
+				),
+			)
+		}
+		holdoutIDMap[mappedHoldout.ID] = mappedHoldout
+		globalHoldouts = append(globalHoldouts, mappedHoldout)
+	}
+
+	// Process local holdouts: `IncludedRules` is REQUIRED. Entries missing it (nil
+	// pointer) are invalid per spec — log and skip, never promote to global.
+	for _, holdout := range localHoldoutsRaw {
+		if holdout.Status != string(entities.HoldoutStatusRunning) {
+			continue
+		}
+
+		if holdout.IncludedRules == nil {
+			if logger != nil {
+				logger.Warning(
+					fmt.Sprintf(
+						"Local holdout %q is missing required \"includedRules\" field and will be excluded from evaluation.",
+						holdoutLabel(holdout),
+					),
+				)
 			}
+			continue
+		}
+
+		mappedHoldout := mapHoldout(holdout)
+		holdoutList = append(holdoutList, mappedHoldout)
+		if _, exists := holdoutIDMap[mappedHoldout.ID]; exists && logger != nil {
+			logger.Warning(
+				fmt.Sprintf(
+					"Duplicate holdout ID %q encountered across datafile sections; later entry overwrites earlier one.",
+					mappedHoldout.ID,
+				),
+			)
+		}
+		holdoutIDMap[mappedHoldout.ID] = mappedHoldout
+
+		// Register the local holdout for each rule it targets. An empty IncludedRules
+		// slice is valid (matches no rules) but is not promoted to global.
+		for _, ruleID := range *mappedHoldout.IncludedRules {
+			ruleHoldoutsMap[ruleID] = append(ruleHoldoutsMap[ruleID], mappedHoldout)
 		}
 	}
 
 	return holdoutList, holdoutIDMap, globalHoldouts, ruleHoldoutsMap
 }
 
+// holdoutLabel returns a stable, user-facing label for an invalid local holdout
+// log message. Prefers the holdout key (human-readable) and falls back to the id.
+func holdoutLabel(h datafileEntities.Holdout) string {
+	if h.Key != "" {
+		return h.Key
+	}
+	if h.ID != "" {
+		return h.ID
+	}
+	return "<unknown>"
+}
+
 func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout {
 	var audienceConditionTree *entities.TreeNode
 	var err error