feat: engine evaluation context #178

gagantrivedi · 2025-09-30T06:19:18Z

No description provided.

Copilot

Pull Request Overview

This PR implements an engine evaluation context feature that introduces a new engine evaluation system to replace the existing feature state evaluation approach. The changes modernize the evaluation architecture by introducing JSON-based context handling and new evaluation models.

Key Changes

Added new engine evaluation context system with JSON-based models
Replaced old feature state evaluation with the new engine evaluation system
Updated client to use the new evaluation context and mappers

Reviewed Changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
models.go	Added functions to convert engine evaluation results to Flag objects
models_test.go	Added comprehensive tests for the new conversion functions
flagengine/engine_eval/	New package with evaluation context, mappers, and evaluators
client.go	Updated to use new engine evaluation context instead of old models
flagengine/engine.go	Added new evaluation result generation function
flagengine/segments/	Simplified segment evaluation by removing complex logic
go.mod	Added new dependency for JSON processing

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

models_test.go

Copilot · 2025-10-01T09:19:35Z

models.go

+	if id, err := strconv.Atoi(flagResult.FeatureKey); err == nil {
+		featureID = id
+	}


The FeatureKey to FeatureID conversion assumes FeatureKey is always a string representation of an integer. If FeatureKey is not numeric, this silently defaults to 0, which could mask data issues. Consider adding error handling or validation.

Copilot · 2025-10-01T09:19:35Z

flagengine/engine_eval/mappers.go

+				if err := json.Unmarshal(jsonBytes, &traitList); err != nil {
+					// Log error or handle gracefully - for now, continue with empty list
+					traitList = nil
+				}


Silent error handling with only a comment is problematic. Consider logging the error or providing a more explicit error handling mechanism to aid debugging when trait conversion fails.

Copilot · 2025-10-01T09:19:36Z

flagengine/engine_eval/context.go

+		return nil
+	}
+
+	return fmt.Errorf("unable to unmarshal FlexibleString from %s", string(data))


The error message could expose sensitive data by including the raw JSON data. Consider sanitizing or limiting the exposed content in error messages.

Suggested change

return fmt.Errorf("unable to unmarshal FlexibleString from %s", string(data))

return fmt.Errorf("unable to unmarshal FlexibleString: invalid input")

Copilot · 2025-10-01T09:19:36Z

flagengine/engine_eval/context.go

+		return nil
+	}
+
+	return fmt.Errorf("unable to unmarshal ValueUnion from %s", string(data))


Same issue as above - the error message could expose sensitive data by including the raw JSON data. Consider sanitizing the exposed content.

Suggested change

return fmt.Errorf("unable to unmarshal ValueUnion from %s", string(data))

return fmt.Errorf("unable to unmarshal ValueUnion: invalid format")

Zaimwa9

Great progress. I'll re-review once 100% finalized but here are some early comments.

pinned some error handling
rest is mostly about abstracting.
Given we're implementing across 15 languages and this is core/critical logic, I'm in favor of extracting where it makes sense to ease future context-switching between SDKs and lowering the techno expertise barrier

Zaimwa9 · 2025-10-01T09:40:14Z

flagengine/engine.go

+				featureKey := override.FeatureKey
+
+				// Get priority, defaulting to 0 if not set
+				overridePriority := defaultPriority


I believe that should be the other way around and defaulting to Infinity

Zaimwa9 · 2025-10-02T08:08:12Z

flagengine/engine.go

+			// Check if we have a segment override for this feature
+			if segmentFeatureCtx, exists := segmentFeatureContexts[featureContext.FeatureKey]; exists {
+				// Use segment override
+				fc := segmentFeatureCtx.featureContext


Maybe worth checking it's not nil, even though it shouldn't be

Should not be a pointer... I have updated it

Zaimwa9 · 2025-10-02T08:09:15Z

models.go

+
+	// Convert FeatureKey (string ID) to integer FeatureID
+	featureID := 0
+	if id, err := strconv.Atoi(flagResult.FeatureKey); err == nil {


we might want to throw if it errors

Since the feature ID isn’t critical for the user, I’d prefer not to throw an error to the client due to a misconfiguration on our side. Let me know if you disagree — I don’t have a strong opinion on this.

flagengine/engine_eval/evaluator.go

Zaimwa9 · 2025-10-02T08:22:20Z

flagengine/engine_eval/mappers.go

+	}
+
+	// Variants
+	if len(fs.MultivariateFeatureStateValues) > 0 {


I could see this part in its own function to clearly isolate it

Zaimwa9 · 2025-10-02T08:29:30Z

flagengine/engine_eval/mappers.go

+		f := float64(v)
+		return &Value{Double: &f}
+	case string:
+		if v == "" {


I'd just extract this string converter into its own function for clarity

Zaimwa9 · 2025-10-02T08:34:33Z

flagengine/engine_eval/evaluator.go

+	if segmentCondition.Property != "" {
+		contextValue = getContextValue(ec, segmentCondition.Property)
+	}
+	if segmentCondition.Operator == PercentageSplit {


this could be its own handler

Zaimwa9 · 2025-10-02T08:35:30Z

flagengine/engine_eval/evaluator.go

+		return matchRegex(traitValue, conditionValue)
+	}
+
+	b1, e1 := strconv.ParseBool(traitValue)


Same here I think it could be extracted

Zaimwa9 · 2025-10-02T08:46:06Z

flagengine/engine.go

-}
+// GetEvaluationResult computes flags and matched segments given a context and a segment matcher.
+// The matcher should return true when the provided segment applies to the provided context.
+func GetEvaluationResult(ec *engine_eval.EngineEvaluationContext) engine_eval.EvaluationResult {


Semi NIT:
Overall I think we could abstract this one in all the SDKs, nothing changing functionally but it's basically doing 2 things:

processSegments

evaluate/process features.

Basically following your comments, imo it could be worth extracting them for better maintainability and unit tests. I know it's not a big impact either so your call

Do you mean abstract them inside GetEvaluationResult?

Yes to have 2 separate functions and it becomes just (exaggerating)

func GetEvaluationResult(ec *engine_eval.EngineEvaluationContext) engine_eval.EvaluationResult { segments := processSegments(xxx) flags := processFeatures(yyy) return engine_eval.EvaluationResult{ Flags: flags, Segments: segments, } }

Yeah, I was going to do that

Okay, should look a little better now

emyller

Looks good overall. Only a handful things to fix/improve.

P.S. I have only reviewed the client work to the extent of what I have full context so far. My PHP project is still a WIP, so I'm hoping to review this again by the time I submit my own work for review.

emyller · 2025-10-03T00:50:45Z

flagengine/engine_eval/evaluator.go

+		conditions := make([]bool, len(segmentRule.Conditions))
+		for i := range segmentRule.Conditions {
+			conditions[i] = contextMatchesCondition(ec, &segmentRule.Conditions[i], segmentKey)
+		}


We likely don't need to evaluate all conditions here. Can we short-circuit this logic depending on segmentRule.Type? See example.

emyller · 2025-10-03T00:52:28Z

flagengine/engine_eval/evaluator.go

+		default:
+			matchesConditions = utils.None(conditions)


For explicitness.

Suggested change

default:

matchesConditions = utils.None(conditions)

case None:

matchesConditions = utils.None(conditions)

default:

return false

emyller · 2025-10-03T01:18:41Z

flagengine/engine_eval/evaluator.go

+	case In:
+		return slices.Contains(strings.Split(v2, ","), v1)


I don't see where in the code we account for the case when the context value is a list of values rather than an explodable string.

Related: Flagsmith/engine-test-data#16

emyller · 2025-10-03T01:26:52Z

flagengine/engine_eval/mappers.go

+
+		// Create overrides for each feature
+		for _, override := range overrides {
+			priority := math.Inf(-1) // Highest possible priority


Suggested change

priority := math.Inf(-1) // Highest possible priority

priority := math.Inf(-1) // Strongest possible priority

emyller · 2025-10-03T01:31:57Z

flagengine/engine_eval/mappers.go

+	var environmentKey string
+	if newContext.Environment.Key != "" {
+		environmentKey = newContext.Environment.Key
+	} else {
+		environmentKey = newContext.Environment.Name
+	}
+
+	identity := IdentityContext{
+		Identifier: identifier,
+		Key:        fmt.Sprintf("%s_%s", environmentKey, identifier),
+		Traits:     identityTraits,
+	}


Suggested change

var environmentKey string

if newContext.Environment.Key != "" {

environmentKey = newContext.Environment.Key

} else {

environmentKey = newContext.Environment.Name

}

identity := IdentityContext{

Identifier: identifier,

Key: fmt.Sprintf("%s_%s", environmentKey, identifier),

Traits: identityTraits,

}

identity := IdentityContext{

Identifier: identifier,

Key: fmt.Sprintf("%s_%s", newContext.Environment.Key, identifier),

Traits: identityTraits,

}

Key should never be empty, right?

emyller · 2025-10-03T01:35:38Z

flagengine/engine.go

+		return *priority
 	}
-	return nil
+	return math.Inf(1)


Suggested change

return math.Inf(1)

return math.Inf(1) // Weakest possible priority

emyller · 2025-10-03T01:41:37Z

flagengine/engine.go

+func GetEvaluationResult(ec *engine_eval.EngineEvaluationContext) engine_eval.EvaluationResult {
+	// Process segments
+	segments, segmentFeatureContexts := processSegments(ec)
+
+	// Process features
+	flags := processFeatures(ec, segmentFeatureContexts)
+
+	return engine_eval.EvaluationResult{
+		Flags:    flags,
+		Segments: segments,
 	}
+}


emyller · 2025-10-03T01:45:05Z

flagengine/flagengine_integration_test.go

+			// Note: Flags are now a map, so no need to sort them
+			// The comparison will be done by comparing the map contents directly
+
+			require.Len(actual.Flags, len(expected.Flags))
+			for featureName, expectedFlag := range expected.Flags {
+				actualFlag, exists := actual.Flags[featureName]
+				require.True(exists, "Expected flag %s not found in actual result", featureName)
+				assert.Equal(expectedFlag.Value, actualFlag.Value)
+				assert.Equal(expectedFlag.Enabled, actualFlag.Enabled)
+				assert.Equal(expectedFlag.FeatureKey, actualFlag.FeatureKey)


Yes! 🎉 Looking forward to see less code here.

gagantrivedi added 11 commits September 30, 2025 10:01

wip: engine eval context

95eb78f

update engine test data

50d47a8

fetch flags using eval ctx for env flags

16b872e

use context for identity flags

0c3e335

store and use engine eval context instead of env doc

984cb77

use eval context for getidentity segments

91a9ad1

Refactor: Move evaluator to engine_eval

4fa190a

rename

b008591

remove makefile trigger

fa8b3fd

misc

ba2074e

squash!

49db0d2

gagantrivedi force-pushed the feat/context-again branch from dbabf4f to 49db0d2 Compare October 1, 2025 09:18

gagantrivedi requested a review from Copilot October 1, 2025 09:18

Copilot AI reviewed Oct 1, 2025

View reviewed changes

gagantrivedi added 7 commits October 1, 2025 15:12

use the new schema

068c709

Bump major version

e214b14

Add missing operators

4da51db

remove old interface

41105e6

update engine test data

86dc538

remove go get

192883f

use go 1.24

c778096

gagantrivedi changed the title ~~feat: engine evlauation context~~ feat: engine evaluation context Oct 2, 2025

Zaimwa9 reviewed Oct 2, 2025

View reviewed changes

gagantrivedi added 5 commits October 2, 2025 15:35

refactor: create a function for processing segments

f85071a

refac: create a diff function for processing features

a52a301

refac: squash

59d405c

Don't use pointer for feature context

a91e5ec

refac default priority

0790608

emyller requested changes Oct 3, 2025

View reviewed changes

refac splitoperator

5f5953d

gagantrivedi added 2 commits October 3, 2025 13:30

use stringArry for in operator

1899cf8

more refac and add reason to mv

379dbf8

	return fmt.Errorf("unable to unmarshal FlexibleString from %s", string(data))
	return fmt.Errorf("unable to unmarshal FlexibleString: invalid input")

	return fmt.Errorf("unable to unmarshal ValueUnion from %s", string(data))
	return fmt.Errorf("unable to unmarshal ValueUnion: invalid format")

	priority := math.Inf(-1) // Highest possible priority
	priority := math.Inf(-1) // Strongest possible priority

	return math.Inf(1)
	return math.Inf(1) // Weakest possible priority

feat: engine evaluation context #178

Are you sure you want to change the base?

feat: engine evaluation context #178

Uh oh!

Conversation

gagantrivedi commented Sep 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Zaimwa9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emyller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!