Skip to content

fix(local-feature-flag-evaluator): define presence operator semantics - #49

Merged
marandaneto merged 5 commits into
mainfrom
fix/define-is-set-null-semantics
Aug 26, 2026
Merged

fix(local-feature-flag-evaluator): define presence operator semantics#49
marandaneto merged 5 commits into
mainfrom
fix/define-is-set-null-semantics

Conversation

@marandaneto

@marandaneto marandaneto commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

  • Define is_set using property-key presence, including an explicitly present null value.
  • Define is_not_set for partial SDK property maps: present properties definitively do not match, while omitted properties remain inconclusive.
  • Keep false, zero, empty strings, and empty collections classified as present for both operators.
  • Clarify that current property maps cannot express a property known to be absent; that requires future SDK and /flags API inputs.
  • Add private acceptance scenarios for null, other falsey values, and missing property context for both operators.

Why

The authoritative flags-service runtime implements is_set with key presence and explicitly tests that a present JSON null matches. For is_not_set, it distinguishes partial from complete property context. A present key does not match, a missing key in partial context is inconclusive, and a missing key matches only when the context is known to be complete.

Server SDKs currently disagree on both operators. Node.js and Rust treat an omitted property as matching is_not_set, which assumes the SDK property map is complete. Python, Go, PHP, Ruby, .NET, and the Android server SDK keep the operator inconclusive even when a property is present. Pending posthog-elixir#192 implements the required partial-property behavior and serves as the reference implementation.

A missing key cannot safely produce a definitive presence result with the current SDK inputs because omission does not distinguish known absence from unavailable context. Adding that distinction is outside this change and requires a separate SDK evaluation option plus a corresponding /flags request contract.

Validation

  • openspec validate --all --strict --no-interactive - 62 passed, 0 failed
  • Parsed acceptance/private/local-feature-flag-evaluator.feature with gherkin-official
  • git diff --check
  • Autoreview passed for the branch tree signed in commit e16ea0a with no actionable findings

@marandaneto
marandaneto requested a review from a team August 25, 2026 18:05
@marandaneto
marandaneto marked this pull request as ready for review August 25, 2026 18:05
@marandaneto
marandaneto requested a review from a team as a code owner August 25, 2026 18:05
@marandaneto

Copy link
Copy Markdown
Member Author

found this by doing PostHog/posthog-elixir#192

Comment thread openspec/specs/local-feature-flag-evaluator/spec.md Outdated
@dustinbyrne
dustinbyrne requested a review from a team August 25, 2026 18:46
@marandaneto marandaneto changed the title fix(local-feature-flag-evaluator): define is_set null semantics fix(local-feature-flag-evaluator): define is_set presence semantics Aug 25, 2026
@haacked

haacked commented Aug 25, 2026

Copy link
Copy Markdown

Just so I’m clear, under this proposal, is_set never evaluates to false locally. Either it’s true if the property key is provided or it’s inconclusive and falls back to remote evaluation because it wasn’t provided.

There would be no way to have is_set evaluate to false locally.

If that’s so, that seems problematic. What if the local environment knows the property is not set? There’s no way to communicate that without a remote eval fallback.

I would think we either allow null to represent unset OR have a special sentinel value that means unset.

@marandaneto

marandaneto commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Just so I’m clear, under this proposal, is_set never evaluates to false locally. Either it’s true if the property key is provided or it’s inconclusive and falls back to remote evaluation because it wasn’t provided.

There would be no way to have is_set evaluate to false locally.

If that’s so, that seems problematic. What if the local environment knows the property is not set? There’s no way to communicate that without a remote eval fallback.

I would think we either allow null to represent unset OR have a special sentinel value that means unset.

Correct

Given the current API limitations, this PR defines the safest behavior:

  • Present key, including null → is_set is true.
  • Missing key → context is unknown, so local evaluation is inconclusive.
  • Remote evaluation remains eligible.

The spec cannot safely make a missing key evaluate to false, because omission does not distinguish known absence from an incomplete property map.

I would only add a clarification that known absence cannot currently be represented and requires a future SDK and /flags API extension
See 0a19bad

To fix this we'd need API change, but I'd say it'd not be a blocker for this change until we have proper API support to distinguish present/missing

example

  1. SDK API change
    SDK evaluation options need a way to express known absence, such as:

      personPropertyKeysKnownAbsent
      groupPropertyKeysKnownAbsent
    

    This can be backward-compatible because the fields are optional.

  2. /flags API change
    If local evaluation cannot resolve the flag and falls back remotely, the SDK must communicate known-absent
    properties. Otherwise the server may use stored values and produce a different result.

    This could use optional request fields such as:

      {
        "person_property_keys_known_absent": ["plan"],
        "group_property_keys_known_absent": {
          "organization": ["subscription"]
        }
      }

Without the /flags change, explicit absence could safely affect only evaluations resolved locally. That would be
incomplete and could create local/remote disagreement.

issue for tracking this PostHog/posthog#88947

@marandaneto

Copy link
Copy Markdown
Member Author

the goal of this spec is to choose the best/correct behavior based on the API limitation so that all SDKs would evaluate in the same way, right now they dont

@haacked haacked left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: LGTM! As discussed, representing not set locally is out of scope of this PR.

@haacked

haacked commented Aug 25, 2026

Copy link
Copy Markdown

Agree with your reasoning. Out of curiosity, how do we handle is not set locally? Does that evaluate to true when property is missing given as you pointed out we can’t tell the difference between incomplete property map and omission?

@marandaneto

marandaneto commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

@haacked Great question. The flags service itself distinguishes partial from complete property context. With partial properties, is_not_set returns false when the key is present and is inconclusive when the key is missing. It only returns true for a missing key when the property context is known to be complete.

Current SDK property inputs have no completeness or known-absence signal, so they should be treated as partial. Therefore the safe local behavior is:

  • Present key, including null: is_not_set = false
  • Missing key: inconclusive

Current SDKs disagree. Node and Rust treat a missing key as true, while most SDKs make the operator entirely inconclusive. Elixir PR #192 currently has the behavior that best matches the flags service's partial-property mode.

SDK Missing property Present property Assessment
Node.js true false Treats the map as complete; unsafe for partial overrides
Rust true false Same as Node; matches complete-context mode
Elixir PR #192 Inconclusive false Best match for current partial SDK inputs
Python Inconclusive Inconclusive Safe but does not resolve the known-present case
Go Inconclusive Inconclusive Same
PHP Inconclusive Inconclusive Same
Ruby Inconclusive Inconclusive Same
.NET Inconclusive Inconclusive Same
Android posthog-server Inconclusive Inconclusive Same

The mobile/client SDKs and analytics wrappers do not run this server-side local-definition evaluator, so they are not applicable. No SDK currently exposes an explicit known-absent set or a property-map completeness marker.

I'll align SDKs with this new change here e16ea0a

@marandaneto marandaneto changed the title fix(local-feature-flag-evaluator): define is_set presence semantics fix(local-feature-flag-evaluator): define presence operator semantics Aug 26, 2026
@marandaneto
marandaneto merged commit dfddf1a into main Aug 26, 2026
10 checks passed
@marandaneto
marandaneto deleted the fix/define-is-set-null-semantics branch August 26, 2026 06:26
@marandaneto

Copy link
Copy Markdown
Member Author

i think PostHog/posthog#52747 is related to that as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants