Repository navigation
feat: report unresolved local flags on evaluate_flags snapshots - #1033
Draft
gustavohstrassburger wants to merge 1 commit into
Draft
gustavohstrassburger wants to merge 1 commit into
gustavohstrassburger wants to merge 1 commit into
Conversation
Implements PostHog/sdk-specs#85: the snapshot exposes `unresolved_flags` (key -> UnresolvedFlagReason), reads of those keys report `local_evaluation_inconclusive` instead of `flag_missing`, experience continuity flags log one warning per definition, and inactive flags resolve to false before the continuity check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 6abf34b0-7fbe-430e-bd75-fbf8684ef581
Contributor
posthog-python Compliance ReportDate: 2026-10-07T18:42:18.166889+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
|
Contributor
|
[Medium risk] Adds unresolved flag tracking to local evaluation. The PR appears safe to merge, with non-blocking fixes for misleading reasons and retained warning history. Reviews (1) · Last reviewed commit: "feat: report unresolved local flags on e..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Evaluation error (bad regex, invalid date, missing property, etc.) | ||
| # Track that we had an inconclusive match, but try other conditions | ||
| is_inconclusive = True | ||
| inconclusive_reason = inconclusive_reason or e.reason |
Contributor
There was a problem hiding this comment.
When one condition lacks a person property and a later condition uses a static cohort, match_feature_flag_properties saves missing_context but then throws the later RequiresServerEvaluation. The snapshot reports unsupported_definition instead of the promised first reason, which can lead callers to fix the wrong problem. Preserve the saved reason when rethrowing without changing the immediate server fallback. Apply the same fix in match_property_group.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/feature_flags.py
Line: 529
Comment:
**First unresolved reason gets replaced**
When one condition lacks a person property and a later condition uses a static cohort, `match_feature_flag_properties` saves `missing_context` but then throws the later `RequiresServerEvaluation`. The snapshot reports `unsupported_definition` instead of the promised first reason, which can lead callers to fix the wrong problem. Preserve the saved reason when rethrowing without changing the immediate server fallback. Apply the same fix in `match_property_group`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| definition = json.dumps(flag, sort_keys=True, default=str) | ||
| if self._warned_unresolvable_flags.get(key) == definition: | ||
| continue | ||
| self._warned_unresolvable_flags[key] = definition |
Contributor
There was a problem hiding this comment.
_warned_unresolvable_flags keeps each flag’s full definition after that flag disappears from loaded definitions. Long-running clients accumulate deleted or archived flags as projects create and remove them. Remove entries for keys no longer loaded so warning history stays bounded by the current flag set.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/client.py
Line: 3203
Comment:
**Removed flags stay in memory**
`_warned_unresolvable_flags` keeps each flag’s full definition after that flag disappears from loaded definitions. Long-running clients accumulate deleted or archived flags as projects create and remove them. Remove entries for keys no longer loaded so warning history stays bounded by the current flag set.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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
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
When local evaluation cannot resolve a flag (most often a flag with experience continuity) and no remote value fills the gap, the flag is absent from the
evaluate_flags()snapshot and reads like a flag that does not exist.$feature_flag_calledreportsflag_missingfor both. In local-only mode this is permanent.Why: implement the spec change in PostHog/sdk-specs#85 so callers can detect these flags and the event data separates them from missing flags. Node counterpart: PostHog/posthog-js#5234.
Changes:
FeatureFlagEvaluations.unresolved_flags: read-only mapping of flag key toUnresolvedFlagReason(experience_continuity,unsupported_definition,missing_context,unresolved_dependency). Empty ononly()/only_accessed()snapshots. Reading it does not count as an access.local_evaluation_inconclusiveinstead offlag_missing. Return values,keysand capture enrichment are unchanged.InconclusiveMatchErrorandRequiresServerEvaluationcarry areason, set at each raise site. The first cause found wins.Falsebefore the experience continuity check (spec: inactive flags are never unresolved). This also applies to the single-flag APIs.💚 How did you test it?
@unresolved_flags_capableacceptance scenarios, filtered snapshots, dependency and cohort reasons, and the warning dedupe.pytest posthog/test/test_evaluate_flags.py posthog/test/test_feature_flags.pypasses. The full suite passes except the AI integration tests that need live API keys; they fail onmaintoo.ruff,mypy(baseline), andmake public_api_checkpass.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Code in PostHog Desktop from the sdk-specs #85 text. Known gaps, left as existing behaviour: a group flag called without its group key still resolves to
False; a dependency's own experience continuity is not checked when it is evaluated only as a dependency.AsyncClient.evaluate_flagsis remote-only, so its map is always empty.Created with PostHog Desktop
🤖 Generated with Claude Code