Repository navigation
Conversation
- Add customFieldValue() method to Context for custom field access - Add Finalize event constant to ContextEventLoggerEvent - Add customFieldValues property to Experiment class - Fix Assignment class: make variables nullable with null default - Fix Assignment class: set exposed default to false - Fix getAssignment() to use array_key_exists() for override check (fixes variant 0) - Fix audienceStrict check from isset() to !empty() (scenario 44 fix) - Fix getVariableValue() to check variables before queueing exposure
- Fix implicit nullable parameter deprecation in HTTPClient::setupRequest - Update test expectations for Finalize event (was incorrectly expecting Close) - Fix type coercion in tests - SDK correctly preserves numeric attribute types
Match JavaScript SDK behavior: queue exposure when assignment.variables is not null, regardless of whether the specific variable key exists. Only return the variable value if the key exists AND the user is assigned or has an override. This fixes exposure tracking for cases where the user is assigned a variant that doesn't contain the specific variable being requested.
Change seedHi/seedLo to trafficSeedHi/trafficSeedLo to match the experiment data structure for traffic split assignment.
- Add failed state testing - Add attribute management tests - Add timeout/retry configuration tests - Add error handling tests - Add event handler scenario tests - Add integration scenario tests Total: 30 new tests added, all 142 tests pass
Create MD5Test (14), Murmur3Test (36), AudienceMatcherTest (3) with parameterized data providers. Refactor VariantAssignerTest to 42 individual cases. Add ~76 context tests covering event logger, exposure queuing, variable values, publish lifecycle, refresh cache invalidation, custom fields, and audience matching.
Fix InOperator and MatchOperator for proper type handling. Add async interfaces for Client, HttpClient, ContextDataProvider, and ContextEventHandler. Add RuntimeException and ABsmartly facade class.
|
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:
WalkthroughThis PR substantially refactors and extends the ABSmartly PHP SDK. The main SDK class is renamed from Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1
✨ Finishing Touches
🐇 Hoppity-hop, the SDK's reborn, Comment |
createWithDefaults() had swapped environment/application params. Add createSimple() with correct order matching ClientConfig constructor. Deprecate createWithDefaults() for backward compatibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
tests/JsonExpression/Operator/InOperatorTest.php (1)
64-67: Consider renaming this test method for clarity.The method name
testArrayDiffNullsuggests it tests null behaviour, but it actually verifies that sub-arrays are not found as elements within other arrays. A clearer name might betestArrayNotFoundAsElementortestNestedArrayNotContained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/JsonExpression/Operator/InOperatorTest.php` around lines 64 - 67, Rename the test method testArrayDiffNull to a clearer name that reflects its behavior (e.g., testArrayNotFoundAsElement or testNestedArrayNotContained) and update any references to it; specifically change the method name in the test class containing the method testArrayDiffNull and ensure any test runner or annotations still recognize it (the body calling $this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]]) etc. should remain unchanged).src/Experiment.php (1)
36-38: Consider importing the exception class.The exception is referenced using its fully-qualified class name. For consistency with other files (e.g.,
ContextConfig.phpwhich imports the exception), consider adding an import statement.♻️ Proposed refactor
Add at the top of the file:
use ABSmartly\SDK\Exception\RuntimeException;Then simplify the throw statement:
- throw new \ABSmartly\SDK\Exception\RuntimeException( + throw new RuntimeException( sprintf('Missing required field "%s" in experiment data', $field) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Experiment.php` around lines 36 - 38, Import the RuntimeException class and use the short name in the throw to match other files: add a top-level use statement for ABSmartly\SDK\Exception\RuntimeException and replace the fully-qualified instantiation in the throw (the throw new \ABSmartly\SDK\Exception\RuntimeException(sprintf(...))) with throw new RuntimeException(sprintf(...))); locate this change around the throw in Experiment.php to keep consistency with files like ContextConfig.php.src/JsonExpression/Operator/MatchOperator.php (1)
27-36: Verify that aggressive delimiter trimming is the intended behaviour.Using
trim($pattern, '/')removes all leading and trailing/characters, not just a single delimiter pair. For example, a pattern like//test//becomestestrather than/test/. If patterns legitimately start or end with/(e.g., matching paths like/api/), this could alter the matching behaviour unexpectedly.If the intent is to strip only wrapping delimiters, consider a more precise approach:
♻️ Proposed refinement to strip only a single delimiter pair
private function runRegexBounded(string $text, string $pattern): ?bool { - $pattern = trim($pattern, '/'); + if (strlen($pattern) >= 2 && $pattern[0] === '/' && $pattern[-1] === '/') { + $pattern = substr($pattern, 1, -1); + } $matches = `@preg_match`('~'. $pattern . '~', $text);If this aggressive trimming is intentional for cross-SDK consistency, please confirm that the other SDK implementations behave identically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/JsonExpression/Operator/MatchOperator.php` around lines 27 - 36, The code in runRegexBounded uses trim($pattern, '/') which removes all leading/trailing slashes; change it to strip only a single wrapping delimiter pair: if (substr($pattern, 0, 1) === '/' && substr($pattern, -1) === '/') { $pattern = substr($pattern, 1, -1); } so patterns like //test// or paths like /api/ are not mangled; update runRegexBounded to use this check before calling preg_match and keep the rest of the error handling unchanged (and confirm consistent behaviour with other SDKs if cross-SDK consistency was intended).src/Http/ReactHttpClient.php (1)
79-79: Consider adding type hint for$reactResponseparameter.The parameter lacks a type declaration. Adding
\Psr\Http\Message\ResponseInterface(or the specific React response type) would improve type safety and IDE support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/ReactHttpClient.php` at line 79, The toResponse method signature lacks a parameter type; update the private function toResponse($reactResponse): Response to include a type hint (e.g. private function toResponse(\Psr\Http\Message\ResponseInterface $reactResponse): Response or the concrete React response type) and adjust imports or use the fully-qualified name; then update any callers if necessary to satisfy the stricter type and ensure IDE/type-checking benefits.tests/Context/AsyncContextDataProviderTest.php (1)
35-52: Consider usingawait()for more robust promise testing.The test relies on the synchronous resolution behaviour of
resolve()in react/promise 3.x. While this works, it's somewhat implicit. UsingReact\Async\await()would make the test more explicit and resilient to future changes.♻️ Proposed refactor using await()
+use function React\Async\await; + public function testGetContextDataAsyncResolvesToContextData(): void { $client = $this->createMock(AsyncClientInterface::class); $contextData = new ContextData(); $contextData->experiments = []; $client->method('getContextDataAsync') ->willReturn(resolve($contextData)); $provider = new AsyncContextDataProvider($client); $promise = $provider->getContextDataAsync(); - $result = null; - $promise->then(function($data) use (&$result) { - $result = $data; - }); - - self::assertSame($contextData, $result); + $result = await($promise); + self::assertSame($contextData, $result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/AsyncContextDataProviderTest.php` around lines 35 - 52, The test testGetContextDataAsyncResolvesToContextData should use React\Async\await() instead of manually attaching a then callback to the promise to make the assertion robust; update the test so the mock client still returns resolve($contextData) and then call await($provider->getContextDataAsync()) to obtain $result synchronously before asserting assertSame($contextData, $result), referencing AsyncContextDataProvider::getContextDataAsync and the resolve() usage when making the change.tests/MD5Test.php (1)
30-37: ExercisePublishEvent::hashUnit()directly.This currently proves PHP's MD5/base64url primitives, not the SDK code path. If
PublishEvent::hashUnit()regresses, the test still passes.💡 Proposed change
+use ABSmartly\SDK\PublishEvent; ... public function testShouldMatchKnownHashes(string $input, string $expectedHash): void { - $hash = hash('md5', $input, true); - $base64url = strtr(base64_encode($hash), [ - '+' => '-', - '/' => '_', - '=' => '', - ]); - self::assertSame($expectedHash, $base64url); + $event = new PublishEvent(); + self::assertSame($expectedHash, $event->hashUnit($input)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MD5Test.php` around lines 30 - 37, The test testShouldMatchKnownHashes is exercising PHP primitives instead of the SDK; replace the manual hash computation with a call to PublishEvent::hashUnit($input) inside the test (keep the same $input and $expectedHash variables), and assert that the returned value equals $expectedHash using self::assertSame; locate this change in the testShouldMatchKnownHashes method so it directly validates PublishEvent::hashUnit rather than using hash/base64_encode logic.src/ABsmartly.php (1)
84-108: Let the deprecated factory delegate tocreateSimple().The two factories are otherwise identical, so keeping both bodies in sync will get harder as this façade evolves.
♻️ Proposed change
public static function createWithDefaults( string $endpoint, string $apiKey, string $environment, string $application, int $retries = 5, int $timeout = 3000, ?callable $eventLogger = null ): ABsmartly { - - $clientConfig = new ClientConfig( - $endpoint, - $apiKey, - $application, - $environment, - ); - $clientConfig->setRetries($retries); - $clientConfig->setTimeout($timeout); - - $client = new Client($clientConfig, new HTTPClient()); - $sdkConfig = new Config($client); - if ($eventLogger !== null) { - $sdkConfig->setContextEventLogger(new \ABSmartly\SDK\Context\ContextEventLoggerCallback($eventLogger)); - } - return new ABsmartly($sdkConfig); + return self::createSimple( + $endpoint, + $apiKey, + $application, + $environment, + $retries, + $timeout, + $eventLogger, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ABsmartly.php` around lines 84 - 108, The createWithDefaults factory duplicates createSimple; change createWithDefaults to delegate to createSimple by calling ABsmartly::createSimple(...) with the same parameters (endpoint, apiKey, environment, application, retries, timeout, eventLogger) instead of duplicating client/config construction, so both factories share one implementation; locate createWithDefaults and replace its body with a call to createSimple while preserving the method signature and any deprecation annotation.tests/Client/ClientAsyncTest.php (1)
55-69: Assert the async path was actually used.Both async-branch tests pass if these methods return any
PromiseInterface, even whengetAsync()/putAsync()are never called or the response mapping is broken. Add mock expectations on the async methods and assert the resolvedContextData/ publish fulfilment.Also applies to: 94-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Client/ClientAsyncTest.php` around lines 55 - 69, The test currently only verifies that getContextDataAsync() returns a PromiseInterface but not that the async branch or response mapping runs; update testGetContextDataAsyncWithAsyncClient to set expectations on the mock from createMockAsyncHttpClient() that getAsync() is invoked (e.g. once with the expected endpoint) and returns the prepared response promise, then resolve the returned PromiseInterface and assert the resolved value is a ContextData instance with the expected fields; apply the same pattern to the publish test (ensure putAsync() is expected and that the resolved promise indicates successful publish/fulfilment).tests/Context/ContextTest.php (1)
1220-1245: This recovery test never verifies recovery.It only checks that
refresh()invoked the provider twice. A context can stay permanently failed after the second call and this test would still pass. Please assert thatisFailed()flips back tofalseand that a subsequent publish reaches the handler.Suggested assertion upgrade
$context->refresh(); self::assertSame(2, $callCount); + self::assertFalse($context->isFailed()); + + $context->track('goal_after_recovery'); + $context->publish(); + self::assertCount(1, $eventHandler->submitted); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/ContextTest.php` around lines 1220 - 1245, The test only verifies the provider was called twice but not that the context recovered; after calling Context::refresh() in testRecoveryFromFailedState you should assert the context is no longer failed (call Context::isFailed() returns false) and then exercise Context::publish(...) and verify ContextEventHandlerMock recorded/received the event (e.g. check the mock's published event list or invocation count) to prove the context actually recovered and can handle publishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 11-20: The Composer manifest currently lists react/http,
react/async, and react/promise as dev dependencies but multiple runtime classes
(e.g., ReactHttpClient which imports React\Http\Browser and React\Async\await,
public APIs like Client, AsyncClientInterface, ABsmartly, and your async context
classes that reference React\Promise\PromiseInterface) require them at runtime;
update composer.json by moving "react/http", "react/async", and "react/promise"
from require-dev into the "require" section (or alternatively fully isolate
async-only code behind a separate package or lazy-loaded wrapper so none of the
autoloaded runtime classes reference these symbols), ensuring consumers
installing without --dev will have these packages available.
In `@README.md`:
- Around line 379-387: Update the table row that currently reads "Close" /
`Context->close()` to use the correct event name `Finalize` to match the emitted
event type; specifically change the Event column label from `Close` to
`Finalize` and indicate that `Context::close()` emits
`ContextEventLoggerEvent::Finalize` (so integrators see the correct event name
to wire up).
- Around line 124-145: Update the async README examples to use the actual async
Context API: replace calls to the non-existent $context->ready() and avoid
chaining .then() on void methods; instead call $sdk->createContextAsync($config)
and attach a PromiseInterface then/catch to the returned promise (it resolves
with a ready Context), or use $sdk->createContextPending($config) which returns
['context' => $context, 'promise' => $promise] and attach .then()/.catch() to
that promise; keep calls to Context::isReady(), and do not chain .then() on
Context::publish() or Context::close() (they return void).
In `@src/ABsmartly.php`:
- Around line 119-125: createContextAsync currently calls
$this->createContext($contextConfig) directly inside resolve(), which runs
synchronously and lets exceptions escape instead of producing a rejected
Promise; change the sync-fallback to defer execution like createContextPending
does by wrapping the call in resolve(null)->then(fn() =>
$this->createContext($contextConfig)) (or equivalently resolve()->then(...)) so
any thrown exception becomes a rejected Promise and the method always returns a
proper PromiseInterface from createContextAsync.
In `@src/Client/Client.php`:
- Around line 82-85: The publish() method allows outbound PUTs without
validating the API key; update Client::publish to perform the same API-key
validation as authRequest()/getContextData() by either calling the existing
authRequest() guard or explicitly checking the stored API key before encoding
and calling $this->httpClient->put; if the API key is missing/invalid, throw or
abort the publish operation (same behavior as authRequest()) to prevent
blank-key requests from being sent.
- Around line 71-95: getContextDataAsync and publishAsync currently run
synchronous code (authRequest, encode, or publish) that can throw, causing sync
exceptions instead of rejected promises, and publish() lacks auth validation;
wrap the synchronous parts of getContextDataAsync (the authRequest call) and
publishAsync (authRequest + encode or the fallback publish call) in try/catch
and return a rejected PromiseInterface on exception, and add authRequest() at
the start of the synchronous publish(PublishEvent) method so both sync and async
publish paths validate auth; update references: getContextDataAsync,
publishAsync, publish, authRequest, encode, AsyncHttpClientInterface, and
PromiseInterface to implement these changes.
In `@src/Context/Context.php`:
- Around line 208-212: The successful-reload paths never clear the failed flag
so after a transient error the instance stays in failed mode; in the try blocks
that repopulate context (e.g. where $this->data = $contextData;
$this->setData($contextData); $this->ready = true; $this->logEvent(...)), reset
$this->failed = false when recovery succeeds so subsequent publish() and close()
take the normal path; apply the same fix to the other identical recovery block
referenced (around lines 751-753) so both successful reloads clear the failed
state.
- Around line 346-348: The branch using $this->cassignments can return a cached
natural Assignment without marking it as custom, so exposures are misreported;
when a custom assignment exists for $experimentName and matches
$assignment->variant, set $assignment->custom = true (and update any relevant
metadata like timestamp if present) before returning. Locate the logic around
$this->cassignments, experimentMatches(...), and audienceMatches(...) and ensure
that whenever a custom assignment is applied (even if the variant equals the
cached natural one) the returned Assignment object has its custom flag set to
true.
- Around line 297-299: The use of str_starts_with in the boolean branch will
break PHP 7.4; change the check in Context.php where the branch inspects $type
(the block using str_starts_with($type ?? '', 'boolean') && is_string($value'))
to a PHP 7.4-safe prefix test (e.g. use strpos(...) === 0 or substr_compare(...,
0) === 0) so the boolean coercion still runs on types that start with "boolean"
while remaining compatible with PHP 7.4; keep the rest of the branch logic (the
is_string($value) check and returning $value === 'true' || $value === '1')
unchanged.
In `@src/Experiment.php`:
- Around line 31-40: The Experiment class's typed property audienceStrict is
left uninitialized and can cause a PHP 8+ TypeError when Context accesses it;
initialize the audienceStrict property with a sensible default (e.g., false or
null matching its declared type) in the Experiment class declaration so it is
always set by default, and do not add audienceStrict to the $requiredFields
array used in the validation loop (leave $requiredFields as-is) so optional
behaviour is preserved.
In `@src/Http/HTTPClient.php`:
- Around line 73-116: The fetchResponse method can throw null when
$this->retries is 0 because the while loop never runs and $lastException stays
null; fix by validating or ensuring at least one attempt: either (A) at the
start of fetchResponse validate $this->retries and throw a clear exception
(e.g., InvalidArgumentException) when it's less than 1, or (B) change the retry
loop to guarantee one request (convert the while to a do/while or use max(1,
$this->retries)) so an actual HttpClientError is assigned to $lastException
before rethrowing; update references in fetchResponse, $this->retries,
$lastException and adjust the final throw to always throw an Exception instance.
In `@src/Http/ReactHttpClient.php`:
- Around line 15-16: ReactHttpClient exposes a public $retries property but
never uses it, causing async methods to lack retry behavior; update the async
request methods (e.g., getAsync, postAsync, requestAsync or sendAsync in
ReactHttpClient) to implement retry logic that uses $this->retries and respects
$this->timeout (loop attempts, catch network/timeout errors, backoff or
immediate retry as HTTPClient does), or if async retries are intentionally
unsupported remove the public $retries property and stop setting
httpClient->retries in the constructor; ensure whichever path you choose keeps
behavior consistent with HTTPClient and updates any constructor assignment that
references $retries.
In `@tests/JsonExpression/Operator/InOperatorTest.php`:
- Around line 29-36: In the test method testReturnFalseOnEmptyArray remove the
duplicate assertion that calls $this->operator->evaluate($this->evaluator,
[false, []]) (it's repeated twice); keep one instance and ensure the method
still asserts the distinct cases for false, "1", true and null against an empty
array; update only the duplicate assertion in the test (method name:
testReturnFalseOnEmptyArray, symbol: $this->operator->evaluate,
$this->evaluator) so the test no longer contains the duplicated line.
---
Nitpick comments:
In `@src/ABsmartly.php`:
- Around line 84-108: The createWithDefaults factory duplicates createSimple;
change createWithDefaults to delegate to createSimple by calling
ABsmartly::createSimple(...) with the same parameters (endpoint, apiKey,
environment, application, retries, timeout, eventLogger) instead of duplicating
client/config construction, so both factories share one implementation; locate
createWithDefaults and replace its body with a call to createSimple while
preserving the method signature and any deprecation annotation.
In `@src/Experiment.php`:
- Around line 36-38: Import the RuntimeException class and use the short name in
the throw to match other files: add a top-level use statement for
ABSmartly\SDK\Exception\RuntimeException and replace the fully-qualified
instantiation in the throw (the throw new
\ABSmartly\SDK\Exception\RuntimeException(sprintf(...))) with throw new
RuntimeException(sprintf(...))); locate this change around the throw in
Experiment.php to keep consistency with files like ContextConfig.php.
In `@src/Http/ReactHttpClient.php`:
- Line 79: The toResponse method signature lacks a parameter type; update the
private function toResponse($reactResponse): Response to include a type hint
(e.g. private function toResponse(\Psr\Http\Message\ResponseInterface
$reactResponse): Response or the concrete React response type) and adjust
imports or use the fully-qualified name; then update any callers if necessary to
satisfy the stricter type and ensure IDE/type-checking benefits.
In `@src/JsonExpression/Operator/MatchOperator.php`:
- Around line 27-36: The code in runRegexBounded uses trim($pattern, '/') which
removes all leading/trailing slashes; change it to strip only a single wrapping
delimiter pair: if (substr($pattern, 0, 1) === '/' && substr($pattern, -1) ===
'/') { $pattern = substr($pattern, 1, -1); } so patterns like //test// or paths
like /api/ are not mangled; update runRegexBounded to use this check before
calling preg_match and keep the rest of the error handling unchanged (and
confirm consistent behaviour with other SDKs if cross-SDK consistency was
intended).
In `@tests/Client/ClientAsyncTest.php`:
- Around line 55-69: The test currently only verifies that getContextDataAsync()
returns a PromiseInterface but not that the async branch or response mapping
runs; update testGetContextDataAsyncWithAsyncClient to set expectations on the
mock from createMockAsyncHttpClient() that getAsync() is invoked (e.g. once with
the expected endpoint) and returns the prepared response promise, then resolve
the returned PromiseInterface and assert the resolved value is a ContextData
instance with the expected fields; apply the same pattern to the publish test
(ensure putAsync() is expected and that the resolved promise indicates
successful publish/fulfilment).
In `@tests/Context/AsyncContextDataProviderTest.php`:
- Around line 35-52: The test testGetContextDataAsyncResolvesToContextData
should use React\Async\await() instead of manually attaching a then callback to
the promise to make the assertion robust; update the test so the mock client
still returns resolve($contextData) and then call
await($provider->getContextDataAsync()) to obtain $result synchronously before
asserting assertSame($contextData, $result), referencing
AsyncContextDataProvider::getContextDataAsync and the resolve() usage when
making the change.
In `@tests/Context/ContextTest.php`:
- Around line 1220-1245: The test only verifies the provider was called twice
but not that the context recovered; after calling Context::refresh() in
testRecoveryFromFailedState you should assert the context is no longer failed
(call Context::isFailed() returns false) and then exercise Context::publish(...)
and verify ContextEventHandlerMock recorded/received the event (e.g. check the
mock's published event list or invocation count) to prove the context actually
recovered and can handle publishes.
In `@tests/JsonExpression/Operator/InOperatorTest.php`:
- Around line 64-67: Rename the test method testArrayDiffNull to a clearer name
that reflects its behavior (e.g., testArrayNotFoundAsElement or
testNestedArrayNotContained) and update any references to it; specifically
change the method name in the test class containing the method testArrayDiffNull
and ensure any test runner or annotations still recognize it (the body calling
$this->operator->evaluate($this->evaluator, [[2, 3], [1, 2, 3]]) etc. should
remain unchanged).
In `@tests/MD5Test.php`:
- Around line 30-37: The test testShouldMatchKnownHashes is exercising PHP
primitives instead of the SDK; replace the manual hash computation with a call
to PublishEvent::hashUnit($input) inside the test (keep the same $input and
$expectedHash variables), and assert that the returned value equals
$expectedHash using self::assertSame; locate this change in the
testShouldMatchKnownHashes method so it directly validates
PublishEvent::hashUnit rather than using hash/base64_encode logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 991f6102-50ca-4a47-b7d7-befe3f8de1c3
📒 Files selected for processing (41)
.gitignoreREADME.mdcomposer.jsonsrc/ABsmartly.phpsrc/Assignment.phpsrc/Client/AsyncClientInterface.phpsrc/Client/Client.phpsrc/Client/ClientConfig.phpsrc/Client/ClientInterface.phpsrc/Context/AsyncContextDataProvider.phpsrc/Context/AsyncContextEventHandler.phpsrc/Context/Context.phpsrc/Context/ContextConfig.phpsrc/Context/ContextDataProvider.phpsrc/Context/ContextEventHandler.phpsrc/Context/ContextEventLoggerEvent.phpsrc/Exception/RuntimeException.phpsrc/Experiment.phpsrc/Http/AsyncHttpClientInterface.phpsrc/Http/HTTPClient.phpsrc/Http/HttpClientInterface.phpsrc/Http/ReactHttpClient.phpsrc/JsonExpression/Operator/InOperator.phpsrc/JsonExpression/Operator/MatchOperator.phpsrc/SDK.phpsrc/VariableParser.phptests/ABsmartlyTest.phptests/AudienceMatcherTest.phptests/Client/ClientAsyncTest.phptests/Client/ClientConfigTest.phptests/Client/ClientInterfaceTest.phptests/Context/AsyncContextDataProviderTest.phptests/Context/AsyncContextEventHandlerTest.phptests/Context/ContextTest.phptests/Fixtures/json/context_custom_fields.jsontests/Fixtures/json/refreshed_iteration.jsontests/Http/HttpClientInterfaceTest.phptests/JsonExpression/Operator/InOperatorTest.phptests/MD5Test.phptests/Murmur3Test.phptests/VariantAssignerTest.php
…Error, and isFinalizing to Context
… messages Adds isFinalized() and finalize() as aliases for isClosed() and close() to align with JS SDK terminology. Standardizes error messages to include trailing periods matching JS SDK format.
…eady or finalized
Use lowercase 'ABsmartly' prefix and standard unit error formats with single quotes and trailing periods.
…ct wording - Match expected format: Unit "x" UID must not be blank (no trailing period) - Match expected format: Unit "x" UID is already set (was "already set.")
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/Client/Client.php (3)
82-85:⚠️ Potential issue | 🟠 MajorMissing authentication validation in
publish().Unlike
getContextData(), thepublish()method does not callauthRequest(), allowing requests with a blank API key to be sent. Contexts built from pre-fetched data can reachpublish()without ever validating credentials.Proposed fix
public function publish(PublishEvent $publishEvent): void { + $this->authRequest(); $data = $this->encode($publishEvent); $this->httpClient->put($this->url, $this->query, $this->headers, $data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 82 - 85, The publish() method in Client (which currently encodes PublishEvent and calls $this->httpClient->put($this->url, $this->query, $this->headers, $data)) lacks authentication validation; add a call to $this->authRequest() at the start of publish() (same pattern used in getContextData()) so that credentials are validated and requests with a blank API key are prevented before calling httpClient->put; ensure the method aborts or throws if authRequest() indicates invalid credentials so no publish is sent with missing auth.
71-80:⚠️ Potential issue | 🟠 MajorSynchronous exceptions break promise contract.
When
getContextDataAsync()falls back to sync mode (line 73), ifgetContextData()throws, the caller receives a synchronous exception rather than a rejected promise. Similarly,authRequest()on line 76 can throw before the promise chain starts.Proposed fix: wrap in try/catch
public function getContextDataAsync(): PromiseInterface { + try { if (!$this->httpClient instanceof AsyncHttpClientInterface) { return resolve($this->getContextData()); } $this->authRequest(); return $this->httpClient ->getAsync($this->url, $this->query, $this->headers) ->then(fn($response) => new ContextData($this->decode($response->content)->experiments)); + } catch (\Throwable $e) { + return \React\Promise\reject($e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 71 - 80, getContextDataAsync currently throws synchronously when getContextData() or authRequest() raises, breaking the PromiseInterface contract; wrap the synchronous path and the authRequest() call in a try/catch and return a rejected promise on exception instead of letting it bubble. Specifically, in getContextDataAsync surround the resolve($this->getContextData()) and the call to $this->authRequest() with try/catch and on catch return a rejected PromiseInterface (using the same promise helper family as resolve/reject used elsewhere) so callers always receive a promise that rejects on error; keep the existing httpClient->getAsync(...)->then(...) chain unchanged.
87-95:⚠️ Potential issue | 🟠 MajorSame promise contract issue in
publishAsync().Synchronous operations (
encode(),publish()) can throw before returning a promise. Wrap in try/catch for consistent error handling.Proposed fix
public function publishAsync(PublishEvent $publishEvent): PromiseInterface { + try { + $this->authRequest(); if (!$this->httpClient instanceof AsyncHttpClientInterface) { $this->publish($publishEvent); return resolve(null); } $data = $this->encode($publishEvent); return $this->httpClient->putAsync($this->url, $this->query, $this->headers, $data); + } catch (\Throwable $e) { + return \React\Promise\reject($e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 87 - 95, The publishAsync method can throw synchronously from encode() or publish(), violating the promise contract; wrap the whole body in a try/catch and return a rejected promise on error. Concretely, in publishAsync(PublishEvent $publishEvent): PromiseInterface wrap the logic (the instanceof AsyncHttpClientInterface check, the $this->publish($publishEvent) call, and the $data = $this->encode($publishEvent) + $this->httpClient->putAsync(...) call) in a try block and catch \Throwable $e to return reject($e) so callers always receive a PromiseInterface (use resolve(null) for the synchronous-success path and reject($e) on exceptions).src/Http/HTTPClient.php (1)
72-115:⚠️ Potential issue | 🟠 MajorPotential null throw when
retriesis 0.If
$this->retriesis0, thewhileloop never executes,$lastExceptionremainsnull, and line 114 throwsnull, causing aTypeErrorin PHP 8+. This is inconsistent withClientConfig::setRetries(0)being explicitly allowed (tested intestSetRetriesZeroIsAllowed).Proposed fix: ensure at least one attempt
private function fetchResponse(): Response { $attempt = 0; $lastException = null; + $maxAttempts = max(1, $this->retries); - while ($attempt < $this->retries) { + while ($attempt < $maxAttempts) { try { // ... existing code ... } catch (HttpClientError $e) { $lastException = $e; // ... - if (!$isRetryable || $attempt >= $this->retries - 1) { + if (!$isRetryable || $attempt >= $maxAttempts - 1) { throw $e; } // ... } } throw $lastException; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` around lines 72 - 115, fetchResponse can throw null when $this->retries is 0 because the while loop is skipped and $lastException remains null; change the loop to guarantee at least one attempt by computing $maxAttempts = max(1, $this->retries) and use while ($attempt < $maxAttempts) (or convert to a do/while) so fetchResponse always performs an HTTP attempt and $lastException will be set before throwing; update references to $this->retries in the backoff logging to still show the configured value if desired.
🧹 Nitpick comments (6)
src/Experiment.php (1)
34-40: Consider importing RuntimeException.The exception class is used with its fully qualified name. For consistency with the rest of the codebase, consider adding an import statement.
Suggested change
Add to imports at the top of the file:
use ABSmartly\SDK\Exception\RuntimeException;Then update the throw statement:
- throw new \ABSmartly\SDK\Exception\RuntimeException( + throw new RuntimeException(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Experiment.php` around lines 34 - 40, The throw uses the fully qualified \ABSmartly\SDK\Exception\RuntimeException; add a use statement for ABSmartly\SDK\Exception\RuntimeException at the top of the file and update the throw in the Experiment class (the foreach checking $requiredFields and the throw site) to throw RuntimeException instead of the FQN to match project import style.tests/Http/ReactHttpClientTest.php (1)
9-37: Consider expanding test coverage for ReactHttpClient.The current tests only cover the
flattenHeadershelper method. Consider adding tests for the core async HTTP methods (getAsync,putAsync) and error handling scenarios, even if they require mocking the ReactPHP Browser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Http/ReactHttpClientTest.php` around lines 9 - 37, Tests only cover flattenHeaders; add unit tests for ReactHttpClient's async methods and error paths: create tests for getAsync and putAsync on ReactHttpClient that mock the ReactPHP Browser and Response/RequestPromise to simulate successful responses (verify status, body handling and headers via flattenHeaders) and failing requests (simulate promise rejection to assert exception handling or error propagation). Use mocks/doubles for BrowserInterface/Browser, ResponseInterface and the promise returned by Browser->get()/put(), and reference the ReactHttpClient::getAsync, ReactHttpClient::putAsync, and flattenHeaders methods to locate where to call and assert behaviors; include cases for non-2xx responses, JSON bodies and empty bodies.src/Client/Client.php (1)
32-37: Property existence checks are fragile.Using
property_exists()to settimeoutandretriesrelies on implementation details of the HTTP client. Consider defining these inHttpClientInterfaceor using a configuration object pattern instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Client/Client.php` around lines 32 - 37, The current use of property_exists($this->httpClient, 'timeout') / 'retries' is fragile; change the design so the HTTP client exposes explicit methods (e.g. add setTimeout(int $seconds) and setRetries(int $count) to HttpClientInterface) and update Client (where $this->httpClient is used) to call $this->httpClient->setTimeout($clientConfig->getTimeout()) and ->setRetries($clientConfig->getRetries()) instead of manipulating properties; implement those methods on all concrete HTTP client classes (or alternatively accept a configuration object via a single setConfig(Config $cfg) method on the interface) and remove the property_exists checks from Client.php.tests/Context/ContextTest.php (1)
814-814: Consider usingassertEmptyorassertArrayNotHasKeyinstead ofproperty_existsfor clearer intent.Using
assertFalse(property_exists($event, 'exposures'), ...)works but PHPUnit's built-in assertions provide clearer failure messages. However, if the object structure genuinely uses dynamic properties, this approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Context/ContextTest.php` at line 814, Replace the assertion that currently uses property_exists($event, 'exposures') with a clearer PHPUnit assertion: if $event is expected to be an array or array-like, use assertArrayNotHasKey('exposures', (array)$event, ...) to assert the key is absent; if $event is an object that may have a public exposures property, use assertEmpty($event->exposures ?? null, ...) to assert there are no exposures. Locate the line containing property_exists($event, 'exposures') and swap it for the appropriate assertion based on whether $event is an array or object.src/ABsmartly.php (1)
71-112: Deprecated methodcreateWithDefaultsduplicates logic fromcreateSimple.Both factory methods contain nearly identical code. Consider having
createWithDefaultsdelegate tocreateSimplewith swapped parameters to reduce duplication and maintenance burden.Suggested refactor
public static function createWithDefaults( string $endpoint, string $apiKey, string $environment, string $application, int $retries = 5, int $timeout = 3000, ?callable $eventLogger = null ): ABsmartly { - $clientConfig = new ClientConfig( - $endpoint, - $apiKey, - $application, - $environment, - ); - $clientConfig->setRetries($retries); - $clientConfig->setTimeout($timeout); - - $client = new Client($clientConfig, new HTTPClient()); - $sdkConfig = new Config($client); - if ($eventLogger !== null) { - $sdkConfig->setContextEventLogger(new \ABSmartly\SDK\Context\ContextEventLoggerCallback($eventLogger)); - } - return new ABsmartly($sdkConfig); + return self::createSimple($endpoint, $apiKey, $application, $environment, $retries, $timeout, $eventLogger); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ABsmartly.php` around lines 71 - 112, The createWithDefaults method duplicates createSimple and uses the wrong parameter order for ClientConfig; refactor createWithDefaults to delegate to createSimple: call createSimple with parameters reordered to match createSimple's signature (endpoint, apiKey, application, environment, retries, timeout, eventLogger) so you keep the same defaults and behavior, including preserving setRetries/setTimeout and the ContextEventLoggerCallback handling inside createSimple (or ensure createSimple already performs those steps and simply return its result from createWithDefaults); update createWithDefaults to return the value from createSimple and remove the duplicated ClientConfig/Client/Config construction.tests/JsonExpression/Operator/MatchOperatorTest.php (1)
33-37: Nice coverage for literal~; add one modifier case to lock behaviour down.This test validates the tilde delimiter path well. I’d also add a
/.../iassertion so delimiter normalisation keeps parity for case-insensitive matches.Suggested test addition
public function testRegexWithTildeDelimiterInPattern(): void { self::assertTrue($this->operator->evaluate($this->evaluator, ["hello~world", "hello~world"])); self::assertTrue($this->operator->evaluate($this->evaluator, ["test~value", "~"])); self::assertFalse($this->operator->evaluate($this->evaluator, ["helloworld", "hello~world"])); + self::assertTrue($this->operator->evaluate($this->evaluator, ["ABC", "/abc/i"])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/JsonExpression/Operator/MatchOperatorTest.php` around lines 33 - 37, Add a case-insensitive modifier assertion to the testRegexWithTildeDelimiterInPattern test: use the existing test method (testRegexWithTildeDelimiterInPattern) and add an assertion calling $this->operator->evaluate($this->evaluator, ...) that verifies a pattern using tilde delimiters with the "i" modifier performs a case-insensitive match (e.g., different-cased subject should still return true); keep the same call shape as the other assertions so it exercises delimiter normalisation with modifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Context/Context.php`:
- Around line 115-118: Replace PHP 8-only string helpers with PHP 7.4-safe
equivalents: in src/Context/Context.php (the block using str_ends_with($k,
'_type')) swap the call to a substr_compare/substr-based check (or compare the
tail via substr and ===) so it behaves identically; in
src/JsonExpression/Operator/InOperator.php replace str_contains($haystack,
$needle) with a strpos($haystack, $needle) !== false check (or an equivalent
strict !== false test) to preserve semantics on PHP 7.4. Ensure both
replacements preserve exact boolean logic and handle edge cases (empty needle)
the same as the originals.
In `@src/JsonExpression/Operator/MatchOperator.php`:
- Around line 28-31: The current code trims leading/trailing slashes from
$pattern and then escapes tildes but fails to handle regex modifiers (e.g.
/abc/i) — detect when $pattern is slash-delimited with trailing modifiers, split
into the pattern body and modifier string (keep the modifiers separate), apply
escaping only to the body (e.g. escape '~' in the body), and then call
preg_match using the delimiter and append the modifiers outside the closing
delimiter (i.e. preg_match('~' . $body . '~' . $modifiers, $text)). Update the
logic around $pattern and the preg_match call in MatchOperator.php so that
slash-delimited modifiers are preserved and passed to preg_match.
In `@tests/Context/ContextTest.php`:
- Around line 1545-1564: The test method testAttributeUpdatesInTemplates assigns
$treatmentBefore and $treatmentAfter in
ContextTest::testAttributeUpdatesInTemplates but never asserts their values;
either remove these unused variables or add explicit assertions comparing them
(e.g., assertNotEquals or assertEquals depending on intended behavior) to verify
that getTreatment('exp_test_ab') changed (or stayed the same) after
setAttribute('age', 25) and publish(); update the test to assert the expected
relationship between $treatmentBefore and $treatmentAfter and keep the existing
event assertions.
---
Duplicate comments:
In `@src/Client/Client.php`:
- Around line 82-85: The publish() method in Client (which currently encodes
PublishEvent and calls $this->httpClient->put($this->url, $this->query,
$this->headers, $data)) lacks authentication validation; add a call to
$this->authRequest() at the start of publish() (same pattern used in
getContextData()) so that credentials are validated and requests with a blank
API key are prevented before calling httpClient->put; ensure the method aborts
or throws if authRequest() indicates invalid credentials so no publish is sent
with missing auth.
- Around line 71-80: getContextDataAsync currently throws synchronously when
getContextData() or authRequest() raises, breaking the PromiseInterface
contract; wrap the synchronous path and the authRequest() call in a try/catch
and return a rejected promise on exception instead of letting it bubble.
Specifically, in getContextDataAsync surround the
resolve($this->getContextData()) and the call to $this->authRequest() with
try/catch and on catch return a rejected PromiseInterface (using the same
promise helper family as resolve/reject used elsewhere) so callers always
receive a promise that rejects on error; keep the existing
httpClient->getAsync(...)->then(...) chain unchanged.
- Around line 87-95: The publishAsync method can throw synchronously from
encode() or publish(), violating the promise contract; wrap the whole body in a
try/catch and return a rejected promise on error. Concretely, in
publishAsync(PublishEvent $publishEvent): PromiseInterface wrap the logic (the
instanceof AsyncHttpClientInterface check, the $this->publish($publishEvent)
call, and the $data = $this->encode($publishEvent) +
$this->httpClient->putAsync(...) call) in a try block and catch \Throwable $e to
return reject($e) so callers always receive a PromiseInterface (use
resolve(null) for the synchronous-success path and reject($e) on exceptions).
In `@src/Http/HTTPClient.php`:
- Around line 72-115: fetchResponse can throw null when $this->retries is 0
because the while loop is skipped and $lastException remains null; change the
loop to guarantee at least one attempt by computing $maxAttempts = max(1,
$this->retries) and use while ($attempt < $maxAttempts) (or convert to a
do/while) so fetchResponse always performs an HTTP attempt and $lastException
will be set before throwing; update references to $this->retries in the backoff
logging to still show the configured value if desired.
---
Nitpick comments:
In `@src/ABsmartly.php`:
- Around line 71-112: The createWithDefaults method duplicates createSimple and
uses the wrong parameter order for ClientConfig; refactor createWithDefaults to
delegate to createSimple: call createSimple with parameters reordered to match
createSimple's signature (endpoint, apiKey, application, environment, retries,
timeout, eventLogger) so you keep the same defaults and behavior, including
preserving setRetries/setTimeout and the ContextEventLoggerCallback handling
inside createSimple (or ensure createSimple already performs those steps and
simply return its result from createWithDefaults); update createWithDefaults to
return the value from createSimple and remove the duplicated
ClientConfig/Client/Config construction.
In `@src/Client/Client.php`:
- Around line 32-37: The current use of property_exists($this->httpClient,
'timeout') / 'retries' is fragile; change the design so the HTTP client exposes
explicit methods (e.g. add setTimeout(int $seconds) and setRetries(int $count)
to HttpClientInterface) and update Client (where $this->httpClient is used) to
call $this->httpClient->setTimeout($clientConfig->getTimeout()) and
->setRetries($clientConfig->getRetries()) instead of manipulating properties;
implement those methods on all concrete HTTP client classes (or alternatively
accept a configuration object via a single setConfig(Config $cfg) method on the
interface) and remove the property_exists checks from Client.php.
In `@src/Experiment.php`:
- Around line 34-40: The throw uses the fully qualified
\ABSmartly\SDK\Exception\RuntimeException; add a use statement for
ABSmartly\SDK\Exception\RuntimeException at the top of the file and update the
throw in the Experiment class (the foreach checking $requiredFields and the
throw site) to throw RuntimeException instead of the FQN to match project import
style.
In `@tests/Context/ContextTest.php`:
- Line 814: Replace the assertion that currently uses property_exists($event,
'exposures') with a clearer PHPUnit assertion: if $event is expected to be an
array or array-like, use assertArrayNotHasKey('exposures', (array)$event, ...)
to assert the key is absent; if $event is an object that may have a public
exposures property, use assertEmpty($event->exposures ?? null, ...) to assert
there are no exposures. Locate the line containing property_exists($event,
'exposures') and swap it for the appropriate assertion based on whether $event
is an array or object.
In `@tests/Http/ReactHttpClientTest.php`:
- Around line 9-37: Tests only cover flattenHeaders; add unit tests for
ReactHttpClient's async methods and error paths: create tests for getAsync and
putAsync on ReactHttpClient that mock the ReactPHP Browser and
Response/RequestPromise to simulate successful responses (verify status, body
handling and headers via flattenHeaders) and failing requests (simulate promise
rejection to assert exception handling or error propagation). Use mocks/doubles
for BrowserInterface/Browser, ResponseInterface and the promise returned by
Browser->get()/put(), and reference the ReactHttpClient::getAsync,
ReactHttpClient::putAsync, and flattenHeaders methods to locate where to call
and assert behaviors; include cases for non-2xx responses, JSON bodies and empty
bodies.
In `@tests/JsonExpression/Operator/MatchOperatorTest.php`:
- Around line 33-37: Add a case-insensitive modifier assertion to the
testRegexWithTildeDelimiterInPattern test: use the existing test method
(testRegexWithTildeDelimiterInPattern) and add an assertion calling
$this->operator->evaluate($this->evaluator, ...) that verifies a pattern using
tilde delimiters with the "i" modifier performs a case-insensitive match (e.g.,
different-cased subject should still return true); keep the same call shape as
the other assertions so it exercises delimiter normalisation with modifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd579d2-dc34-46e4-ac0d-58a150e427df
📒 Files selected for processing (20)
src/ABsmartly.phpsrc/Client/Client.phpsrc/Client/ClientConfig.phpsrc/Config.phpsrc/Context/Context.phpsrc/Context/ContextEventHandler.phpsrc/Context/ContextPublisher.phpsrc/Experiment.phpsrc/Http/HTTPClient.phpsrc/Http/ReactHttpClient.phpsrc/JsonExpression/Operator/MatchOperator.phptests/ABsmartlyTest.phptests/Client/ClientConfigTest.phptests/Client/ClientTest.phptests/ConfigTest.phptests/Context/ContextTest.phptests/ExperimentTest.phptests/Fixtures/json/context_custom_fields.jsontests/Http/ReactHttpClientTest.phptests/JsonExpression/Operator/MatchOperatorTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Fixtures/json/context_custom_fields.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ABsmartlyTest.php
- src/Http/ReactHttpClient.php
- Replace str_ends_with() with substr() for PHP 7.4 compatibility - Fix null $lastException when retries=0 by ensuring at least one attempt - Remove duplicate test case in InOperatorTest
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Context/Context.php (1)
740-759:⚠️ Potential issue | 🟡 MinorUse the custom
RuntimeExceptionclass for consistency with the SDK.Line 751 uses the PHP built-in
\RuntimeException, but the SDK defines a customRuntimeExceptionclass insrc/Exception/RuntimeException.phpthat extends the built-in class and implementsABSmartlyException. Other exception classes in the SDK (TimeoutException, HttpClientError) properly import and use the custom exception. Adduse ABSmartly\SDK\Exception\RuntimeException;to the imports and changenew \RuntimeException($errorMsg)tonew RuntimeException($errorMsg)to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 740 - 759, In Context::flush(), replace the global PHP exception with the SDK's custom one: add an import for ABSmartly\SDK\Exception\RuntimeException and change the instantiation new \RuntimeException($errorMsg) to new RuntimeException($errorMsg) where ContextEventLoggerEvent is constructed so the SDK uses its custom RuntimeException class consistently with other exceptions like TimeoutException and HttpClientError.
♻️ Duplicate comments (1)
src/Context/Context.php (1)
815-818:⚠️ Potential issue | 🟠 MajorSuccessful refresh does not clear the failed state.
Similar to
setContextData, if the context was previously in a failed state, a successfulrefresh()call does not reset$this->failed. Subsequentflush()/close()calls will still discard data.🐛 Proposed fix
try { $data = $this->dataProvider->getContextData(); $this->setData($data); + $this->failed = false; + $this->readyError = null; $this->logEvent(ContextEventLoggerEvent::Refresh, $data); } catch (Exception $exception) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 815 - 818, refresh() currently sets data and logs success but does not clear the previous failed state, so later flush()/close() still discard data; update the refresh() implementation (the try block that calls $this->dataProvider->getContextData(), $this->setData($data), $this->logEvent(ContextEventLoggerEvent::Refresh, $data)) to reset $this->failed to false on successful refresh (mirroring setContextData behavior), ensuring subsequent flush() and close() treat the context as healthy.
🧹 Nitpick comments (2)
src/Http/HTTPClient.php (1)
104-110: Minor log message inconsistency with retry count.The log message uses
$this->retriesfor the denominator, but$maxAttemptsis used to control the loop. When$this->retries = 5, we getmaxAttempts = 5total attempts, meaning 4 retries. The log would show "attempt 4/5" on the last retry, suggesting a 5th retry exists when it doesn't.Consider using
$maxAttempts - 1for the retry count in the log message for accuracy.♻️ Proposed fix
error_log(sprintf( 'ABsmartly SDK: Retrying HTTP request (attempt %d/%d) after %dms due to error: %s', $attempt, - $this->retries, + $maxAttempts - 1, $backoffMs, $e->getMessage() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/HTTPClient.php` around lines 104 - 110, The retry log currently uses $this->retries as the denominator while the loop is controlled by $maxAttempts, causing an off-by-one message; update the error_log call in HTTPClient::(where the retry loop runs) to display the total retry attempts as ($maxAttempts - 1) instead of $this->retries (and guard so you never print a negative value by using max(0, $maxAttempts - 1)), keeping $attempt as the numerator so the message correctly reads "attempt X/Y" for retries.src/Context/Context.php (1)
124-142: Consider extracting helper for parsing customFieldValues.The logic for parsing
customFieldValues(handling string/object/array) is duplicated ingetCustomFieldKeys(),getCustomFieldValueType(), andcustomFieldValue(). A private helper would reduce repetition.♻️ Proposed helper extraction
private function parseCustomFieldValues($customFieldValues): ?array { if (is_string($customFieldValues)) { $customFieldValues = json_decode($customFieldValues, true) ?? []; } if (is_object($customFieldValues)) { $customFieldValues = get_object_vars($customFieldValues); } return is_array($customFieldValues) ? $customFieldValues : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Context/Context.php` around lines 124 - 142, Introduce a private helper (e.g., parseCustomFieldValues) that normalizes the incoming $customFieldValues (handles string -> json decode, object -> get_object_vars, and returns an array or null) and replace the duplicated parsing logic in getCustomFieldValueType, getCustomFieldKeys, and customFieldValue to call this helper; ensure each method returns early when the helper returns null and otherwise uses the normalized array to access keys like $key . '_type' or enumerated keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Context/Context.php`:
- Around line 740-759: In Context::flush(), replace the global PHP exception
with the SDK's custom one: add an import for
ABSmartly\SDK\Exception\RuntimeException and change the instantiation new
\RuntimeException($errorMsg) to new RuntimeException($errorMsg) where
ContextEventLoggerEvent is constructed so the SDK uses its custom
RuntimeException class consistently with other exceptions like TimeoutException
and HttpClientError.
---
Duplicate comments:
In `@src/Context/Context.php`:
- Around line 815-818: refresh() currently sets data and logs success but does
not clear the previous failed state, so later flush()/close() still discard
data; update the refresh() implementation (the try block that calls
$this->dataProvider->getContextData(), $this->setData($data),
$this->logEvent(ContextEventLoggerEvent::Refresh, $data)) to reset $this->failed
to false on successful refresh (mirroring setContextData behavior), ensuring
subsequent flush() and close() treat the context as healthy.
---
Nitpick comments:
In `@src/Context/Context.php`:
- Around line 124-142: Introduce a private helper (e.g., parseCustomFieldValues)
that normalizes the incoming $customFieldValues (handles string -> json decode,
object -> get_object_vars, and returns an array or null) and replace the
duplicated parsing logic in getCustomFieldValueType, getCustomFieldKeys, and
customFieldValue to call this helper; ensure each method returns early when the
helper returns null and otherwise uses the normalized array to access keys like
$key . '_type' or enumerated keys.
In `@src/Http/HTTPClient.php`:
- Around line 104-110: The retry log currently uses $this->retries as the
denominator while the loop is controlled by $maxAttempts, causing an off-by-one
message; update the error_log call in HTTPClient::(where the retry loop runs) to
display the total retry attempts as ($maxAttempts - 1) instead of $this->retries
(and guard so you never print a negative value by using max(0, $maxAttempts -
1)), keeping $attempt as the numerator so the message correctly reads "attempt
X/Y" for retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a83fcab-d27a-461a-b63a-ed4d5ddee0d5
📒 Files selected for processing (3)
src/Context/Context.phpsrc/Http/HTTPClient.phptests/JsonExpression/Operator/InOperatorTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/JsonExpression/Operator/InOperatorTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Config.php (1)
34-40: Delegate deprecated setter to the new setter.Line 37-Line 39 currently duplicates assignment logic. Routing through
setContextPublisher()keeps one update path and avoids future drift.♻️ Proposed refactor
/** * `@deprecated` Use setContextPublisher() instead. */ public function setContextEventHandler(ContextPublisher $contextEventHandler): Config { - $this->contextEventHandler = $contextEventHandler; - return $this; + return $this->setContextPublisher($contextEventHandler); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Config.php` around lines 34 - 40, The deprecated setter setContextEventHandler currently duplicates assignment logic; change it to delegate to the new setter by calling setContextPublisher($contextEventHandler) (or returning its result) instead of directly assigning $this->contextEventHandler, so all updates use the single code path in setContextPublisher and avoid future drift between setters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Config.php`:
- Around line 34-40: The deprecated setter setContextEventHandler currently
duplicates assignment logic; change it to delegate to the new setter by calling
setContextPublisher($contextEventHandler) (or returning its result) instead of
directly assigning $this->contextEventHandler, so all updates use the single
code path in setContextPublisher and avoid future drift between setters.
The IN operator is a CONTAINS check with haystack-first operand order ([haystack, needle]), matching the collector and production behavior. A prior change on this branch had swapped the operands to needle-first, diverging from the collector. Revert the operator and its tests to the canonical haystack-first order.
The collector returns customFieldValues as a JSON array of field-value objects,
but the property was typed ?object, causing a TypeError ("Cannot assign array to
property ... of type ?object") that failed every context creation against a live
backend. Match the canonical SDKs (java CustomFieldValue[], python list[...]).
BinaryOperator.evaluate now short-circuits to null when either operand is null, matching every other SDK and the collector. Previously PHP always invoked binary() even with null operands, making eq(null,null) true and the comparison operators evaluate null operands — a known deviation. Update the operator tests to expect null for null-operand cases.
The collector emits customFieldValues as an array of objects with name/type/ value keys. Rewrite getCustomFieldKeys/getCustomFieldValueType/customFieldValue to consume that shape directly instead of the old flat-object + _type-suffix form, and update the fixture to match. Fixes cross-SDK custom-field scenarios on the live-fetch path.
Acceptance criteria verified satisfied at ab65e08 in round 11; no code blocker survives. Exact-head CI approval deferred in review 5632663303.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
✅ Approved
Exact-head CI has finished green, as confirmed by the orchestrator, and no code blocker remains. This submits the verdict withheld in the round 11 review.
Next step: None — ready to merge.
Review details
Head: ab65e08e54792f729f0dea809387d76a60fc08e4. Base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd.
The head, base, PR description and substantive discussion are unchanged since the deferred review. Its findings, census and non-blocking suggestions remain unchanged; this is only the CI-settlement verdict, not a new review.
The orchestrator explicitly confirms that CI for this exact head finished green. The accessible Actions API still exposes zero runs and the commit-status API only exposes CodeRabbit's “Review paused”; those API results are not being presented as test evidence. The orchestrator's exact-head settlement supplies the previously missing CI evidence.
Prior local validation passed: 394 tests / 1,256 assertions, public Context equality/audience probes, Composer validation, changed runtime PHP syntax and diff whitespace checks. All earlier blocking acceptance criteria are satisfied, with no remaining material limitation.
New commits were added after this approval. Jervasion will reassess the updated head.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
✅ Approved
The Windows-compatible local test-server startup is sound, and the full PHP/platform CI matrix passes on this exact head. No blocking regression was introduced since the earlier approval.
Next step: None — ready to merge.
What I checked
- ✅ Default suite: 394 tests, 1,256 assertions passed locally on PHP 8.5.4.
- ✅ Focused real-HTTP integration: 1 test, 25 assertions passed, covering context GET and event PUT through the public SDK.
- ✅ Exact-head CI: all 10 matrix jobs passed across Ubuntu and Windows with PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ Composer validation, changed-file PHP syntax and diff whitespace checks passed.
Review details
Head: 20863566aaf1557492cc03823235dc0cbb83bce3. Base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 634f8bd6a3e590ebb9ccec163348ae64276949a3 (approval PRR_kwDOFYHMjc8AAAABT75v2A). Responsibility and all counts use approved..head.
Intent and impact: the sole intervening commit changes tests/Http/LocalServerIntegrationTest.php: array-form proc_open avoids a Windows command-shell wrapper, getenv supplies the inherited environment including SystemRoot, and the bounded readiness allowance increases from 5 to 15 seconds. No SDK runtime, public contract, dependency, assertion or test-selection change is introduced. These are verified from the delta; passing Windows CI independently validates portability. The original SDK parity, async transport, lifecycle and equality changes remain accepted. No rollout flag applies to this test-harness-only delta. No applicable AGENTS.md or CLAUDE.md was found.
Read the PR description, commits, top-level discussion, reviews and inline comments/replies. Earlier blocking criteria were satisfied before the accepted baseline and their implementations are unchanged. The author reports unit and cross-SDK results; the external cross-SDK suite was not rerun for this test-process-only change.
Validation:
composer install --no-interaction --prefer-dist: passed; the repository has no committed lockfile, so dependencies were resolved locally.composer test -- --do-not-cache-result --no-coverage: passed, 394 tests / 1,256 assertions.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/Http/LocalServerIntegrationTest.php: passed, 1 test / 25 assertions.composer validate --no-check-publish,php -l tests/Http/LocalServerIntegrationTest.php, andgit diff --check 634f8bd6a3e590ebb9ccec163348ae64276949a3 HEAD: passed.- Exact-head Actions runs
40481940013and40481932489both completed successfully. Latest-run jobs and their install/test steps all passed. Commit statuses contain no entries, not a pending named check. CodeRabbit is explicitly paused, not additional test evidence. Windows execution was validated through CI rather than local emulation. - The pinned introduced-content helper completed successfully over approved..head and enumerated one content write: this test file in commit
20863566aaf1557492cc03823235dc0cbb83bce3. Its content was inspected.
Passes (non-blocking): run: tests/CI/packaging/end-to-end harnesses (the only changed file starts the default-suite HTTP server), cross-cutting simplicity, comment hygiene, diff composition | not run: core runtime/API/security/data/error paths (no SDK executable source changed), deployment/operations/observability (no deployed process, manifest, migration or telemetry changed), shared-component blast radius and interaction complexity (no frontend source changed), behaviour-bearing data and policy (no consumed configuration, policy, schema or fixture changed). Selected passes were conducted locally without delegation because this is one small test-harness hunk.
Comment hygiene (non-blocking): 2 comment / 3 code lines, ratio ~1:1.5, 0 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
None flagged: the two-line comment records a non-obvious Windows shell/environment constraint and warrants preservation.
Diff composition (non-blocking): every file is required for the change; none flagged. No credential-bearing content or unnecessary artifact was found in introduced content.
Simplifications (non-blocking): none. Direct argv removes shell-escaping machinery; environment inheritance and the bounded readiness wait serve the portability requirement.
Convergence summary — review round 12
- Unresolved original blockers: 0
- Remediation regressions: 0
- Independent PR-change findings: 0
- Acceptance criteria still open: none
- Human adjudication requested: no
…ith the JavaScript SDK - Units keep their string or number type; numeric units hash like JS (rounded integer). - Unit error messages match the other SDKs. - track() raises "Goal '<name>' properties must be of type object." for non-object properties. - A rejected context fetch leaves the context ready and failed with an error event. - close() rethrows a failed final publish and stays open with events queued. - Add Utils helpers shared by hashing and diagnostics.
|
Aligned more behavior with the JavaScript SDK so the cross-SDK tests no longer depend on wrapper workarounds:
These are breaking changes and are listed in CHANGELOG under 2.0.0. Regression tests are in tests/Context/ContextConformanceTest.php. |
New commits were added after this approval. Jervasion will reassess the updated head.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
❌ Changes requested
The new failure/retry transitions work and CI passes, but two findings remain: numeric hashing disagrees with JavaScript for valid float UIDs, and the new hashing test passes even when hashing is replaced by a constant.
Next step: Correct numeric UID serialization and make the new hash test reject incorrect hashing.
Must fix
- Match JavaScript numeric UID serialization — otherwise the same unit can receive different treatments and publish a different identity across SDKs.
src/Utils.php:52-54 - Make the rounding/hash test reject a constant hash — its current assertions do not establish the claimed behavior.
tests/Context/ContextConformanceTest.php:92-95
Suggestions
- [nit] Equal integer/float UIDs can no longer be re-set interchangeably; see inline note.
- One local simplification and three short comment blocks to tidy are listed below; these are non-blocking.
What I checked
- ✅ Default suite: 405 tests, 1,311 assertions passed on PHP 8.5.4. Focused conformance suite: 11 tests, 49 assertions passed.
- ✅ All ten exact-head Ubuntu/Windows CI jobs pass for PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ Failed async loads resolve ready/failed contexts; failed final publication remains open and retryable. A separate synchronous-provider pending-load probe also passed.
- ❌ Public Context and publication probes reproduce the numeric identity mismatch. The constant-hash mutation leaves the named rounding test green.
- ✅ Composer validation, changed runtime PHP syntax and whitespace checks pass.
Review details
Head: b5dcab9a4d00f65e6e269649e7e3c5e153a7fc6e. Base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 20863566aaf1557492cc03823235dc0cbb83bce3 (approval PRR_kwDOFYHMjc8AAAABUalDrQ). Responsibility, comment counts and composition audit use approved..head. No unchanged approved behavior is reopened.
Intent and inventory: the author's new top-level comment and CHANGELOG explicitly select breaking v2 JavaScript parity for retained numeric unit types, rounding/hash semantics, unit errors, goal-property validation, failed async loads resolving to failed contexts, and close() propagating final-publish errors while preserving retryability. The delta changes six runtime files, adds a conformance test, modifies two existing test files, and updates the changelog. The Utils helper also changes assignment hashing and emitted unit identities. No rollout flag is involved. Actual customer adoption is not asserted; findings concern reachable public SDK APIs and the explicit numeric contract. Prior blocking criteria remain satisfied and untouched. No applicable AGENTS.md/CLAUDE.md was found. The PR description, commits, discussion and review/thread history were inspected.
Evidence classification: serialization outputs, changed treatments/wire hashes, repeated-numeric setter rejection, suite results and mutant survival are directly verified facts. No finding depends on assumed production deployment or customer adoption. JavaScript reference absmartly/javascript-sdk/src/utils.ts implements numeric hash input with value.toFixed(0); Node directly confirms the cited outputs.
Validation:
composer install --no-interaction --prefer-dist: passed; no committed lockfile exists, so dependencies were resolved locally.composer test -- --do-not-cache-result --no-coverage: 405 tests / 1,311 assertions, passed.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/Context/ContextConformanceTest.php: 11 tests / 49 assertions, passed.composer validate --no-check-publish, php -l on all six changed runtime files, andgit diff --check 20863566aaf1557492cc03823235dc0cbb83bce3 HEAD: passed. No separate formatter/typechecker gate is declared.- Exact-head Actions runs
40500038006and40500030552completed successfully; all ten jobs in each passed. No named commit statuses exist. CodeRabbit is paused, not test evidence. - A public Context probe uses split [0.5,0.5], trafficSplit [0,1], seedLo=0, fullOnVariant=0. At seedHi=1 numeric -0.1 returns treatment 0 versus treatment 1 for its required serialized string '-0'; at seedHi=2 numeric 1e21 returns 0 versus 1 for '1e+21'. All contexts remain healthy. PublishEvent produces the corresponding different unit hashes.
- In a separate scratch tree, replacing only Utils::hashUnit's return with 'constant' changes hashUnit(125.75) from BpBZt--EDwx0qBTskje27A to constant. Running
vendor/bin/phpunit --do-not-cache-result --no-coverage --filter testNumericUnitHashesLikeItsRoundedIntegerString tests/Context/ContextConformanceTest.phpunmodified still passes (1 test, 3 assertions). - The pinned introduced-content helper succeeded and enumerated all ten content writes in the one intervening commit. All postimages were inspected; no transient hidden artifact or secret was found.
- The author's reported external cross-SDK suite was not rerun; direct JavaScript/PHP and public API probes supply the specific cross-SDK counterexamples.
Passes (non-blocking): run: core runtime/API/security/data/error paths (units, hashing, goal validation and failed-context/finalization transitions), tests/CI/packaging/end-to-end harnesses (new/modified tests and contract sensitivity), deployment/operations/observability (new failed-load Error emission and final-publish recovery), cross-cutting simplicity, comment hygiene, diff composition | not run: shared-component blast radius and interaction complexity (no frontend source changed), behaviour-bearing data and policy (no consumed config, fixture, schema or policy changed). Two independent read-only agents reviewed runtime behavior and tests/simplicity; primary verified findings and performed mutation/reproduction.
Comment hygiene (non-blocking): 36 comment / 323 code lines, ratio ~1:9.0, 3 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
promotion declined: these five qualifying lines are short, accurate, harmless local explanations rather than a pattern warranting a merge block.
- src/Context/Context.php:253-254: restates failed-data transition and logging (2 lines).
- src/Context/Context.php:899-900: restates flush preservation and retry behavior (2 lines).
- tests/Context/ContextConfigTest.php:18: appended comment restates numeric type retention (1 line).
Counts exclude pure comment delimiters, include three empty documentation-content lines, and count the appended comment line as both comment and code. Type annotations and external JavaScript/PHP constraints are preserved.
Diff composition (non-blocking): every file is required for the change; none flagged.
Simplifications (non-blocking): 1 candidate
- The new asynchronous callback pipeline in createContextAsync duplicates createContextPending's construction and completion transitions. Smaller alternative: retain the existing synchronous-provider branch, then delegate the async branch to createContextPending($contextConfig)['promise']. Physical LOC delta: -12. This is a local suggestion, not a demonstrated measured simplification; no equivalent scratch alternative or threshold claim is made.
Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.
Convergence summary — review round 13
- Unresolved original blockers: 0
- Remediation regressions: 0
- Independent PR-change findings: 2
- Acceptance criteria still open: numeric unit hashing matches JavaScript serialization at the demonstrated boundaries; the named hash test rejects the demonstrated constant-hash mutation or another test pins its claim under that mutation.
- Human adjudication requested: no — both findings are introduced by this independent post-approval commit, not repeated failed remediation.
…al numbers as the same unit
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
❌ Changes requested
The updated hash test is fixed and all ten CI jobs pass, but two numeric-identity issues remain: large-float hashes depend on PHP configuration, and distinct large integer UIDs can now be silently treated as the same unit.
Next step: Make numeric serialization deterministic and preserve the distinction between integer IDs that hash differently.
Must fix
- Keep large-number hashes independent of PHP precision settings — the earlier JavaScript-parity requirement remains partially unmet; evidence below continues the existing finding.
- Do not collapse distinct integer UIDs during reassignment — the new numeric equality check can retain the wrong user identity without an error.
src/Context/Context.php:751
Suggestions
- One local simplification and three short comment blocks remain non-blocking; see details.
What I checked
- ✅ Default suite: 407 tests, 1,325 assertions passed; focused conformance suite: 13 tests, 63 assertions passed.
- ✅ All ten exact-head CI jobs pass across Ubuntu/Windows and PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ The revised numeric-hash test now fails against the earlier constant-hash mutation. Failed-load and failed-finalize recovery tests pass.
- ❌ Public setter and publication probes reproduce both numeric-identity issues.
- ✅ Composer validation, all six changed runtime files' syntax checks, and whitespace checks pass.
Remaining evidence for the earlier hashing finding
[P2] Keep numeric UID serialization independent of PHP configuration
What goes wrong: For the same numeric unit 1e23, serialize_precision=-1 produces 1e+23 and published UID C2kIOZMVyzHIQ9hfVjhzvw; serialize_precision=17 produces 9.9999999999999992e+22 and UID WB9NH-OxMc2fZFeY627_XA. JavaScript (1e23).toFixed(0) is 1e+23.
Why: The new var_export($abs, true) at src/Utils.php:85-91 inherits PHP's serialization precision. Public numeric setUnit → VariantAssigner → Utils::hashUnit and PublishEvent::setUnits → the same formatter therefore depend on an unrelated PHP setting. I directly reproduced both emitted hashes with unchanged production code. No installation-specific setting is required by the SDK contract.
Fix / acceptance criterion: Preserve the earlier requirement that numeric UID serialization match JavaScript toFixed(0), including supported large numbers, independently of ambient serialization precision. Silent assignment/accounting divergence for the same UID outweighs a local deterministic-serialization correction.
Lineage: Unresolved original blocker from round 13; the cited negative-zero and 1e21 examples now pass, but the large-number invariant is not fully satisfied. This is additional evidence for that existing finding, not a duplicate inline thread.
Review details
Head: b2d1b9a3a12aa302b28ed2bbd1f34493b82b2868. Supplied base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 20863566aaf1557492cc03823235dc0cbb83bce3 (approval PRR_kwDOFYHMjc8AAAABUalDrQ). Responsibility and all counts use approved..head. The cumulative PR, commits, description, five top-level comments, reviews and all inline threads/replies were re-read; unchanged approved behavior is not reopened. No applicable AGENTS.md or CLAUDE.md exists.
Intent and actual impact: breaking v2 JavaScript parity for retained numeric units, numeric hashing, goal validation, failed async loads resolving with ready/failed contexts, and retryable failed final publication. The delta changes six runtime files, three test files, and CHANGELOG. Assignment hashing and published unit identities share Utils. The author reports unit and 202 cross-SDK scenarios; no rollout flag applies. The latest commit specifically remediates round-13 serialization, constant-hash sensitivity and equal int/float setters.
Evidence classification: PHP serialization, emitted hashes, public setter behavior, Node output, mutation failure and test/CI outcomes are verified facts. No blocker depends on inferred customer adoption or deployment. The external cross-SDK suite was not independently rerun; focused PHP/Node probes directly assess the disputed numeric contract.
Validation:
composer install --no-interaction --prefer-dist: passed; no committed lockfile, so dependencies resolved locally.composer test -- --do-not-cache-result --no-coverage: 407 tests / 1,325 assertions, passed on PHP 8.5.4.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/Context/ContextConformanceTest.php: 13 tests / 63 assertions, passed.composer validate --no-check-publish, php -l on the six runtime files, andgit diff --check 20863566 HEAD: passed. No separate formatter or static-check gate is declared.- Exact-head latest Actions run
40501770999: completed success; all ten jobs completed success. Earlier run40501764003also completed success. Commit statuses contain no named results (an empty aggregate's pending label is not a pending check). CodeRabbit is paused, not a missing material validation. - Constant mutation: scratch Utils::hashUnit returns 'constant'. Loading that class before the unchanged autoloader, then running
vendor/bin/phpunit --bootstrap <scratch-bootstrap> --do-not-cache-result --no-coverage --filter testNumericUnitHashesLikeItsRoundedIntegerString tests/Context/ContextConformanceTest.phpfails at the first hardcoded expected hash (1 test / 1 assertion / 1 failure). The old test blocker is satisfied. - Public probes construct SDK/Context with ContextConfig and empty ContextData, then call setUnit twice; no reflection, network or hypothetical caller is required. PublishEvent::setUnits directly confirms the precision-dependent wire identity.
- The pinned introduced_blobs.py helper succeeded, enumerating all 13 content writes across the two post-approval commits. Their postimages were inspected, including the superseded Utils and conformance test; no hidden transient artifact or credential found.
Passes (non-blocking): run: core runtime/API/security/data/error paths (numeric identity, goal validation, failed-load/finalize transitions), tests/CI/packaging/end-to-end harnesses (conformance tests, mutation sensitivity and declared matrix), deployment/operations/observability (changed failed-load Error emission and final-publication recovery), cross-cutting simplicity, comment hygiene, diff composition | not run: shared-component blast radius and interaction complexity (no frontend source changed), behaviour-bearing data and policy (no consumed config, fixture, schema or policy changed). Independent read-only agents covered runtime and tests/simplicity; primary reproduced and judged candidates.
Comment hygiene (non-blocking): 45 comment / 385 code lines, ratio ~1:8.6, 3 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
promotion declined: these unchanged five qualifying lines remain short, accurate, harmless local explanations under the standing round-13 decline; no new qualifying line was identified.
- src/Context/Context.php:253-254: restates failed-data transition and logging (2 lines).
- src/Context/Context.php:917-918: restates flush preservation and retry behavior (2 lines).
- tests/Context/ContextConfigTest.php:18: appended comment restates numeric type retention (1 line).
JavaScript numeric constraints, type annotations and expected-hash provenance are preserved; pure comment delimiters are excluded and empty documentation-content lines included.
Diff composition (non-blocking): every file is required for the change; none flagged.
Simplifications (non-blocking): 1 candidate
- createContextAsync duplicates createContextPending's async construction/completion pipeline. Smaller alternative: retain the existing synchronous-provider branch and delegate the async branch to createContextPending($contextConfig)['promise']. Physical LOC delta: -12. No equivalent scratch alternative or measured-threshold claim is made.
Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.
Convergence summary — review round 14
- Unresolved original blockers: 1
- Remediation regressions: 1
- Independent PR-change findings: 0
- Acceptance criteria still open: numeric serialization matches JavaScript independently of PHP serialization precision; distinct integer UIDs that hash differently cannot silently pass the immutable-unit equality check.
- Human adjudication requested: no — one remediation attempt since round 13, not two consecutive unsuccessful rounds or two consecutive rounds introducing remediation regressions.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
❌ Changes requested
The large-integer identity fix works and all ten CI jobs pass. One earlier blocker remains: the same numeric UID still hashes differently depending on PHP's serialize_precision setting, so assignment and published identity can disagree across hosts.
Next step: Make numeric UID serialization match JavaScript independently of PHP serialization precision.
Must fix
- Finish the numeric hashing correction — the configuration-dependent large-number case from the existing finding remains unchanged at
src/Utils.php:85. Details below continue that thread rather than opening a duplicate.
Suggestions
- Two non-blocking simplification candidates and three short comment blocks are listed in the details; none requires another review round.
What I checked
- ✅ Default suite: 408 tests, 1,326 assertions passed; focused conformance suite: 14 tests, 64 assertions passed.
- ✅ All ten exact-head CI jobs pass across Ubuntu/Windows and PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ Distinct large integer UIDs now conflict; equivalent
125/125.0remain accepted, and string/number identities remain distinct. - ❌ Direct publication probes still produce different UID hashes for
1e23under serialization precision-1versus17. - ✅ Composer validation, syntax checks for all six changed runtime files, and diff whitespace checks pass.
Remaining evidence for the earlier hashing finding
[P2] Keep numeric UID serialization independent of PHP configuration
What goes wrong: With UID 1e23, precision -1 produces the string 1e+23 and published UID C2kIOZMVyzHIQ9hfVjhzvw; precision 17 produces 9.9999999999999992e+22 and published UID WB9NH-OxMc2fZFeY627_XA. JavaScript (1e23).toFixed(0) returns 1e+23. These results were reproduced against this exact head.
Why: var_export($abs, true) at src/Utils.php:85 still inherits application serialization precision. Public numeric units reach this formatter both through Context → VariantAssigner → Utils::hashUnit and through PublishEvent::setUnits → Utils::hashUnit. The latest commit changes the immutable-unit comparison, not this formatter.
Fix / acceptance criterion: Supported numeric UID serialization must match JavaScript's toFixed(0), including large numbers, independently of ambient serialization precision; assignment and publication must share that identity. Silent cross-host assignment/accounting divergence outweighs a local deterministic-formatting correction.
Lineage: Unresolved original blocker from round 13, with the remaining configuration-dependent case established in round 14. No new inline finding is added.
Review details
Head: 83b005fb952eed4370ba99e6c1cf423b459abce6. Supplied base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 20863566aaf1557492cc03823235dc0cbb83bce3 (approval PRR_kwDOFYHMjc8AAAABUalDrQ). Responsibility and all census counts use approved..head; unchanged approved behavior is not reopened. The cumulative changed-file inventory, current description, commits, top-level discussion, previous reviews and inline replies were re-read. No applicable AGENTS.md or CLAUDE.md was found.
Intent and actual impact: breaking v2 JavaScript parity for numeric units and hashing, goal-property validation, failed async loads resolving with ready/failed contexts, and retryable failed final publication. The post-approval delta changes six runtime files, three test files and CHANGELOG. Assignment hashing and wire unit identities share Utils. The latest commit specifically preserves distinct integer identities and guards mixed numeric equality with the serialized identity. The owner reports unit and 202 cross-SDK scenarios; the external cross-SDK harness was not independently rerun. No rollout flag applies.
Evidence classification: formatter output, emitted hashes, JavaScript output, public setter behavior and test/CI outcomes are verified facts. The surviving finding requires no assumption about a particular customer's adoption or deployment.
Validation:
composer install --no-interaction --prefer-dist: passed; the repository has no committed lockfile, so dependencies resolved locally.composer test -- --do-not-cache-result --no-coverage: passed on PHP 8.5.4, 408 tests / 1,326 assertions, including local HTTP, retry/cancellation and lifecycle coverage.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/Context/ContextConformanceTest.php: passed, 14 tests / 64 assertions. Covers failed async loads, failed final publication/retry, goal validation and numeric-unit behavior. Independent test review also ran this unchanged file under serialization precision 14 and 17; both remained green, so the existing examples do not detect the demonstrated boundary defect. This is not asserted as a separate vacuous-test finding.composer validate --no-check-publish,php -lon ABsmartly, Context, ContextConfig, PublishEvent, Utils and VariantAssigner, andgit diff --check 20863566 HEAD: passed. No separate formatter/static-analysis gate is declared.- Direct probe calls Utils::unitToString/hashUnit and PublishEvent::setUnits with the same
1e23under precision -1, 17 and 14; Node independently supplies the JavaScript result above. - Latest exact-head Actions run
40504768180: completed success, all ten matrix jobs completed success. Earlier run40504761752also completed success. Commit statuses contain no named results; the empty aggregate's pending label is not a pending check. CodeRabbit is paused and is not missing material test evidence. - Pinned
introduced_blobs.pycompleted successfully, enumerating 15 content writes across the three post-approval commits. Head files and superseded postimages were inspected: no credential or transient scratch/build artifact found. - The PR head/base were re-fetched before submission and remain the supplied pair.
Passes (non-blocking): run: core runtime/API/security/data/error paths (numeric identity, goal validation, load/finalize transitions), tests/CI/packaging/end-to-end harnesses (conformance coverage and exact-head matrix), deployment/operations/observability (changed failed-load Error emission and final-publication recovery), cross-cutting simplicity, comment hygiene, diff composition | not run: shared-component blast radius and interaction complexity (no frontend source changed), behaviour-bearing data and policy (no consumed configuration, fixture, schema or policy changed after approval). Independent read-only agents covered runtime, tests/packaging, and simplicity; primary reproduced the surviving blocker and owns the verdict.
Comment hygiene (non-blocking): 46 comment / 394 code lines, ratio ~1:8.6, 3 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
promotion declined: the unchanged five qualifying lines remain short, accurate, harmless local explanations under the standing round-13 decline; no new qualifying lines were identified.
- src/Context/Context.php:253-254: restates the failed-data transition and logging (2 lines).
- src/Context/Context.php:922-923: restates flush preservation and retry behavior (2 lines).
- tests/Context/ContextConfigTest.php:18: appended restatement of numeric-type retention (1 line).
JavaScript numeric constraints, type annotations and expected-hash provenance are preserved. Comment delimiters are excluded; empty documentation-content lines are included.
Diff composition (non-blocking): every file is required for the change; none flagged.
Simplifications (non-blocking): 2 candidates
- ABsmartly::createContextAsync duplicates the async construction/completion pipeline in createContextPending. Smaller alternative: keep its synchronous-provider branch and delegate only its async branch to createContextPending($contextConfig)['promise']. Physical LOC delta: -12. Retained prior non-blocking candidate; no demonstrated equivalent scratch alternative or measured-threshold claim.
- Context::sameUnit can keep the strict string branch and combine numeric value equality with unitToString equality, removing its separate integer/integer branch and explicit float casts. Physical LOC delta: approximately -3, depending on wrapping. Independent comparison matched on 484 boundary/value pairs. This is optional readability feedback, not a tested replacement or blocker.
Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.
Convergence summary — review round 15
- Unresolved original blockers: 1
- Remediation regressions: 0
- Independent PR-change findings: 0
- Acceptance criteria still open: numeric serialization matches JavaScript independently of PHP serialization precision.
- Human adjudication requested: no — this commit successfully fixes the separate large-integer regression; it does not attempt another correction to the remaining formatter case.
All acceptance criteria from this review are verified satisfied at 9abb2dd; numeric serialization, identity preservation and the constant-hash regression test pass.
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
✅ Approved with suggestions
The numeric-identity fixes now satisfy the earlier review criteria: hashing is independent of PHP serialization precision, distinct large integers remain distinct, and the hashing regression test rejects a constant implementation. All ten exact-head CI jobs pass; no blocking issue remains.
Next step: None — ready to merge.
Suggestions
- One optional deduplication and three short comment blocks remain in the details; none requires another review round.
What I checked
- ✅ Default suite: 409 tests, 1,335 assertions passed on PHP 8.5.4, including real local HTTP, retry/cancellation and lifecycle coverage.
- ✅ Focused conformance suite: 15 tests, 73 assertions passed, including precision restoration, failed loads, goal validation and retryable failed final publication.
- ✅ All ten exact-head Ubuntu/Windows CI jobs pass across PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ Numeric formatting matches JavaScript at the previously failing boundaries; the earlier constant-hash mutation now makes the named test fail.
- ✅ Composer validation, syntax checks and diff whitespace checks pass.
Review details
Head: 9abb2ddb92d90c78539ee6ee0277890bb9e97565. Supplied base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 20863566aaf1557492cc03823235dc0cbb83bce3 (approval PRR_kwDOFYHMjc8AAAABUalDrQ). Responsibility and counts use approved..head, without reopening unchanged approved behavior. Review round 16.
Re-read the current cumulative changes, commits, description, top-level discussion, reviews and inline replies. No applicable AGENTS.md or CLAUDE.md exists. The owner explicitly selects breaking v2 JavaScript parity: typed numeric units, goal-property validation, rejected loads resolving ready/failed contexts, and retryable final-publication failure. The delta touches six runtime files, three test files and CHANGELOG; no rollout flag applies. Assignment and publication share Utils hashing. The original PR additionally covers operators, facade/configuration, async transport and lifecycle behavior already accepted at the baseline. The author reports unit and 202 cross-SDK scenarios; that external harness was not independently rerun, and is not claimed as executed evidence here.
Earlier acceptance criteria are satisfied: -0.1 serializes as -0; 1e21 uses exponent form; 1e23 serializes as 1e+23 and hashes to C2kIOZMVyzHIQ9hfVjhzvw under serialize_precision 17, 14 and -1, restoring the caller's setting. Equivalent 125/125.0 are accepted, strings remain distinct from numbers, and distinct integers beyond floating-point precision conflict. The production constant-hash mutation now fails testNumericUnitHashesLikeItsRoundedIntegerString at its hardcoded expected hash. No unresolved original blocker or remediation regression survives.
Verification and evidence:
composer install --no-interaction --prefer-dist: passed; there is no committed lockfile, so dependencies resolved locally.composer test -- --do-not-cache-result --no-coverage: passed, 409 tests / 1,335 assertions.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/Context/ContextConformanceTest.php: passed, 15 tests / 73 assertions.composer validate --no-check-publish,php -lon all nine changed PHP files, andgit diff --check 20863566 HEAD: passed. No separate formatter or static-analysis gate is declared.- Independent runtime pass compared 199,894 finite doubles against Node 22 Number.prototype.toFixed(0): zero mismatches. Direct setter and lifecycle probes confirmed identity conflicts, ready/failed loads with error logging, and failed-close queue retention followed by successful retry. These are observed checks, not deployment/adoption assumptions.
- In a detached scratch tree, only Utils::hashUnit was changed to return
constant. The unmodified named hash test was run with the mutant production class preloaded and failed at its expected hash assertion (1 test, 1 assertion, 1 failure). This clears the earlier test acceptance criterion. - Exact-head Actions runs
40508415104and40508407652completed successfully; all ten jobs in each completed successfully. The latest run supplies matrix evidence. Commit statuses have no named results; the empty aggregate's pending label is not a pending check. CodeRabbit is paused, not missing material test evidence. - Pinned introduced_blobs.py completed successfully over approved..head, enumerating 18 content writes across four commits. Current files and superseded postimages were inspected; no credentials or unnecessary artifacts found.
- The PR head/base and new discussion were rechecked before publication and remain the supplied pair.
Passes (non-blocking): run: core runtime/API/security/data/error paths (numeric identity and load/finalize transitions), tests/CI/packaging/end-to-end harnesses (conformance tests and platform matrix), deployment/operations/observability (changed failed-load Error emission and final-publication recovery), cross-cutting simplicity, comment hygiene, diff composition | not run: shared-component blast radius and interaction complexity (no frontend source changed), behaviour-bearing data and policy (no consumed fixture, configuration, schema or policy changed after approval). Independent read-only agents covered runtime/simplicity and tests/packaging; primary verified the prior criteria and owns the verdict.
Comment hygiene (non-blocking): 48 comment / 410 code lines, ratio ~1:8.5, 3 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
promotion declined: the five unchanged qualifying lines remain short, accurate, harmless explanations covered by the standing round-13 decline; no new qualifying lines were added.
- src/Context/Context.php:253-254 — restates the failed-data transition and logging (2 lines).
- src/Context/Context.php:918-919 — restates flush preservation and retry behavior (2 lines).
- tests/Context/ContextConfigTest.php:18 — appended restatement of numeric-type retention (1 line).
JavaScript constraints, precision-independent round-trip rationale, type annotations and expected-hash provenance are preserved. Comment delimiters are excluded; empty documentation-content lines are included.
Diff composition (non-blocking): every file is required for the change; none flagged.
Simplifications (non-blocking): 1 candidate
- ABsmartly::createContextAsync repeats createContextPending's async construction and settlement pipeline. Smaller alternative: retain its synchronous-provider branch and delegate only its async branch to createContextPending($contextConfig)['promise']. Physical LOC delta: -12. This is the existing optional candidate; no scratch-equivalence or measured-threshold claim is made. The previous sameUnit simplification has been applied.
Run a simplification pass locally before pushing: these candidates cost less to address now than as a further review round.
Convergence summary — review round 16
- Unresolved original blockers: 0
- Remediation regressions: 0
- Independent PR-change findings: 0
- Acceptance criteria still open: none
- Human adjudication requested: no — all earlier criteria are satisfied and exact-head CI is green.
New commits were added after this approval. Jervasion will reassess the updated head.
|
Implemented the historical optional follow-ups in 0c368c2:
Validation: |
jervasion-absmartly
left a comment
jervasion-absmartly
left a comment
There was a problem hiding this comment.
✅ Approved
The cleanup preserves the previously approved SDK behavior while removing duplicate implementation and unnecessary comments. Local tests and all ten exact-head CI jobs pass; no new blocking issue remains.
Next step: None — ready to merge.
What I checked
- ✅ Default suite: 409 tests, 1,335 assertions passed, including real local HTTP, context lifecycle, retry and cancellation coverage.
- ✅ Factory, async creation and deep-equality tests pass, including numeric-key objects and the empty-object/list distinction.
- ✅ Exact-head CI passes on Ubuntu and Windows across PHP 7.4, 8.0, 8.1, 8.2 and nightly.
- ✅ Composer validation, changed-PHP syntax checks and diff whitespace checks pass.
Review details
Head: 0c368c2757a93d5065963140cebf90b23b6bf567. Supplied base and merge-base: fd814153dccdc3d07cc9d993038688497fc9c1cd. Accepted baseline: 9abb2ddb92d90c78539ee6ee0277890bb9e97565, approval PRR_kwDOFYHMjc8AAAABUdLE1g. Review round 17. Responsibility and census counts use approved..head; unchanged accepted behavior is not reopened.
The single new commit changes eleven files: five runtime PHP files, four test/router PHP files, one audience fixture and Composer metadata. The author's latest top-level comment scopes this to historical optional follow-ups. The original operator, PHP compatibility, facade, async transport and lifecycle work remains accepted. No rollout flag or new public contract is involved. No applicable AGENTS.md or CLAUDE.md exists. Description, commits, conversation, reviews and inline replies were inspected; head/base and discussion timestamps were rechecked before publication.
Verified behavior: createSimple retains buildConfig argument order and options; the async-provider path delegates to the existing pending-context pipeline while the synchronous-provider branch stays unchanged. Config's legacy setter assigns the same publisher. setData still assigns context data before indexing. Header forwarding retains keys and values. Deep equality retains object/object map semantics, object/list separation and recursive list/map comparisons. The trimmed fixture retains the experiment asserted by its only consumer. Removed Composer suggestions duplicate unchanged runtime requirements. No new externally observable contract or newly claimed test behavior requires mutation/revert validation in this delta.
Validation:
composer install --no-interaction --prefer-dist: passed; no committed lockfile exists, so dependencies resolved locally.composer test -- --do-not-cache-result --no-coverage: passed on PHP 8.5.4, 409 tests / 1,335 assertions.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/ABsmartlyTest.php: passed, 7 tests / 17 assertions.vendor/bin/phpunit --do-not-cache-result --no-coverage tests/JsonExpression/EvaluatorComparisonTest.php: passed, 6 tests / 92 assertions.composer validate --no-check-publish,php -lfor all nine changed PHP files, andgit diff --check 9abb2dd HEAD: passed. No separate formatter, static-analysis or compilation gate is declared.- Independent read-only runtime and tests/packaging passes found no delta-caused blocker. The runtime pass additionally compared 11,664 accepted/head deep-equality cases without a result difference. Primary inspected changed control/data flows and executed repository checks.
- Exact-head Actions runs
40565475651and40565464979: completed/success, including all ten matrix jobs in each, verified through paginated Actions APIs. Commit statuses contain zero named results; the empty aggregate's pending label is not a pending check. CodeRabbit is paused, not a missing material validation. - Pinned introduced_blobs.py completed successfully over approved..head: eleven content-write occurrences in the single new commit, matching the current changed paths. Contents were inspected, including a credential-pattern scan; no secrets or unnecessary artifacts found.
- External cross-SDK harness and live-httpbin suite were not rerun. Cross-SDK results remain author-reported; default hermetic real-HTTP tests and exact-head native CI provide relevant evidence for this cleanup. No material verification limitation remains.
Passes (non-blocking): run: core runtime/API/security/data/error paths (factory, context, headers and equality refactors), tests/CI/packaging/end-to-end harnesses (test source, fixture reduction and Composer metadata), behaviour-bearing data and policy (consumed audience fixture), cross-cutting simplicity (locally audited delegation and remaining constructs), comment hygiene, diff composition | not run: deployment/operations/observability (no deployment, migration, operational constant or emitted log/metric/trace changes), shared-component blast radius and interaction complexity (no frontend source changed).
Comment hygiene (non-blocking): 2 comment / 13 code lines, ratio ~1:6.5, 0 blocks flagged
threshold T=5; restatements counted: yes (10 × C ≥ E)
None flagged. Added documentation-content lines preserve the legacy argument-order contract and HTTPS-only transport constraint. Previously flagged comments were removed or shortened.
Diff composition (non-blocking): every file is required for the change; none flagged.
Simplifications (non-blocking): none. Earlier optional async-pipeline deduplication and historical cleanup candidates have been applied; no further concrete equivalent simplification identified in this delta.
Convergence summary — review round 17
- Unresolved original blockers: 0
- Remediation regressions: 0
- Independent PR-change findings: 0
- Acceptance criteria still open: none
- Human adjudication requested: no — accepted behavior is preserved and exact-head validation is green.
Summary
Context::flush()now propagates the underlying exception instead of just marking the context failed, and preserves pending exposures/achievements for retry (was previously returning success even when the HTTP publish call failed)Test plan
composer test)Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation