Repository navigation
feat(flags): report unresolved local flags on EvaluateFlagsAsync snapshots - #357
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 <[email protected]> Generated-By: PostHog Desktop Task-Id: 39793b31-809e-4689-a5d8-b4359cbb22c1
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.
posthog-dotnet Compliance ReportDate: 2026-10-28T19:59:03.027696+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
|
dustinbyrne
left a comment
There was a problem hiding this comment.
The cohort-regex precedence fix looks correct and the focused regression checks pass. I reproduced a separate dependency-evaluation regression where the SDK selects a later multivariate variant instead of the server’s earlier matching variant. Consider preserving outcome-dependent server uncertainty before approval.
AI-assisted review.
| } | ||
| catch (RequiresServerEvaluationException e) | ||
| { | ||
| throw new InconclusiveMatchException( |
There was a problem hiding this comment.
F1 — [P1] Preserve uncertainty when a server-required dependency can change the variant
IMPORTANT · REQUIRED · Introduced by the full PR; unchanged by this follow-up
Location: src/PostHog/Features/LocalEvaluator.cs:1100–1105, particularly line 1102, RIGHT.
When a flag’s first condition depends on unavailable static-cohort membership, head can choose a later, different variant locally. For a person who matches the first condition on the server, the SDK now returns b where base correctly fell back and returned a.
The minimal valid definition setup is:
{
"flags": [
{
"id": 1, "key": "dep", "active": true,
"filters": {
"groups": [{
"properties": [{"key": "id", "type": "cohort", "value": 999}],
"rollout_percentage": 100
}]
}
},
{
"id": 2, "key": "target", "active": true,
"filters": {
"groups": [
{
"properties": [{
"key": "dep", "type": "flag", "value": true,
"operator": "flag_evaluates_to",
"dependency_chain": ["dep"]
}],
"rollout_percentage": 100, "variant": "a"
},
{
"properties": [],
"rollout_percentage": 100, "variant": "b"
}
],
"multivariate": {
"variants": [
{"key": "a", "rollout_percentage": 50},
{"key": "b", "rollout_percentage": 50}
]
}
}
}
],
"cohorts": {}
}For a person belonging to static cohort 999, remote evaluation returns:
{"featureFlags":{"dep":true,"target":"a"}}Ordinary public caller:
var snapshot = await client.EvaluateFlagsAsync(
"member",
new AllFeatureFlagsOptions {
FlagKeysToEvaluate = new[] { "target" }
},
CancellationToken.None);
var variant = snapshot.GetFlag("target")?.VariantKey;Fresh executed results:
| Configuration | Merge base | Previous head | Current head |
|---|---|---|---|
| Scoped target, fallback enabled | a, one remote request |
b, none |
b, none |
| Unscoped, fallback enabled | a, one request |
b, one request |
b, despite remote a |
| Scoped, local-only | Target absent | Local b |
Local b |
| Dependency locally resolves true — control | a, no request |
Same | Same |
Retained GetFeatureFlagAsync |
a, one request |
b, none |
b, none |
The new catch converts RequiresServerEvaluationException into ordinary InconclusiveMatchException. The latter permits later conditions; the former previously forced fallback. In the unscoped case, the existing local-wins merge subsequently retains unsafe local b over correct remote a.
Actual SDK exposure batches also changed from response a, remotely evaluated, to response b, locally evaluated. Applications can branch on that wrong variant. Both variants are enabled: this reproduction does not demonstrate changed boolean enablement or a security boundary.
Contract qualification: The PR intentionally discloses continuation, current Node behaves similarly, and the canonical local-evaluator contract generally permits continuing inconclusive groups. Continuation itself is not the defect.
The narrower issue is accepting b while an earlier condition can select a. The same contract requires ordered variant selection. The backend compares uncertain answers including variants, while Python preserves the server-required signal. There is no universal peer consensus, but the base/head comparison demonstrates a newly unsafe public path. Disclosure is not explicit acceptance of this particular consequence.
Regression evidence: The identical 28-case harness exercised actual SDK public methods on all three revisions with the fixture above. All compilations/runs and positive controls exited 0. python3 assertions.py dependency first confirms base returns a with one request, then fails on current head:
AssertionError:
('dependency-scoped-True-localOnly-False-localDep-False', 'b', 0)
The unscoped, local-only, legacy-getter and positive-control outcomes are independently recorded in the retained JSON results. Execution used exact SDK source with .NET 9 framework substitution; native shipping-framework xUnit execution was not performed.
Recommendation: Consider retaining the server-forcing signal while carrying unresolved_dependency, or permitting continuation only when earlier uncertainty cannot change the selected answer. A conservative repair does not require a general possible-value engine.
Add native regressions for the scoped/unscoped conflicting variants, local-only unresolved result, locally resolvable dependency and retained getter. No corrective implementation was applied or tested.
There was a problem hiding this comment.
Fixed in 4b7c331. A dependency that needs server data now raises the server-required error again (reason unresolved_dependency), so the flag falls back to /flags as on main, and a higher-precedence reason elsewhere in the flag still wins. Regression tests: FlagWhoseDependencyNeedsServerDataFallsBackToTheServer (GetFeatureFlagAsync, scoped and unscoped EvaluateFlagsAsync all return the server's a), LocalOnlyEvaluationReportsFlagWhoseDependencyNeedsServerDataAsUnresolvedDependency, the control FlagWhoseDependencyResolvesLocallyDoesNotCallTheServer, DependencyThatRequiresServerForcesServerFallbackAsUnresolvedDependency, and new precedence cases in ReportsTheHighestPrecedenceReasonWhateverTheOrder.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 4b7c331: 1 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
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
There was a problem hiding this comment.
The dependency fallback and malformed-definition fixes address the earlier findings. Verified scoped/unscoped snapshots, the legacy getter and local-only behavior; current CI passes.
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
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