Skip to content

fix: Reasons for environment default flags - #347

Open
bakirFS wants to merge 3 commits into
mainfrom
fix/reason-environment-default
Open

bakirFS wants to merge 3 commits into
mainfrom
fix/reason-environment-default

Conversation

@bakirFS

@bakirFS bakirFS commented Sep 30, 2026

Copy link
Copy Markdown

Closes #341

When a flag falls back to its environment default, the engine always returned the DEFAULT reason.

Now:

  • STATIC if no segment overrides the feature
  • DEFAULT if the feature has segment overrides, but none matched

@bakirFS
bakirFS requested a review from a team as a code owner September 30, 2026 09:36
@bakirFS
bakirFS requested review from emyller and removed request for a team September 30, 2026 09:36
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6947b05c-9564-45cf-b772-81bfd9d0aaf7

📥 Commits

Reviewing files that changed from the base of the PR and between 1549b48 and fdf0448.

📒 Files selected for processing (1)
  • tests/engine_tests/engine-test-data

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Feature evaluation now assigns DEFAULT when segment overrides target a feature and STATIC when they do not. Dependency resolution applies the same distinction through its feature-to-segment index when no override matches. Unit tests update expected reasons and cover an unmatched segment override. The engine test-data subproject reference also changes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to fdf04

The fallback reasons align with the requested STATIC versus DEFAULT distinction. The test-data revision discrepancy does not change fixture contents; this change is mergeable subject to normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to fdf04

The change affects 2 systems.

Changed systems: tests, flag_engine

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — tests (service) was modified; 2 changed files map to changed impact.
  • observed — flag_engine (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in flag_engine/segments/evaluator.py: evaluate_features now collects feature names targeted by segment overrides before evaluating features.
  • observed — Modified behavior in flag_engine/segments/evaluator.py: Fallback reasons now come from get_fallback_reason rather than always being DEFAULT; features targeted by any segment get DEFAULT, while untargeted features get STATIC. The new helper gathers override feature names across all segments and applies that rule.
  • observed — Modified behavior in flag_engine/segments/evaluator.py: Adds get_targeted_feature_names to collect override names from segment contexts and get_fallback_reason to return DEFAULT for targeted features and STATIC otherwise.
  • observed — Modified behavior in flag_engine/segments/evaluator.py: When dependency resolution finds no matching segment override, the fallback reason now uses the feature’s segment-key index: DEFAULT if it appears in that index, otherwise STATIC, replacing the unconditional DEFAULT.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 51341f3c-fda6-4137-8c93-7e7e4d96bded

📥 Commits

Reviewing files that changed from the base of the PR and between 31dc381 and 1549b48.

📒 Files selected for processing (2)
  • flag_engine/segments/evaluator.py
  • tests/unit/test_engine.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Comment thread tests/unit/test_engine.py
@github-actions

Copy link
Copy Markdown

File Coverage Missing
All files 100% ✅

Minimum allowed coverage is 100%

Generated by 🐒 cobertura-action against fdf0448

@codspeed

codspeed Bot commented Sep 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 1 untouched benchmark


Comparing fix/reason-environment-default (fdf0448) with main (31dc381)

Open in CodSpeed

@khvn26 khvn26 changed the title fix: reason environment default fix: Reasons for environment default Oct 6, 2026
@khvn26 khvn26 changed the title fix: Reasons for environment default fix: Reasons for environment default flags Oct 6, 2026

@khvn26 khvn26 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a couple changes needed.

Comment thread tests/unit/test_engine.py
Comment on lines +71 to +103
def test_get_evaluation_result__unmatched_segment_override__returns_default_reason(
context: EvaluationContext,
) -> None:
# Given
context["segments"] = {
"1": {
"key": "1",
"name": "unmatched_segment",
"rules": [
{
"type": "ALL",
"conditions": [
{"property": "foo", "operator": "EQUAL", "value": "no match"}
],
}
],
"overrides": [
{
"key": "4",
"name": "feature_1",
"enabled": False,
"value": "segment_override",
}
],
}
}

# When
result = get_evaluation_result(context)

# Then
assert result["flags"]["feature_1"]["reason"] == "DEFAULT"
assert result["flags"]["feature_2"]["reason"] == "STATIC"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this; engine-test-data covers it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"""
Get the reason for a flag evaluated to its environment default.

`DEFAULT` if the feature has targeting rules that did not match,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DEFAULT if the feature has targeting rules that did not match,

@matthewelwell is this the behaviour we're looking for? I was under the impression that we'd decided to reserve DEFAULT for SDK-side defaults?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The implementation is correct (I checked the issue body with @aepfli back in September)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, we need another PR to engine-test-data adding a test case for DEFAULT.

@bakirFS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect reason used for environment default flags

4 participants