feat(flags): IsEnabled accepts a caller-supplied default value - #277
Closed
posthog[bot] wants to merge 2 commits into
Closed
feat(flags): IsEnabled accepts a caller-supplied default value#277posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
Brings `FeatureFlagEvaluations.IsEnabled` into compliance with the cross-SDK `is-feature-enabled` contract: a hard SHALL requirement that the SDK accept a caller-supplied boolean default and return it whenever the flag has no value (missing key, flags not loaded, or a failed /flags request), while any real flag value — including false or a variant — always wins over the default. Adds an optional variadic `defaultValue ...bool` parameter so existing calls remain source-compatible. Generated-By: PostHog Code Task-Id: 7ee84317-3a73-41e1-bc25-906b7c14be53
Contributor
posthog-go Compliance ReportDate: 2026-08-09 16:40:16 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Generated-By: PostHog Code Task-Id: 7ee84317-3a73-41e1-bc25-906b7c14be53
dustinbyrne
marked this pull request as ready for review
August 10, 2026 16:32
Contributor
Author
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 1 consider. Published 1 finding (view the review). |
Contributor
Author
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+84
to
86
| return resolveIsEnabledDefault(defaultValue) | ||
| } | ||
| return flag.Enabled |
Contributor
Author
There was a problem hiding this comment.
Per-flag evaluation failures ignore the caller default
Why we think it's a valid issue
- Checked:
recordFromFlagDetail(posthog.go:1343-1368), map population (posthog.go:1236), theIsEnabledmiss path (feature_flag_evaluations.go:77-87),FlagDetail.Failed(flags.go:50-51), and the cross-SDKis-feature-enabledspec the PR cites. - Found: Premise is factually correct. A server-marked
failed:trueflag is forced toEnabled=falsewithErrorset (posthog.go:1351-1355) but still inserted into the map, soflag, ok := e.flags[key]givesok==true, the!okbranch (feature_flag_evaluations.go:83) is skipped, andflag.Enabled(false) is returned, ignoring the caller default. Reachable:Failedis a documented v4 API state with dedicated handling. Real inconsistency: a whole-request failure leaves the flag absent → default applies, but a per-flag transient failure returns false. - Found (severity counter-evidence): The fetched spec enumerates only three default-triggering cases — 'flags not loaded yet, a failed flags request, or no flag with that key' — and is silent on a returned-but-failed flag; its principle is 'a flag that has a value including false always wins over the default.' Treating a failed flag as a false value, with the error surfaced separately via the existing
GetFeatureFlagErrors(feature_flag_evaluations.go:309-310), is a defensible, arguably spec-consistent reading, so the finding's 'violates intended behavior' claim is overstated. - Impact: Genuine reachable gap —
IsEnabled(key, true)returns false during a transient per-flag failure, contrary to the caller's fallback intent — worth the author's conscious decision. But it is a narrow, transient partial-degradation in a spec-undefined area with failures already observable, so it is not release-blocking. - Priority: Lowered from
must_fixtoconsider: real and directly related, but not a clear spec violation and defensible as-is, so down-rank rather than treat as a mandatory fix.
Issue description
The default is used only when the key is absent. However, recordFromFlagDetail stores failed evaluations as present records with Enabled=false and Error set, so IsEnabled returns false instead of the caller-supplied default. This violates the intended behavior when a flag has no usable value due to an evaluation failure.
Suggested fix
Treat records with flag.Error != nil as unresolved: after recording access, return resolveIsEnabledDefault(defaultValue) when !ok or flag.Error != nil. Add a test using a FlagDetail with Failed=true and an opposite caller default.
Prompt to fix with AI (copy-paste)
## Context
@feature_flag_evaluations.go#L84-86
<issue_description>
The default is used only when the key is absent. However, recordFromFlagDetail stores failed evaluations as present records with Enabled=false and Error set, so IsEnabled returns false instead of the caller-supplied default. This violates the intended behavior when a flag has no usable value due to an evaluation failure.
</issue_description>
<issue_validation>
- **Checked:** `recordFromFlagDetail` (posthog.go:1343-1368), map population (posthog.go:1236), the `IsEnabled` miss path (feature_flag_evaluations.go:77-87), `FlagDetail.Failed` (flags.go:50-51), and the cross-SDK `is-feature-enabled` spec the PR cites.
- **Found:** Premise is factually correct. A server-marked `failed:true` flag is forced to `Enabled=false` with `Error` set (posthog.go:1351-1355) but still inserted into the map, so `flag, ok := e.flags[key]` gives `ok==true`, the `!ok` branch (feature_flag_evaluations.go:83) is skipped, and `flag.Enabled` (false) is returned, ignoring the caller default. Reachable: `Failed` is a documented v4 API state with dedicated handling. Real inconsistency: a whole-request failure leaves the flag absent → default applies, but a per-flag transient failure returns false.
- **Found (severity counter-evidence):** The fetched spec enumerates only three default-triggering cases — 'flags not loaded yet, a failed flags request, or no flag with that key' — and is silent on a returned-but-failed flag; its principle is 'a flag that has a value including false always wins over the default.' Treating a failed flag as a false value, with the error surfaced separately via the existing `GetFeatureFlagErrors` (feature_flag_evaluations.go:309-310), is a defensible, arguably spec-consistent reading, so the finding's 'violates intended behavior' claim is overstated.
- **Impact:** Genuine reachable gap — `IsEnabled(key, true)` returns false during a transient per-flag failure, contrary to the caller's fallback intent — worth the author's conscious decision. But it is a narrow, transient partial-degradation in a spec-undefined area with failures already observable, so it is not release-blocking.
- **Priority:** Lowered from `must_fix` to `consider`: real and directly related, but not a clear spec violation and defensible as-is, so down-rank rather than treat as a mandatory fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Treat records with flag.Error != nil as unresolved: after recording access, return resolveIsEnabledDefault(defaultValue) when !ok or flag.Error != nil. Add a test using a FlagDetail with Failed=true and an opposite caller default.
</potential_solution>
dustinbyrne
reviewed
Aug 10, 2026
| func (e *FeatureFlagEvaluations) GetFlagPayload(key string) string | ||
|
|
||
| func (e *FeatureFlagEvaluations) IsEnabled(key string) bool | ||
| func (e *FeatureFlagEvaluations) IsEnabled(key string, defaultValue ...bool) bool |
Contributor
There was a problem hiding this comment.
i don't know about the variadic usage to keep this backwards compatible? PR description calls it idiomatic but... really?
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
1 participant
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
Brings
FeatureFlagEvaluations.IsEnabledinto compliance with the cross-SDKis-feature-enabledcontract: a hardSHALL,@both-tagged requirement with no server-SDK carve-out — the SDK must accept a caller-supplied boolean default and return it whenever the flag has no value (missing key, flags not loaded yet, or a failed/flagsrequest). A flag that resolves to a real value, includingfalseor a variant string, always wins over the default.Flagged in the sdk-specs compliance matrix: posthog-go.md#n12 ("Is Feature Enabled — ❌ Fail"). Neither
IsEnablednor the deprecated legacyIsFeatureEnabled/GetFeatureFlag(FeatureFlagPayload) had a way for a caller to override a miss — both collapsed unconditionally tofalse.This PR only touches the canonical, non-deprecated
FeatureFlagEvaluations.IsEnabledpath (the API the package doc already recommends for new code). The legacyIsFeatureEnabled/GetFeatureFlagsurface is marked// Deprecated: Prefer EvaluateFlags for new code.inposthog.go, so it's intentionally left alone — the identical remediation in posthog-python's legacyfeature_enabled()was attempted and closed without merge (posthog-python#859), while fixing the canonicalis_enabled()there was accepted and merged (posthog-python#800).Backwards compatibility
Purely additive —
defaultValueis a new variadic...boolparameter. Go has no optional/default parameters or overloading, so a variadic tail is the idiomatic way to add an optional argument without breaking any existing call site;IsEnabled(key)continues to behave exactly as before (falls back tofalseon a miss). Callers can now opt in withIsEnabled(key, true)to get atruefallback instead.💚 How did you test it?
feature_flag_evaluations_test.go:TestIsEnabled_ReturnsCallerDefaultOnMiss— an unknown flag key returnsfalsewith no default, and returns the caller-suppliedtrue/falsedefault when passed.TestIsEnabled_RealValueWinsOverCallerDefault— a flag that resolves to a realfalse(ortrue) value is never overridden by an opposite caller-supplied default.TestIsEnabled_NilSnapshotReturnsCallerDefault— a nil*FeatureFlagEvaluationsreceiver also honors the caller-supplied default.go test ./...) — all passing.gofmt -l .andgo vet ./...— clean.📝 Checklist
IsEnabledupdated; no README signature to update)If releasing new changes
.changeset/is-enabled-default-value.mdchangeset file🤖 Agent context
Autonomy: Fully autonomous — this PR was generated by a scheduled compliance-audit loop that reads the SDK compliance matrices in PostHog/sdk-specs and implements one well-scoped, backward-compatible gap per run. No human directed this specific change; left unassigned for the owning team to triage.
compliance/*.mdin PostHog/sdk-specs.SHALL(not a nice-to-have), high-impact (core flag-evaluation API used on nearly every request path), narrowly scoped (one additive parameter), and — after checking open/closed PRs across all affected repos — the only one of 6 affected server SDKs (python, node, php, ruby, go, dotnet) with neither an existing open PR (posthog-php has one open) nor a prior rejected attempt (posthog-python's legacy-method attempt was closed) covering it.IsFeatureEnabled/GetFeatureFlaglegacy path, following the precedent set by posthog-python's closed PR #859.Closes the
posthog-go×Is Feature Enabledcell in the sdk-specs compliance matrix.