Repository navigation
feat(flags): report unresolved local flags on EvaluateFlagsAsync snapshots - #357
gustavohstrassburger wants to merge 2 commits into
Conversation
…shots 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 <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}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 3c0989a: 3 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.
| throw new InconclusiveMatchException( | ||
| "Unicode regex matching requires server evaluation", | ||
| UnresolvedFlagReason.UnsupportedDefinition); |
There was a problem hiding this comment.
P2 · Classify unsupported Unicode input as missing context
With a valid pattern such as ^pro$ and property value café, this branch reports UnsupportedDefinition in UnresolvedFlags. The definition works with ASCII input; the supplied context causes the failure. The SDK contract required by AGENTS.md:11 classifies unusable context as MissingContext; report that reason here.
| case ComparisonOperator.Regex or ComparisonOperator.NotRegex: | ||
| if (value.StringValue is { } pattern && !RegexHelpers.IsCompatiblePattern(pattern)) | ||
| { | ||
| throw new InconclusiveMatchException("Regex syntax requires server evaluation", UnresolvedFlagReason.UnsupportedDefinition); | ||
| } | ||
| return; |
There was a problem hiding this comment.
P2 · Validate malformed regex syntax when scanning cohort definitions
For a dynamic cohort whose sole filter uses regex [, IsCompatiblePattern returns true without compiling it. GetDefinitionUnresolvedReason skips the warning, although MatchPropertyGroup always reports UnsupportedDefinition. This misses the definition warning covered by AGENTS.md:11; validate regex syntax when scanning cohort filters.
| switch (filter) | ||
| { | ||
| case FilterSet filterSet: | ||
| return filterSet.Values.Any(child => IsUnsupportedFilter(child, visitedCohortIds, inCohort)); |
There was a problem hiding this comment.
P2 · Handle null cohort values before scanning for unsupported filters
When a loaded cohort contains "values": null, MatchPropertyGroup treats it as an empty group, but this new scan calls Any on null. The resulting ArgumentNullException escapes PollForFeatureFlagsAsync, which excludes argument exceptions from recovery, permanently stopping definition polling and leaving flags stale. Treat null values as empty here.
posthog-dotnet Compliance ReportDate: 2026-10-28T03:58:46.170819+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
💡 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
EvaluateFlagsAsyncsnapshot, 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.UnresolvedFlagsmaps each unresolved flag to anUnresolvedFlagReason(ExperienceContinuity,UnsupportedDefinition,UnresolvedDependency,MissingContext;ToValueString()and the JSON form give the spec strings). When several causes apply, the spec's fixed precedence picks one.Only,OnlyAccessedandEmptysnapshots have no entries, and reading the map is not an access.local_evaluation_inconclusiveinstead offlag_missing.InconclusiveMatchExceptionandRequiresServerEvaluationExceptioncarry aninternalreason, so their shipped public surface does not change. The date and semver parsers tell a bad filter value (UnsupportedDefinition) from a bad person value (MissingContext).LocalFeatureFlagsLoaderlogs a warning for flags local evaluation can never resolve, deduplicated per flag on the definition plus the reason and rebuilt on each200. A304does not repeat it. The per-evaluation static-cohort error log is removed.Behavior changes (in the changeset):
falselocally.Divergences from the spec or posthog-node:
falseand there is no device-id bucketing, so neither givesMissingContextand the device-id acceptance row does not apply.versionfield, so the warning fingerprint compares the parsed definition. There is no shared definitions cache, so the shared-cache warning row does not apply.OnlyAccessed()with nothing accessed still returns all flags (existing behavior), so the spec step "only accessed returns no flags" differs. Its unresolved map is empty.💚 How did you test it?
About 100 new xUnit tests mirror every scenario tagged
@unresolved_flags_capableand@unresolvable_flag_warning_capableinacceptance/public/evaluate-flags.feature, plus precedence, the condition loop, dependencies, warning dedup and pruning, and unexpected errors. Run locally with the .NET 9 SDK:dotnet restore --locked-modedotnet build --configuration Release --no-restore(0 warnings, 0 errors, PublicApiAnalyzers included)dotnet test --configuration Release --no-build -f net8.0(UnitTests 1504 passed, 2 skipped; AspNetCore 68; AI 19)bin/fmt --checkThe
netcoreapp3.1tests did not run locally because the machine has no libssl 1.1; CI covers them. No manual testing in an app.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
FeatureFlagsLogWarnings, because that option is documented for the snapshot helpers only.Created with PostHog Desktop
🤖 Generated with Claude Code