Repository navigation
feat(flags): serve the typed v3 record on /flags?v=3 behind a service setting - #107732
Conversation
On /flags, v >= 3 now returns the v3 envelope: one typed `value` per record instead of `enabled` and `variant`, the evaluator's reason code and terminal rule for a v2 flag, and `config_version` in the metadata. v=2 keeps today's record byte for byte, and the /decide arms are untouched. Both shapes are built from the same internal result: the v2 evaluator's typed outcome now rides on FeatureFlagMatch and FlagDetails in fields the v2 wire never serializes. The vendored contract package 2.2.0 subset gains the v3 producer schema, its presence companion, the presence matrix and the wire fixtures; tests validate every produced record against them.
|
😎 Merged successfully - details. |
github-actions
Bot
added
feature/feature-flags
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
💤 Files with no reviewable changes (6)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe API now serves V3 flag records for enabled non-Decide requests at version 3 or higher. The setting defaults to disabled, and Decide response mapping remains unchanged. V3 conversion uses retained evaluation data. Body and canonical logs record the served response format. The change also adds wire schemas, validation helpers, fixtures, and integration tests for response fields, typed values, filtering, and version compatibility. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The typed v3 /flags response is behind a setting that is off by default, so existing v2, legacy, and /decide callers keep their current behavior. No unresolved merge-blocking issue remains at the current head. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new response is disabled by default and preserves the reviewed project authorization and filtering controls. A conditional failure-status mismatch could undermine failed-record handling. Production rollout and client fallback behavior remain unverified. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/feature-flags/src/api/types.rs (1)
557-564: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the description match depend on
code, not onevaluation.The
(None, _)arm testsevaluation. Therollout_missguard tests thecodestring. Both values come from onematchblock just above. The logic works now. It depends on string comparison, so it will break without warning if someone renames a code. Computedescriptioninside the firstmatchso that each arm returns all four values.♻️ Proposed refactor
- let (value, code, rule) = match evaluation { - Some(Evaluation::TargetingMatch { value, rule }) => { - (Value::from(value), "targeting_match", Some(rule)) - } - Some(Evaluation::RolloutMiss { value, rule }) => { - (Value::from(value), "rollout_miss", Some(rule)) - } - Some(Evaluation::NoRuleMatch { value }) => { - (Value::from(value), "no_rule_match", None) - } - None => (Value::Null, "error", None), - }; - let description = match (evaluation, rule) { ... }; + let (value, code, rule, description) = match evaluation { + Some(Evaluation::TargetingMatch { value, rule }) => ( + Value::from(value), "targeting_match", Some(rule), + Some(format!("Matched rule {}", rule.index + 1)), + ), + Some(Evaluation::RolloutMiss { value, rule }) => ( + Value::from(value), "rollout_miss", Some(rule), + Some(format!("Rule {} rollout miss", rule.index + 1)), + ), + Some(Evaluation::NoRuleMatch { value }) => ( + Value::from(value), "no_rule_match", None, + Some("No rule matched".to_string()), + ), + None => (Value::Null, "error", None, flag.reason.description), + };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: db268e23-d403-452e-b550-0a0b686ed0c1
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
rust/feature-flags/Cargo.tomlrust/feature-flags/src/api/endpoint.rsrust/feature-flags/src/api/types.rsrust/feature-flags/src/flags/flag_matching.rsrust/feature-flags/src/flags/test_flag_matching.rsrust/feature-flags/src/handler/body_logger.rsrust/feature-flags/src/handler/tests.rsrust/feature-flags/tests/fixtures/rules_v2_evaluation/2.2.0/SOURCE.jsonrust/feature-flags/tests/fixtures/rules_v2_evaluation/2.2.0/fixtures/wire/responses.jsonrust/feature-flags/tests/fixtures/rules_v2_evaluation/2.2.0/rules/response_presence.jsonrust/feature-flags/tests/fixtures/rules_v2_evaluation/2.2.0/schemas/flags_response_v3.schema.jsonrust/feature-flags/tests/fixtures/rules_v2_evaluation/2.2.0/schemas/flags_response_v3_presence.schema.jsonrust/feature-flags/tests/test_flag_matching_consistency.rsrust/feature-flags/tests/test_flags.rsrust/feature-flags/tests/test_flags_v3.rsrust/feature-flags/tests/test_rules_v2_evaluation.rsrust/feature-flags/tests/test_rules_v2_evaluation/wire.rsrust/feature-flags/tests/test_rules_v2_v1_contract.rs
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
|
[Medium risk] Adds a new API response format for feature flags. The PR should not merge until v3 accepts variant keys already valid in stored v1 flags. Reviews (1) · Last reviewed commit: "feat(flags): serve the typed v3 record o..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…t 2.2.1 The vendored v3 schema restricted metadata.variant_key to [a-zA-Z0-9_-], but the API has never restricted v1 variant keys: stored flags carry dots, slashes and spaces because variant keys double as configuration values. The correction lives in the contract (PostHog/posthog-sdk-test-harness#70, contract package 2.2.1, wire schemas 1.0.1); this pins that revision provisionally until it is released and serves a v1 flag with the variant key "provider/model-1.2 beta" through /flags?v=3 in the tests. Also computes the v3 reason description in the same match as its code.
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 0 should fix, 1 consider. Published 1 finding (view the review). Resolved comments: 1 fixed |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Looks good, and nothing inline is blocking. Greptile's variant_key comment does check out, though: filters_schema.py deliberately allows keys like provider/model-1.2, which the vendored v3 pattern rejects.
The opt-in body log recorded the internal FlagsResponse before the versioning step. For /flags?v=3 that record shows `enabled` and the v1 reason code, and it cannot show the typed `value`, the v2 reason code or the rule fields, so a null default logged as `enabled: false`. When the request gets the v3 record, the body logger now serializes each filtered flag as its v3 record. The v3 routing check moves into `serves_flags_v3`, which both `get_versioned_response` and the logger call, so the logged shape follows the served shape. Other versions log the internal record as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 441c3d40-fb89-44b3-a6e1-6435d1a3a4ca
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve missing-dependency failures in V3 conversion. · types.rs:544-571
rust/feature-flags/src/api/types.rs:544-571
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve missing-dependency failures in V3 conversion.
A reachable missing dependency produces
ConfigOutcome::V2(None)with reason codemissing_dependency. The conversion changes that reason to genericerrorand keepsfailedfalse, so serialization omitsfailed. The V3 schema requiresfailedfor both error codes, and the V2 contract definesdependency_errorfor unresolvable dependencies.Suggested fix
ConfigOutcome::V2(evaluation) => { + let evaluation_failed = evaluation.is_none(); + let is_dependency_error = flag.reason.code == "missing_dependency"; let (value, code, rule, description) = match evaluation { ... - None => (Value::Null, "error", None, flag.reason.description), + None => ( + Value::Null, + if is_dependency_error { + "dependency_error" + } else { + "error" + }, + None, + flag.reason.description, + ), }; ... - (value, reason, rule, 2, None) + (value, reason, rule, 2, None, flag.failed || evaluation_failed) } ... - failed: flag.failed, + failed,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: abc24e04-199d-4637-80e1-92aaf44aa38e
📒 Files selected for processing (3)
rust/feature-flags/src/api/endpoint.rsrust/feature-flags/src/api/types.rsrust/feature-flags/src/handler/body_logger.rs
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 2 remain after this review.
- Evaluation and RuleKind name their own wire code, value, rule and description, so the v3 conversion no longer compares code strings. - The panic fallback keeps a v2 flag's config label, so its v3 record says config_version 2 with code error instead of posing as v1. - Test the failed v1 record, the v2 reason descriptions, minimalFlagCalledEvents and supportedCompression on v=3. - The wire seed scan follows the harness: camelCase, dash and $ spellings count, and only the flags map keys are exempt. - Drop assert_presence_row, which the presence schema already covers. - Doc and indentation fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…flags-v3-route Master now evaluates string, number and object rules v2 values, and its `Evaluation` borrows the value from the parsed config. `FeatureFlagMatch` keeps an owned `EvaluationDetail` for the v3 record instead, and `get_match_v2` keeps master's typed value and legacy payload alongside it. A v2 record at v3 carries its value only in `value`; `metadata.payload` stays null, as the v3 producer schema requires. The vendored rules v2 contract moves to harness release 1.13.1 (contract 2.3.1), which carries both the typed value corpus and the corrected v3 wire schemas, replacing the branch's unreleased 2.2.1 pin and master's 2.3.0 pin.
…is on `/flags` with `v >= 3` returns the v2 record, exactly as master does, unless the new service setting `FLAGS_V3_RESPONSE_ENABLED` is true. It defaults to false: some clients already send `v=3` and read the v2 record, so the arm ships dark and is turned on once they have moved. The `/decide` arms, `v=2`, `v=1` and an unparsable `v` do not depend on it, and the body log follows the shape actually served. The canonical log line records that shape as `response_format`. Tests: the v3 suites run with the setting on, and a new test serves every successful typed value corpus case over `/flags`. With the setting off, `v=3` and `v=99` match `v=2` for v1, v2 boolean and v2 string flags, and `it_get_new_response_when_version_is_2_or_more` is back to master's cases.
andehen
changed the title
Problem
Feature flag rules v2 ships a new remote response protocol,
/flags?v=3, whose record carries one typedvalueinstead ofenabledandvariant, the evaluator's own reason code, and the terminal rule. Until now the Rust service answered everyv >= 2on/flagswith the v2 record, so SDKs that will adopt v3 had only harness fixtures to develop against.Some clients already send
v=3to/flagsand read the v2 record they get today. So the v3 arm ships behind a service setting that is off by default: deploying this PR changes nothing on the wire.Changes
FLAGS_V3_RESPONSE_ENABLED(Config::flags_v3_response_enabled), defaultfalse, one switch for the whole service. Off:/flagswithv >= 3returns the v2 record, exactly as master does. On: it returns the v3 record described below. The/decidearms,v=2,v=1, and an absent or unparsablevbehave the same in both states. The body log follows the shape actually served, and the canonical log line gainsresponse_format(FlagsV1,FlagsV2,FlagsV3,DecideV1,DecideV2), so the served shape is visible per request.get_match_v2keeps its typed outcome onFeatureFlagMatch(evaluation_v2, an ownedEvaluationDetail, because the evaluator'sEvaluationborrows from the parsed config).FlagDetailscarries the config format plus that outcome in a serde-skipped field. The v3 record is built from the sameFlagsResponseas the v2 record, so nothing is re-evaluated. The v2 projection ignores both fields.FlagDetailsV3) haskey,value,failed(only when true),reason,metadataand the unchanged optionalconditions.valueis the configured value as raw JSON: boolean, string, number or object, never a JSON-encoded string. It isnullfor a null default or a failed record.reason.codeistargeting_match,rollout_missorno_rule_match, andcondition_indexis the rule index.metadataaddsconfig_version: 2, plusrule_typeandrule_idwhen a rule terminated.metadata.payloadis alwaysnull, because the value travels only invalue; the v2 record still carries the JSON-encoded payload for number and object flags. A failed v2 flag reportserrorwithvalue: nulland no rule fields.valueis the variant key or the boolean,metadata.config_versionis1, andpayloadis unchanged.variant_keycarries the matched variant key verbatim; the API has never restricted v1 variant keys, so dots, slashes and spaces pass through.jsonschemais a new dev-dependency (no default features).flag_matching.rs, where both sides are kept, and in the vendored contract directory, which is now the single 1.13.1 copy.Example with the setting on: a v2 string flag whose first rule matched.
{ "key": "checkout-layout", "value": "compact", "reason": { "code": "targeting_match", "condition_index": 0, "description": "Matched rule 1" }, "metadata": { "id": 123, "version": 4, "config_version": 2, "description": null, "payload": null, "has_experiment": false, "rule_type": "targeted_release", "rule_id": "6bb788aa-df76-4a77-8bed-9f247e279ccd" } }Out of scope: experiment context, additive metadata on the v2 record, any
/decidechange, a per-team allowlist, and SDK support. The protocol version selects the shape; it never proves that a client understands it.Turning it on
FLAGS_V3_RESPONSE_ENABLED=true bin/start-rust-service feature-flags, or addFLAGS_V3_RESPONSE_ENABLED: 'true'to thefeature-flagsservice environment indocker-compose.dev.yml.v=3today have moved off it. It is not part of this PR.Compatibility
Every maintained SDK at HEAD requests
/flags/?v=2(Python, browser and Node JS, Android, iOS, Flutter through the native SDKs, Go, .NET, PHP, Ruby, Rust), so no SDK changes shape at any setting. The service used to answer anyv >= 2with the v2 record, so clients that already sendv=3to/flagsare the only ones the setting affects: tens of thousands of requests a day, almost all from one hand-written Node caller. While the setting is off they keep receiving the v2 record. Once it is on, a caller that readsenabledmust readvalueinstead.Rollback
v >= 3returns the v2 record again.How did you test this code?
v3_and_above_serve_the_v2_record_while_the_setting_is_offserves v1 flags, v2 boolean flags and a v2 string flag.POST /flagswithv=3orv=99returns the same body asv=2(same request ID,evaluatedAtdropped), with bothconfig=falseandconfig=true.GET /flags?v=3matchesGET /flags?v=2. Intest_flags.rs,it_get_new_response_when_version_is_2_or_moreis back to master'sv=2/v=3cases.test_rules_v2_v1_contract,test_legacy_decide_formatsandtest_config_formatpass unchanged.v=3andv=99,flag_keys,detailed_analysis, thev=2/v=1/v=abc/decide shapes, andGET.v3_carries_every_typed_corpus_value_through_the_request_pathserves the 38 successful cases of the typed value corpus (string, number, object, null defaults) as flags over/flags. Against the corpus, it checks value, reason, rule fields andpayload: nullatv=3, andenabled,variantand the payload atv=2.test_rules_v2_evaluation.rsprojects 163 corpus cases (114 boolean, 49 typed) through the matcher into schema-valid v3 records, and the 150 wire fixtures agree with the validator.feature-flagssuite, locally. Every integration binary passes. In the lib tests, 1854 pass and 1 fails:test_realtime_provider_queries_cohort_membership_tableneeds a table this machine lacks, and it fails without the change too. Strict Clippy,cargo fmt --check,git diff --checkandhogli ci:preflight --strictare clean.v=3returned the same body asv=2and the canonical log saidresponse_format=FlagsV2.v=3returnedtrue,"compact",12.5and the configured object withpayload: null, and the log saidFlagsV3.Release status
The switch is the service setting above, not a PostHog feature flag.
Automatic notifications
🤖 Generated with Claude Code