Skip to content

feat(flags): honor experiment holdouts in local flag evaluation - #254

Merged
ioannisj merged 4 commits into
mainfrom
posthog/php-local-eval-holdouts
Oct 22, 2026
Merged

ioannisj merged 4 commits into
mainfrom
posthog/php-local-eval-holdouts

Conversation

@posthog

@posthog posthog Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Brings posthog-php into compliance with the Local Feature Flag Evaluator contract in PostHog/sdk-specs — specifically the requirements Experiment holdouts precede release conditions and Holdout membership uses backend-compatible bucketing.

The SDK compliance matrix flagged the gap (compliance/posthog-php.md, Local Feature Flag Evaluator — 🟡 Partial):

In computeFlagLocally(), before iterating condition groups, read filters.holdout; when present, hash the bucketing identity with the holdout's salt per the spec's "Holdout membership uses backend-compatible bucketing" requirement and return the holdout outcome when the identity falls inside exclusion_percentage. Keep the percentage as a float — the spec explicitly forbids truncating fractional percentages.

Today filters.holdout is dropped on the floor during local evaluation (grep -rn holdout lib/ returned nothing), so a user the backend puts in an experiment holdout gets an ordinary variant locally. That is a silent correctness divergence between local and remote evaluation for every experiment that uses a holdout.

What changed

  • FeatureFlag::matchFeatureFlagProperties() resolves filters.holdout before iterating release conditions, returning holdout-<id> when the bucketing identity is held out. Because both the single-flag/bulk path (Client::computeFlagLocally()) and the dependency path (FeatureFlag::evaluateFlagDependency()) check active and then call this method, all three evaluation paths share the inactive check and the holdout precedence: an inactive flag still resolves to false, and a dependent flag comparing against "holdout-727" matches.
  • FeatureFlag::holdoutHash() implements the backend hash: SHA-1 over holdout-<bucketing_value> with no separator or salt, first 15 hex digits over 0xfffffffffffffff. Neither the flag key nor the holdout id participates, so the ordinary dot-separated flag hash is deliberately not reused.
  • Membership uses the inclusive comparison hash <= percentage / 100, with exclusion_percentage clamped to 0–100 as a float (no truncation) and a clamped 100 short-circuiting without computing a hash.
  • A holdout that is absent, not an object, or missing/null on id or exclusion_percentage is skipped and ordinary evaluation continues unchanged.
  • The holdout uses the flag-level bucketing identity, i.e. the group key for group-aggregated flags (already resolved by the caller) — per-condition aggregation does not change membership. One exception: evaluateFlagDependency() passes the parent flag's distinct id down to the dependency, so a group-aggregated dependency with a partial holdout gets bucketed by the person instead of its group key. Rollout hashing for those dependencies already works this way on main, and this PR doesn't change that.

Backwards compatibility

Additive. Flags without a (complete) filters.holdout evaluate exactly as before, and no public signature changes — composer api:check reports the public API snapshot is unchanged. Values change only for flags that actually carry a holdout, where the current locally-evaluated value is wrong and disagrees with the server.

💚 How did you test it?

New test/FeatureFlagHoldoutTest.php covers each acceptance scenario from the spec: holdout winning over an unavailable targeting property, rollout 0 and a variant override; a synthetic variant absent from filters.multivariate.variants; inactive flag staying false; no-holdout and incomplete-holdout preserving ordinary assignment; dependency evaluation comparing against the holdout string; the spec's reference hashes (user-1 → ~0.17805599206573022 held out at 20%, user-5 → ~0.6563813925994418 not); fractional percentages not truncated; inclusive/clamped boundaries at 0, -10, 100 and 150; and the group key deciding membership for a group-aggregated flag.

./vendor/bin/phpunit --bootstrap vendor/autoload.php --configuration phpunit.xml
# 751 tests, 5411 assertions, 0 failures
./vendor/bin/phpcs --standard=phpcs.xml --extensions=php lib test   # no new findings
php scripts/check-public-api.php                                    # snapshot up to date

No manual testing against a live PostHog project.

Follow-up work

  • Device-id bucketing (Device identity determines membership for device-bucketed flags) is untested here because posthog-php has no device-id bucketing concept; it would land with that feature.
  • The same holdout gap is recorded for posthog-ruby and is not addressed here.
  • Have evaluateFlagDependency() resolve each dependency's own bucketing identity, the way posthog-js does by re-entering computeFlagValueLocally(). That would fix rollout, variant and holdout bucketing for group-aggregated dependencies together.

📝 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.

If releasing new changes

  • Ran pnpm change to generate a change intent file (added .changeset/local-flag-holdouts.md by hand, matching the existing format)

🤖 Agent context

Autonomy: Fully autonomous

Opened by a scheduled PostHog agent run that reads the SDK compliance matrices in PostHog/sdk-specs and implements one backwards-compatible gap per run. Candidate selection skipped gaps with a Breaking verdict, gaps on deprecated methods, and gaps already covered by an open PR on this repo. Holdout support was chosen over the other eligible PHP candidates (an additive getAllFlagsAndPayloads(), payload-decode warning logging, and the flag-request backoff constant) because it is a user-visible correctness divergence from the server rather than an ergonomics or diagnostics gap.

Implementation note: the holdout check was placed in matchFeatureFlagProperties() rather than Client::computeFlagLocally() as the matrix suggested, so the dependency-evaluation path gets the same semantics without duplicating the logic.


Created with PostHog Desktop

🤖 Generated with Claude Code

Local evaluation ignored `filters.holdout`, so a held-out identifier got an
ordinary variant instead of `holdout-<id>`, disagreeing with the server.

Resolve the holdout before release conditions, using the backend-compatible
hash over `holdout-<bucketing_value>` and the flag-level bucketing identity.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 1bb30980-7c8c-4de3-8b45-5f1b1563230b
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-fork_curl Compliance Report

Date: 2026-10-22T08:48:52.282670+00:00
Duration: 112470ms

⚠️ Some Tests Failed

36/47 tests passed, 11 failed


Capture Tests

⚠️ 19/30 tests passed, 11 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 29ms
Format Validation.Event Has Uuid ✅ 546ms
Format Validation.Event Has Lib Properties ✅ 519ms
Format Validation.Distinct Id Is String ✅ 525ms
Format Validation.Token Is Present ✅ 525ms
Format Validation.Custom Properties Preserved ✅ 526ms
Format Validation.Event Has Timestamp ✅ 524ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 526ms
Retry Behavior.Retries On 503 ❌ 5528ms
Retry Behavior.Does Not Retry On 400 ✅ 2526ms
Retry Behavior.Does Not Retry On 401 ✅ 2529ms
Retry Behavior.Respects Retry After Header ❌ 5530ms
Retry Behavior.Implements Backoff ❌ 15540ms
Retry Behavior.Retries On 500 ❌ 5532ms
Retry Behavior.Retries On 502 ❌ 5532ms
Retry Behavior.Retries On 504 ❌ 5529ms
Retry Behavior.Max Retries Respected ❌ 15537ms
Deduplication.Generates Unique Uuids ✅ 528ms
Deduplication.Preserves Uuid On Retry ❌ 5526ms
Deduplication.Preserves Uuid And Timestamp On Retry ❌ 10534ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ❌ 5528ms
Deduplication.No Duplicate Events In Batch ✅ 528ms
Deduplication.Different Events Have Different Uuids ✅ 522ms
Compression.Sends Gzip When Enabled ✅ 524ms
Batch Format.Uses Proper Batch Structure ✅ 522ms
Batch Format.Flush With No Events Sends Nothing ✅ 512ms
Batch Format.Multiple Events Batched Together ✅ 513ms
Error Handling.Does Not Retry On 403 ✅ 2522ms
Error Handling.Does Not Retry On 413 ✅ 2528ms
Error Handling.Retries On 408 ❌ 5526ms

Failures

retry_behavior.retries_on_503

Expected at least 3 requests, got 1

retry_behavior.respects_retry_after_header

Expected at least 2 requests, got 1

retry_behavior.implements_backoff

Expected at least 3 requests, got 1

retry_behavior.retries_on_500

Expected at least 2 requests, got 1

retry_behavior.retries_on_502

Expected at least 2 requests, got 1

retry_behavior.retries_on_504

Expected at least 2 requests, got 1

retry_behavior.max_retries_respected

Expected 4 requests, got 1

deduplication.preserves_uuid_on_retry

Need at least 2 requests to check retry

deduplication.preserves_uuid_and_timestamp_on_retry

Expected at least 3 requests, got 1

deduplication.preserves_uuid_and_timestamp_on_batch_retry

Expected at least 2 requests, got 1

error_handling.retries_on_408

Expected at least 2 requests, got 1

Feature_Flags Tests

✅ 17/17 tests passed

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

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-lib_curl Compliance Report

Date: 2026-10-22T08:48:58.360887+00:00
Duration: 116743ms

⚠️ Some Tests Failed

46/47 tests passed, 1 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 30ms
Format Validation.Event Has Uuid ✅ 523ms
Format Validation.Event Has Lib Properties ✅ 526ms
Format Validation.Distinct Id Is String ✅ 526ms
Format Validation.Token Is Present ✅ 526ms
Format Validation.Custom Properties Preserved ✅ 525ms
Format Validation.Event Has Timestamp ✅ 525ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 526ms
Retry Behavior.Retries On 503 ✅ 5835ms
Retry Behavior.Does Not Retry On 400 ✅ 2529ms
Retry Behavior.Does Not Retry On 401 ✅ 2528ms
Retry Behavior.Respects Retry After Header ❌ 6336ms
Retry Behavior.Implements Backoff ✅ 16250ms
Retry Behavior.Retries On 500 ✅ 5636ms
Retry Behavior.Retries On 502 ✅ 5631ms
Retry Behavior.Retries On 504 ✅ 5636ms
Retry Behavior.Max Retries Respected ✅ 17037ms
Deduplication.Generates Unique Uuids ✅ 246ms
Deduplication.Preserves Uuid On Retry ✅ 5634ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10841ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5637ms
Deduplication.No Duplicate Events In Batch ✅ 534ms
Deduplication.Different Events Have Different Uuids ✅ 527ms
Compression.Sends Gzip When Enabled ✅ 526ms
Batch Format.Uses Proper Batch Structure ✅ 525ms
Batch Format.Flush With No Events Sends Nothing ✅ 521ms
Batch Format.Multiple Events Batched Together ✅ 513ms
Error Handling.Does Not Retry On 403 ✅ 2528ms
Error Handling.Does Not Retry On 413 ✅ 2527ms
Error Handling.Retries On 408 ✅ 5634ms

Failures

retry_behavior.respects_retry_after_header

Retry delay too short: 804ms < 2500ms

Feature_Flags Tests

✅ 17/17 tests passed

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

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

posthog-php-socket Compliance Report

Date: 2026-10-22T08:49:32.146621+00:00
Duration: 148286ms

⚠️ Some Tests Failed

36/47 tests passed, 11 failed


Capture Tests

⚠️ 19/30 tests passed, 11 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 28ms
Format Validation.Event Has Uuid ✅ 521ms
Format Validation.Event Has Lib Properties ✅ 525ms
Format Validation.Distinct Id Is String ✅ 524ms
Format Validation.Token Is Present ✅ 524ms
Format Validation.Custom Properties Preserved ✅ 525ms
Format Validation.Event Has Timestamp ✅ 524ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 524ms
Retry Behavior.Retries On 503 ❌ 9228ms
Retry Behavior.Does Not Retry On 400 ✅ 2033ms
Retry Behavior.Does Not Retry On 401 ✅ 2526ms
Retry Behavior.Respects Retry After Header ❌ 9236ms
Retry Behavior.Implements Backoff ❌ 19247ms
Retry Behavior.Retries On 500 ❌ 9242ms
Retry Behavior.Retries On 502 ❌ 9231ms
Retry Behavior.Retries On 504 ❌ 8738ms
Retry Behavior.Max Retries Respected ❌ 19248ms
Deduplication.Generates Unique Uuids ✅ 533ms
Deduplication.Preserves Uuid On Retry ❌ 9234ms
Deduplication.Preserves Uuid And Timestamp On Retry ❌ 14232ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ❌ 8747ms
Deduplication.No Duplicate Events In Batch ✅ 531ms
Deduplication.Different Events Have Different Uuids ✅ 524ms
Compression.Sends Gzip When Enabled ✅ 524ms
Batch Format.Uses Proper Batch Structure ✅ 524ms
Batch Format.Flush With No Events Sends Nothing ✅ 519ms
Batch Format.Multiple Events Batched Together ✅ 513ms
Error Handling.Does Not Retry On 403 ✅ 2525ms
Error Handling.Does Not Retry On 413 ✅ 2526ms
Error Handling.Retries On 408 ❌ 5528ms

Failures

retry_behavior.retries_on_503

Expected at least 3 requests, got 1

retry_behavior.respects_retry_after_header

Expected at least 2 requests, got 1

retry_behavior.implements_backoff

Expected at least 3 requests, got 1

retry_behavior.retries_on_500

Expected at least 2 requests, got 1

retry_behavior.retries_on_502

Expected at least 2 requests, got 1

retry_behavior.retries_on_504

Expected at least 2 requests, got 1

retry_behavior.max_retries_respected

Expected 4 requests, got 1

deduplication.preserves_uuid_on_retry

Need at least 2 requests to check retry

deduplication.preserves_uuid_and_timestamp_on_retry

Expected at least 3 requests, got 1

deduplication.preserves_uuid_and_timestamp_on_batch_retry

Expected at least 2 requests, got 1

error_handling.retries_on_408

Expected at least 2 requests, got 1

Feature_Flags Tests

✅ 17/17 tests passed

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

@ioannisj

ioannisj commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🤖 An AI agent wrote this on behalf of @ioannisj.

Fixed:

  • Scoped the description's dependency claim. evaluateFlagDependency() passes the parent flag's distinct id into the dependency, so a group-aggregated dependency with a partial holdout gets bucketed by the person instead of its group key. In one evaluateFlags('user-5', ['company' => 'user-1']) call, a group flag resolves to holdout-727 while a person flag gated on it sees false. Rollout hashing already does this on main, so I left the code alone and added a follow-up to resolve each dependency's own bucketing identity like posthog-js does.

The holdoutHash() docblock says the range is [0, 1) when it can hit exactly 1.0. Comment only, so I left it.

@ioannisj
ioannisj marked this pull request as ready for review October 7, 2026 13:37
@ioannisj
ioannisj requested a review from a team as a code owner October 7, 2026 13:37

@dustinbyrne dustinbyrne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The direct holdout calculation matches the specification. Consider correcting the dependency identity mismatch and adding the public-entry regression test described inline.

AI-assisted review.

Comment thread lib/FeatureFlag.php
$flagFilters = $flag["filters"] ?? [];

// Experiment holdouts win over every release condition, variant override, and rollout.
$holdoutVariant = FeatureFlag::matchHoldout($flagFilters["holdout"] ?? null, $distinctId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Resolve a dependency's own identity before applying its holdout

Location: lib/FeatureFlag.php:963 (RIGHT).

The new holdout check treats $distinctId as the evaluated flag's bucketing identity, but recursive evaluateFlagDependency() passes the parent condition's identity without resolving the referenced flag's aggregation (lib/FeatureFlag.php:1224–1232). Direct group evaluation correctly supplies the group key (lib/Client.php:1411–1425).

A valid public call can therefore return contradictory results:

$flags = $client->evaluateFlags(
    'user-1',
    ['company' => 'user-5'],
    [],
    ['company' => []],
    true
);

With a group-aggregated checkout flag carrying holdout 727 at 20%, otherwise selecting control, and a person checkout-banner flag requiring checkout == control:

Result Base, inferred Reviewed head, inferred Expected head
checkout control control control
checkout-banner true false true

The company user-5 hashes to approximately 0.6563813925994418, outside the holdout. The person user-1 hashes to approximately 0.17805599206573022, inside it. Only recursive evaluation returns holdout-727, incorrectly failing the banner condition. Both results are conclusive, so enabling normal remote fallback does not repair this.

Inherited-identity recursion existed at base, but 100% ordinary rollout and 100% control neutralize its old rollout/variant-bucketing defect in this example. Base correctly returns control through both paths; the new holdout branch alone changes the dependent flag. Impact is incorrect application gating and emitted flag values for partial-holdout dependencies whose own identity differs from the parent's; direct and remote-only evaluation remain unaffected.

The governing pinned specification requires shared dependency semantics and the flag-level group identity. The backend resolves the referenced flag's aggregation, and Node re-enters full evaluation for dependencies. Backend reference validation permits this person-parent/group-dependency configuration.

Consider resolving each dependency's own flag-level identity while retaining the original person/group context through recursion. When required context is unavailable, use the existing inconclusive path rather than substituting an unrelated identity. No new user-facing API is necessary.

Test sketch — NOT EXECUTED: Add this method to FeatureFlagHoldoutTest, using its existing holdoutFlag() builder. Explicit condition aggregation mirrors backend normalization; the noop capture consumer isolates flag-request assertions.

public function testGroupHoldoutDependencyUsesItsOwnIdentity(): void
{
    $checkout = self::holdoutFlag([
        'id' => 727,
        'exclusion_percentage' => 20,
    ]);
    $checkout['id'] = 1;
    $checkout['filters']['aggregation_group_type_index'] = 0;
    $checkout['filters']['groups'][0]['aggregation_group_type_index'] = 0;

    $banner = [
        'id' => 2,
        'key' => 'checkout-banner',
        'active' => true,
        'filters' => [
            'groups' => [[
                'aggregation_group_type_index' => null,
                'rollout_percentage' => 100,
                'properties' => [[
                    'key' => 'checkout',
                    'type' => 'flag',
                    'operator' => 'flag_evaluates_to',
                    'value' => 'control',
                    'dependency_chain' => ['checkout'],
                ]],
            ]],
        ],
    ];

    $http = new MockedHttpClient(
        'app.posthog.com',
        flagEndpointResponse: [
            'flags' => [$checkout, $banner],
            'group_type_mapping' => ['0' => 'company'],
        ]
    );
    $client = new Client(
        'test-project-key',
        ['consumer' => 'noop'],
        $http,
        secretKey: 'test-secret-key'
    );

    foreach ([true, false] as $onlyEvaluateLocally) {
        $http->calls = [];
        $flags = $client->evaluateFlags(
            'user-1',
            ['company' => 'user-5'],
            [],
            ['company' => []],
            $onlyEvaluateLocally
        );

        self::assertSame('control', $flags->getFlag('checkout'));
        self::assertTrue($flags->getFlag('checkout-banner'));
        self::assertSame([], $http->calls);
    }
}

Expected: both values match the assertions without remote evaluation in either mode. Reviewed-source implication: checkout is control, but banner is false, failing the second assertion. The equivalent fixture and builder are expected to pass against base and after correction. These outcomes are inferred, not executed.

Proposed command, not run:

./vendor/bin/phpunit --bootstrap vendor/autoload.php --configuration phpunit.xml --filter testGroupHoldoutDependencyUsesItsOwnIdentity

Also cover the opposite membership direction: person user-5, company user-1, dependency expecting holdout-727, with checkout holdout-727 and banner true. This companion tests newly supported holdouts; unlike the control fixture, it is not a base-passing regression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 An AI agent wrote this on behalf of @ioannisj.

Fixed in 2e8dc97. evaluateFlagDependency() now carries the root flag's identity aggregation through the recursion, and a dependency with an active holdout aggregated differently from that identity goes inconclusive instead of being bucketed by the parent's distinct id.

So evaluateFlags('user-1', ['company' => 'user-5']) no longer returns holdout-727 for a group-aggregated checkout, and checkout-banner falls back to /flags when remote evaluation is allowed.

I went with deferring to the server rather than resolving the dependency's own group key locally, since that key isn't reachable inside the recursion today. Covered both membership directions plus the same-aggregation and nested-dependency cases in test/FeatureFlagHoldoutTest.php. No public API change, the identity parameter lives on private workers and api/public-api.json is untouched.

@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 Oct 15, 2026
@ioannisj
ioannisj requested a review from dustinbyrne October 16, 2026 18:22

@marandaneto marandaneto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Automated advisory code review.

Comment thread lib/FeatureFlag.php
$properties,
$cohortProperties,
$propertyMatchingVersion,
false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

blocking: Preserve aggregation context through cohort dependencies — matchCohort() → matchPropertyGroup() still calls this public wrapper, so passing false disables the new aggregation guard on that path. With a 20% company-aggregated holdout, person user-1 and company user-5, checkout correctly resolves to control, but a person flag targeting a cohort that requires checkout = holdout-727 incorrectly evaluates to true: dependency evaluation hashes the person ID instead of the company key. Please carry identity aggregation through cohort/property-group recursion too, so this path also defers when the identities differ. Reproduction: reproduced — ./vendor/bin/phpunit --bootstrap vendor/autoload.php --configuration phpunit.xml --no-coverage --do-not-cache-result test/HoldoutCohortReviewTest.php fails on 2e8dc97 with banner=true despite checkout=control, and passes on merge base 5f16426.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 An AI agent wrote this on behalf of @ioannisj.

Fixed in 464a394. matchCohort() and matchPropertyGroup() now hand the dependent flag's identity aggregation down to evaluateFlagDependencyForIdentity(), so a person flag targeting a cohort that requires checkout = holdout-727 goes inconclusive (and falls back to /flags) instead of hashing the person id. The public wrappers keep their signatures and pass unknown, same shape as the existing dependency path.

Covered your scenario plus the same-aggregation and remote-fallback cases in test/FeatureFlagHoldoutTest.php, and f9ca21d adds the nested property-group and nested-cohort variants so a regression in either recursion branch wouldn't pass silently.

One gap I left alone: the guard only runs on an evaluation-cache miss, so a flag with per-condition aggregation overrides could reuse a dependency cached under another condition's identity. That predates this thread and I couldn't confirm it's reachable, worth a follow-up if you think it is?

@marandaneto marandaneto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

approving to unblock but found #254 (comment)

@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Oct 16, 2026
@github-actions github-actions Bot removed the stale label Oct 19, 2026
@posthog-project-board-bot posthog-project-board-bot Bot moved this from Approved to In Review in Feature Flags Oct 22, 2026

@ioannisj ioannisj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 An AI agent wrote this on behalf of @ioannisj.

  • Cohort and property-group dependencies now carry the identity aggregation, fixed in 464a394 with nested-path tests in f9ca21d, replied in the thread.

Comment thread lib/FeatureFlag.php
$properties,
$cohortProperties,
$propertyMatchingVersion,
false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 An AI agent wrote this on behalf of @ioannisj.

Fixed in 464a394. matchCohort() and matchPropertyGroup() now hand the dependent flag's identity aggregation down to evaluateFlagDependencyForIdentity(), so a person flag targeting a cohort that requires checkout = holdout-727 goes inconclusive (and falls back to /flags) instead of hashing the person id. The public wrappers keep their signatures and pass unknown, same shape as the existing dependency path.

Covered your scenario plus the same-aggregation and remote-fallback cases in test/FeatureFlagHoldoutTest.php, and f9ca21d adds the nested property-group and nested-cohort variants so a regression in either recursion branch wouldn't pass silently.

One gap I left alone: the guard only runs on an evaluation-cache miss, so a flag with per-condition aggregation overrides could reuse a dependency cached under another condition's identity. That predates this thread and I couldn't confirm it's reachable, worth a follow-up if you think it is?

@ioannisj
ioannisj merged commit 49d54ed into main Oct 22, 2026
26 checks passed
@ioannisj
ioannisj deleted the posthog/php-local-eval-holdouts branch October 22, 2026 08:55
@github-project-automation github-project-automation Bot moved this from In Review to Done in Feature Flags Oct 22, 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.

3 participants