Repository navigation
feat(flags): treat failed evaluations as unavailable - #5693
Open
marandaneto wants to merge 15 commits into
Open
marandaneto wants to merge 15 commits into
marandaneto wants to merge 15 commits into
Conversation
Contributor
|
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
Coherence📄 0 spec file errors · ⛔ 0 broken rules · 🚪 0 unchecked inputs · 🐛 0 bugs that came back What is Coherence?Coherence compares the code with its spec files ( This check read 44 parts of the code and 7 rules. Tip Nothing to fix. |
Contributor
Contributor
|
Size Change: +30.8 kB (+0.11%) Total Size: 27.4 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
posthog-js Compliance ReportDate: 2026-11-14 10:01:52 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
Node SDK acceptance v2Commit: 488a5b0 Capture v0✅ 229 passed / ❌ 8 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:135 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.dependency_error — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1174 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.error — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1213 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.error_with_value — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1252 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.v1_failed — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1425 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: Harness exit: 1; report validation exit: 1. Capture v1✅ 294 passed / ❌ 8 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:135 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.dependency_error — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1174 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.error — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1213 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.error_with_value — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1252 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: ❌ migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature::Legacy getter reads reader fixture readers.v1_failed — failed_assertionGetter value differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v4-legacy-getter.feature:1425 the public flag getter should return exactly JSON false Operation: Arguments: {"key":"garden-layout","distinct_id":"flags-v4-person"}
Expected: false Actual outcome: Harness exit: 1; report validation exit: 1. |
Contributor
SDK performance: posthog-nodeBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
marandaneto
marked this pull request as ready for review
November 12, 2026 12:13
posthog-security-review-bot
Bot
added
the
security-review
|
✅ Security review complete — No findings above the confidence threshold, in 8 min. Add the |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed cb379cc: 3 Should fix.
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-security-review-bot[bot]](https://avatars.githubusercontent.com/in/4551195?s=60&v=4){kind=small}
posthog-security-review-bot
Bot
removed
the
security-review
Member
Author
|
Follow-up to the compatibility-risk check and changeset notice. Reviewed Class 1's four compatibility questions. The persistence change adds an internal Executed The Final reviewed commit: c376aa7. |
haacked
reviewed
Nov 12, 2026
haacked
left a comment
haacked
left a comment
Contributor
There was a problem hiding this comment.
Nice fix! One blocking issue inline, plus a couple of questions and small suggestions.
ioannisj
approved these changes
Nov 12, 2026
ioannisj
left a comment
ioannisj
left a comment
Contributor
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Nothing blocking from me. The Node evaluateFlags() snapshot still carries failed records with their values and payloads, where the single-flag path drops them, and on the core path a failed record followed by a failed or quota-limited reload loses evaluation_failed from $feature_flag_error. Nothing else came up in the browser, core and Node read paths.
This was referenced Nov 13, 2026
haacked
approved these changes
Nov 13, 2026
haacked
left a comment
haacked
left a comment
Contributor
There was a problem hiding this comment.
Looks good! A few non-blocking suggestions inline.
marandaneto
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Implements sdk-specs #211: detailed server-failed records have no evaluated value/payload. Placeholder values could bypass defaults or leak into event enrichment and persisted state.
Changes
EVALUATION_FAILEDdiagnostic independently of response/request/quota errors. Preserve suppression/dedupe and native no-result/default mappings.constructor/toString.requestError, preserve own cached false/payload pairs before suppressing failed details, and add v1 errored cross-tab coverage/private helper cleanup plus the generated mangled-name entry.mainat480395c443c5916d83b8c8fd8f6084f1ab244db2, followed by a separate signed Node fix. Incoming main adds a strict v4 reader where parser-unreadable records also carryfailed. Distinguish those from readable server failures: malformed records keep their disabled/unknown-error compatibility.evaluation_failed; filtered unresolved maps and slice-missing suppression remain; explicit-key filters warn only for genuinely unknown keys. This supersedes the earlier skip-only snapshot implementation, which lost the new v4 attribution.undefined; the derived failed-event validator permits only the omitted response while retaining every other pinned schema constraint, with independent no-result/error assertions. No production sentinel coercion.Validation
Final reviewer suggestions
Current signed HEAD
488a5b0bfb5a422943cdab50b955abbbacddd7ddimplements both remaining suggestions from @haacked, superseding the earlier decisions below:only()looks up both evaluated and failed-record maps before warning. Known failed records retain private diagnostics without a misleading missing-key warning; genuinely unknown keys still warn. Failed keys remain absent from evaluated values, payloads, keys and capture enrichment.evaluation_failedevents, no extra request and no failed enrichment. A controlled blanket-warning-suppression mutation fails both cases, then restoration passes.autoreview --mode branch --base origin/mainclean; TruffleHog clean. No source changes afterward, history rewriting, main synchronization, CI retry/watch/wait or fresh native/browser/shared-harness conformance claim.Previous review follow-up
Preceding signed HEAD
74b196ddd3b97fb40b496e202e75e6641aa47940, following the earlier review plan:constructor/toStringprovenance when bootstrap did not supply those keys: 2 genuine assertion failures / 29 controls with the final regression tests. An own-key bootstrap check fixes both; the 31-case core failed suite covers own-false replacement and reopen without claiming broader prototype-key event-dedupe hardening.flag_missing/dedupe/no extra request, and retained/legacy payload assertions. These coverage additions passed the original implementation; no speculative defects claimed. Separate ordinary-object mutations of Node's value and diagnostic maps each fail both new cases; dropping legacy payloads fails the added payload assertion. All mutations restored exactly.autoreview --mode branch --base origin/maincompleted; TruffleHog clean. No source changes afterward, no history rewriting or main synchronization. New-head CI is not awaited/retried or claimed green.Previous browser deployment CI follow-up
Preceding signed HEAD
74239c9d350168afdd7a540585ced406f1b52ef4changed only the compatibility normalizer, its tests and README; SDK runtime, dependencies, versions and all seven golden files were unchanged.fetchStart=-4andresponseStart=responseEnd=endTime=-2. The error's(0 vs 1)denotes repetition indices, not milliseconds. The existing -2 ms exception covered only request phases.autoreview --mode branch --base origin/maincompleted; TruffleHog clean. No source edits afterward.SDK behavioral validation before the CI-only follow-up
The following results were obtained at preceding signed SDK head
cb0488f192398c4a1f680fb0ef2cab20cb627cb3, Node 24.21.0 / pnpm 11.7.0. All previously submitted SDK files and feature tests remain byte-identical in the CI-only follow-up; these broad suites were not needlessly rerun:WRITE_MANGLED_PROPERTIES=1browser build: 13/13 tasks, registry diff empty. API check, regenerated Node reference diff, ES5/ES6, formatting and feature-delta checks pass.autoreview --mode branch --base origin/main: clean; TruffleHog clean. No source edits afterward.No real-browser compatibility matrix, rrweb browser, physical RN/device, live-service or full shared-harness conformance claim. Existing unrelated acceptance falsy-payload/GeoIP and performance reports remain untouched. New-head CI was not awaited or retried; local validation is not an aggregate CI pass.
Release info Sub-libraries affected
Also affected:
@posthog/coreand@posthog/browser. Existing seven-package release intent now requests minor bumps; diagnostic references retained. Main’s version/lock changes are inherited upstream; no authored dependency/version/pin change, duplicate intent or release.Checklist
Agent context
Autonomy: Human-driven (agent-assisted). Human DRI and assignee: @marandaneto. Pi handled JS integration while an isolated worker handled the separate Python checkout; one writer per worktree. Parent independently accepted source/proofs and owns review/publication. Human review is required before merge; no auto-merge or release requested.