fix(context): forward every per-call bucketing attribute on the feature path - #63
Merged
Merged
Conversation
…on the feature path CAP-1 (SPEC-per-call-bucketing-attributes): 8 failing tests covering forwarding at the FeatureManagerInterface hand-off, the ignoreLocationProperties location gate (including the strict-comparison case), forceVariationId binding, and enableTracking:false producing no enqueue while the sticky write still happens. CAP-3: one new preview guard asserting zero trace survives a caller passing enableTracking:true / suppressPersistence:false. It passes today by omission — the caller's values are dropped before they reach the engine — so it is a guard against the forwarding fix, not a RED test. Tests only grow here: two new files, one appended method, zero existing lines changed (41 insertions / 0 deletions). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e/runFeatures The two feature entry points rebuilt the attributes DTO from a five-key literal, so enableTracking, forceVariationId, ignoreLocationProperties and suppressPersistence were accepted by the public DTO and then dropped before reaching the engine — silently, with a plausible-looking result. They now spread the caller's object, the same idiom the experience entry points have used since b9e692b. The sharpest consequence was ignoreLocationProperties: with no location properties supplied, PHP rejects an experience even when it carries no location restrictions, so a caller with no URL — a CLI worker, a queue consumer, a webhook handler — was told the feature was disabled with no way to say otherwise. Behaviour change for existing callers: enableTracking: false now suppresses the bucketing enqueue on the feature path, where it previously had no effect. Callers relying on that will see tracked-exposure volume drop. The returned decision and the sticky write are unaffected — persistence is gated on suppressPersistence alone. The Context-set suppressPersistence preview override stays after the spread, so a caller cannot re-enable tracking or persistence on a previewing context. CAP-1, CAP-3 (SPEC-per-call-bucketing-attributes) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CAP-2 (SPEC-per-call-bucketing-attributes): 4 failing tests. The forwarding assertion reads the spy's THIRD argument ($filter['experiences']), never the captured DTO. Since CAP-1's spread now carries experienceKeys inside the attributes object, a DTO-only assertion would pass today and stay green forever while the filter is still null — the silent pass D-5 exists to prevent. Also asserts the filter carries no 'features' key: the disabled-feature extension that gives runFeatures its run-all contract is gated on that key's absence. Two provider rows and the key-order test pass today. That is expected and stated rather than hidden: for absent and empty-array input the correct behaviour equals today's unfiltered behaviour, and order-invariance is a property of the entity lookup that holds whether or not Context forwards a filter. All three remain regression guards once CAP-2 lands. Growth only: 148 insertions, 0 deletions, fixture untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
runFeatures accepted experienceKeys through the public attributes DTO and then called FeatureManager with no filter at all, so a caller narrowing a run-all feature call silently got every experience evaluated. runFeature has honoured the same control all along, through its own positional argument. The filter carries 'experiences' only. A 'features' key is never added: the disabled-feature extension that gives runFeatures its run-all contract is gated on that key's absence, so adding one would drop every feature the visitor was not bucketed into. Narrowing therefore reports an excluded feature as disabled rather than omitting it. The keys are passed uniformly, including as null when the caller supplies none — the downstream guard is an emptiness test, so null and [] both mean no filter. CAP-2 (SPEC-per-call-bucketing-attributes) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four decision entry points described their attributes parameter as 'Attributes for the visitor' and named no control at all, so the controls this branch makes reachable were undiscoverable from the IDE — which for a PHP SDK is where a developer actually meets the documentation. Each docblock now names the set it honours, with typeCasting and experienceKeys marked inert on the experience path. Two clauses are load-bearing rather than descriptive. Only the boolean true bypasses the location gate: the documented construction idiom is the constructor array, which coerces nothing, so 'true' as a string constructs fine and then silently does not work. This docblock is now the only surface carrying that caveat. And forceVariationId is explicitly not preview — it steers selection inside normal gating, where preview bypasses every gate. suppressPersistence stays undocumented while remaining reachable through the DTO. CAP-4 (SPEC-per-call-bucketing-attributes) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aimed Decision-audit round 2. All three blocks were added by this branch, so these are still additions from main and no test-intent declaration applies. The CAP-3 preview guard used assertInstanceOf(BucketedFeature::class) and assertNotEmpty() as proof that bucketing happened. Neither does that job: a feature that resolved to nothing still returns a disabled DTO, and runFeatures pads its result with every declared feature when no feature filter is given. Both now assert FeatureStatus::Enabled, so a total bucketing failure can no longer satisfy the zero-trace assertions vacuously. suppressPersistence was asserted nowhere, though D-2 knowingly makes it caller-reachable on the feature path. It is now forwarded in the fixture and covered behaviourally on a non-preview context: zero enqueues AND zero visitor-state writes, which is the shape that distinguishes it from enableTracking:false, where the sticky write still happens. The key-order test compared two calls carrying the same key set, so it passed whether or not the filter was applied. It now filters to a pair excluding the only experience carrying feature-2 and asserts that feature Disabled in both orders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onId write-through Five code-review items, all in test paths, all growth on this branch. forceVariationId's write-through had no coverage anywhere in the repo, on either entry-point path. It is the only newly-reachable control that mutates persisted visitor state: a force disagreeing with a stored decision takes the recompute path, so the forced variation becomes the visitor's new sticky decision. The new test buckets naturally, forces the other variation, asserts the store was overwritten, then re-runs with no force and asserts the forced value comes back. Proven by mutating the force guard and watching it fail. The two counting/recording doubles moved to tests/Support/FeaturePathTestDoubles behind a require_once, following the six-consumer pattern packages/Data/tests/Support/MutualExclusionTestSupport.php already establishes — ApiManagerInterface has 8 methods and both copies implemented all of them. Both test files still run standalone, which is what the duplication existed to protect. Two docblocks described what the code did BEFORE the fix landed on this same branch and would have been false on merge; two others carried an alternative considered and a restatement of the spec. Replaced with what the tests model, cited by identifier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The interface documented the filter as ['experienceKeys' => ['exp1']]. Every implementation reads $filter['experiences'], so a caller following the docblock would build a key nothing reads and get run-all instead of narrowing, with no error. The declared value type also excluded null, which is what the new Context::runFeatures call site passes when the caller supplies no keys. Dormant until now — nothing passed a filter to this method before this branch. Context::runFeatures is the first caller, and phpstan.neon suppresses argument.type for Context.php, so a later alignment of the call site to the docblock would not be caught. CAP-2 (SPEC-per-call-bucketing-attributes) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JosephSamirL
left a comment
JosephSamirL
left a comment
There was a problem hiding this comment.
Review — convertcom/php-sdk PR #63 @ c4e6354
Reviewer: convert-code-reviewer (independent pass, 2026-09-17). Spot-checked by the session: the stale docblock comment in ContextFeatureBucketingAttributesTest, the testRunFeaturesIgnoresExperienceKeysOrder method, the PR body's AgDR-0171/0172 citations, and the committed <<<<<<<< conflict markers in #98's AgDR-0182/0183/0184 — all verified on disk.
VERDICT: APPROVED (four IMPORTANT, none blocking)
Summary
Reviewed git diff origin/main...HEAD (merge base 7e96158, head c4e6354): 6 files, +848/-26, against SPEC-per-call-bucketing-attributes (SPEC.md + bucketing-attributes.md) and AgDR-0182.
The code change is small and correct. Context::runFeature / Context::runFeatures now use byte-for-byte the same idiom as Context::runExperience (get_object_vars spread → visitorProperties merge → environment fallback → preview suppressPersistence override applied last), and runFeatures passes ['experiences' => $attributes?->getExperienceKeys()] as FeatureManager::runFeatures' third argument. All four CAPs land (CAP-4 minus the wiki row, correctly — wikis are refreshed by the drift routine). No engine file is touched; the parity suite is green. Zero CRITICAL findings.
Shared semantics verified in code and tests:
- enableTracking suppresses the wire event only —
DataManager::_retrieveBucketinggatesputDataon!$suppressPersistenceand the enqueue on$enableTracking && !$suppressPersistence; the variation is chosen before either gate.ContextFeatureTrackingSuppressionTestassertsenqueueCalls === 0withsetCalls > 0on both entry points. experienceKeys: []= no filter —FeatureManager::runFeaturesguards on!empty($filter['experiences']), so[]andnullboth fall togetEntitiesList. Data-provider row covers it.- Unknown key skipped —
DataManager::getItemsByKeysiterates the config list within_array(..., true); unknown keys never match, nothing raises. Rows "one unknown among known" and "every key unknown" cover it and fail on main. - Key order ignored / config order — same
getItemsByKeysloop; measured at theFeatureManagerboundary: both key orders yield[-2, -2, -3, null]. See IMPORTANT-1 for the test gap. - Bucketing contract untouched — no diff in
packages/Bucketing,packages/Data;--testsuite cross-sdk474 tests / 651 assertions OK. - Preview zero-trace (CAP-3) — override applied after the spread on all four entry points;
ContextPreviewTest::previewContextLeavesZeroTraceAcrossFeatureMethodsEvenWhenCallerAsksToBeTrackedhas explicit "not trivially passing" guards and fails when the override order is reversed.
FeatureManagerInterface change is docblock-only; the native signature runFeatures(string, BucketingAttributes, ?array $filter = null) is unchanged, so no runtime BC break. A probe implementor carrying the old array<string, string[]>|null docblock compiled against the new interface under phpstan.neon (level 6) with no errors.
PHP pitfalls: none introduced. ?? $this->environment matches the experience path; in_array uses are strict; no empty() on "0"-shaped values; isset($filter['features']) correctly keeps the disabled-feature extension gated on the absence of a features key.
CRITICAL
None.
IMPORTANT
1. The order test cannot observe evaluation order (confidence 90%)
- File:
packages/Php-sdk/tests/ContextFeatureBucketingAttributesTest.php—testRunFeaturesIgnoresExperienceKeysOrder - Rule: tests must exercise the behaviour claimed — bucketing-attributes.md's
experienceKeysrow states "order is ignored — evaluation follows config order", load-bearing forrunFeaturefirst-enabled precedence. - Evidence: mutating
FeatureManager::runFeaturesto evaluate in request order (array_map(getEntity)over$filter['experiences']) leaves all six CAP-2 tests passing.featuresByKey()collapses to a status map, and PHPUnitassertEqualsis key-order-insensitive on associative arrays.BucketedFeaturecarries no experience identity, so the property is not observable at theContextsurface at all. - Fix: assert at the
FeatureManager::runFeaturesboundary, whose raw arrays carryexperienceKey: for keys[-3, -2]and[-2, -3]on the same visitor,assertSame(array_column($result, 'experienceKey'), ...)is identical for both orders and starts withtest-experience-ab-fullstack-2(config-first). ArunFeaturevariant (feature-1 is carried by both -2 and -3) pins the precedence claim directly.
2. Undisclosed third behaviour change: loosely-typed values now throw instead of being ignored (confidence 80%)
- File:
packages/Php-sdk/src/Context.php(the spread inrunFeature/runFeatures) →packages/Data/src/DataManager.php_retrieveBucketing(?string $forceVariationId, bool $enableTracking, …)understrict_types=1 - Rule: SPEC constraint "the one behaviour change for existing callers ships visibly"; PR body lists two.
- Evidence (measured, head vs base):
runFeature('feature-1', new BucketingAttributes([... 'forceVariationId' => 100299456]))→ main:BucketedFeature; head:TypeError: Argument #5 ($forceVariationId) must be of type ?string, int given. Same for'enableTracking' => 'false'(Argument #6).runExperiencealready throws on main, so this is consistent, and typing the DTO is a spec non-goal — no code change required. But variation ids are numeric and the constructor-array idiom coerces nothing, so a feature-path caller passing an int today (silently a no-op) gets a fatal after upgrading. - Fix: add it to the PR body's "behaviour changes" section and the
fix:release note; optionally statestringforforceVariationIdin the four CAP-4 docblocks.
3. Stale comment contradicts this PR's own docblock fix (confidence 90%)
- File:
packages/Php-sdk/tests/ContextFeatureBucketingAttributesTest.php— comment above theassertSameon$capturedFilter['experiences'] - Evidence: comment says "FeatureManagerInterface's own docblock says 'experienceKeys'"; commit
daf9f7ein this PR changed that docblock to['experiences' => ['exp-key']]. - Fix: delete the two-line comment.
4. PR body cites the wrong decision records; the real ones carry committed conflict markers in spec PR #98 (confidence 95%)
- File: PR #63 body "Decision records";
ai-driven-product-dev@2563bfa(PR #98 head)docs/agdr/AgDR-0182-…php-feature-path.mdfrontmatter, alsoAgDR-0183-…,AgDR-0184-… - Evidence: body names AgDR-0171/0172; in #98's tree those are
same-request-window-supersessionandcreated-pair-undo-shape-vs-replay-identity. This PR's decisions are AgDR-0182 and AgDR-0183 there, and 0182/0183/0184 have<<<<<<<< HEAD … id: AgDR-0187 / ======== / id: AgDR-0182 >>>>>>>>committed in their YAML frontmatter (git show HEAD:confirms it is committed). - Fix: here, update the body to AgDR-0182/0183; in #98, resolve the three conflict blocks before merge (frontmatter with conflict markers will not parse as an AgDR).
Below threshold: the ~30-line rig builder is duplicated between ContextFeatureTrackingSuppressionTest::buildContext and ContextFeatureBucketingAttributesTest::buildSuppressionRig (php-sdk has no Sonar gate per D-7); fn (BucketedFeature $f) in the new preview test; the PR says twelve new cases fail on main — count is 15 rows across 14 methods (more RED than claimed).
What I checked
Scratch copies (review clone untouched — git status clean, HEAD c4e6354):
composer installin a clone of the PR head → OK.vendor/bin/phpuniton the two new test files → OK (23 tests, 81 assertions);ContextPreviewTest.php→ OK (15 tests, 77).composer test(full) → OK, 1198 tests, 4552 assertions, 34 skipped (skips are theCONVERT_STAGING_SDK_KEYlive cases).vendor/bin/phpunit --testsuite cross-sdk→ OK (474 tests, 651 assertions).composer analyze→ PHPStan level 6 No errors;composer cs-check→ 0 of 133 files.- RED replay: second clone at merge base
7e96158(src untouched) + the PR's four test files → 15 failures / 38 tests; the preview CAP-3 guard passes on main, as the PR discloses. - Mutation: preview override moved before the spread → both preview zero-trace tests fail. Engine evaluated in request order → CAP-2 tests all still pass (IMPORTANT-1).
- Probe scripts for loosely-typed inputs at head vs base (IMPORTANT-2) and for
FeatureManagerraw output order. - PHPStan probe of a legacy-docblock
FeatureManagerInterfaceimplementor → no errors. gh pr view 63 --repo convertcom/php-sdkandgh pr view 98 --repo convertcom/ai-driven-product-dev(head2563bfa== worktree HEAD).- Read:
Context.php(all four entry points,mapToBucketedFeatureDto),FeatureManager::runFeature/runFeatures,DataManager::_getBucketingByField/_retrieveBucketing/matchRulesByField/getItemsByKeys,BucketingAttributesDTO,FeatureManagerInterface,phpstan.neon,qa.yml,test-config.jsonfixture topology, SPEC.md, bucketing-attributes.md, AgDR-0182. - No source files edited anywhere outside the scratchpad; no background processes left running.
JosephSamirL
approved these changes
Sep 17, 2026
JosephSamirL
left a comment
JosephSamirL
left a comment
There was a problem hiding this comment.
Approved via /convert:approve. An independent code review ran through /convert:review, and this issues the B-G4 human marker at c4e6354.
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
2 participants
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.
What this fixes
The two feature entry points rebuilt the attributes DTO from a five-key literal, so four controls were accepted by the public
BucketingAttributesDTO and then dropped before reaching the engine — silently, with a plausible-looking result. The experience entry points have forwarded the object whole sinceb9e692b; these now do the same.The sharpest consequence was
ignoreLocationProperties. In PHP, a call with nolocationPropertiesrejects the experience even when it carries no location restrictions at all — the "not restricted ⇒ matched" branch sits inside a block guarded on truthylocationProperties, so with none supplied it is unreachable. That makes the flag the only way a caller with no URL gets past the gate, and it was exactly the flag the feature path dropped. A CLI worker, queue consumer or webhook handler either fabricated a URL or was told the feature was disabled.runFeaturesadditionally never passed an experience filter at all, soexperienceKeyswas accepted and ignored on the run-all call whilerunFeaturehonoured it.runFeature/runFeaturesspread the caller's object —enableTracking,forceVariationId,ignoreLocationProperties,suppressPersistencenow reach the enginerunFeaturespasses['experiences' => …]asFeatureManager::runFeatures' third argumentenableTracking: true/suppressPersistence: falseContextdocblocks name the control set (partially delivered — see below)Two behaviour changes for existing callers
1.
enableTracking: falsenow works on the feature path. A caller passing it torunFeature/runFeatureshas been tracked anyway; after this they are not. Tracked-exposure volume will drop for such callers. The returned decision and the sticky write are unaffected — persistence is gated onsuppressPersistencealone.2.
forceVariationIdnow binds on the feature path — and writes through. This one is larger and the spec does not call it out. A force that disagrees with a stored decision takes the recompute path, so the forced variation becomes the visitor's new sticky decision and a bucketing event is enqueued. A caller who has been passingforceVariationIdto a feature entry point ineffectively will now start overwriting their visitors' stored decisions. Nothing in the repo covered this on either path;testForceVariationIdDisagreeingWithStoredDecisionWritesThroughnow does.Both ship as
fix:-scoped subjects, so they appear under Bug Fixes in the generated release notes.Test evidence
composer test)composer analyzecomposer cs-checkBaseline at the fork point
028e373was 1174 / 4465 / 34 skipped and 130 files. The branch adds 24 cases and 3 files, so the arithmetic closes exactly and the skip count is unchanged — the 34 are pre-existing live-staging cases needingCONVERT_STAGING_SDK_KEY, not anything this change caused.Twelve of the new cases fail against
main, verified by replaying the test files against a throwaway clone at the fork point. The ones that pass pre-implementation are declared rather than hidden: constraint guards (the "no flag" and string-'true'rows, the absent/empty-array filter rows) and the preview guard, which passes today only because the caller's values are dropped before reaching the engine.Every safety property was mutation-proven rather than asserted — the preview override reversed, the third argument removed, the
suppressPersistencegates flipped one at a time, and theforceVariationIddisagreement guard neutered. Each failed the expected test, and each source file was restored and verified clean.Scope notes for the reviewer
CAP-4 is deliberately partial. Its success criterion names both of PHP's attribute tables.
php-sdk.wiki/CodeExamples.mdis not updated, and should not be: a wiki page is never a deliverable of the feature that changed the code it describes — the daily drift routine owns that refresh, and a wiki clause inside a spec is a defect in the spec rather than a task. Please read the delivery as complete-minus-that-clause, not as an oversight. The spec's definition of done needs correcting.Because of that, and because the shared backend row omits it, the PHPDoc is now the only surface in the project stating that only the boolean
truebypasses the location gate. That matters: the documented construction idiom is the constructor array, which coerces nothing, so['ignoreLocationProperties' => 'true']constructs fine and then silently does not work.FeatureManagerInterface::runFeatures'$filterdocblock is corrected here (daf9f7e) although it predates this work. It documented the key asexperienceKeyswhile every implementation readsexperiences, and this PR makesContext::runFeaturesthe first caller anywhere to pass a filter — so a later "fix" aligning the call site with the docblock would silently turn narrowing back into run-all, andphpstan.neonsuppressesargument.typeforContext.php, so nothing would catch it.Two known-minor items left unfixed, disclosed rather than carried silently: in
ContextFeatureBucketingAttributesTest.php, theassertGreaterThan(0, $rig['dataStore']->setCalls, …)is redundant because the preceding natural call already incremented the counter, and one assertion message says "return" where it inspects the store. Both scored ~55 in review, below the 75 threshold this run acted on; the load-bearing assertion two lines later already carries the property.Related
backend #7391 — the shared full-stack-docs half of CAP-4. Its run-all note said
experienceKeyswas "available in the JavaScript SDK for this call", which was accurate until CAP-2 here falsified it; commit9f2570d56aon that PR restores PHP to the note and adds the strict-boolean clause. That PR should merge alongside this one — merged alone it publishes a claim this PR makes false into all six SDK wikis.Decision records
🤖 Generated with Claude Code
Effort
Agent effort —
per-call-bucketing-attributes-specsBy agent
Time = summed gaps between API responses. A gap after a turn ENDED is a wait on a human or a parent agent and is capped at 120s; a gap mid-turn is the agent generating or running its own tool and is counted up to 1800s, which bounds a hung tool without discarding a long test run. An orchestrator's time OVERLAPS the agents it spawned, so the total counts supervision as well as the work supervised. A task's own last response is followed by no gap, so an n-response task contributes n-1 intervals and its final generation is not counted. The orchestrating session is not a task and is not in the table. Cached is cache reads plus cache writes and is normally most of the prompt, because the same prefix is re-read on every response — it therefore tracks how OFTEN an agent was called as much as how much it handled. The UNCACHED prompt remainder is a small fraction of that, so it is not a column here;
--jsonstill carries it. Output excludes nothing. They bill at different rates, so a row is a volume, not a cost;/convert:costwithout --per-task prices the run.