Skip to content

fix(flags): honor versioned local property matching - #200

Merged
marandaneto merged 11 commits into
mainfrom
fix/versioned-boolean-local-evaluation
Sep 19, 2026
Merged

marandaneto merged 11 commits into
mainfrom
fix/versioned-boolean-local-evaluation

Conversation

@marandaneto

@marandaneto marandaneto commented Sep 5, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

Local flag evaluation needs to follow the property_matching_version returned with flag definitions. This implements the behavior in the backend change and the shared SDK contract.

This intentionally corrects a compatibility mismatch in the old Elixir implementation. Missing metadata and version 1 now use released service legacy matching, not the SDK's former array-membership behavior. For example, a false filter matches "banana" under service v1 truthiness, but not under v2. Existing applications can therefore see different local results even without version 2 metadata.

Only numeric version 2 selects explicit scalar equality and equality against individual members of a nonempty filter array. Empty filters retain recursive truthiness in both modes. For known properties, is_not complements exact. Missing properties and ambiguous numeric comparisons remain inconclusive.

Composite equality uses recursively sorted JSON object keys. Composite values containing floats or integers outside the service's i64/u64 range remain inconclusive because Jason and the service can serialize those numbers differently. For example, Jason writes {"n":0.00001} as {"n":1.0e-5}, which must not produce a definitive local mismatch. These cases can fall back to /flags, while local-only evaluation omits the unresolved flag.

Composite comparisons also stay inconclusive when either serialized side contains uppercase Greek sigma, whose contextual lowercase differs between Elixir and Rust, or when atom and string object keys collide after Jason-compatible conversion. Nested maps and arrays use the same safeguards. Atom object keys use their names, matching Jason, so nil becomes "nil", not an empty key. A map with both nil and "nil" stays inconclusive, while nil and "" remain distinct keys. Ordinary composite integers, noncolliding object keys and scalar Date/Time string conversion are preserved. Pre-existing scalar-string Unicode behavior is unchanged.

Composite normalization also leaves nested structs inconclusive, including Jason.OrderedObject and custom or derived Jason encoders. Their encoders can hide unsorted keys, duplicate keys or ambiguous numbers from recursive validation. This deliberately reduces local coverage even for nested structs that happen to encode safely, including nested Date/Time values. Plain maps and lists still normalize locally, and top-level scalar Date/Time string comparisons are unchanged. The evaluator does not serialize and decode arbitrary structs to guess their meaning.

When boolean truthiness reaches an opaque property struct, directly or inside an array, the result also stays inconclusive rather than assuming false. Native non-boolean property atoms use their JSON string names, so :TRUE behaves like "TRUE". Ordinary JSON truthiness and is_not complements remain unchanged. These fallback cases omit the flag in local-only mode and can use the server result otherwise.

The definition loader keeps the version in the existing immutable snapshot and complete cache-provider envelope. Person, group, recursive cohort and flag-dependency evaluation use that snapshot. HTTP and provider refreshes apply version-only changes even when flags are unchanged. Older envelopes without metadata default to version 1.

The existing Sampo patch changeset documents the service-v1 correction. No dependencies or harness adapters change. Optional harness coverage is separate.

💚 How did you test it?

  • Ran mix test: 507 passed, 18 live-service integration tests excluded by repository defaults. Live-service tests were not run. Existing deprecated getter and Req test-adapter warnings remain.
  • Independently reproduced the composite-number review finding in all four version/operator cases before fixing it. The same reproducer passes after the fix. An offline serde_json probe confirmed the decimal/exponent and out-of-range integer differences from Jason.
  • Added six permanent regression tests. They cover omitted/1/2 metadata, exact and is_not, nested maps and arrays on either side of equality, numeric range boundaries, mocked /flags fallback and local-only omission without affecting unrelated local flags.
  • Independently reproduced the composite Unicode finding with four failures and a Rust lowercase oracle. Added ten permanent tests covering missing/1/2 metadata, both operators, nested maps and arrays, keys, either comparison side, /flags fallback and local-only omission.
  • Committed-branch autoreview found colliding atom/string JSON keys. Six permanent tests failed before the repair and pass afterward. An offline serde_json oracle confirmed duplicate-key parsing and the service result. The repair leaves these comparisons inconclusive instead of guessing which key survives.
  • Independently reproduced the residual nil atom-key finding with 12 failing tests before the repair. Added six permanent tests and extended six collision tests across missing/1/2 metadata and both operators. These cover nested maps and arrays, either comparison side, Jason wire values, nil/string-key collisions and valid noncolliding atom and empty-string keys. All pass after the repair.
  • Independently reproduced the nested opaque-encoder bypass with 37 failing permanent tests before repair. Added 38 tests covering OrderedObject, three derived-encoder failure families, custom object/array/scalar encoders, nested maps and lists on both comparison sides, omitted/1/2 metadata, both operators, scalar and plain-JSON controls, local-only omission and mocked /flags fallback.
  • Committed-branch autoreview identified opaque scalar property truthiness. A wire-level probe confirmed JSON strings "true" and "TRUE" for a custom scalar encoder and a native atom, with correct merge-base results and incorrect new v1 results. After approval, a narrow truthiness repair made 18 additional failing tests pass. They cover nested arrays, v2 empty filters, both operators, atom/string equivalence, nil/boolean/map controls and server fallback.
  • Existing matcher and integration tests cover version-only HTTP/provider refreshes, stale responses, frozen snapshots, missing versus known null, nested boolean filters, sorted composite JSON and Date/Time scalar strings.
  • Ran mix format --check-formatted, mix compile --warnings-as-errors, mix posthog.public_api --check, mix credo --strict and git diff --check: passed.
  • Final committed-branch isolated autoreview against origin/main completed without actionable findings on b306825798e3607a2f062c3e7574bb9307351c78. Validation used Elixir 1.20.4 / OTP 29, not the exact 1.20.0-otp-29 / 29.0.1 pins. Local results do not establish CI status.

CI formatting repair

The CI Format job uses Elixir 1.18.3 / OTP 27.3. Its formatter wrapped one long two-argument assertion differently from local Elixir 1.20.4 / OTP 29. The assertion now uses explicit parentheses and multiline arguments. Its parsed AST is unchanged, including the pattern match and diagnostic message. No matcher, dependency or workflow changes were needed.

  • Rechecked the failing job log and its exact expected formatting diff. The exact CI toolchain was not installed locally. The original file passed the local formatter, so that result alone did not reproduce CI.
  • Ran the updated evaluator tests: 97 passed. Ran the full suite: 507 passed, 18 live-service tests excluded by repository defaults.
  • Ran local formatting, warnings-as-errors compilation, public API snapshot checks, strict Credo and git diff --check: passed. An AST comparison confirmed the edit changes layout only.
  • Isolated committed-branch autoreview against origin/main completed without actionable findings on d3bc85f555655e3e6439100535c77b04394f8b31.
  • Watched GitHub checks to completion on d3bc85f555655e3e6439100535c77b04394f8b31: all 28 active checks passed, with no failures or pending checks. The CI run passed Format on Elixir 1.18.3 / OTP 27.3, all 12 build/test combinations, Credo, warnings-as-errors compilation, public API checks and package build. SDK compliance also passed. One superseded PR-title run was cancelled when this description was updated, and its replacement passed. Local checks used Elixir 1.20.4 / OTP 29.0.5.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

The changelog entry is supplied through the Sampo changeset. The intentional behavior correction is described above.

If releasing new changes

  • Ran sampo add to generate a changeset file

The previously generated .sampo/changesets/roguish-lady-louhi.md is retained unchanged.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Pi implementation agents prepared and reviewed the changes under human direction. The publication and fresh-review fix sessions used Pi, Git, GitHub CLI, Mix, offline Rust serialization and lowercase probes and the isolated autoreview helper. Local session identifiers: sdk-pr-publication/posthog-elixir and pi-review-all/posthog-elixir/fix-1 and elixir-final-pass/fix and elixir-opaque-pass/fix and ci-fixes/posthog-elixir (no public session links).

The implementation keeps the existing provider envelope and immutable snapshot model. The numeric, Unicode, duplicate-key and opaque-struct repairs use the existing inconclusive fallback rather than adding a serializer, Unicode implementation or dependency. Human review is required before merge.

@marandaneto marandaneto self-assigned this Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

posthog-elixir Compliance Report

Date: 2026-09-19T06:21:49.539914+00:00
Duration: 108527ms

⚠️ Some Tests Failed

40/47 tests passed, 7 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 610ms
Format Validation.Event Has Uuid ✅ 610ms
Format Validation.Event Has Lib Properties ✅ 609ms
Format Validation.Distinct Id Is String ✅ 610ms
Format Validation.Token Is Present ✅ 609ms
Format Validation.Custom Properties Preserved ✅ 610ms
Format Validation.Event Has Timestamp ✅ 610ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ❌ 608ms
Retry Behavior.Retries On 503 ✅ 5615ms
Retry Behavior.Does Not Retry On 400 ✅ 2612ms
Retry Behavior.Does Not Retry On 401 ✅ 2611ms
Retry Behavior.Respects Retry After Header ✅ 5615ms
Retry Behavior.Implements Backoff ✅ 15620ms
Retry Behavior.Retries On 500 ✅ 5616ms
Retry Behavior.Retries On 502 ✅ 5615ms
Retry Behavior.Retries On 504 ✅ 5615ms
Retry Behavior.Max Retries Respected ✅ 15626ms
Deduplication.Generates Unique Uuids ✅ 620ms
Deduplication.Preserves Uuid On Retry ✅ 5614ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10620ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5617ms
Deduplication.No Duplicate Events In Batch ✅ 615ms
Deduplication.Different Events Have Different Uuids ✅ 612ms
Compression.Sends Gzip When Enabled ✅ 609ms
Batch Format.Uses Proper Batch Structure ✅ 609ms
Batch Format.Flush With No Events Sends Nothing ✅ 607ms
Batch Format.Multiple Events Batched Together ✅ 614ms
Error Handling.Does Not Retry On 403 ✅ 2611ms
Error Handling.Does Not Retry On 413 ✅ 2612ms
Error Handling.Retries On 408 ✅ 5615ms

Failures

format_validation.non_utc_event_timestamp_is_converted_to_utc

Event 0 field 'timestamp' instant '2026-09-19T06:20:05.494447Z' != expected '2025-01-02T03:04:05Z'

Feature_Flags Tests

⚠️ 11/17 tests passed, 6 failed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 8ms
Request Payload.Flags Request Uses V2 Query Param ✅ 7ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 6ms
Request Payload.Flags Request Omits Authorization Header ✅ 7ms
Request Payload.Token In Flags Body Matches Init ✅ 6ms
Request Payload.Groups Round Trip ✅ 6ms
Request Payload.Groups Default To Empty Object ❌ 6ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 6ms
Request Payload.Disable Geoip Omitted Defaults To False ❌ 6ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 6ms
Request Lifecycle.No Flags Request On Init Alone ✅ 2ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 607ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 10ms
Request Lifecycle.Mock Response Value Is Returned To Caller ❌ 6ms
Retry Behavior.Retries Flags On 502 ❌ 310ms
Retry Behavior.Retries Flags On 504 ❌ 311ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ❌ 610ms

Failures

request_payload.groups_default_to_empty_object

Field 'groups' not found in /flags request body at path 'groups'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']

request_payload.disable_geoip_omitted_defaults_to_false

Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']

request_lifecycle.mock_response_value_is_returned_to_caller

Last action result missing field 'value'. Keys: ['error', 'success']

retry_behavior.retries_flags_on_502

Last action result missing field 'value'. Keys: ['error', 'success']

retry_behavior.retries_flags_on_504

Last action result missing field 'value'. Keys: ['error', 'success']

side_effect_events.get_feature_flag_captures_feature_flag_called_event

Expected 1 events with name '$feature_flag_called', got 0

@github-actions

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@github-actions github-actions Bot added the stale label Sep 14, 2026
@marandaneto
marandaneto marked this pull request as ready for review September 18, 2026 11:13
@marandaneto
marandaneto requested a review from a team as a code owner September 18, 2026 11:13
@marandaneto
marandaneto requested a review from a team September 18, 2026 11:13
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Sep 18, 2026
@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown

Retrigger

The PR appears safe to merge with no actionable correctness, security, or repository-rule issues identified.

Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."

@marandaneto marandaneto removed the stale label Sep 18, 2026
Comment thread lib/posthog/feature_flags/local_evaluator.ex
@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Sep 18, 2026
@turnipdabeets
turnipdabeets requested a review from a team September 18, 2026 14:17
@posthog-project-board-bot posthog-project-board-bot Bot moved this from Approved to In Review in Feature Flags Sep 18, 2026
@turnipdabeets
turnipdabeets removed the request for review from a team September 18, 2026 14:19
Comment thread lib/posthog/feature_flags/local_evaluator.ex Outdated
@veria-ai

veria-ai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@marandaneto
marandaneto merged commit a481ed7 into main Sep 19, 2026
30 checks passed
@marandaneto
marandaneto deleted the fix/versioned-boolean-local-evaluation branch September 19, 2026 06:48
@github-project-automation github-project-automation Bot moved this from In Review to Done in Feature Flags Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants