Repository navigation
feat(flags): accept a caller default in FeatureFlagEvaluations.IsEnabled - #349
posthog[bot] wants to merge 1 commit into
Conversation
The cross-SDK is-feature-enabled spec requires the boolean check to accept a caller-supplied default and return it whenever the flag has no value. IsEnabled hardcoded false for an unknown flag, so callers could not tell "the flag is off" from "we never got an answer", nor choose to fail open. IsEnabled now takes an optional variadic defaultValue, used only when the key is absent from the snapshot or the snapshot itself is nil. A flag with a value, including false and variant strings, still wins. $feature_flag_called keeps reporting the evaluated response and flag_missing, not the caller's default. Co-Authored-By: Claude Opus 5 <[email protected]> Generated-By: PostHog Desktop Task-Id: 009e935f-fc26-4bc4-b2e4-c362ddb2de44
posthog-go Compliance ReportDate: 2026-10-02 06:10:43 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
|
🤖 An AI agent wrote this on behalf of @ioannisj.
Pulling my approval for now, I want to take another look.
| // evaluation metadata; subsequent calls with the same response are deduped | ||
| // against the client's per-distinct_id cache. The event always reports the | ||
| // evaluated response, never the caller-supplied default. | ||
| func (e *FeatureFlagEvaluations) IsEnabled(key string, defaultValue ...bool) bool { |
There was a problem hiding this comment.
fwiw i think i've closed this same PR twice before. varargs here keeps it backwards compatible, but it's kind of hacky
maybe we just change this in the next major? open to other opinions here
There was a problem hiding this comment.
@dustinbyrne ping on the above. would that work or do you think we just close this one?
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| 'posthog-go': minor | |||
There was a problem hiding this comment.
blocking: Do not release this signature break as a minor addition — although direct one-argument calls still work, the variadic method no longer satisfies interface{ IsEnabled(string) bool }, and snapshot.IsEnabled no longer fits a func(string) bool callback. Existing consumers using this already-published API therefore stop compiling after a minor upgrade. Preserve the existing method contract or classify and document this as a breaking release under the repository's major-version policy. Reproduction: reproduced — prior exact-head go test -v -count=1 -timeout=90s -run '^TestReviewExistingInterfaceContract$' . executed and failed both interface/method-value type assertions, while the identical test passed on the exact merge base.
💡 Motivation and Context
The cross-SDK
is-feature-enabledspec states a hard requirement with no server-SDK carve-out:The compliance matrix records this as a ❌ Fail for posthog-go:
Today a caller cannot tell "the flag is off" apart from "we never got an answer", and cannot choose to fail open when flag data is unavailable (empty snapshot, failed
/flagsrequest, quota-limited response).Explanation of the change
FeatureFlagEvaluations.IsEnabledgains an optional variadicdefaultValue, Go's idiomatic spelling of an optional parameter:The default is returned only when the key is absent from the snapshot, or when the snapshot pointer itself is nil (the
IsEnabledreceiver is already nil-safe, which is how "flags were never loaded" surfaces in Go). Any flag that has a value —true,false, or a variant string — still wins over it, and omitting the argument preserves the historicalfalseresult for an unknown flag.$feature_flag_calledreporting is deliberately untouched: the event still carries the real evaluated response (nilplus$feature_flag_error: flag_missingfor a miss), not the caller's default, so exposure data keeps reflecting what the server actually said.Scope is limited to this one contract. The deprecated
Client.IsFeatureEnabled/Client.GetFeatureFlaglegacy path has the same gap and is not changed here — both already point callers atEvaluateFlags, which is now the compliant surface. The matrix's other open gaps for this SDK are left alone.Why this is backwards-compatible
Purely additive. A variadic parameter keeps every existing
IsEnabled(key)call site compiling and behaving identically, and the miss path is only diverted when a caller explicitly passes a value.api/public-api.txtis regenerated to record the new optional argument. The one source-level caveat worth noting: a method value previously typedfunc(string) boolis nowfunc(string, ...bool) bool, which is vanishingly rare in practice but is why this ships as aminor, not a patch.💚 How did you test it?
Four new tests in
feature_flag_evaluations_test.go, mapping onto the spec's acceptance scenarios (acceptance/public/is-feature-enabled.feature):trueandfalse), and staysfalsewith no default$feature_flag_calledstill reports the evaluated response andflag_missing, not the defaultLocally, on Go 1.23:
go vet ./...— cleangofmt -l .— cleango test -race -count=1 ./...— all packages passmake api-update— snapshot regenerated, somake api-diffis cleanNo manual testing against a live project.
Follow-up work
sdk_compliance_adapter) exposes nois_feature_enabledroute, so the harness cannot exercise these scenarios end-to-end yet.FeatureFlagPayloadpath could gain aDefaultValue *boolfield for parity, if maintainers want the deprecated surface brought along too.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
Opened by a scheduled PostHog agent run that reads the
PostHog/sdk-specscompliance matrices and implements one safe, well-scoped gap per run. This gap was picked over the other open ❌ rows because it is a core, user-facing flag contract, the remediation is concrete, the backwards-compatibility verdict is additive, and no PR was already open for it in this repo.Decisions along the way: a variadic
defaultValuewas chosen over a separateIsEnabledWithDefaultmethod (one way to do a thing, and the spec asks for "parameter placement per platform idiom") and over touching the deprecatedFeatureFlagPayloadpath (out of scope for a single-contract change). The nil-receiver case was included because Go's nil-safe snapshot is how the spec's "flags not loaded yet / failed flags request" states actually reachIsEnabled.Per
AGENTS.md, the published sdk-spec is the public-API agreement, so no separate issue was opened.Created with PostHog Desktop
🤖 Generated with Claude Code