feat(flags): support mixed targeting in local evaluation#523
Open
patricio-posthog wants to merge 7 commits intomainfrom
Open
feat(flags): support mixed targeting in local evaluation#523patricio-posthog wants to merge 7 commits intomainfrom
patricio-posthog wants to merge 7 commits intomainfrom
Conversation
Contributor
posthog-python Compliance ReportDate: 2026-04-17 21:12:02 UTC ✅ All Tests Passed!30/30 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 1/1 tests passed View Details
|
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/feature_flags.py
Line: 326-331
Comment:
**Redundant guard variable (`has_condition_aggregation`)**
`has_condition_aggregation` is always `True` whenever `condition_aggregation != flag_aggregation` is `True`. When the key is absent from `condition`, `.get("aggregation_group_type_index", flag_aggregation)` returns `flag_aggregation`, so `condition_aggregation == flag_aggregation` is guaranteed — the `!=` test is already `False` before `has_condition_aggregation` is checked. The extra variable adds complexity without changing behavior (simplicity rule 4: no superfluous parts).
```suggestion
condition_aggregation = condition.get(
"aggregation_group_type_index", flag_aggregation
)
if condition_aggregation != flag_aggregation:
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3829-3867
Comment:
**Duplicate test — identical to `test_mixed_targeting_person_condition_matches`**
`test_mixed_targeting_group_without_groups_skips_to_person` is byte-for-byte the same flag definition, the same `get_feature_flag` call (no `groups` arg, `person_properties={"email": "test@example.com"}`), and the same assertions as `test_mixed_targeting_person_condition_matches` at line 3710. One of them should be removed; the surviving test can carry the inline comment explaining the skip behaviour. This violates the OnceAndOnlyOnce simplicity rule.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3869-3896
Comment:
**Unused mock setup and missing call-count assertion**
`patch_flags.return_value = {"featureFlags": {"mixed-flag": "from-api"}}` is dead code here — since all group conditions skip locally and `is_inconclusive` stays `False`, the function returns `False` without ever calling `flags`, so the mock return value is never read. Every other new test also asserts `patch_flags.call_count == 0` to prove no network fallback occurred; this test omits that guard, leaving the no-fallback behaviour unverified.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3709-3940
Comment:
**Prefer parameterised tests**
Five of the six new tests use an identical flag definition (two conditions: one group, one person). Per the project's coding standards, parameterised tests are preferred. Factoring the common flag fixture out and using `@parameterized.expand` (or a data-driven equivalent) would reduce the duplication significantly and make it straightforward to add further inputs in future.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore: ruff format" | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Enables local evaluation of feature flags that use mixed person + group targeting by resolving aggregation context (properties + bucketing value) per condition rather than only at the flag level.
Changes:
- Update
match_feature_flag_propertiesto support per-conditionaggregation_group_type_index, selecting the correct properties and bucketing value per condition. - Pass group context (
group_type_mapping,groups,group_properties) fromClient._compute_flag_locallyintomatch_feature_flag_properties. - Add new unit tests covering mixed targeting scenarios and add a changeset entry for release notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
posthog/feature_flags.py |
Implements per-condition aggregation handling for mixed local flag evaluation (properties + bucketing selection). |
posthog/client.py |
Ensures local evaluation passes group context needed for mixed targeting conditions. |
posthog/test/test_feature_flags.py |
Adds test coverage for mixed person/group targeting behavior and bucketing correctness. |
.sampo/changesets/mixed-targeting-local-eval.md |
Documents the patch change for release automation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Flags using mixed user+group targeting (the beta feature) cannot be evaluated locally by the SDK. The SDK reads
aggregation_group_type_indexat the flag level only. For mixed flags this isNone, so all conditions are evaluated as person targeting, causing group conditions to fail and fall back to an HTTP call.This breaks customers using flags in environments that cannot make HTTP calls (e.g., Temporal workflows).
Changes
Updated
match_feature_flag_propertiesto resolve aggregation per condition when a condition explicitly sets its ownaggregation_group_type_indexthat differs from the flag level. Each condition uses the correct properties and bucketing value for its aggregation type. Backwards compatible with existing pure person and pure group flags.How did you test this code?
All 115 existing tests pass unchanged. Added 6 new tests covering mixed targeting scenarios: person match, group match, no match, group skips to person, no groups passed, and correct bucketing per condition.