Repository navigation
feat: report unresolved local flags on evaluate_flags snapshots - #1133
Draft
gustavohstrassburger wants to merge 2 commits into
Draft
gustavohstrassburger wants to merge 2 commits into
gustavohstrassburger wants to merge 2 commits into
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.unresolved_flags`, 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, also for dependencies. - Log one warning per flag that local evaluation can never resolve, repeated only when its definition changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> 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}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 5572e1c: 3 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
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.
Comment on lines
564
to
568
| except RequiresServerEvaluation as e: | ||
| # Static cohort or other missing server-side data - must fallback to API. | ||
| # A reason an earlier condition found can still outrank this one. | ||
| e.reason = _pick_unresolved_reason(inconclusive_reason, e.reason) | ||
| raise |
Contributor
There was a problem hiding this comment.
P2 · Preserve reason precedence across server-only conditions
When the first condition depends on a static-cohort flag and a later condition uses an unknown operator, this rethrow reports unresolved_dependency. Reversing the conditions reports unsupported_definition, violating the spec precedence required by AGENTS.md:21. Inspect later definition-side causes before propagating the server error.
Comment on lines
+3702
to
+3709
| e.reason = ( | ||
| _get_definition_unresolved_reason( | ||
| feature_flag, | ||
| flags_by_key, | ||
| snapshot["cohorts"], | ||
| group_type_mapping, | ||
| ) | ||
| or e.reason |
Contributor
There was a problem hiding this comment.
P2 · Check unresolved dependencies before reporting a missing device ID
For a device-bucketed flag depending on an experience-continuity flag, omitting device_id reports missing_context instead of the higher-priority unresolved_dependency. This classifier never detects unresolved dependencies, violating the spec precedence required by AGENTS.md:21; account for those dependencies before choosing the missing-device reason.
|
|
||
| if fingerprint != self._flag_definition_fingerprint: | ||
| self._flag_definition_fingerprint = fingerprint | ||
| self._warn_unresolvable_flags() |
Contributor
There was a problem hiding this comment.
P2 · Compute warning diagnostics outside the publication lock
Shared large cohorts make _warn_unresolvable_flags() repeat the same parsing under _flag_definition_publication_lock. A synthetic 1,000-flag/1,000-property refresh holds it for 2.6s, blocking concurrent evaluations in _local_evaluation_snapshot(); slow warning handlers also extend the stall. Compute diagnostics and emit warnings outside the publication lock.
Contributor
posthog-python Compliance ReportDate: 2026-10-28T04:00:31.083053+00:00 ✅ All Tests Passed!121/121 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
This was referenced Oct 27, 2026
Draft
This branch has not been deployed
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
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
evaluate_flags()snapshot, so it looks the same as a flag that does not exist, and$feature_flag_calledreportsflag_missingfor both. Withonly_evaluate_locally=Truethis is permanent. posthog-node already implements this (PostHog/posthog-js#5234).This replaces #1033, which was closed as stale. It is rebased on
mainand adds the spec parts that #1033 did not have.Changes:
FeatureFlagEvaluations.unresolved_flagsmaps each unresolved flag to anUnresolvedFlagReason(experience_continuity,unsupported_definition,unresolved_dependency,missing_context). When several causes apply, the spec's fixed precedence picks one. Snapshots fromonly()/only_accessed()have no entries, and reading the map is not an access.local_evaluation_inconclusiveinstead offlag_missing. Values, keys and capture enrichment do not change.missing_context; a bad filter value isunsupported_definition.AsyncClientevaluates only remotely, so its snapshots have no unresolved flags. This is documented.Behavior changes (in the changeset):
/flags. A static cohort still forces the fallback only when it is the first filter in its condition that cannot be checked, as in posthog-node.falselocally.Divergences from the spec or posthog-node:
false, as before, so it is not reported asmissing_context. posthog-node does the same.unsupported_definitionbut are not in the load-time warning, because the definition alone does not show them.unresolved_dependency), which keeps the current fallback. posthog-node turns it into an inconclusive error.💚 How did you test it?
Unit tests mirror every scenario tagged
@unresolved_flags_capableand@unresolvable_flag_warning_capableinacceptance/public/evaluate-flags.feature, including the device-id row, plus precedence, the condition loop, dependencies, warning pruning and unexpected errors. Run locally:pytest --timeout=30(5706 passed, 25 skipped). Live AI-provider tests were run with the provider API keys unset.ruff format --check .,ruff check ., mypy with the baselinemake public_api_checkNo manual testing in an app.
📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
main. Then added reasons for the newer regex raise sites, the spec's reason precedence (so a later server error cannot overwrite a higher reason), the condition loop change, dependency checks, theunsupported_definitionwarning and per-flag pruning of the warning history.Created with PostHog Desktop
🤖 Generated with Claude Code