feat(feature-flags): support early_exit in local evaluation#166
Open
gustavohstrassburger wants to merge 2 commits into
Open
feat(feature-flags): support early_exit in local evaluation#166gustavohstrassburger wants to merge 2 commits into
gustavohstrassburger wants to merge 2 commits into
Conversation
Port the server-side early_exit behavior to Ruby local feature flag evaluation. When filters.early_exit is true and a condition group's property filters match (or it has none) but the rollout percentage excludes the user, return a definitive disabled result instead of falling through to later groups. Property-mismatch groups still fall through, and behavior is unchanged when early_exit is unset or false. Generated-By: PostHog Code Task-Id: 707b13a5-0e5d-4764-915a-21e1f2a80c63
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
spec/posthog/flags_spec.rb:1671-1693
Three of the new tests are structurally identical — they all call `build_flag` with a different `early_exit` value and assert on the result. The repo's stated convention is to always prefer parameterised tests; these three contexts fit that pattern exactly and could be collapsed into a single table-driven loop.
```suggestion
[
[true, false, 'enabled'],
[nil, true, 'unset'],
[false, true, 'explicitly false']
].each do |early_exit_val, expected, description|
context "when early_exit is #{description}" do
it "returns #{expected}" do
flag = build_flag(early_exit: early_exit_val)
result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache)
expect(result).to be expected
end
end
end
```
Reviews (1): Last reviewed commit: "chore(feature-flags): trim redundant com..." | Re-trigger Greptile |
Comment on lines
+1671
to
+1693
| context 'when early_exit is enabled' do | ||
| it 'returns false without evaluating a later matching group' do | ||
| flag = build_flag(early_exit: true) | ||
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | ||
| expect(result).to be false | ||
| end | ||
| end | ||
|
|
||
| context 'when early_exit is unset' do | ||
| it 'falls through to the later matching group and returns true' do | ||
| flag = build_flag(early_exit: nil) | ||
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | ||
| expect(result).to be true | ||
| end | ||
| end | ||
|
|
||
| context 'when early_exit is explicitly false' do | ||
| it 'falls through to the later matching group and returns true' do | ||
| flag = build_flag(early_exit: false) | ||
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | ||
| expect(result).to be true | ||
| end | ||
| end |
There was a problem hiding this comment.
Three of the new tests are structurally identical — they all call
build_flag with a different early_exit value and assert on the result. The repo's stated convention is to always prefer parameterised tests; these three contexts fit that pattern exactly and could be collapsed into a single table-driven loop.
Suggested change
| context 'when early_exit is enabled' do | |
| it 'returns false without evaluating a later matching group' do | |
| flag = build_flag(early_exit: true) | |
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | |
| expect(result).to be false | |
| end | |
| end | |
| context 'when early_exit is unset' do | |
| it 'falls through to the later matching group and returns true' do | |
| flag = build_flag(early_exit: nil) | |
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | |
| expect(result).to be true | |
| end | |
| end | |
| context 'when early_exit is explicitly false' do | |
| it 'falls through to the later matching group and returns true' do | |
| flag = build_flag(early_exit: false) | |
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | |
| expect(result).to be true | |
| end | |
| end | |
| [ | |
| [true, false, 'enabled'], | |
| [nil, true, 'unset'], | |
| [false, true, 'explicitly false'] | |
| ].each do |early_exit_val, expected, description| | |
| context "when early_exit is #{description}" do | |
| it "returns #{expected}" do | |
| flag = build_flag(early_exit: early_exit_val) | |
| result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache) | |
| expect(result).to be expected | |
| end | |
| end | |
| end |
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/posthog/flags_spec.rb
Line: 1671-1693
Comment:
Three of the new tests are structurally identical — they all call `build_flag` with a different `early_exit` value and assert on the result. The repo's stated convention is to always prefer parameterised tests; these three contexts fit that pattern exactly and could be collapsed into a single table-driven loop.
```suggestion
[
[true, false, 'enabled'],
[nil, true, 'unset'],
[false, true, 'explicitly false']
].each do |early_exit_val, expected, description|
context "when early_exit is #{description}" do
it "returns #{expected}" do
flag = build_flag(early_exit: early_exit_val)
result = poller.send(:match_feature_flag_properties, flag, distinct_id, properties, evaluation_cache)
expect(result).to be expected
end
end
end
```
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
dustinbyrne
approved these changes
Jun 5, 2026
Contributor
dustinbyrne
left a comment
There was a problem hiding this comment.
greptile loves parameterization but this looks good
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.
💡 Motivation and Context
PostHog added an "early exit" option to feature flags in the server-side (Rust) evaluation engine, and it has already been ported to posthog-node and posthog-python. This PR ports the same behavior to the Ruby SDK's local evaluation.
When a flag's
filters.early_exitistrue, local condition evaluation must STOP and return a definitive disabled result as soon as a condition group's property filters match (or the group has no property filters) but the rollout percentage EXCLUDES the user — instead of falling through to evaluate later condition groups.To mirror the Rust engine exactly, the single-group matcher now returns a tri-state outcome instead of a boolean:
:match— property filters matched AND rollout included the user (existing behavior).:no_match— a property filter did NOT match -> always falls through to the next group, even withearly_exitenabled.:out_of_rollout_bound— property filters matched (or there were none) but the rollout percentage excluded the user. Withearly_exitenabled, the loop returns a definitive disabled result immediately; otherwise existing fall-through behavior is preserved.early_exitis read from the flag'sfiltersand defaults tofalse/absent, so behavior is unchanged for existing flags.💚 How did you test it?
match_feature_flag_propertiescovering: (a)early_exitenabled returnsfalsewithout evaluating a later group that would match; (b)early_exitunset falls through to the later matching group (returnstrue); (c)early_exitexplicitlyfalsefalls through (returnstrue); (d) a group failing on a property filter (not rollout) does NOT early-exit even when enabled and falls through to a later matching group. The first group uses matching properties withrollout_percentage: 0and the second uses matching properties withrollout_percentage: 100.condition_matchtests still pass via a thin boolean wrapper over the new tri-state method.bundle exec rspec-> 446 examples, 0 failures.bundle exec rubocop-> no offenses on changed files.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset fileCreated with PostHog Code