Skip to content

fix(flags): multi-condition flags with static cohorts returning wrong variants#343

Merged
haacked merged 3 commits intomasterfrom
haacked/static-cohort-local-eval-fix
Oct 21, 2025
Merged

fix(flags): multi-condition flags with static cohorts returning wrong variants#343
haacked merged 3 commits intomasterfrom
haacked/static-cohort-local-eval-fix

Conversation

@haacked
Copy link
Copy Markdown
Collaborator

@haacked haacked commented Oct 20, 2025

When a feature flag has multiple conditions and one contains a static cohort, the SDK now correctly falls back to the API instead of evaluating subsequent conditions locally and returning incorrect variants.

Introduce RequiresServerEvaluation exception to distinguish between:

  • Missing server-side data (static cohorts) → immediate API fallback
  • Evaluation errors (bad regex, missing properties) → try next condition

Changes:

  • Add RequiresServerEvaluation exception class
  • Update match_cohort() to throw RequiresServerEvaluation for static cohorts
  • Update match_property_group() to propagate RequiresServerEvaluation
  • Update match_feature_flag_properties() to handle both exception types
  • Update client.py to catch both exceptions for API fallback
  • Export RequiresServerEvaluation in __init__.py
  • Add test for multi-condition static cohort scenario

Follows same pattern as Node.js SDK fix: PostHog/posthog-js#2465

When a feature flag has multiple conditions and one contains a static
cohort, the SDK now correctly falls back to the API instead of
evaluating subsequent conditions locally and returning incorrect variants.

Introduce RequiresServerEvaluation exception to distinguish between:
- Missing server-side data (static cohorts) → immediate API fallback
- Evaluation errors (bad regex, missing properties) → try next condition

Changes:
- Add RequiresServerEvaluation exception class
- Update match_cohort() to throw RequiresServerEvaluation for static cohorts
- Update match_property_group() to propagate RequiresServerEvaluation
- Update match_feature_flag_properties() to handle both exception types
- Update client.py to catch both exceptions for API fallback
- Export RequiresServerEvaluation in __init__.py
- Add test for multi-condition static cohort scenario

All 84 feature flag tests pass.
@haacked haacked marked this pull request as ready for review October 21, 2025 00:05
@haacked haacked requested a review from a team October 21, 2025 00:05
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Oct 21, 2025
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. posthog/feature_flags.py, line 126-141 (link)

    logic: RequiresServerEvaluation isn't caught and propagated in the recursive flag dependency evaluation. If a dependent flag contains a static cohort, the exception will propagate but won't be cached in evaluation_cache, which could cause issues if the same dependency is checked again.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Oct 21, 2025
@haacked haacked merged commit 9a05db8 into master Oct 21, 2025
10 checks passed
@haacked haacked deleted the haacked/static-cohort-local-eval-fix branch October 21, 2025 20:40
@github-project-automation github-project-automation Bot moved this from Approved to Done in Feature Flags Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants