From a5ce74f9593298411391d8912ddb147c322b74f0 Mon Sep 17 00:00:00 2001 From: Kayvan Zahiri <123409429+Kayvan-Zahiri@users.noreply.github.com> Date: Thu, 1 Oct 2026 15:33:55 -0700 Subject: [PATCH] fix(flags): do not match empty OR cohorts Use the group identity for empty cohort filters so local evaluation agrees with the published SDK contract. Cover nested groups and single/bulk membership evaluation. --- .sampo/changesets/stalwart-king-ilmatar.md | 5 ++ posthog/feature_flags.py | 2 +- posthog/test/test_feature_flags.py | 57 ++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 .sampo/changesets/stalwart-king-ilmatar.md diff --git a/.sampo/changesets/stalwart-king-ilmatar.md b/.sampo/changesets/stalwart-king-ilmatar.md new file mode 100644 index 000000000..a5e16ecfc --- /dev/null +++ b/.sampo/changesets/stalwart-king-ilmatar.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: patch +--- + +Fix local feature flag evaluation matching empty OR cohorts as though they contained every user. diff --git a/posthog/feature_flags.py b/posthog/feature_flags.py index 87da025b5..1a4a69311 100644 --- a/posthog/feature_flags.py +++ b/posthog/feature_flags.py @@ -932,7 +932,7 @@ def match_property_group( if not isinstance(properties, list): raise RequiresServerEvaluation("Cohort property group values must be a list") if not properties: - return True + return is_and decisive_result = None error_matching_locally = False diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index a126a0092..227e756bf 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -72,6 +72,7 @@ def test_cohort_membership_operators(self): def test_only_canonical_empty_groups_match(self): self.assertTrue(match_property_group({}, {}, {})) self.assertTrue(match_property_group({"type": "AND", "values": []}, {}, {})) + self.assertFalse(match_property_group({"type": "OR", "values": []}, {}, {})) self.assertTrue( match_property_group( {"type": "AND", "values": [{"type": "AND", "values": []}, {}]}, @@ -93,6 +94,16 @@ def test_only_canonical_empty_groups_match(self): with self.assertRaises(RequiresServerEvaluation): match_property_group(group, {}, {}) + @parameterized.expand([("AND",), ("OR",)]) + def test_nested_empty_or_group_does_not_match(self, group_type): + self.assertFalse( + match_property_group( + {"type": group_type, "values": [{"type": "OR", "values": []}]}, + {}, + {}, + ) + ) + def test_missing_nested_cohort_always_requires_server_evaluation(self): matching_leaf = { "key": "country", @@ -1665,6 +1676,52 @@ def test_feature_flags_local_evaluation_None_values(self, patch_flags): self.assertEqual(feature_flag_match, True) + @parameterized.expand( + [ + ("default_single", None, False, False), + ("in_single", "in", False, False), + ("not_in_single", "not_in", True, False), + ("default_bulk", None, False, True), + ("in_bulk", "in", False, True), + ("not_in_bulk", "not_in", True, True), + ] + ) + @mock.patch("posthog.client.flags") + def test_empty_or_cohort_local_evaluation( + self, _name, operator, expected, bulk, patch_flags + ): + self.client.feature_flags = [ + { + "id": 1, + "key": "empty-cohort", + "active": True, + "filters": { + "groups": [ + { + "properties": [ + { + "key": "id", + "value": 42, + "type": "cohort", + "operator": operator, + } + ], + "rollout_percentage": 100, + } + ] + }, + } + ] + self.client.cohorts = {"42": {"type": "OR", "values": []}} + + if bulk: + result = self.client.evaluate_flags("user-1").get_flag("empty-cohort") + else: + result = self.client.get_feature_flag("empty-cohort", "user-1") + + self.assertIs(result, expected) + patch_flags.assert_not_called() + @mock.patch("posthog.client.flags") def test_feature_flags_local_evaluation_for_cohorts(self, patch_flags): client = Client(FAKE_TEST_API_KEY, secret_key=FAKE_TEST_API_KEY)