Skip to content

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

Draft
marandaneto wants to merge 8 commits into
mainfrom
fix/versioned-boolean-local-evaluation
Draft

fix(flags): honor versioned local property matching#200
marandaneto wants to merge 8 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-05 17:39:44 UTC
Duration: 117728ms

✅ All Tests Passed!

46/46 tests passed


Capture Tests

29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 611ms
Format Validation.Event Has Uuid 610ms
Format Validation.Event Has Lib Properties 612ms
Format Validation.Distinct Id Is String 610ms
Format Validation.Token Is Present 610ms
Format Validation.Custom Properties Preserved 611ms
Format Validation.Event Has Timestamp 611ms
Retry Behavior.Retries On 503 5616ms
Retry Behavior.Does Not Retry On 400 2613ms
Retry Behavior.Does Not Retry On 401 2614ms
Retry Behavior.Respects Retry After Header 5617ms
Retry Behavior.Implements Backoff 15627ms
Retry Behavior.Retries On 500 5617ms
Retry Behavior.Retries On 502 5616ms
Retry Behavior.Retries On 504 5617ms
Retry Behavior.Max Retries Respected 15626ms
Deduplication.Generates Unique Uuids 624ms
Deduplication.Preserves Uuid On Retry 5617ms
Deduplication.Preserves Uuid And Timestamp On Retry 10622ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 5618ms
Deduplication.No Duplicate Events In Batch 618ms
Deduplication.Different Events Have Different Uuids 614ms
Compression.Sends Gzip When Enabled 611ms
Batch Format.Uses Proper Batch Structure 611ms
Batch Format.Flush With No Events Sends Nothing 607ms
Batch Format.Multiple Events Batched Together 616ms
Error Handling.Does Not Retry On 403 2613ms
Error Handling.Does Not Retry On 413 2614ms
Error Handling.Retries On 408 5617ms

Feature_Flags Tests

17/17 tests passed

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

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.

1 participant