From b1caf234c22f4b67c7e5d13973ae192b5b254d60 Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Thu, 9 Jul 2026 23:40:29 -0300 Subject: [PATCH 1/2] implement fix --- posthog/feature_flags.py | 157 +++++++++-------- posthog/test/test_feature_flags.py | 271 +++++++++++++++++++++++++++++ 2 files changed, 349 insertions(+), 79 deletions(-) diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 3ebf45e4..a94b6ead 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -115,7 +115,13 @@ def evaluate_flag_dependency( device_id=None, ): """ - Evaluate a flag dependency property according to the dependency chain algorithm. + Evaluate a flag dependency condition under local evaluation. + + The dependency_chain only establishes the order in which flags are evaluated + and cached (the referenced flag itself is always the last member). The + outcome is decided solely by comparing the referenced flag's evaluated value + against the condition's expected value; ancestors influence it only through + the referenced flag's own recursive evaluation. Args: property: Flag property with type="flag" and dependency_chain @@ -127,7 +133,12 @@ def evaluate_flag_dependency( device_id: The device ID for bucketing (optional) Returns: - bool: True if all dependencies in the chain evaluate to True, False otherwise + bool: Whether the referenced flag's evaluated value matches the + condition's expected value. + + Raises: + InconclusiveMatchError: If the condition is malformed or the chain + cannot be conclusively evaluated locally. """ if flags_by_key is None or evaluation_cache is None: # Cannot evaluate flag dependencies without required context @@ -151,93 +162,81 @@ def evaluate_flag_dependency( f"Circular dependency detected for flag '{property.get('key', 'unknown')}'" ) - # Evaluate all dependencies in the chain order - for dep_flag_key in dependency_chain: - if dep_flag_key not in evaluation_cache: - # Need to evaluate this dependency first - dep_flag = flags_by_key.get(dep_flag_key) - if not dep_flag: - # Missing flag dependency - cannot evaluate locally - evaluation_cache[dep_flag_key] = None - raise InconclusiveMatchError( - f"Cannot evaluate flag dependency '{dep_flag_key}' - flag not found in local flags" - ) - else: - # Check if the flag is active (same check as in client._compute_flag_locally) - if not dep_flag.get("active"): - evaluation_cache[dep_flag_key] = False - else: - # Recursively evaluate the dependency - try: - dep_flag_filters = dep_flag.get("filters") or {} - dep_aggregation_group_type_index = dep_flag_filters.get( - "aggregation_group_type_index" - ) - if dep_aggregation_group_type_index is not None: - # Group flags should continue bucketing by the group key - # from the current evaluation context. - dep_bucketing_value = distinct_id - else: - dep_bucketing_value = resolve_bucketing_value( - dep_flag, distinct_id, device_id - ) - dep_result = match_feature_flag_properties( - dep_flag, - distinct_id, - properties, - cohort_properties=cohort_properties, - flags_by_key=flags_by_key, - evaluation_cache=evaluation_cache, - device_id=device_id, - bucketing_value=dep_bucketing_value, - ) - evaluation_cache[dep_flag_key] = dep_result - except InconclusiveMatchError as e: - # If we can't evaluate a dependency, store None and propagate the error - evaluation_cache[dep_flag_key] = None - raise InconclusiveMatchError( - f"Cannot evaluate flag dependency '{dep_flag_key}': {e}" - ) from e - - # Check the cached result - cached_result = evaluation_cache[dep_flag_key] - if cached_result is None: - # Previously inconclusive - raise error again - raise InconclusiveMatchError( - f"Flag dependency '{dep_flag_key}' was previously inconclusive" - ) - elif not cached_result: - # Definitive False result - dependency failed - return False - - # All dependencies in the chain have been evaluated successfully - # Now check if the final flag value matches the expected value in the property + # Validate the condition shape before walking the chain. Test `is None`, not + # truthiness: `False` is a valid expected value (the case this exists for). flag_key = property.get("key") expected_value = property.get("value") operator = property.get("operator", "exact") - if flag_key and expected_value is not None: - # Get the actual value of the flag we're checking - actual_value = evaluation_cache.get(flag_key) + if operator != "flag_evaluates_to": + raise InconclusiveMatchError( + f"Flag dependency property for '{flag_key or 'unknown'}' has invalid operator '{operator}'" + ) + if not flag_key or expected_value is None: + raise InconclusiveMatchError( + f"Flag dependency property for '{flag_key or 'unknown'}' is missing a key or value" + ) + + # Evaluate and cache each flag in the chain; members already cached are + # skipped. This does not decide the outcome — it only populates the cache. + for dep_flag_key in dependency_chain: + if dep_flag_key in evaluation_cache: + continue - if actual_value is None: - # Flag wasn't evaluated - this shouldn't happen if dependency chain is correct + dep_flag = flags_by_key.get(dep_flag_key) + if not dep_flag: + # Missing flag dependency - cannot evaluate locally + evaluation_cache[dep_flag_key] = None raise InconclusiveMatchError( - f"Flag '{flag_key}' was not evaluated despite being in dependency chain" + f"Cannot evaluate flag dependency '{dep_flag_key}' - flag not found in local flags" ) - # For flag dependencies, we need to compare the actual flag result with expected value - # using the flag_evaluates_to operator logic - if operator == "flag_evaluates_to": - return matches_dependency_value(expected_value, actual_value) - else: - # This should never happen, but just to be defensive. - raise InconclusiveMatchError( - f"Flag dependency property for '{property.get('key', 'unknown')}' has invalid operator '{operator}'" + # Check if the flag is active (same check as in client._compute_flag_locally) + if not dep_flag.get("active"): + evaluation_cache[dep_flag_key] = False + continue + + # Recursively evaluate the dependency + try: + dep_flag_filters = dep_flag.get("filters") or {} + dep_aggregation_group_type_index = dep_flag_filters.get( + "aggregation_group_type_index" + ) + if dep_aggregation_group_type_index is not None: + # Group flags should continue bucketing by the group key + # from the current evaluation context. + dep_bucketing_value = distinct_id + else: + dep_bucketing_value = resolve_bucketing_value( + dep_flag, distinct_id, device_id + ) + dep_result = match_feature_flag_properties( + dep_flag, + distinct_id, + properties, + cohort_properties=cohort_properties, + flags_by_key=flags_by_key, + evaluation_cache=evaluation_cache, + device_id=device_id, + bucketing_value=dep_bucketing_value, ) + evaluation_cache[dep_flag_key] = dep_result + except InconclusiveMatchError as e: + # If we can't evaluate a dependency, store None and propagate the error + evaluation_cache[dep_flag_key] = None + raise InconclusiveMatchError( + f"Cannot evaluate flag dependency '{dep_flag_key}': {e}" + ) from e - # If no value check needed, return True (all dependencies passed) - return True + # The condition matches iff the referenced flag's value matches the expected + # value. None means inconclusive or not evaluated — distinct from a + # definitive False, which must be allowed to match `expected_value=False`. + actual_value = evaluation_cache.get(flag_key) + if actual_value is None: + raise InconclusiveMatchError( + f"Flag dependency '{flag_key}' was inconclusive or not evaluated" + ) + return matches_dependency_value(expected_value, actual_value) def matches_dependency_value(expected_value, actual_value): diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 74e55520..f6c26138 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -2079,6 +2079,277 @@ def test_flag_dependencies_without_context_raises_inconclusive(self): self.assertIn("Cannot evaluate flag dependency", str(cm.exception)) self.assertIn("some-flag", str(cm.exception)) + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_evaluates_to_false(self, patch_get, patch_flags): + """A `flag_evaluates_to: false` condition matches when the referenced flag is conclusively False, without falling back to /flags.""" + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + # 0% rollout: conclusively False for everyone + "filters": {"groups": [{"properties": [], "rollout_percentage": 0}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": False, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + for distinct_id in ["user-1", "user-2", "another-user-xyz"]: + self.assertEqual( + client.get_feature_flag("dependent-flag", distinct_id), True + ) + + # Control: base flag returning a variant does not match `value: false` + client.feature_flags[0]["filters"] = { + "groups": [ + {"properties": [], "rollout_percentage": 100, "variant": "test"} + ], + "multivariate": {"variants": [{"key": "test", "rollout_percentage": 100}]}, + } + self.assertEqual(client.get_feature_flag("dependent-flag", "user-1"), False) + + # Control: base flag at 100% (True) does not match `value: false` + client.feature_flags[0]["filters"] = { + "groups": [{"properties": [], "rollout_percentage": 100}] + } + self.assertEqual(client.get_feature_flag("dependent-flag", "user-1"), False) + + # Everything above resolves locally - no /flags fallback + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_multi_level_evaluates_to_false( + self, patch_get, patch_flags + ): + """Multi-level chain with a legitimately-False ancestor: A(false) -> B(true) -> D(true).""" + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Flag A", + "key": "flag-a", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 0}]}, + }, + { + "id": 2, + "name": "Flag B", + "key": "flag-b", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "flag-a", + "operator": "flag_evaluates_to", + "value": False, + "type": "flag", + "dependency_chain": ["flag-a"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + { + "id": 3, + "name": "Flag D", + "key": "flag-d", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "flag-b", + "operator": "flag_evaluates_to", + "value": True, + "type": "flag", + # Ancestor A is False; only B's value decides D + "dependency_chain": ["flag-a", "flag-b"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + self.assertEqual(client.get_feature_flag("flag-a", "user-1"), False) + self.assertEqual(client.get_feature_flag("flag-b", "user-1"), True) + self.assertEqual(client.get_feature_flag("flag-d", "user-1"), True) + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_inactive_base_evaluates_to_false( + self, patch_get, patch_flags + ): + """An inactive base flag caches False, so `flag_evaluates_to: false` matches.""" + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": False, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": False, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + self.assertEqual(client.get_feature_flag("dependent-flag", "user-1"), True) + self.assertEqual(patch_flags.call_count, 0) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_inconclusive_base_falls_back( + self, patch_get, patch_flags + ): + """An inconclusive base (missing person properties) is distinct from False and falls back to /flags.""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "email", + "operator": "icontains", + "value": "@example.com", + "type": "person", + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": False, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + # No person properties => base is inconclusive (None), not False + result = client.get_feature_flag("dependent-flag", "user-1") + self.assertIsNone(result) + self.assertEqual(patch_flags.call_count, 1) + + @mock.patch("posthog.client.flags") + @mock.patch("posthog.client.get") + def test_flag_dependencies_null_value_falls_back(self, patch_get, patch_flags): + """A malformed `value: null` condition is inconclusive and falls back to /flags rather than silently matching.""" + patch_flags.return_value = {"featureFlags": {}} + client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + client.feature_flags = [ + { + "id": 1, + "name": "Base Flag", + "key": "base-flag", + "active": True, + "filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}, + }, + { + "id": 2, + "name": "Dependent Flag", + "key": "dependent-flag", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "base-flag", + "operator": "flag_evaluates_to", + "value": None, + "type": "flag", + "dependency_chain": ["base-flag"], + } + ], + "rollout_percentage": 100, + } + ], + }, + }, + ] + + result = client.get_feature_flag("dependent-flag", "user-1") + self.assertIsNone(result) + self.assertEqual(patch_flags.call_count, 1) + @mock.patch("posthog.client.flags") @mock.patch("posthog.client.get") def test_multi_level_multivariate_dependency_chain(self, patch_get, patch_flags): From 9e3808331a4e664266a60d69273ac14c5c60ff21 Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Thu, 9 Jul 2026 23:44:16 -0300 Subject: [PATCH 2/2] add changeset --- .sampo/changesets/stalwart-iceseeker-sampsa.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .sampo/changesets/stalwart-iceseeker-sampsa.md diff --git a/.sampo/changesets/stalwart-iceseeker-sampsa.md b/.sampo/changesets/stalwart-iceseeker-sampsa.md new file mode 100644 index 00000000..37032487 --- /dev/null +++ b/.sampo/changesets/stalwart-iceseeker-sampsa.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Fix local evaluation of flag dependencies with a `flag_evaluates_to: false` condition: such conditions never matched, forcing the dependent flag to `false` for every locally-evaluated user.