Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
posthog wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b56440
feat(flags): honor experiment holdouts in local flag evaluation
posthog[bot] Oct 5, 2026
2e8dc97
fix(flags): defer holdout dependencies aggregated differently from th…
ioannisj Oct 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/local-flag-holdouts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"posthog-php": minor
---

Honor experiment holdouts when evaluating feature flags locally, so a held-out identifier resolves to `holdout-<id>` instead of an ordinary variant.
175 changes: 167 additions & 8 deletions lib/FeatureFlag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,74 @@ private static function hash($key, $distinctId, $salt = "")
return $hashVal / self::LONG_SCALE;
}

/**
* Resolve the holdout variant for a flag, when its bucketing identity falls inside the holdout.
*
* @param mixed $holdout The flag's filters.holdout configuration, if any.
* @param string $bucketingValue Flag-level bucketing identity (distinct id or group key).
* @return string|null The "holdout-<id>" variant, or null when no holdout applies.
*/
private static function matchHoldout($holdout, $bucketingValue)
{
$percentage = FeatureFlag::holdoutPercentage($holdout);
if (is_null($percentage)) {
return null;
}

$holdoutKey = "holdout-" . $holdout["id"];

if ($percentage >= 100.0) {
return $holdoutKey;
}

return FeatureFlag::holdoutHash($bucketingValue) <= $percentage / 100 ? $holdoutKey : null;
}

/**
* @param mixed $holdout The flag's filters.holdout configuration, if any.
* @return float|null Exclusion percentage clamped to 0-100, or null when the holdout is unusable.
*/
private static function holdoutPercentage($holdout): ?float
{
if (!is_array($holdout) || is_null($holdout["id"] ?? null)) {
return null;
}

$exclusionPercentage = $holdout["exclusion_percentage"] ?? null;
if (!is_numeric($exclusionPercentage)) {
return null;
}

// Fractional percentages are preserved; only the 0-100 range is clamped.
return min(100.0, max(0.0, (float) $exclusionPercentage));
}

private static function hasActiveHoldout($holdout): bool
{
return (FeatureFlag::holdoutPercentage($holdout) ?? 0.0) > 0.0;
}

private static function aggregationKey($aggregation): ?string
{
return is_null($aggregation) ? null : strval($aggregation);
}

/**
* Hash a bucketing identity for holdout membership, matching the backend.
*
* Unlike the ordinary flag hash, neither the flag key nor the holdout id participates:
* the digest is taken over "holdout-<bucketing_value>" with no separator or salt.
*
* @param string $bucketingValue Flag-level bucketing identity (distinct id or group key).
* @return float Hash in the range [0, 1).
*/
private static function holdoutHash($bucketingValue)
{
$hashVal = base_convert(substr(sha1("holdout-" . $bucketingValue), 0, 15), 16, 10);

return $hashVal / self::LONG_SCALE;
}

private static function getMatchingVariant($flag, $distinctId)
{
$variants = FeatureFlag::variantLookupTable($flag);
Expand Down Expand Up @@ -909,8 +977,49 @@ public static function matchFeatureFlagProperties(
$groupProperties = [],
$groupTypeMapping = [],
$propertyMatchingVersion = 1
) {
return FeatureFlag::matchFeatureFlagPropertiesForIdentity(
$flag,
$distinctId,
$properties,
$cohorts,
$flagsByKey,
$evaluationCache,
$groups,
$groupProperties,
$groupTypeMapping,
$propertyMatchingVersion,
($flag["filters"] ?? [])["aggregation_group_type_index"] ?? null
);
}

/**
* Same as matchFeatureFlagProperties, but bucketing by an identity whose aggregation is already known.
* Dependency recursion keeps the root flag's identity, which may differ from the dependency's own.
*
* @param int|string|null $identityAggregation Null for a person, a group type index otherwise.
*/
private static function matchFeatureFlagPropertiesForIdentity(
$flag,
$distinctId,
$properties,
$cohorts,
$flagsByKey,
$evaluationCache,
$groups,
$groupProperties,
$groupTypeMapping,
$propertyMatchingVersion,
$identityAggregation
) {
$flagFilters = $flag["filters"] ?? [];

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

@dustinbyrne dustinbyrne Oct 7, 2026

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.

@ioannisj ioannisj Oct 16, 2026

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.

if (!is_null($holdoutVariant)) {
return $holdoutVariant;
}

$flagConditions = $flagFilters["groups"] ?? [];
$flagAggregation = $flagFilters["aggregation_group_type_index"] ?? null;
$earlyExitEnabled = $flagFilters["early_exit"] ?? false;
Expand All @@ -928,6 +1037,7 @@ public static function matchFeatureFlagProperties(

$effectiveProperties = $properties;
$effectiveBucketing = $distinctId;
$effectiveAggregation = $identityAggregation;

// Mixed-override path: condition-level aggregation differs from flag-level.
// This assumes flag-level aggregation is null for mixed flags.
Expand All @@ -943,6 +1053,7 @@ public static function matchFeatureFlagProperties(
}
$effectiveProperties = $groupProperties[$groupName];
$effectiveBucketing = $groups[$groupName];
$effectiveAggregation = $conditionAggregation;
}
}

Expand All @@ -954,7 +1065,8 @@ public static function matchFeatureFlagProperties(
$cohorts,
$flagsByKey,
$evaluationCache,
$propertyMatchingVersion
$propertyMatchingVersion,
$effectiveAggregation
);

if ($matchResult === ConditionMatch::Match) {
Expand Down Expand Up @@ -1010,9 +1122,10 @@ private static function isConditionMatch(
$condition,
$properties,
$cohorts,
$flagsByKey = null,
$evaluationCache = null,
$propertyMatchingVersion = 1
$flagsByKey,
$evaluationCache,
$propertyMatchingVersion,
$identityAggregation
): ConditionMatch {
$rolloutPercentage = array_key_exists("rollout_percentage", $condition) ? $condition["rollout_percentage"] : null;

Expand All @@ -1031,14 +1144,15 @@ private static function isConditionMatch(
$propertyMatchingVersion
);
} elseif ($propertyType == 'flag') {
$matches = FeatureFlag::evaluateFlagDependency(
$matches = FeatureFlag::evaluateFlagDependencyForIdentity(
$property,
$flagsByKey,
$evaluationCache,
$distinctId,
$properties,
$cohorts,
$propertyMatchingVersion
$propertyMatchingVersion,
$identityAggregation
);
} else {
$matches = FeatureFlag::matchProperty($property, $properties, $propertyMatchingVersion);
Expand Down Expand Up @@ -1114,6 +1228,32 @@ public static function evaluateFlagDependency(
$properties,
$cohortProperties,
$propertyMatchingVersion = 1
) {
return FeatureFlag::evaluateFlagDependencyForIdentity(
$property,
$flagsByKey,
$evaluationCache,
$distinctId,
$properties,
$cohortProperties,
$propertyMatchingVersion,
false

@marandaneto marandaneto Oct 16, 2026

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.

);
}

/**
* @param int|string|null|false $identityAggregation Aggregation of the identity being bucketed (null for a
* person, a group type index otherwise). False means unknown and skips the holdout aggregation guard.
*/
private static function evaluateFlagDependencyForIdentity(
$property,
$flagsByKey,
$evaluationCache,
$distinctId,
$properties,
$cohortProperties,
$propertyMatchingVersion,
$identityAggregation
) {
if ($flagsByKey === null || $evaluationCache === null) {
throw new InconclusiveMatchException(sprintf(
Expand Down Expand Up @@ -1165,16 +1305,35 @@ public static function evaluateFlagDependency(
if (!($depFlag["active"] ?? false)) {
$evaluationCache[$depFlagKey] = false;
} else {
// Holdout bucketing uses the evaluated identity, which can't be resolved for a
// dependency aggregated differently, so defer to server evaluation.
$depFilters = $depFlag["filters"] ?? [];
if (
$identityAggregation !== false &&
FeatureFlag::hasActiveHoldout($depFilters["holdout"] ?? null) &&
FeatureFlag::aggregationKey($depFilters["aggregation_group_type_index"] ?? null) !==
FeatureFlag::aggregationKey($identityAggregation)
) {
throw new InconclusiveMatchException(sprintf(
"Cannot evaluate flag dependency '%s' with a holdout aggregated differently from the dependent flag", //phpcs:ignore
$depFlagKey
));
}

// Recursively evaluate the dependency
try {
$depResult = FeatureFlag::matchFeatureFlagProperties(
$depResult = FeatureFlag::matchFeatureFlagPropertiesForIdentity(
$depFlag,
$distinctId,
$properties,
$cohortProperties,
$flagsByKey,
$evaluationCache,
propertyMatchingVersion: $propertyMatchingVersion
[],
[],
[],
$propertyMatchingVersion,
$identityAggregation
);
$evaluationCache[$depFlagKey] = $depResult;
} catch (InconclusiveMatchException $e) {
Expand Down
Loading
Loading