Repository navigation
feat(flags): report unresolved local flags on EvaluateFlags snapshots - #396
Conversation
Implements the "Snapshot unresolved flag reporting" and "Warning for flags local evaluation never resolves" requirements of the evaluate-flags spec (PostHog/sdk-specs#85). - Add `FeatureFlagEvaluations.UnresolvedFlags()`, with an `UnresolvedFlagReason` for each flag and the spec's fixed precedence. - Report `local_evaluation_inconclusive` instead of `flag_missing` when a snapshot read hits an unresolved flag. - Do not stop a condition at the first inconclusive filter, so a filter that does not match resolves the condition locally. - Check `Active` before experience continuity. - Log one warning per flag that local evaluation can never resolve, repeated only when its definition changes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Generated-By: PostHog Desktop Task-Id: 39793b31-809e-4689-a5d8-b4359cbb22c1
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 7eb1932: 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
posthog-go Compliance ReportDate: 2026-10-28 20:12:40 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
|
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
The payload-fingerprint change looks sound. One reporting-contract decision remains before approval: known group flags with missing group keys are still omitted from unresolved reporting. Cohort reason precedence is smaller, optional feedback. I’m not repeating the mixed-group comment following the explicit Node-parity decision.
AI-assisted review.
| ) | ||
| if err != nil { | ||
| c.debugf("Unable to compute flag '%s' locally - %s", storedFlag.Key, err) | ||
| if reason, ok := unresolvedReasonOf(err); ok { |
There was a problem hiding this comment.
[P2] Include missing flag-level group context in snapshot reporting
Disposition: REQUIRED reporting-contract correction or explicit public limitation acceptance
Location: posthog.go:1371–1373, RIGHT
Relationship: New reporting gap introduced by this PR; unchanged by the follow-up.
For a loaded active company flag, omitting its required group key leaves the flag absent from both Keys() and UnresolvedFlags(). Reading it still reports flag_missing.
Expected: An unresolved entry with missing_context, followed by local_evaluation_inconclusive exposure metadata.
Source-inferred result: No unresolved entry and flag_missing; the flag value remains absent.
The group-key branch at featureflags.go:1577–1581 returns a plain error. The new collector recognizes only dedicated SDK evaluator errors, so it omits this known flag.
The canonical reporting contract explicitly includes absent group keys in missing_context. Public wording at feature_flag_evaluations.go:136–142 and posthog.go:1175–1178 also promises broader reporting.
Base comparison and impact: Base already returned this plain error and omitted the value. This is not a new incorrect flag decision; the new API fails its central known-versus-missing distinction for ordinary group context.
Consider classifying this case for snapshot reporting while preserving legacy getter errors and values. Alternatively, explicitly accept and publicly document the reporting exception. Documentation would disclose—not satisfy—the current canonical contract.
Regression sketch — NOT EXECUTED:
func TestReviewMissingGroupKey(t *testing.T) {
const defs = `{"flags":[{"key":"checkout","active":true,"filters":{"aggregation_group_type_index":0,"groups":[{"properties":[],"rollout_percentage":100}]}}],"group_type_mapping":{"0":"company"},"cohorts":{}}`
c, capture, calls := newUnresolvedFlagsClient(t, defs, remoteNoFlags)
control, err := c.EvaluateFlags(EvaluateFlagsPayload{
DistinctId: "control", Groups: Groups{"company": "acme"},
OnlyEvaluateLocally: true,
})
require.NoError(t, err)
require.Equal(t, []string{"checkout"}, control.Keys())
s, err := c.EvaluateFlags(EvaluateFlagsPayload{
DistinctId: "user", OnlyEvaluateLocally: true,
})
require.NoError(t, err)
require.Equal(t, UnresolvedFlagReasonMissingContext,
s.UnresolvedFlags()["checkout"])
require.Nil(t, s.GetFlag("checkout"))
require.Equal(t, FeatureFlagErrorLocalEvaluationInconclusive,
flagCalledError(t, c, capture, "checkout"))
require.Zero(t, calls.Load())
}Intended command: go test -v -count=1 -run '^TestReviewMissingGroupKey$' .
The supplied-group control is silent, avoiding interference with the exposure assertion. Base has no unresolved accessor and serves as an existing-value/exposure control, not a compilation-failure regression.
Execution blocker: No working isolated sandbox; Docker unavailable and sandbox-exec aborts with exit 134. No test failure or passing fix is claimed.
There was a problem hiding this comment.
Fixed in 239c619: a flag-level group flag whose group key is missing now returns an InconclusiveMatchError with missing_context (a definition reason such as experience continuity or a static cohort still wins), so EvaluateFlags lists it in UnresolvedFlags() and a read reports local_evaluation_inconclusive. The legacy getters keep the same value, error message, log line, $feature_flag_error and /flags fallback. Covered by TestEvaluateFlags_GroupFlagWithoutGroupKey and TestLegacyGettersGroupFlagWithoutGroupKey.
| if isServerEvalError(err) { | ||
| return false, err | ||
| } else if isInconclusiveError(err) { | ||
| if serverErr, ok := err.(*RequiresServerEvaluationError); ok { |
There was a problem hiding this comment.
[P2] Merge cohort definition reasons before returning its server error
Disposition: OPTIONAL
Location: feature_flag_property_group.go:78–80, RIGHT; nested counterpart at lines 45–47
Relationship: New reason-reporting gap introduced by this PR; unchanged by the follow-up.
A dynamic AND cohort containing a server-required flag dependency followed by an unsupported property criterion reports unresolved_dependency. Reverse the same leaves and it reports unsupported_definition.
Expected: unsupported_definition in either order under the fixed precedence contract.
Source-inferred result: The reason depends on cohort-leaf order.
A concrete supported definition uses is_date_exact: the pinned backend’s filters_schema.py:28–53 lists it as supported, while this Go evaluator lacks that operator. Its absence is pre-existing and is not a separate finding; it provides a realistic unsupported-definition trigger.
The dependency’s server error is tagged unresolved_dependency, then returned immediately from the cohort wrapper. The later-condition scan does not inspect the skipped remainder of the current cohort. Existing cohort-dependency tests and cache allocation establish the supported public path.
Base comparison and impact: Both orders already lacked a local value and required fallback. Only the new remediation category is wrong; no new value or fallback regression is asserted.
Consider merging definition-only reasons from the skipped cohort subtree while preserving the server-required error type and matching/fallback behavior.
Regression sketch — NOT EXECUTED:
func TestReviewCohortPrecedence(t *testing.T) {
dep := flagDefinition("dep", flagCondition(100, staticCohortFilter))
target := flagDefinition("checkout", flagCondition(100,
`{"key":"id","type":"cohort","value":1}`))
dependency := flagDependencyFilter("dep", true, "dep")
unsupported := `{"key":"signup","type":"person","operator":"is_date_exact","value":"2025-01-01"}`
for _, leaves := range []string{
unsupported + "," + dependency,
dependency + "," + unsupported,
} {
defs := `{"flags":[` + dep + `,` + target +
`],"group_type_mapping":{},"cohorts":{"1":{"type":"AND","values":[` +
leaves + `]}}}`
c, _, calls := newUnresolvedFlagsClient(t, defs, remoteNoFlags)
s, err := c.EvaluateFlags(EvaluateFlagsPayload{
DistinctId: "user", FlagKeys: []string{"checkout"},
PersonProperties: Properties{"signup": "2025-01-01"},
OnlyEvaluateLocally: true,
})
require.NoError(t, err)
require.Empty(t, s.Keys())
require.Equal(t, UnresolvedFlagReasonUnsupportedDefinition,
s.UnresolvedFlags()["checkout"])
require.Zero(t, calls.Load())
}
}Intended command: go test -v -count=1 -run '^TestReviewCohortPrecedence$' .
Definition-first is the control; dependency-first is inferred to fail. The equivalent base controls retain the same missing-value/exposure behavior without treating the absent new API as a regression.
Execution blocker: The same unavailable isolation environment as F1. No reviewer-test execution result is claimed.
There was a problem hiding this comment.
Fixed in 55d276b. When a server-required error leaves the condition loop, the reason now merges the whole-flag definition reason from definitionUnresolvedReason, which walks every condition and cohort subtree, so the narrower later-conditions helper is gone. Covered in TestConditionLoopDoesNotStopAtFirstInconclusiveFilter by "server-required dependency then a higher reason in a cohort", its reversed order, and "server-required dependency does not hide a higher reason in a condition ruled out earlier".
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 55d276b: 1 Should fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
Both earlier findings are fixed, and the regression, legacy-getter and fallback checks pass. No further actionable findings.
AI-assisted review.
💡 Motivation and Context
Why: sdk-specs #85 defines how a server SDK reports flags that local evaluation could not resolve. Today such a flag is absent from the
EvaluateFlagssnapshot, so it looks the same as a flag that does not exist, and$feature_flag_calledreportsflag_missingfor both. WithOnlyEvaluateLocallythis is permanent. posthog-node already implements this (PostHog/posthog-js#5234).Changes:
FeatureFlagEvaluations.UnresolvedFlags()returns a copy of a map from each unresolved flag to anUnresolvedFlagReason(experience_continuity,unsupported_definition,unresolved_dependency,missing_context). When several causes apply, the spec's fixed precedence picks one.Only/OnlyAccessedsnapshots have no entries, and reading the map is not an access.local_evaluation_inconclusiveinstead offlag_missing.InconclusiveMatchErrorandRequiresServerEvaluationErrorhave a newReasonfield, set at every raise site and kept through the wrappers. A missing group key is now anInconclusiveMatchErrorwithmissing_context, so it is reported; other plainerrors.Newfailures are unchanged.Logger.Warnfline for flags local evaluation can never resolve. The fingerprint is the reason plus a hash of the flag's JSON with sorted keys, so 304 responses and cache re-applies do not repeat it.Behavior changes (in the changeset):
falselocally.Divergences from the spec or posthog-node:
distinct_id(so the device-id acceptance row does not apply), and an invalid regex still resolves as no match. A missing group key is reported asmissing_context(posthog-node resolves it tofalse), with the same fallback and no local value as before.unsupported_definition), and non-numeric or malformed-date values.unsupported_definitioncauses: an exact filter whose values are all integral JSON numbers, a regex RE2 cannot run like the service, and a cross-aggregation dependency.missing_contextunless the definition has a higher reason.💚 How did you test it?
New testify tests mirror every scenario tagged
@unresolved_flags_capableand@unresolvable_flag_warning_capableinacceptance/public/evaluate-flags.feature, plus precedence, the condition loop, dependency reasons, holdouts, warning dedup (cache and 304) and unexpected errors. One existing case inTestFeatureFlagWithDependenciesnow resolves locally, as the spec requires. Run locally:go build ./...,go vet ./...,gofmt -l .go test -count=1 ./...andgo test -race -count=1 ./...make api-diffGOTOOLCHAIN=go1.21.13(the CI minimum):go vet .,go test .,go mod tidy(no changes)No manual testing in an app.
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Activeorder changes the spec needs.Created with PostHog Desktop
🤖 Generated with Claude Code