Swift SDK: Cross-SDK tests passing (202/202) - #7
Conversation
- Fix race condition in flush() by locking access to units and attributes - Remove problematic clearInternalState() from deinit to prevent deadlocks - Use atomic wrappingDecrement for pendingCount manipulation - Skip timer scheduling when publishDelay is negative - Add Linux compatibility with conditional imports and swift-crypto - Fix DefaultScheduler to use dedicated timer queue instead of main queue - Add proper session cleanup in DefaultHTTPClient - Fix compiler warnings for unused variables This fixes the SIGSEGV crash that occurred after ~85 HTTP requests, improving test pass rate from 17/80 (21.2%) to 55/80 (68.8%).
- Add explicit DispatchQueue.global() to PromiseKit callbacks to ensure they execute in Vapor's event loop environment - Make Assignment.variables optional to properly handle nil vs empty dictionary (matches JavaScript behavior for fullOnVariant = -1) - Clear assignment cache in setData() to ensure proper cache invalidation after context refresh - Add getCustomFieldKeys() method for experiment custom field enumeration - Fix various code style issues (semicolons, var->let) Test results: 80/80 scenarios passing (improved from 55/80)
Security fixes: - Fix ReDoS vulnerability in MatchOperator by adding pattern/input length limits - Fix potential crash from force-unwrap on URL components in DefaultHTTPClient - Fix off-by-one error in Buffers.encodeUTF8 causing index out of bounds - Make failed flag atomic in Context to prevent data races - Add HTTP status code validation in DefaultClient for proper error handling Code quality improvements: - Replace force-unwraps with safe optional binding in Context custom fields - Add input length validation for unit UIDs (max 256 chars) - Add configurable applicationVersion to ClientConfig - Add explicit public access control to ABSmartlyConfig properties - Replace forEach with for-in loops for better debugging - Fix NSRange creation for Unicode string compatibility - Add documentation for non-cryptographic MD5 usage
- Add concurrency and thread safety tests - Add error recovery tests - Add HTTP client integration tests - Add custom field handling tests - Add state machine tests - Add performance tests Total: 39 new tests added
Expand MD5 (1→14), Murmur3 (1→36), VariantAssigner (2→43), DefaultClient (2→23) to individual parameterized tests. Add context cache invalidation test for iteration change with new refreshed_iteration.json fixture.
- ReDoS protection in MatchOperator with timeout and nested quantifier detection - Data loss prevention: flush only clears queues after publish success - Thread-safe isReady with atomic flag - Comprehensive error logging and throws-based error propagation - Dead code removal (jsonToNative, MurmurHash.updateInternal, Buffers.getUInt24) - Force unwrap elimination in production code - forEach to for..in conversion in ContextConfig - Final annotation on OrCombinator, struct Application
|
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:
WalkthroughThe PR renames public types from ABSmartly* to ABsmartly* with deprecated aliases, adds a new convenience SDK initializer, and integrates swift-crypto into Package.swift. It replaces ContextEventHandler with a ContextPublisher protocol and DefaultContextPublisher, converts many Context APIs to throwing variants and strengthens lifecycle, concurrency and deallocation logic. DefaultHTTPClient, DefaultClient, hashing and regex handling were hardened and made cross‑platform. Application changed to a struct. Tests, resources and README were substantially expanded and .gitignore entries were reorganised. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- DefaultHTTPClient: skip URLSession invalidation on Linux deinit - Add TestResources helper to avoid Bundle.module crash on Linux - Fix missing try on throwing calls across test files - Thread-safe mock for concurrent test execution - Fix type cast crash on Linux (NSNumber Bool vs Int)
Fix EqualsOperator to handle numeric type coercion and InOperator to properly check string containment and collection membership.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Sources/ABSmartly/JsonExpr/Operators/OrCombinator.swift (1)
10-10:⚠️ Potential issue | 🟡 MinorFix OR identity for empty arguments.
When
argsis empty, the method returnsJSON(true), but the Boolean identity for disjunction (OR) isfalse. OR of no operands should yieldfalse, nottrue. Only AND of no operands should yieldtrue(which AndCombinator correctly does).The test suite does not currently exercise the empty-args OR path, so the bug goes undetected.
Proposed fix
- return JSON(args.isEmpty) + return JSON(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/JsonExpr/Operators/OrCombinator.swift` at line 10, The OrCombinator currently returns JSON(args.isEmpty) which uses the wrong identity for OR; change the evaluate logic in OrCombinator (the method handling "args") to use false as the identity for an empty args list (i.e. return JSON(false) when args.isEmpty) or otherwise fold the argument booleans with a false identity (reduce/fold with || starting from false) so an empty-disjunction yields false.Tests/ABSmartlyTests/Mocks/SourceryGenerated.swift (1)
1-2:⚠️ Potential issue | 🟠 MajorManual changes to a Sourcery-generated "DO NOT EDIT" file will be silently lost on regeneration.
The
NSLockand all accompanyinglock.lock()/lock.unlock()calls added toContextEventLoggerMockwill be overwritten the next time Sourcery regenerates this file, reverting the thread-safety fix without any warning.Consider one of these approaches to preserve the fix:
- Subclass
ContextEventLoggerMockin a separate, non-generated file and add the locking there.- Add a Sourcery annotation (e.g. a custom template or
// sourcery: threadSafe) so the generated template emits the locking code.- If Sourcery is no longer actively used to regenerate this file, remove the header comment and explicitly mark the file as hand-maintained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/Mocks/SourceryGenerated.swift` around lines 1 - 2, The generated file Sourcery comment warns manual edits will be overwritten; to preserve the thread-safety fix for ContextEventLoggerMock, stop editing the generated file directly and instead either (1) create a new non-generated subclass or extension of ContextEventLoggerMock in a separate source file and add an NSLock instance with lock.lock()/lock.unlock() around the relevant methods, (2) add a Sourcery annotation (e.g. // sourcery: threadSafe) and update the Sourcery template to emit the NSLock and lock/unlock calls for ContextEventLoggerMock, or (3) if you truly intend to maintain the file by hand, remove the Sourcery header and mark the file as hand-maintained so future regeneration won’t overwrite the NSLock changes.Sources/ABSmartly/Context.swift (1)
406-418:⚠️ Potential issue | 🟡 MinorExposure/achievement buffer eviction uses
wrappingDecrementwhich can underflow.If
pendingCountis somehow less thanremoveCount(e.g., due to concurrent modifications between the store and decrement),wrappingDecrementon aUIntwill wrap around to a very large number. Whileflushon line 647 recalculatespendingCountfrom actual array sizes, there's a window wheregetPendingCount()could return an incorrect (huge) value.Consider using a saturating subtraction or recalculating from array sizes in the eviction path as well, similar to what
flushalready does on line 647.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 406 - 418, The eviction path currently removes removeCount items from exposures while calling pendingCount.wrappingDecrement(by: UInt(removeCount)), which can underflow and produce huge counts if pendingCount < removeCount; instead, after removing the items, compute the new pendingCount from the actual exposures.count (or use saturating subtraction) and set pendingCount accordingly so it cannot wrap. Update the block around eventLock.lock()/defer to either (a) recalculate pendingCount = UInt(exposures.count) (using the same atomic ordering used elsewhere) after exposures.removeFirst(removeCount), or (b) perform a safe subtract that clamps at zero rather than using wrappingDecrement; reference pendingCount, exposures, maxExposures, wrappingDecrement and flush to match existing behavior.Sources/ABSmartly/Experiment.swift (1)
57-65:⚠️ Potential issue | 🟡 Minor
Equatableconformance is missingaudienceStrictandaudience.The
==operator compares all public properties exceptaudienceStrict(line 17) andaudience(line 18). TwoExperimentinstances that differ only in these fields will be considered equal, which could cause subtle bugs in collections, deduplication, or test assertions.🐛 Proposed fix
public static func == (lhs: Experiment, rhs: Experiment) -> Bool { return lhs.id == rhs.id && lhs.name == rhs.name && lhs.unitType == rhs.unitType && lhs.iteration == rhs.iteration && lhs.seedHi == rhs.seedHi && lhs.seedLo == rhs.seedLo && lhs.split == rhs.split && lhs.trafficSeedHi == rhs.trafficSeedHi && lhs.trafficSeedLo == rhs.trafficSeedLo && lhs.trafficSplit == rhs.trafficSplit && lhs.fullOnVariant == rhs.fullOnVariant && lhs.applications == rhs.applications - && lhs.variants == rhs.variants && lhs.customFieldValues == rhs.customFieldValues + && lhs.variants == rhs.variants && lhs.audienceStrict == rhs.audienceStrict + && lhs.audience == rhs.audience && lhs.customFieldValues == rhs.customFieldValues }Alternatively, since all stored properties are already
Equatable, you could remove the manual conformance and let the compiler synthesise it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Experiment.swift` around lines 57 - 65, The custom Equatable implementation for Experiment omits the audienceStrict and audience properties, so update the static func ==(lhs: Experiment, rhs: Experiment) to include lhs.audienceStrict == rhs.audienceStrict and lhs.audience == rhs.audience in the chain of comparisons (or remove the manual Equatable conformance entirely so the compiler synthesises equality for all stored properties); locate the extension Experiment: Equatable and either add those two comparisons to the returned Bool expression or delete the extension to rely on compiler-synthesized Equatable.
🧹 Nitpick comments (24)
Tests/ABSmartlyTests/VariantAssignerTest.swift (1)
48-214: Consider a table‑driven loop to cut repetition.The many seed/split cases are solid, but a data table loop would be easier to maintain while keeping the same coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/VariantAssignerTest.swift` around lines 48 - 214, The test file has many repetitive testXXX methods (e.g., testEmailBinarySplit_ZeroSeeds, testNumericThreeWaySplit_Seeds1, testHashStringBinarySplit_Seeds5) all calling assertAssignment; replace them with one or a few table-driven tests that iterate a data array of cases (each case: keyString, splits array, seedHi, seedLo, expectedIndex) and call assertAssignment for each entry, grouping by input type (email, numeric, hash) or split type (binary/three-way) to preserve coverage while removing duplicate test methods; implement the loop inside new test methods (e.g., testEmailAssignments_tableDriven, testNumericAssignments_tableDriven, testHashStringAssignments_tableDriven) so existing assertAssignment(...) is reused and maintenance is simplified.Sources/ABSmartly/Application.swift (1)
6-8: Internalinitsuppresses the struct's memberwise initialiser — consider making itpublic.In Swift, a custom initialiser defined in the struct body (not in an extension) suppresses the synthesised memberwise initialiser. Because
init(_ name: String)carries no access modifier it isinternal, so public consumers of the SDK cannot construct anApplicationvalue other than throughCodabledecoding. If a caller ever needs to build one programmatically (e.g., in tests or configuration), they are blocked.Two idiomatic remedies:
♻️ Option A — expose a
publicinitialiser- init(_ name: String) { + public init(_ name: String) { self.name = name }♻️ Option B — move the internal init to an extension, restoring the synthesised
publicmemberwise initpublic struct Application: Codable, Equatable { public let name: String? - - init(_ name: String) { - self.name = name - } } + +extension Application { + init(_ name: String) { + self.name = name + } +}This gives external consumers
Application(name: "my-app")via the synthesised memberwise initialiser while preserving the internal convenience form.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Application.swift` around lines 6 - 8, The struct Application currently defines an internal initializer init(_ name: String) which suppresses the synthesised public memberwise initializer, preventing public callers from constructing Application(name:). Fix by either marking the initializer public (change init(_ name: String) to public init(_ name: String)) or move that init into an extension (put init(_ name: String) inside an extension Application { ... }) so the compiler can synthesize the public memberwise initializer; refer to the Application type and its init(_ name: String) when applying the change.Sources/ABSmartly/DefaultScheduler.swift (2)
12-12: Reorder theguardconditions to short-circuit oncancelledbefore the optional binding.The current form
guard let timer = handle, !cancelledperforms an optional bind before the cheaper Boolean test. Checking!cancelledfirst gives an immediate early exit when the handle is already cancelled, without the cost of the optional unwrap, and makes the intent (idempotency guard first, then resource retrieval) clearer.♻️ Proposed reorder
- guard let timer = handle, !cancelled else { return } + guard !cancelled, let timer = handle else { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultScheduler.swift` at line 12, In DefaultScheduler.swift change the guard so the cancelled boolean is tested first to short-circuit before the optional bind: replace the current "guard let timer = handle, !cancelled else { return }" with a guard that checks "!cancelled" first and then binds "let timer = handle" (so use "guard !cancelled, let timer = handle else { return }"), referencing the existing symbols cancelled, handle, and timer.
3-30: MarkDefaultScheduledHandleasfinalto prevent unsafe overriding of methods called indeinit.
DefaultScheduledHandleis a public, subclassable class with a publiccancel()method. Itsdeinitcallsself.cancel(), which uses dynamic dispatch in Swift. If a subclass overridescancel()and accesses subclass-stored properties, those properties will already be deinitialized when the superclassdeinitinvokes the override — resulting in undefined behaviour.The codebase already marks
OrCombinatorasfinalfor similar cohesion reasons; the same rationale applies here.♻️ Proposed fix
-public class DefaultScheduledHandle: ScheduledHandle { +public final class DefaultScheduledHandle: ScheduledHandle {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultScheduler.swift` around lines 3 - 30, DefaultScheduledHandle is a public, subclassable class whose deinit calls self.cancel(), which can be dangerously overridden; change its declaration to final to prevent subclassing and ensure safe deinit behavior by marking DefaultScheduledHandle as final (so that methods like cancel() cannot be overridden), leaving existing implementations of cancel(), isCancelled(), init(handle:), and deinit intact.Tests/ABSmartlyTests/Mocks/SourceryGenerated.swift (2)
162-191: Thread-safety is applied only toContextEventLoggerMock, leaving other concurrently-exercised mocks unprotected.
ClientMock,ContextEventHandlerMock, andHTTPClientMockall maintainReceivedInvocationsarrays that are mutated without any locking. IfConcurrencyTestsdrives those mocks from multiple threads, the unprotected writes are data races. For consistency, the sameNSLockguard should be applied to at least theClientMockandContextEventHandlerMockinvocation-capture paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/Mocks/SourceryGenerated.swift` around lines 162 - 191, Other mocks mutate their ReceivedInvocations arrays without synchronization causing data races; add the same NSLock pattern used in ContextEventLoggerMock to ClientMock and ContextEventHandlerMock (and HTTPClientMock if exercised concurrently): introduce a private let lock = NSLock() on each mock, wrap mutations and reads of properties like <ReceivedInvocations>, <*_CallsCount>, <*_ReceivedArguments> and the body of clearInvocations() with lock.lock()/lock.unlock(), and keep the pattern of unlocking before invoking any user-supplied closure (e.g., <*_Closure>) to avoid holding the lock during callbacks.
175-181: Preferdefer { lock.unlock() }over barelock.lock()/lock.unlock()pairs.The two-step
lock.lock()…lock.unlock()pattern withoutdeferleaves the lock permanently held if any code is inserted between them in future that can exit (throw, return, etc.). The idiomatic Swift pattern is:♻️ Suggested refactor
func handleEvent(context: Context, event: ContextEventLoggerEvent) { - lock.lock() + lock.lock(); defer { lock.unlock() } handleEventContextEventCallsCount += 1 handleEventContextEventReceivedArguments = (context: context, event: event) handleEventContextEventReceivedInvocations.append((context: context, event: event)) - lock.unlock() handleEventContextEventClosure?(context, event) } func clearInvocations() { - lock.lock() + lock.lock(); defer { lock.unlock() } handleEventContextEventCallsCount = 0 handleEventContextEventReceivedArguments = nil handleEventContextEventReceivedInvocations = [] - lock.unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/Mocks/SourceryGenerated.swift` around lines 175 - 181, In handleEvent(context:event), replace the manual lock.lock() ... lock.unlock() pair with the idiomatic pattern lock.lock(); defer { lock.unlock() } so the lock is always released even if the function exits early; update the body around handleEventContextEventCallsCount, handleEventContextEventReceivedArguments, and handleEventContextEventReceivedInvocations to remain under the lock and be followed by the defer, and if you need handleEventContextEventClosure?(context, event) to run outside the critical section capture it into a local let closure = handleEventContextEventClosure and call closure?(context, event) after the locked section (i.e., after the defer scope) to preserve original semantics.Sources/ABSmartly/Context.swift (1)
708-724: Force-unwrap inaudienceMatchesis safe but could be more idiomatic.Line 714:
let newAudienceMismatch = result != nil ? !result! : false— the nil check protects the force-unwrap, but this is cleaner expressed as:♻️ Suggested simplification
- let result = matcher.evaluate(audience, attrs) - let newAudienceMismatch = result != nil ? !result! : false + let result = matcher.evaluate(audience, attrs) + let newAudienceMismatch = result.map { !$0 } ?? false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 708 - 724, In audienceMatches, the current line computing newAudienceMismatch uses a guarded force-unwrap (result != nil ? !result! : false); replace it with an idiomatic optional operation to avoid force-unwrapping — for example use result.map { !$0 } ?? false or negate with !(result ?? false); keep the surrounding logic and references to matcher.evaluate, newAudienceMismatch, and assignment.attrsSeq unchanged.Tests/ABSmartlyTests/PerformanceTests.swift (2)
65-68:try?insidemeasureblocks silently swallows errors — performance numbers may be misleading.If any of the throwing calls fail (e.g. context not ready, context closed), the error is discarded and the iteration effectively becomes a no-op, skewing the timing measurement. Consider using
try!ordo/catchwithXCTFailinside measure blocks for test code, so failures are visible.Also applies to: 78-81, 94-99, 109-112, 124-128, 137-140, 151-154, 165-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/PerformanceTests.swift` around lines 65 - 68, The measure blocks in PerformanceTests.swift are using try? which silently swallows thrown errors (e.g., context.peekTreatment), making timing results unreliable; update each measure block to either use try! to surface failures immediately or wrap the throwing call (e.g., context.peekTreatment(...)) in a do/catch and call XCTFail(err.localizedDescription) on error so failures are visible during measurement—apply this change to all occurrences of try? inside measure blocks (including the shown context.peekTreatment usage and the other similar calls in the file).
38-44: HelpercreateContexthas a subtle double-trythat works but reads oddly.Line 39:
let data = try data ?? Promise<ContextData>.value(try getContextData())— the innertryis nested inside the nil-coalescing expression. This is valid Swift, but can be clearer with alet-binding of the fallback first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/PerformanceTests.swift` around lines 38 - 44, The createContext helper uses a nested double-try in the nil-coalescing expression which reads oddly; refactor by computing the fallback Promise first (call getContextData() with try and wrap in Promise<ContextData>.value) into a local binding, then use let data = try data ?? fallback when constructing the Context (symbols: createContext(config: ContextConfig, data: Promise<ContextData>?), getContextData(), Promise<ContextData>.value, and the Context(...) initializer) so the error-throwing call is clearer and not nested inside the nil-coalescing operation.Tests/ABSmartlyTests/TestResources.swift (1)
14-18: Force-unwrap onBundle.module.pathwill crash with an unhelpful message if a resource is missing.While this is test-only code, replacing the
!with aguard let+fatalErrorincluding the resource name would make debugging much easier when a resource file is accidentally omitted from the test bundle.💡 Suggested improvement
static func path(forResource name: String, ofType ext: String) -> String { - return Bundle.module.path(forResource: name, ofType: ext, inDirectory: "Resources")! + guard let path = Bundle.module.path(forResource: name, ofType: ext, inDirectory: "Resources") else { + fatalError("Test resource '\(name).\(ext)' not found in Resources directory") + } + return path }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/TestResources.swift` around lines 14 - 18, The force-unwrap in static func path(forResource name: String, ofType ext: String) uses Bundle.module.path(...)! which will crash with an unhelpful message if the resource is missing; replace the `!` with a `guard let` that unwraps Bundle.module.path(forResource: name, ofType: ext, inDirectory: "Resources") and call fatalError with a clear message that includes the resource name and extension (e.g., "Missing test resource: \(name).\(ext)"), so missing files produce a descriptive failure instead of a generic crash.Sources/ABSmartly/DefaultHTTPClient.swift (2)
114-142: Potentialselfretention issue after weak capture.On line 71,
selfis captured weakly and guarded on line 73. However, inside thecompletionHandlerclosure on line 116,selfis accessed directly (e.g.,self.config.retrieson line 119) without a weak/unowned capture or a re-guard. Ifselfis deallocated between the dataTask start and callback, this will access a dangling reference.Since line 73 already guards
selfinto a strong reference (the closure capturesselfstrongly from that point via the implicit strong reference in the outer closure scope), the callback'sselfreferences are actually to the strongselffrom line 73's guard. So this is safe — but only because the outer closure (the Promise body) capturesselfstrongly after the guard. Worth a brief comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 114 - 142, The completionHandler references self (e.g., self.config.retries) but that reference is safe only because the outer Promise body already guarded and promoted self to a strong reference; add a brief clarifying comment around the Promise body/guard and/or above the completionHandler stating that guard let self = self else { ... } earlier promotes self to a strong reference for the inner closure, so the completionHandler's direct self access is intentional and safe (refer to DefaultHTTPClient, the Promise body where self is guarded, and the completionHandler closure).
154-165:ManagedAtomicis unnecessary for a counter accessed sequentially through promise chaining.The
attempt()function is invoked sequentially — each.then(attempt)waits for the previous attempt to complete before starting the next. There's no concurrent access totryCounter. A plainvarwould suffice and be simpler.However, this doesn't cause correctness issues, so this is a minor simplification opportunity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 154 - 165, The retry implementation uses a ManagedAtomic (tryCounter) though attempts run sequentially via promise chaining; replace ManagedAtomic with a plain UInt var (e.g., var tryCounter: UInt = 0) inside retry and increment it (tryCounter += 1) in attempt() before calling body(currentTry) so attempt() and retry(times:delay:body:) keep the same logic without atomic overhead; update references to currentTry and ensure the <= times check and recover behavior remain identical.Tests/ABSmartlyTests/Internal/MD5Test.swift (1)
8-23:testCasesarray is declared but never referenced by any test method.Each test method below hardcodes its own input and expected value, making this array dead code. Either remove it or refactor the tests to iterate over it.
♻️ Option: Remove unused constant
- let testCases: [(input: String, expected: String)] = [ - ("", "1B2M2Y8AsgTpgAmY7PhCfg"), - (" ", "chXunH2dwinSkhpA6JnsXw"), - ("t", "41jvpIn1gGLxDdcxa2Vkng"), - ("te", "Vp73JkK-D63XEdakaNaO4Q"), - ("tes", "KLZi2IO212_Zbk3cXpungA"), - ("test", "CY9rzUYh03PK3k6DJie09g"), - ("testy", "K5I_V6RgP8c6sYKz-TVn8g"), - ("testy1", "8fT8xGipOhPkZ2DncKU-1A"), - ("testy12", "YqRAtOz000gIu61ErEH18A"), - ("testy123", "pfV2H07L6WvdqlY0zHuYIw"), - ("special characters açb↓c", "4PIrO7lKtTxOcj2eMYlG7A"), - ("The quick brown fox jumps over the lazy dog", "nhB9nTcrtoJr2B01QqQZ1g"), - ("The quick brown fox jumps over the lazy dog and eats a pie", "iM-8ECRrLUQzixl436y96A"), - ("Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.", "24m7XOq4f5wPzCqzbBicLA"), - ] -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/Internal/MD5Test.swift` around lines 8 - 23, The testCases constant is defined but never used; either remove it or update the MD5 tests to iterate over it. Replace the duplicated hardcoded assertions in the MD5 test methods with a single loop that iterates testCases (the tuple array named testCases) and runs the existing MD5 computation and assertion for each (use the existing test helper or the MD5 function under test), asserting actual == expected for every entry; if you prefer removal, simply delete the unused testCases constant. Ensure references to testCases in the test class (e.g., in the test method(s) that currently hardcode inputs) are updated to the loop so the symbol is no longer unused.README.md (1)
358-374: SwiftUI example usestry!which will crash the app if initialisation fails.The
ABSmartlyServicesingleton usestry!on lines 371 and 373. If the SDK or client fails to initialise (e.g. invalid config), the app crashes. A production-quality example should demonstrate proper error handling, as the other examples in the README do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 358 - 374, The singleton ABSmartlyService initializer uses forced tries (try!) for DefaultClient and ABsmartlySDK which will crash on failure; change the private init in ABSmartlyService to handle errors instead (use a do-catch around DefaultClient(...) and ABsmartlySDK(...)), avoid try!, and handle failure by either making sdk an optional (private let sdk: ABsmartlySDK?) and setting it to nil on error, or by converting init() to a throwing initializer and surfacing the error; ensure you log the error and provide a safe fallback path so callers can check SDK availability instead of crashing.Sources/ABSmartly/Internal/Hashing/Hashing.swift (1)
17-28: Consider suppressing theCC_MD5deprecation warning or migrating toInsecure.MD5.
CC_MD5was deprecated in iOS 13.0 / macOS 10.15 due to MD5 being cryptographically broken. Your code already usesCryptoKit.Insecure.MD5in the non-CommonCrypto branch, which is the recommended approach since it clearly signals non-cryptographic usage. For the CommonCrypto branch, you could either suppress the deprecation warning with a compiler directive (since the usage is intentionally non-cryptographic), or align both branches to useInsecure.MD5consistently (available on iOS 13+/macOS 10.15+).Note:
CC_MD5_Init,CC_MD5_Update, andCC_MD5_Finalare also deprecated starting in iOS 13.0, so those do not provide a viable alternative path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Internal/Hashing/Hashing.swift` around lines 17 - 28, The CommonCrypto branch in Hashing.swift uses the deprecated CC_MD5 symbol; either suppress the deprecation warning around that call or unify both branches to use CryptoKit's Insecure.MD5 for clarity. Fix option A: wrap the CC_MD5 invocation with the appropriate Swift compiler directive to silence deprecation warnings (e.g., push/ignore/pop deprecation) surrounding the CC_MD5 call. Fix option B (preferred): replace the CommonCrypto path with the same Insecure.MD5 usage (convert data to Digest via Insecure.MD5.hash(data:)) so both branches use Insecure.MD5 and remove CC_MD5 references.Sources/ABSmartly/DefaultClient.swift (1)
37-44:X-Application-Versionheader sent with an empty string when no version is configured.When
config.applicationVersiondefaults to"", this header is still included with an empty value in every PUT request. Consider omitting the header entirely when the version is not set.♻️ Proposed fix
putHeaders = [ "Content-Type": "application/json; charset=utf-8", "X-Agent": "absmartly-swift-sdk", "X-API-Key": config.apiKey, "X-Environment": config.environment, "X-Application": config.application, - "X-Application-Version": config.applicationVersion, ] + if !config.applicationVersion.isEmpty { + putHeaders["X-Application-Version"] = config.applicationVersion + }Note: this requires changing
putHeadersfromlettovarduring initialisation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultClient.swift` around lines 37 - 44, The PUT requests include an "X-Application-Version" header even when config.applicationVersion is an empty string; change the initialization in DefaultClient (where putHeaders is defined) to use a mutable variable (change putHeaders from let to var) and only insert the "X-Application-Version" key into putHeaders when config.applicationVersion is non-empty (e.g., check config.applicationVersion.isEmpty or equivalent) so the header is omitted when no version is configured.Example/Example/ViewController.swift (1)
66-66: Force-unwrapsdk!is fragile in example code.Although protected by the early
returnin thecatchblock above, a force-unwrap in example/demo code sets a bad precedent for adopters who may copy-paste. Consider usingguard let sdk = sdk else { return }instead.♻️ Proposed fix
- context = sdk!.createContext(config: contextConfig) + guard let sdk = sdk else { return } + context = sdk.createContext(config: contextConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Example/Example/ViewController.swift` at line 66, The line force-unwraps sdk when calling createContext (context = sdk!.createContext(config: contextConfig)); replace this with a safe guard: use guard let sdk = sdk else { return } before calling createContext, then call context = sdk.createContext(config: contextConfig) so the example avoids force-unwrapping and potential crashes while keeping the same behavior.Tests/ABSmartlyTests/DefaultClientTest.swift (1)
99-103: Force-casts (as! ABSmartlyHTTPError) in catch blocks will crash the test runner on type mismatch.If the error type is unexpectedly different, the preceding
XCTAssertTruewould report a failure, but the force-cast on the next line would crash the entire test process, preventing remaining tests from running. Consider usingguard letoras?instead.♻️ Proposed fix (example for one occurrence)
.catch { error in - XCTAssertTrue(error is ABSmartlyHTTPError) - let httpError = error as! ABSmartlyHTTPError - XCTAssertEqual(500, httpError.statusCode) + guard let httpError = error as? ABSmartlyHTTPError else { + XCTFail("Expected ABSmartlyHTTPError, got \(type(of: error))") + expectation.fulfill() + return + } + XCTAssertEqual(500, httpError.statusCode) expectation.fulfill() }Also applies to: 299-305, 357-363, 511-517, 543-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/DefaultClientTest.swift` around lines 99 - 103, Replace unsafe force-casts in the catch blocks to safely downcast errors to ABSmartlyHTTPError; e.g., in the catch after the promise where you currently do "let httpError = error as! ABSmartlyHTTPError", change to a safe conditional cast (use guard let or if let) and fail the test if the cast fails (call XCTFail with a message and fulfill the expectation), then return—apply the same change to all similar catch sites (the occurrences around the current block and the other blocks referenced for lines 299-305, 357-363, 511-517, 543-548) so the test runner won’t crash on unexpected error types.Tests/ABSmartlyTests/DefaultHTTPClientTest.swift (1)
15-68: Tests depend on an external service (httpstat.us) — inherently flaky in CI.All network tests hit
https://httpstat.us/*, making them dependent on external availability and network access. These will fail in air-gapped CI environments or when the service is down. Consider marking them with a custom trait/category so they can be excluded from default CI runs, or mocking the network layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/DefaultHTTPClientTest.swift` around lines 15 - 68, Tests in DefaultHTTPClientTest (testConnectionTimeout and testReadTimeout) depend on the external service httpstat.us and are flaky in CI; either make the HTTP calls injectable/mocked or skip these tests when network tests are disabled. Fix by refactoring DefaultHTTPClientTest to inject a protocol (e.g., an HTTPClient protocol implemented by DefaultHTTPClient and a MockHTTPClient) and replace real network calls in testConnectionTimeout/testReadTimeout with deterministic mock responses that simulate timeouts, or add an environment-gated skip at the top of each test (use ProcessInfo.processInfo.environment["RUN_NETWORK_TESTS"] or similar and call XCTSkip when not enabled) so DefaultHTTPClient.get and these test methods don’t hit the external httpstat.us service in CI.Tests/ABSmartlyTests/ABSmartlySDKTest.swift (1)
163-177: Tests using real HTTP clients may be flaky or leak network requests.
testNamedParameterInitialization,testNamedParameterInitializationWithOptionalParameters,testNamedParameterInitializationWithCustomEventLogger, andtestBackwardsCompatibilityall instantiate a realDefaultClient(via named-param init or explicit construction) pointing athttps://test.absmartly.io/v1. Callingsdk.createContext(config:)fires a real HTTP request throughDefaultContextDataProvider. If the endpoint is unreachable in CI, the dangling promise won't cause a test failure here (you don't await it), but:
- It introduces hidden network I/O in unit tests, which can slow runs or produce spurious log noise.
- If the endpoint is ever torn down, debugging unexpected test behaviour becomes harder.
Consider injecting a
ClientMock(as the existing tests do) or at minimum verifying only SDK construction without callingcreateContextin these tests.Also applies to: 179-196, 250-262, 264-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ABSmartlySDKTest.swift` around lines 163 - 177, The tests (e.g., testNamedParameterInitialization) instantiate a real DefaultClient (and thus DefaultContextDataProvider) pointing at https://test.absmartly.io/v1 and then call sdk.createContext(config:), which triggers real HTTP I/O; replace the real client with the existing ClientMock (or inject a mock into ABsmartlySDK) in these tests so creation is validated without making network calls, or alternatively stop calling createContext and only assert SDK construction; update tests named testNamedParameterInitialization, testNamedParameterInitializationWithOptionalParameters, testNamedParameterInitializationWithCustomEventLogger, and testBackwardsCompatibility to use ClientMock or remove the createContext call to avoid flaky external requests.Sources/ABSmartly/ABSmartlySDK.swift (1)
25-31: Guard inelsebranch is unreachable — consider simplifying.The
ifon Line 18 enters when at least one provider/handler is nil; theelseis reached only when both are non-nil. Theguard leton Line 26 therefore cannot fail, and the error on Line 27 is dead code. While the guard does serve as a safe unwrap, the construct is misleading.Simplification
} else { - guard let provider = config.contextDataProvider, let handler = config.contextEventHandler else { - throw ABSmartlyError("Missing contextDataProvider or contextEventHandler") - } - contextDataProvider = provider - contextEventHandler = handler + contextDataProvider = config.contextDataProvider! + contextEventHandler = config.contextEventHandler! }Or, if you prefer to avoid force-unwraps (given the PR's stated goal of removing them), you could restructure the init to use a single
guard let/switchpattern for both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/ABSmartlySDK.swift` around lines 25 - 31, The else-branch guard is unreachable; simplify the initializer in ABSmartlySDK by removing the redundant guard let in the else branch and directly assign config.contextDataProvider and config.contextEventHandler to contextDataProvider and contextEventHandler (or refactor the whole init to a single guard/switch that validates and unwraps both config.contextDataProvider and config.contextEventHandler once), ensuring you no longer throw ABSmartlyError from an unreachable guard and avoid force-unwraps.Tests/ABSmartlyTests/ConcurrencyTests.swift (2)
140-173: Concurrent refresh + publish test verifies no crash but not outcomes.The test only asserts
isReady()and!isFailed()after concurrent refresh and publish. It doesn't verify that the refreshed data was actually applied or that the publish event was sent. This is acceptable as a crash/deadlock smoke test, but consider adding outcome assertions if you want stronger guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ConcurrencyTests.swift` around lines 140 - 173, The test testConcurrentRefreshAndPublish only checks for no crash by asserting context.isReady() and !context.isFailed(); to verify outcomes, after the concurrent refresh/publish complete assert that provider.getContextDataReturnValue (refreshedContextData) was applied (e.g., check a treatment or tracked property reflecting refreshedContextData via context.getTreatment("exp_test_ab") or context.someState accessor) and that handler.publishEventReturnValue resulted in a publish call (e.g., verify the handler received the expected event or incremented publish call count). Update the test to read the treatment or state from the context after wait(for: [...]) and add an assertion that it matches refreshedContextData, and add an assertion (or mock verification) that handler.publishEventReturnValue triggered the expected publish invocation.
66-74:try?silently swallows errors, weakening assertions.Throughout the concurrent blocks (e.g. Lines 69, 94, 116, 158, 164, 216, 245),
try?is used for throwing calls. If the code under test starts throwing unexpectedly, the tests will still pass because:
XCTAssertNotNil(treatment)on Line 70 would fail, but the expectation is unconditionally fulfilled on Line 72, so thewaitsucceeds regardless.- In
testConcurrentGoalTracking, a silently swallowedtrackerror means the pending count assertion on Line 101 would catch it, but the failure message would be misleading (count mismatch rather than the real error).Consider using
do { try ... } catch { XCTFail("...") }inside the concurrent blocks, or at least movingexpectation.fulfill()into the success path so a thrown error causes a timeout failure.Example for testConcurrentTreatmentAccess
for i in 0..<100 { concurrentQueue.async { let experimentName = experimentNames[i % experimentNames.count] - let treatment = try? context.getTreatment(experimentName) - XCTAssertNotNil(treatment) - XCTAssertGreaterThanOrEqual(treatment ?? 0, 0) - expectation.fulfill() + do { + let treatment = try context.getTreatment(experimentName) + XCTAssertGreaterThanOrEqual(treatment, 0) + expectation.fulfill() + } catch { + XCTFail("getTreatment threw unexpectedly: \(error)") + } } }Also applies to: 91-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ConcurrencyTests.swift` around lines 66 - 74, In the concurrent test blocks (e.g. testConcurrentTreatmentAccess and testConcurrentGoalTracking) replace the silent `try?` calls to `context.getTreatment(...)` and `context.track(...)` with explicit do-try-catch so thrown errors cause test failures, and move `expectation.fulfill()` into the success path; specifically, wrap calls to the throwing methods `context.getTreatment` and `context.track` in `do { try ...; XCTAssert...; expectation.fulfill() } catch { XCTFail("...: \(error)") }` so failures are reported rather than swallowed.Tests/ABSmartlyTests/ContextTest.swift (1)
2106-2114: Overrides value assertion is fragile and hard to follow.The nested
as?casts and ternary fallback (as? Bool == true ? 1 : 0) on Lines 2110–2111 make this block difficult to read and maintain. IfJSONdecoding behaviour changes, this could silently pass with incorrect logic.Consider a more explicit assertion or a helper that normalises the value before comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ContextTest.swift` around lines 2106 - 2114, The assertions around overridesValue returned by context.getCustomFieldValue are fragile due to nested optional casts and ternary fallbacks; replace them with a clear normalisation step (e.g., a small helper in the test) that converts the result into a canonical [String: Int] map and then assert equality against ["123":1, "456":0]; specifically, add a helper that accepts the overridesValue (from getCustomFieldValue) and handles types JSON, [String: JSON], [String: Any], Bool/Int conversions to produce [String:Int], then use that helper in the test instead of the current if/else with multiple as? casts and ternary expressions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitignoreExample/Example/ViewController.swiftPackage.resolvedPackage.swiftREADME.mdSources/ABSmartly/ABSmartlyConfig.swiftSources/ABSmartly/ABSmartlySDK.swiftSources/ABSmartly/Application.swiftSources/ABSmartly/AudienceMatcher.swiftSources/ABSmartly/ClientConfig.swiftSources/ABSmartly/Context.swiftSources/ABSmartly/ContextConfig.swiftSources/ABSmartly/ContextData.swiftSources/ABSmartly/DefaultClient.swiftSources/ABSmartly/DefaultHTTPClient.swiftSources/ABSmartly/DefaultScheduler.swiftSources/ABSmartly/DefaultVariableParser.swiftSources/ABSmartly/Experiment.swiftSources/ABSmartly/Internal/Hashing/Buffers.swiftSources/ABSmartly/Internal/Hashing/Hashing.swiftSources/ABSmartly/Internal/Hashing/MurmurHash.swiftSources/ABSmartly/JsonExpr/ExprEvaluator.swiftSources/ABSmartly/JsonExpr/Operators/EqualsOperator.swiftSources/ABSmartly/JsonExpr/Operators/InOperator.swiftSources/ABSmartly/JsonExpr/Operators/MatchOperator.swiftSources/ABSmartly/JsonExpr/Operators/OrCombinator.swiftSources/ABSmartly/Logger.swiftSources/ABSmartly/VariantAssigner.swiftTests/ABSmartlyTests/ABSmartlySDKTest.swiftTests/ABSmartlyTests/ConcurrencyTests.swiftTests/ABSmartlyTests/ContextDataDeserializerTest.swiftTests/ABSmartlyTests/ContextTest.swiftTests/ABSmartlyTests/DefaultClientTest.swiftTests/ABSmartlyTests/DefaultHTTPClientTest.swiftTests/ABSmartlyTests/DefaultVariableParserTest.swiftTests/ABSmartlyTests/Internal/MD5Test.swiftTests/ABSmartlyTests/Internal/MurmurHashTest.swiftTests/ABSmartlyTests/Mocks/SourceryGenerated.swiftTests/ABSmartlyTests/PerformanceTests.swiftTests/ABSmartlyTests/Resources/refreshed_iteration.jsonTests/ABSmartlyTests/TestResources.swiftTests/ABSmartlyTests/VariantAssignerTest.swift
💤 Files with no reviewable changes (1)
- Sources/ABSmartly/Internal/Hashing/MurmurHash.swift
| @@ -1,10 +1,12 @@ | |||
| # A/B Smartly SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a> | |||
| # ABsmartly Swift SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a> | |||
There was a problem hiding this comment.
Badge image is missing alt text.
Per the markdownlint hint (MD045), the <img> tag should include an alt attribute for accessibility.
🔧 Suggested fix
-# ABsmartly Swift SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a>
+# ABsmartly Swift SDK <a href="https://github.com/apple/swift-package-manager" title="Swift Package Manager compatible"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" alt="Swift Package Manager compatible" /></a>Also note: the alt and title attributes on the <a> tag still reference "RxSwift" which appears to be a copy-paste artefact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ABsmartly Swift SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a> | |
| # ABsmartly Swift SDK <a href="https://github.com/apple/swift-package-manager" title="Swift Package Manager compatible"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" alt="Swift Package Manager compatible" /></a> |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1-1: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 1, The README's badge <img> tag is missing an alt
attribute and the surrounding <a> tag still references "RxSwift" (copy/paste
artifact); add a descriptive alt text to the <img> (e.g., "Swift Package Manager
compatible badge") and update the <a> tag's alt and title attributes to reflect
this repository (e.g., "ABsmartly Swift SDK" / "Swift Package
Manager-compatible") so the badge is accessible and correctly labeled.
| ### Selecting a Treatment | ||
| ```swift | ||
| context.setAttribute(name: "device", value: UIDevice.current.model) | ||
| context.setAttributes(["customer_age": "new_customer", "screen": "product"]) | ||
| let treatment = context.getTreatment("exp_test_experiment") | ||
| if treatment == 0 { | ||
| // user is in control group (variant 0) | ||
| } else { | ||
| // user is in treatment group | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Code examples are missing try for throwing methods — they won't compile.
getTreatment, peekTreatment, getVariableValue, peekVariableValue, track, publish, setAttribute, setAttributes, setOverride, and setOverrides all now throw (per Context.swift). Nearly every example in this README omits the required try keyword, which means users copying these snippets will get compilation errors.
For example, line 202 should be:
let treatment = try context.getTreatment("exp_test_experiment")This affects lines 202, 214, 222, 233, 241–242, 254–258, 266–269, 277, 411–412, 467, 470–475, 545, 549, and 554 (at minimum). Please audit all examples for throwing API calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 200 - 208, Update the README code examples to call
the now-throwing Context API methods with Swift's try syntax (or wrap them in a
do/catch) so they compile: add "try" before calls to getTreatment,
peekTreatment, getVariableValue, peekVariableValue, track, publish,
setAttribute, setAttributes, setOverride, and setOverrides in every example
(e.g., change let treatment = context.getTreatment(...) to let treatment = try
context.getTreatment(...)), and where multiple throwing calls appear in a
snippet, either prefix each with try or wrap the block in a do { ... } catch {
... } to handle errors.
| public func close() -> Promise<Void> { | ||
| if client == nil { | ||
| guard let clientToClose = client else { | ||
| return Promise<Void>.value(()) | ||
| } | ||
|
|
||
| return Promise<Void> { seal in | ||
| if client != nil { | ||
| client!.close().done { | ||
| seal.fulfill(()) | ||
| }.catch { error in | ||
| seal.reject(error) | ||
| } | ||
| client = nil | ||
| } else { | ||
| seal.fulfill(()) | ||
| } | ||
| } | ||
| client = nil | ||
| return clientToClose.close() | ||
| } |
There was a problem hiding this comment.
close() is not thread-safe — concurrent calls may double-close the client.
Two threads calling close() simultaneously can both pass the guard let on Line 115 before either executes client = nil on Line 118, resulting in clientToClose.close() being called twice. While DefaultHTTPClient.close() has its own sessionLock, the duplicate close at this layer returns two independent promises for the same operation and may confuse callers.
Consider adding a lock or using an atomic compare-and-swap pattern, consistent with the thread-safety improvements elsewhere in the PR.
Sketch using a lock
+private let closeLock = NSLock()
+
public func close() -> Promise<Void> {
- guard let clientToClose = client else {
- return Promise<Void>.value(())
+ closeLock.lock()
+ guard let clientToClose = client else {
+ closeLock.unlock()
+ return Promise<Void>.value(())
+ }
+ client = nil
+ closeLock.unlock()
+ return clientToClose.close()
- }
- client = nil
- return clientToClose.close()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/ABSmartlySDK.swift` around lines 114 - 120, The close()
method is not thread-safe: concurrent callers can both read client and call
close twice; to fix, protect access to the client property (and the
read-then-nil pattern) with a synchronization primitive or atomic
compare-and-swap so only one caller obtains and clears client and calls
clientToClose.close(); update the close() implementation (referencing close(),
client and clientToClose) to acquire the same lock/atomic operation used
elsewhere in the PR (or add a dedicated lock) before checking/clearing client
and returning the single Promise from the one close() invocation.
| override func evaluate(_ evaluator: Evaluator, _ args: JSON) -> JSON { | ||
| if args.type == .array { | ||
| let lhs = evaluator.evaluate(args[0]) | ||
| let rhs = evaluator.evaluate(args[1]) | ||
| return binary(evaluator, lhs, rhs) | ||
| } | ||
| return JSON.null |
There was a problem hiding this comment.
Consider restoring BinaryOperator’s null short‑circuiting (or handle null equality explicitly).
This override now evaluates both operands and calls compare even when either side is .null (for example, missing args). That diverges from BinaryOperator.evaluate and can introduce unexpected side‑effects or null handling. If null equality isn’t intended, keep the guards; if it is, make the behaviour explicit.
💡 Proposed guarded variant (aligns with BinaryOperator)
override func evaluate(_ evaluator: Evaluator, _ args: JSON) -> JSON {
- if args.type == .array {
- let lhs = evaluator.evaluate(args[0])
- let rhs = evaluator.evaluate(args[1])
- return binary(evaluator, lhs, rhs)
- }
- return JSON.null
+ guard args.type == .array else { return JSON.null }
+ let lhs = evaluator.evaluate(args[0])
+ guard lhs.type != .null else { return JSON.null }
+ let rhs = evaluator.evaluate(args[1])
+ guard rhs.type != .null else { return JSON.null }
+ return binary(evaluator, lhs, rhs)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override func evaluate(_ evaluator: Evaluator, _ args: JSON) -> JSON { | |
| if args.type == .array { | |
| let lhs = evaluator.evaluate(args[0]) | |
| let rhs = evaluator.evaluate(args[1]) | |
| return binary(evaluator, lhs, rhs) | |
| } | |
| return JSON.null | |
| override func evaluate(_ evaluator: Evaluator, _ args: JSON) -> JSON { | |
| guard args.type == .array else { return JSON.null } | |
| let lhs = evaluator.evaluate(args[0]) | |
| guard lhs.type != .null else { return JSON.null } | |
| let rhs = evaluator.evaluate(args[1]) | |
| guard rhs.type != .null else { return JSON.null } | |
| return binary(evaluator, lhs, rhs) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/JsonExpr/Operators/EqualsOperator.swift` around lines 2 -
8, EqualsOperator.evaluate currently evaluates both operands and calls
binary/compare even when args are missing or one side is JSON.null, diverging
from BinaryOperator.evaluate; restore the null short‑circuit or make null
equality explicit by first ensuring args.type == .array and there are at least
two elements, then check the raw arg tokens for JSON.null (or evaluate the first
and if it is .null return JSON.null immediately before evaluating the second)
and only call binary(evaluator, lhs, rhs) when neither side is null (or, if null
equality is intended, document and implement an explicit compare for nulls in
compare/binary). Target: EqualsOperator.evaluate, binary(_:_:_:), and compare to
implement the guard or explicit null handling.
…P client - EqualsOperator: remove evaluate() override that bypassed parent null guards, causing null == null to return true instead of null - InOperator: swap haystack/needle parameter order to match BinaryOperator calling convention, fixing type switching and preventing index-out-of-range crash - Context.flush(): clear events synchronously instead of in async .done callback to prevent double-publish on concurrent flush calls - Context.waitUntilReady(): handle rejected readyPromise via .catch so the method resolves on both success and failure (fixes 3 timeout tests) - Context.setData(): remove full assignmentCache wipe on refresh since getAssignment() already handles per-experiment invalidation - DefaultHTTPClient: use ABSmartlyError instead of URLError(.cancelled) for closed sessions so PromiseKit .catch handlers can intercept it
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/ABSmartly/DefaultHTTPClient.swift (1)
114-126:⚠️ Potential issue | 🟠 MajorOff-by-one: the body condition
attempt < config.retriesburns one retry slot early.The
retry()wrapper allows up totimes + 1total invocations ofbody(the guardcurrentTry <= timesstill permits a retry whencurrentTry == times). However, the body conditionattempt < self.config.retriesflips from "reject" to "fulfil" whencurrentTry == retries, so thetimesth call tobodyalways fulfils — the final slot in the wrapper is dead code and one retry is silently lost.Concrete example with
retries = 3:
currentTryBody condition attempt < 3Outcome 1 truerejects ABSmartlyHTTPError → retry 2 truerejects ABSmartlyHTTPError → retry 3 false fulfils with 502/503 response 4 never reached — Effective retries = 2, not 3. With
retries = 1there are zero retries.Align the condition so the body keeps rejecting through the last available slot:
🐛 Proposed fix
- && (attempt < self.config.retries) + && (attempt <= self.config.retries)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 114 - 126, The retry gating is off-by-one: in the dataTask completion you check `attempt < self.config.retries`, which causes the final allowed retry slot to be treated as a fulfil instead of a reject; replace that comparison with `attempt <= self.config.retries` so that when `attempt == self.config.retries` the code still rejects with `ABSmartlyHTTPError` (using `seal.reject`) and allows the retry wrapper to perform the final retry; locate this change around the `session.dataTask` completion handler where `attempt`, `self.config.retries`, `ABSmartlyHTTPError`, and `seal.reject`/`seal.fulfill` are referenced.Sources/ABSmartly/Context.swift (1)
600-650:⚠️ Potential issue | 🟠 MajorEvents are permanently lost when
handler.publishfails.Exposures and achievements are moved into local variables and the instance buffers are zeroed (inside
eventLock, lines 606–617) beforehandler.publishis invoked. If the network call fails, therecoverblock (line 645) rethrows but makes no attempt to re-enqueuelocalExposuresorlocalAchievements. Those events are silently dropped.The PR description states "data-loss prevention by clearing buffers only after successful publish", which is the opposite of the current behaviour. The commit message clarifies the actual intent: clearing synchronously prevents double-publish on concurrent
flush()calls. If this is the intended design trade-off, it should be explicitly documented; otherwise the buffers must be re-enqueued on failure:♻️ Option A — re-enqueue events on publish failure (prevents data loss, accepts double-publish risk on retry)
return handler.publish(event: event).done(on: DispatchQueue.global()) { [weak self] in guard let self = self else { return } self.logEvent(event: .publish(event: event)) }.recover { [weak self] error -> Promise<Void> in guard let self = self else { return Promise.value(()) } + // Re-enqueue lost events so they can be retried. + self.eventLock.lock() + self.exposures = localExposures + self.exposures + self.achievements = localAchievements + self.achievements + self.pendingCount.wrappingIncrement( + by: UInt(localExposures.count + localAchievements.count), + ordering: .releasing) + self.eventLock.unlock() Logger.error("Publish failed: \(error.localizedDescription)") self.logError(error: error) throw error }📝 Option B — document the deliberate trade-off (no code change needed)
Add a comment above the
eventLockblock making the intentional data-loss-on-failure trade-off explicit, e.g.:// Events are cleared before the network call to avoid double-publishing // if flush() is re-entered. Failed publishes will silently drop these events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 600 - 650, The code clears exposures/achievements and sets pendingCount to 0 under eventLock before calling handler.publish, which causes permanent data loss if publish fails; modify the recover block (the closure on handler.publish in Context.publish/flush) to re-acquire eventLock and re-enqueue localExposures and localAchievements back into the instance buffers (exposures and achievements) and restore pendingCount appropriately, doing this thread-safely to avoid races; keep the success path unchanged, and ensure you reference eventLock, exposures, achievements, pendingCount, localExposures, localAchievements, handler.publish, and the existing logError call when implementing the re-enqueue on failure.
♻️ Duplicate comments (2)
Sources/ABSmartly/Context.swift (2)
28-28:promiseLockis still declared but never referenced.The previous review flagged this variable as unused; the fix has not been applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` at line 28, The private NSLock property promiseLock is declared but never used; either remove the unused declaration to clean up the class or actually use promiseLock to protect concurrent access to any promise-related state in Context (e.g., wrap reads/writes to the promise, resolve, or fulfill methods with promiseLock.lock()/unlock()). Update the Context implementation accordingly so promiseLock is either referenced where promise state is mutated or the declaration is deleted.
162-180: Race onreadyPromiseinwaitUntilReadyremains unresolved.The check
self.readyPromise == nil(line 168) and the optional bindif let ready = self.readyPromise(line 170) are still unguarded. The previous review identified that the done/catch callbacks (which nilreadyPromiseon lines 126 and 136) can interleave between these two reads, leavingsealpermanently unfulfilled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 162 - 180, The waitUntilReady race comes from reading self.readyPromise multiple times; capture readyPromise once into a local strong/immutable variable at the start of waitUntilReady (e.g., let readyRef = self.readyPromise) and then base the isReady()/nil check and the subsequent readyRef.done/catch attachment on that single captured reference so the callbacks cannot interleave with a second read; update waitUntilReady (and its use of isReady() and the done/catch closures) to use the captured readyRef and fulfill/seal from those handlers accordingly, referencing the waitUntilReady function and the readyPromise property.
🧹 Nitpick comments (4)
Sources/ABSmartly/DefaultHTTPClient.swift (2)
92-94:compactMapshould bemap—URLQueryItem.initis non-failable.
URLQueryItem(name:value:)always returns a non-optional value, socompactMapis semantically equivalent tomapbut misleadingly implies the initialiser can returnnil.♻️ Suggested fix
- components.queryItems = query.compactMap { (key, value) in + components.queryItems = query.map { (key, value) in URLQueryItem(name: key, value: value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 92 - 94, Replace the misleading use of compactMap when building URL query items: change the closure that assigns components.queryItems (currently using compactMap { (key, value) in URLQueryItem(name: key, value: value) }) to use map instead, since URLQueryItem(name:value:) is non-failable; update the assignment to components.queryItems = query.map { (key, value) in URLQueryItem(name: key, value: value) } so intent matches reality and avoids implying nils.
154-165:ManagedAtomicis unnecessary — the retry chain is strictly sequential.
attempt()is only ever called once at startup, then recursively fromthen(attempt)after the prior promise has settled. No two invocations ofattempt()can be in-flight simultaneously, so the counter is never accessed concurrently. A simple captured reference cell is sufficient and avoids both the heap allocation forManagedAtomicand the stronger-than-needed.acquiringAndReleasingordering.♻️ Suggested refactor
func retry<T>(times: UInt, delay: TimeInterval, body: `@escaping` (UInt) -> Promise<T>) -> Promise<T> { - let tryCounter = ManagedAtomic<UInt>(0) + final class Counter { var value: UInt = 0 } + let tryCounter = Counter() func attempt() -> Promise<T> { - let currentTry = tryCounter.wrappingIncrementThenLoad(ordering: .acquiringAndReleasing) + tryCounter.value += 1 + let currentTry = tryCounter.value return body(currentTry).recover(policy: CatchPolicy.allErrorsExceptCancellation) { error -> Promise<T> in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 154 - 165, The retry implementation uses ManagedAtomic unnecessarily; replace the ManagedAtomic<UInt> tryCounter in retry(...) with a plain captured UInt variable (e.g. var tryCounter: UInt = 0) and update attempt() to increment that variable (tryCounter += 1) and read its value into currentTry, keeping the existing logic in attempt() and recover; update references to wrappingIncrementThenLoad and remove the atomic ordering, preserving the sequential recursive calls in retry(...) and the same boundary check against times.Sources/ABSmartly/Context.swift (2)
11-14:deinitis sandwiched between stored-property declarations.Placing
deinitbetweenprovider(line 9) andlogger(line 15) is valid Swift but breaks the conventional layout (properties → init → deinit → methods). Consider moving it after all stored properties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 11 - 14, The deinit implementation (calling clearRefreshTimer() and clearTimeout()) is placed between stored properties (notably between provider and logger), breaking conventional layout; move the deinit block so it appears after all stored-property declarations (i.e., after logger and any other stored properties) and before or with the rest of lifecycle methods so that deinit sits in the normal properties → init → deinit → methods order, keeping the calls to clearRefreshTimer() and clearTimeout() intact.
621-632: Preferdeferfor the manualcontextLockacquire/release inflush().The pattern is safe today (no throws between lock and unlock), but is fragile against future edits.
♻️ Proposed refactor
- contextLock.lock() - localUnits = units.map { - let hashBytes = getUnitHash($0.key, $0.value) - if let hashString = String(bytes: hashBytes, encoding: .ascii) { - return Unit(type: $0.key, uid: hashString) - } else { - Logger.error("Failed to encode unit hash for type '\($0.key)' to ASCII. Using base64 fallback.") - return Unit(type: $0.key, uid: Data(hashBytes).base64EncodedString()) - } - } - localAttributes = attributes - contextLock.unlock() + contextLock.lock() + defer { contextLock.unlock() } + localUnits = units.map { + let hashBytes = getUnitHash($0.key, $0.value) + if let hashString = String(bytes: hashBytes, encoding: .ascii) { + return Unit(type: $0.key, uid: hashString) + } else { + Logger.error("Failed to encode unit hash for type '\($0.key)' to ASCII. Using base64 fallback.") + return Unit(type: $0.key, uid: Data(hashBytes).base64EncodedString()) + } + } + localAttributes = attributes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 621 - 632, In flush(), replace the manual contextLock.lock()/contextLock.unlock() pair with a scoped lock using defer: call contextLock.lock() then immediately schedule contextLock.unlock() with defer so any early returns or future throws still release the lock; keep the existing logic that computes localUnits via getUnitHash and maps to Unit(type:uid:) (with ASCII check and base64 fallback) and assigns localAttributes — the only change is to ensure contextLock.unlock() is invoked in a defer right after acquiring the lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ABSmartly/Context.swift`:
- Line 976: The log unconditionally appends "..." after customValue.prefix(100),
which misleads when the value is shorter than 100 chars; update the Logger.error
call used where parsing JSON for a custom field (referencing Logger.error,
fieldName, experiment.name and customValue) to compute a displayedValue =
customValue.count > 100 ? String(customValue.prefix(100)) + "..." :
String(customValue) and use that displayedValue in the log so the ellipsis is
only added when truncation actually occurred.
- Around line 299-303: The guard in setUnit that checks !isClosed() &&
!isClosing() is duplicating logic and emits an incorrect "finalized" message
when the context is only closing; replace that guard with a call to the existing
checkNotClosed() helper so the correct, distinct error messages for closing vs
closed are used (i.e., remove the manual guard in setUnit and invoke
checkNotClosed() at the start of the function).
- Around line 928-931: The race is that refreshTimer is read/written without its
own synchronization causing setRefreshTimer (called under dataLock) and
clearRefreshTimer (called from close/deinit without locks) to interleave;
protect refreshTimer with a dedicated lock (e.g., a new DispatchQueue or NSLock)
and update setRefreshTimer and clearRefreshTimer to acquire that lock before
reading, scheduling, cancelling, or nil-ing refreshTimer; keep dataLock usage in
setData unchanged but ensure any access to refreshTimer uses the new lock (also
update deinit/close to use the same lock) so no timer can be created after
clearRefreshTimer observes nil.
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Around line 145-151: The close() method currently calls
session?.finishTasksAndInvalidate() and immediately returns
Promise<Void>.value(()), which resolves before the URLSession actually becomes
invalid; change close() to return a Promise that is fulfilled only when the
session is invalidated: create/attach a pending resolver stored alongside
sessionLock, call finishTasksAndInvalidate(), and resolve or reject that promise
from the URLSession delegate method urlSession(_:didBecomeInvalidWithError:) (or
reject on error); ensure session is set to nil and the resolver cleared under
sessionLock to avoid races.
---
Outside diff comments:
In `@Sources/ABSmartly/Context.swift`:
- Around line 600-650: The code clears exposures/achievements and sets
pendingCount to 0 under eventLock before calling handler.publish, which causes
permanent data loss if publish fails; modify the recover block (the closure on
handler.publish in Context.publish/flush) to re-acquire eventLock and re-enqueue
localExposures and localAchievements back into the instance buffers (exposures
and achievements) and restore pendingCount appropriately, doing this
thread-safely to avoid races; keep the success path unchanged, and ensure you
reference eventLock, exposures, achievements, pendingCount, localExposures,
localAchievements, handler.publish, and the existing logError call when
implementing the re-enqueue on failure.
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Around line 114-126: The retry gating is off-by-one: in the dataTask
completion you check `attempt < self.config.retries`, which causes the final
allowed retry slot to be treated as a fulfil instead of a reject; replace that
comparison with `attempt <= self.config.retries` so that when `attempt ==
self.config.retries` the code still rejects with `ABSmartlyHTTPError` (using
`seal.reject`) and allows the retry wrapper to perform the final retry; locate
this change around the `session.dataTask` completion handler where `attempt`,
`self.config.retries`, `ABSmartlyHTTPError`, and `seal.reject`/`seal.fulfill`
are referenced.
---
Duplicate comments:
In `@Sources/ABSmartly/Context.swift`:
- Line 28: The private NSLock property promiseLock is declared but never used;
either remove the unused declaration to clean up the class or actually use
promiseLock to protect concurrent access to any promise-related state in Context
(e.g., wrap reads/writes to the promise, resolve, or fulfill methods with
promiseLock.lock()/unlock()). Update the Context implementation accordingly so
promiseLock is either referenced where promise state is mutated or the
declaration is deleted.
- Around line 162-180: The waitUntilReady race comes from reading
self.readyPromise multiple times; capture readyPromise once into a local
strong/immutable variable at the start of waitUntilReady (e.g., let readyRef =
self.readyPromise) and then base the isReady()/nil check and the subsequent
readyRef.done/catch attachment on that single captured reference so the
callbacks cannot interleave with a second read; update waitUntilReady (and its
use of isReady() and the done/catch closures) to use the captured readyRef and
fulfill/seal from those handlers accordingly, referencing the waitUntilReady
function and the readyPromise property.
---
Nitpick comments:
In `@Sources/ABSmartly/Context.swift`:
- Around line 11-14: The deinit implementation (calling clearRefreshTimer() and
clearTimeout()) is placed between stored properties (notably between provider
and logger), breaking conventional layout; move the deinit block so it appears
after all stored-property declarations (i.e., after logger and any other stored
properties) and before or with the rest of lifecycle methods so that deinit sits
in the normal properties → init → deinit → methods order, keeping the calls to
clearRefreshTimer() and clearTimeout() intact.
- Around line 621-632: In flush(), replace the manual
contextLock.lock()/contextLock.unlock() pair with a scoped lock using defer:
call contextLock.lock() then immediately schedule contextLock.unlock() with
defer so any early returns or future throws still release the lock; keep the
existing logic that computes localUnits via getUnitHash and maps to
Unit(type:uid:) (with ASCII check and base64 fallback) and assigns
localAttributes — the only change is to ensure contextLock.unlock() is invoked
in a defer right after acquiring the lock.
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Around line 92-94: Replace the misleading use of compactMap when building URL
query items: change the closure that assigns components.queryItems (currently
using compactMap { (key, value) in URLQueryItem(name: key, value: value) }) to
use map instead, since URLQueryItem(name:value:) is non-failable; update the
assignment to components.queryItems = query.map { (key, value) in
URLQueryItem(name: key, value: value) } so intent matches reality and avoids
implying nils.
- Around line 154-165: The retry implementation uses ManagedAtomic
unnecessarily; replace the ManagedAtomic<UInt> tryCounter in retry(...) with a
plain captured UInt variable (e.g. var tryCounter: UInt = 0) and update
attempt() to increment that variable (tryCounter += 1) and read its value into
currentTry, keeping the existing logic in attempt() and recover; update
references to wrappingIncrementThenLoad and remove the atomic ordering,
preserving the sequential recursive calls in retry(...) and the same boundary
check against times.
| private func clearRefreshTimer() { | ||
| if refreshTimer != nil { | ||
| refreshTimer!.cancel() | ||
| refreshTimer = nil | ||
| } | ||
| refreshTimer?.cancel() | ||
| refreshTimer = nil | ||
| } |
There was a problem hiding this comment.
refreshTimer is accessed without a dedicated lock, creating a race between setRefreshTimer and clearRefreshTimer.
setRefreshTimer() is always called while dataLock is held (from setData), but clearRefreshTimer() is called from close() and deinit without holding any lock. If the asynchronous init done-handler calls setData concurrently with close(), the sequence can be:
close()→clearRefreshTimer()nilsrefreshTimersetData→setRefreshTimer()seesrefreshTimer == niland schedules a new timer
The orphaned timer will fire and invoke refresh(), which immediately throws because the context is closed, but it still produces spurious work and log noise.
🛠️ Proposed fix — guard `refreshTimer` with a dedicated lock
+ private let refreshTimerLock = NSLock()
private var refreshTimer: ScheduledHandle?
private func setRefreshTimer() {
+ refreshTimerLock.lock()
+ defer { refreshTimerLock.unlock() }
if refreshInterval > 0 && refreshTimer == nil {
refreshTimer = scheduler.scheduleWithFixedDelay(
after: refreshInterval, repeating: refreshInterval,
execute: { ... })
}
}
private func clearRefreshTimer() {
+ refreshTimerLock.lock()
+ defer { refreshTimerLock.unlock() }
refreshTimer?.cancel()
refreshTimer = nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/Context.swift` around lines 928 - 931, The race is that
refreshTimer is read/written without its own synchronization causing
setRefreshTimer (called under dataLock) and clearRefreshTimer (called from
close/deinit without locks) to interleave; protect refreshTimer with a dedicated
lock (e.g., a new DispatchQueue or NSLock) and update setRefreshTimer and
clearRefreshTimer to acquire that lock before reading, scheduling, cancelling,
or nil-ing refreshTimer; keep dataLock usage in setData unchanged but ensure any
access to refreshTimer uses the new lock (also update deinit/close to use the
same lock) so no timer can be created after clearRefreshTimer observes nil.
| public func close() -> Promise<Void> { | ||
| sessionLock.lock() | ||
| session?.finishTasksAndInvalidate() | ||
| session = nil | ||
| sessionLock.unlock() | ||
| return Promise<Void>.value(()) | ||
| } |
There was a problem hiding this comment.
close() resolves its promise before pending tasks complete.
finishTasksAndInvalidate() returns immediately and completes asynchronously — pending tasks continue to run after it returns. Because close() fulfils Promise<Void>.value(()) on the very next line, any caller that chains work off the returned promise (e.g. resource teardown, test assertions) will proceed before in-flight requests have actually settled.
Consider either documenting that the promise only signals initiation of the close, or returning a Promise<Void> that is fulfilled via the URLSession delegate's urlSession(_:didBecomeInvalidWithError:) callback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 145 - 151, The
close() method currently calls session?.finishTasksAndInvalidate() and
immediately returns Promise<Void>.value(()), which resolves before the
URLSession actually becomes invalid; change close() to return a Promise that is
fulfilled only when the session is invalidated: create/attach a pending resolver
stored alongside sessionLock, call finishTasksAndInvalidate(), and resolve or
reject that promise from the URLSession delegate method
urlSession(_:didBecomeInvalidWithError:) (or reject on error); ensure session is
set to nil and the resolver cleared under sessionLock to avoid races.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ABSmartly/Context.swift (1)
604-617:⚠️ Potential issue | 🔴 Critical
flush()can still lose events when publish fails.Buffers are cleared before
handler.publishsucceeds, and the failure path rethrows without restoring them. This drops exposures/achievements on transient publish errors.Suggested fix (requeue on failure)
return handler.publish(event: event).done(on: DispatchQueue.global()) { [weak self] in guard let self = self else { return } self.logEvent(event: .publish(event: event)) }.recover { [weak self] error -> Promise<Void> in guard let self = self else { return Promise.value(()) } + self.eventLock.lock() + self.exposures.insert(contentsOf: localExposures, at: 0) + self.achievements.insert(contentsOf: localAchievements, at: 0) + let restored = UInt(localExposures.count + localAchievements.count) + self.pendingCount.wrappingIncrement(by: restored, ordering: .releasing) + self.eventLock.unlock() Logger.error("Publish failed: \(error.localizedDescription)") self.logError(error: error) throw error }Also applies to: 642-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 604 - 617, In flush(), avoid clearing exposures/achievements and resetting pendingCount before calling handler.publish; instead call handler.publish with the prepared localExposures/localAchievements and only clear the shared buffers (exposures, achievements) and store(0) after publish succeeds, or if publish throws, restore/requeue the localExposures/localAchievements back into the shared exposures/achievements and restore pendingCount before rethrowing; apply the same change to the other publish site that mirrors this logic (the block using localExposures/localAchievements and pendingCount around handler.publish) so transient publish failures do not drop events.
♻️ Duplicate comments (4)
README.md (2)
188-193:⚠️ Potential issue | 🟠 MajorExamples call throwing
ContextAPIs withouttry, and onepeekTreatmentcall uses the wrong label.These snippets will not compile as written. Add
try(ordo/catch) for throwing calls, and usepeekTreatment("...")(no external label).Suggested fixes (representative)
-context.refresh().done { +try context.refresh().done { -let treatment = context.getTreatment("exp_test_experiment") +let treatment = try context.getTreatment("exp_test_experiment") -let treatment = context.peekTreatment(experimentName: "exp_test_experiment") +let treatment = try context.peekTreatment("exp_test_experiment") -context.publish().done { +try context.publish().done {#!/bin/bash python - <<'PY' import re from pathlib import Path lines = Path("README.md").read_text().splitlines() throwing = re.compile(r'context\??\.(getTreatment|peekTreatment|getVariableValue|peekVariableValue|track|publish|refresh|setOverride|setOverrides|setAttribute|setAttributes|setUnit|setUnits)\s*\(') for i, line in enumerate(lines, 1): if throwing.search(line) and ('try ' not in line and 'try?' not in line and 'try!' not in line): print(f"missing try @ line {i}: {line.strip()}") for i, line in enumerate(lines, 1): if "peekTreatment(experimentName:" in line: print(f"wrong label @ line {i}: {line.strip()}") PYAlso applies to: 204-208, 215-235, 246-257, 267-294, 418-430, 480-485, 549-555, 642-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 188 - 193, Several README examples call throwing Context APIs (e.g., context.refresh(), context.getTreatment(...), context.peekTreatment(...), context.track(...), context.setOverride(...)) without using try/try?/try! or wrapping them in a do/catch, and one peekTreatment call uses an external label (peekTreatment(experimentName: "...")) which is incorrect; update each offending snippet to either prefix the call with try or wrap the sequence in do { try ... } catch { ... } and change any peekTreatment(experimentName: "...") to peekTreatment("...") (no external label) so the examples compile; search for occurrences of context.refresh(), context.getTreatment, context.peekTreatment, context.getVariableValue, context.peekVariableValue, context.track, context.publish, context.setOverride, context.setOverrides, context.setAttribute, context.setAttributes, context.setUnit, context.setUnits and fix them accordingly.
1-1:⚠️ Potential issue | 🟡 MinorAdd alt text to the badge image and remove stale RxSwift labelling.
The
<img>is missingalt, and the anchor metadata still references RxSwift.Suggested fix
-# A/B Smartly Swift SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a> +# A/B Smartly Swift SDK <a href="https://github.com/apple/swift-package-manager" title="Swift Package Manager-compatible"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" alt="Swift Package Manager-compatible badge" /></a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 1, The badge markup at the top of README.md uses an <a> and <img> fragment that lacks an alt attribute and still references RxSwift in the anchor metadata; update that fragment so the <img> includes a meaningful alt (e.g., "Swift Package Manager compatible badge") and remove or replace any RxSwift-specific text in the <a> attributes (href/title) so they correctly reference Swift Package Manager/this SDK instead of RxSwift; locate the HTML snippet containing the <a href="https://github.com/apple/swift-package-manager" ...> and <img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /> and update the attributes accordingly.Sources/ABSmartly/Context.swift (2)
116-137:⚠️ Potential issue | 🟠 MajorSynchronise
readyPromisereads/writes to prevent hanging waiters.There is still a race between the readiness check and binding to
readyPromise; under an unlucky interleave, the returned promise is never fulfilled.Suggested fix
- self.readyPromise = nil + self.promiseLock.lock() + self.readyPromise = nil + self.promiseLock.unlock() ... - self.readyPromise = nil + self.promiseLock.lock() + self.readyPromise = nil + self.promiseLock.unlock() ... - if self.isReady() || self.readyPromise == nil { + self.promiseLock.lock() + let localReadyPromise = self.readyPromise + let readyNow = self.isReady() + self.promiseLock.unlock() + + if readyNow || localReadyPromise == nil { seal.fulfill(self) - } else if let ready = self.readyPromise { + } else if let ready = localReadyPromise {Also applies to: 163-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 116 - 137, The readyPromise is read and mutated from multiple async continuations causing a race where a waiter can miss fulfillment; to fix, serialize all accesses to readyPromise (both reads and writes) by using a dedicated synchronization mechanism (e.g., a private serial DispatchQueue or a lock) and always capture the promise reference under that synchronization before leaving the critical section or starting async work; update the Promise<Void> creation and the catch/done continuations in the block that references readyPromise (and the analogous block at the second occurrence) so they read/write readyPromise only while synchronized, clear readyPromise under the same sync, and schedule any async work or fulfill/fulfill/ reject actions after releasing the lock to avoid deadlocks.
1003-1013:⚠️ Potential issue | 🔴 Critical
assignmentCacheis mutated under the wrong lock.
assignmentCacheis normally protected bycontextLock(for example ingetAssignment), butsetData(_:)writes it while onlydataLockis held. That introduces a data race.Suggested fix (split lock scopes)
dataLock.lock() - defer { dataLock.unlock() } self.data = data self.index = index self.indexVariables = indexVariables self.customFieldValues = customFieldValues - // A new payload should force assignment/exposure recomputation. - self.assignmentCache = [:] ready.store(true, ordering: .releasing) + dataLock.unlock() + + contextLock.lock() + assignmentCache = [:] + contextLock.unlock() setRefreshTimer()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 1003 - 1013, setData(_:) is clearing assignmentCache while holding dataLock, but assignmentCache is protected by contextLock (see getAssignment), causing a race; change the lock scopes so dataLock only guards self.data, self.index, self.indexVariables, self.customFieldValues and ready.store, then after releasing dataLock acquire contextLock and set self.assignmentCache = [:] (and any other context-protected fields) — i.e., move the assignmentCache mutation out of the dataLock defer into a separate contextLock critical section to avoid cross-locking races.
🧹 Nitpick comments (1)
Sources/ABSmartly/Internal/Hashing/Hashing.swift (1)
8-12: ConvertHashingto a namespaceenumto prevent accidental instantiation.
Hashingcontains only static methods and is never instantiated in the codebase. Usingpublic enum Hashingbetter communicates that this is a utility type and prevents accidental instantiation at compile time.♻️ Proposed refactor
-public class Hashing { +public enum Hashing {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Internal/Hashing/Hashing.swift` around lines 8 - 12, The Hashing utility is declared as a public class but only contains static methods and should be converted to a namespaced utility to prevent instantiation: change `public class Hashing` to `public enum Hashing {}` (an enum with no cases) and keep all existing static methods (e.g., any `static func` like your MD5/hash helpers) inside it; remove any initializers or instance-level code and ensure callers still reference `Hashing.methodName` so the API remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 170: Update the README example so it calls the correct public API:
replace the invalid call to context.getData() with context.getContextData(),
since getContextData() returns Promise<ContextData> and is the public method
used with sdk.createContextWithData(config:contextData:); ensure the example
reflects this method name change (context.getContextData()) when constructing
anotherContext.
In `@Sources/ABSmartly/Context.swift`:
- Around line 321-323: The immutability check is comparing the stored value
`previous` with the raw `uid` instead of the trimmed `trimmed`, so equivalent
UIDs with whitespace are incorrectly treated as a change; update the guard in
the `units[unitType]` check to compare `previous` against `trimmed` (and use
`trimmed` in the error message) and ensure any subsequent assignment/logic for
`units[unitType]` uses the normalized `trimmed` value rather than the raw `uid`
(refer to symbols: `units`, `unitType`, `uid`, `trimmed`, `Logger.error`).
---
Outside diff comments:
In `@Sources/ABSmartly/Context.swift`:
- Around line 604-617: In flush(), avoid clearing exposures/achievements and
resetting pendingCount before calling handler.publish; instead call
handler.publish with the prepared localExposures/localAchievements and only
clear the shared buffers (exposures, achievements) and store(0) after publish
succeeds, or if publish throws, restore/requeue the
localExposures/localAchievements back into the shared exposures/achievements and
restore pendingCount before rethrowing; apply the same change to the other
publish site that mirrors this logic (the block using
localExposures/localAchievements and pendingCount around handler.publish) so
transient publish failures do not drop events.
---
Duplicate comments:
In `@README.md`:
- Around line 188-193: Several README examples call throwing Context APIs (e.g.,
context.refresh(), context.getTreatment(...), context.peekTreatment(...),
context.track(...), context.setOverride(...)) without using try/try?/try! or
wrapping them in a do/catch, and one peekTreatment call uses an external label
(peekTreatment(experimentName: "...")) which is incorrect; update each offending
snippet to either prefix the call with try or wrap the sequence in do { try ...
} catch { ... } and change any peekTreatment(experimentName: "...") to
peekTreatment("...") (no external label) so the examples compile; search for
occurrences of context.refresh(), context.getTreatment, context.peekTreatment,
context.getVariableValue, context.peekVariableValue, context.track,
context.publish, context.setOverride, context.setOverrides,
context.setAttribute, context.setAttributes, context.setUnit, context.setUnits
and fix them accordingly.
- Line 1: The badge markup at the top of README.md uses an <a> and <img>
fragment that lacks an alt attribute and still references RxSwift in the anchor
metadata; update that fragment so the <img> includes a meaningful alt (e.g.,
"Swift Package Manager compatible badge") and remove or replace any
RxSwift-specific text in the <a> attributes (href/title) so they correctly
reference Swift Package Manager/this SDK instead of RxSwift; locate the HTML
snippet containing the <a href="https://github.com/apple/swift-package-manager"
...> and <img
src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg"
/> and update the attributes accordingly.
In `@Sources/ABSmartly/Context.swift`:
- Around line 116-137: The readyPromise is read and mutated from multiple async
continuations causing a race where a waiter can miss fulfillment; to fix,
serialize all accesses to readyPromise (both reads and writes) by using a
dedicated synchronization mechanism (e.g., a private serial DispatchQueue or a
lock) and always capture the promise reference under that synchronization before
leaving the critical section or starting async work; update the Promise<Void>
creation and the catch/done continuations in the block that references
readyPromise (and the analogous block at the second occurrence) so they
read/write readyPromise only while synchronized, clear readyPromise under the
same sync, and schedule any async work or fulfill/fulfill/ reject actions after
releasing the lock to avoid deadlocks.
- Around line 1003-1013: setData(_:) is clearing assignmentCache while holding
dataLock, but assignmentCache is protected by contextLock (see getAssignment),
causing a race; change the lock scopes so dataLock only guards self.data,
self.index, self.indexVariables, self.customFieldValues and ready.store, then
after releasing dataLock acquire contextLock and set self.assignmentCache = [:]
(and any other context-protected fields) — i.e., move the assignmentCache
mutation out of the dataLock defer into a separate contextLock critical section
to avoid cross-locking races.
---
Nitpick comments:
In `@Sources/ABSmartly/Internal/Hashing/Hashing.swift`:
- Around line 8-12: The Hashing utility is declared as a public class but only
contains static methods and should be converted to a namespaced utility to
prevent instantiation: change `public class Hashing` to `public enum Hashing {}`
(an enum with no cases) and keep all existing static methods (e.g., any `static
func` like your MD5/hash helpers) inside it; remove any initializers or
instance-level code and ensure callers still reference `Hashing.methodName` so
the API remains unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdSources/ABSmartly/Context.swiftSources/ABSmartly/Internal/Hashing/Hashing.swiftSources/ABSmartly/JsonExpr/Operators/BinaryOperator.swiftSources/ABSmartly/JsonExpr/Operators/InOperator.swift
- Fix ready promise to fulfill after setTimeout setup - Log error before rejecting promise in context init - Reorder refresh completion to log before fulfilling - Update operator tests for consistent comparison behavior
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ABSmartly/Context.swift (1)
587-648:⚠️ Potential issue | 🔴 Critical
flush()still loses queued events when publish fails.Lines 605-615 empty
exposures,achievements, andpendingCountbefore the network promise on Line 640 resolves. Ifhandler.publish(event:)rejects, those events are unrecoverable and the next publish sees an empty queue. Keep the buffers intact until.done, or restore them in.recoverbefore rethrowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 587 - 648, The flush() implementation clears exposures, achievements and pendingCount before calling handler.publish(event:) which loses events if publish fails; change the logic so buffers are not emptied until publish succeeds (move the exposures/achievements reset and pendingCount.store(0) to after the publish Promise resolves in the .done block) or, alternately, capture the localExposures/localAchievements as now but restore them into exposures/achievements and reset pendingCount in the .recover block before rethrowing the error; adjust uses of eventLock (and contextLock where needed) to perform clearing or restoration under the same lock to avoid races, and keep the existing logEvent/logError and handler.publish(event:) usage.
♻️ Duplicate comments (4)
README.md (2)
160-170:⚠️ Potential issue | 🟠 MajorSeveral README snippets still do not match the current
ContextAPI.
getContextData(),getTreatment,peekTreatment,getVariableValue,track,publish,refresh,setAttribute,setAttributes,setOverride, andsetOverridesnow throw inSources/ABSmartly/Context.swift, so these examples needtry/do-catch. Line 170 also callscontext.getData(), which does not exist, and Line 235 uses the wrongpeekTreatmentlabel.#!/bin/bash # Verify README calls against the current Context API. rg -n 'context\.(getData|getContextData|getTreatment|peekTreatment|getVariableValue|peekVariableValue|track|publish|refresh|setAttribute|setAttributes|setOverride|setOverrides)\(' README.md rg -n 'public func (getContextData|getTreatment|peekTreatment|getVariableValue|peekVariableValue|track|publish|refresh|setAttribute|setAttributes|setOverride|setOverrides)\(' Sources/ABSmartly/Context.swiftAlso applies to: 187-294, 423-430, 474-480, 543-550, 637-638
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 160 - 170, The README examples call Context methods that now throw and use a removed method name; update all examples to use getContextData() instead of getData(), add try/await or wrap calls in do-catch where Context methods are invoked (for getContextData(), getTreatment, peekTreatment, getVariableValue, peekVariableValue, track, publish, refresh, setAttribute, setAttributes, setOverride, setOverrides) and fix the incorrect peekTreatment label usage to match the current signature in Sources/ABSmartly/Context.swift; search for usages of context.getData and any context.<method>(...) listed in the review and update each snippet to use try (or try await) and proper error handling consistent with surrounding examples.
1-1:⚠️ Potential issue | 🟡 MinorFix the badge labelling and alt text.
Line 1 still has the old “RxSwift” copy and the
<img>has noalt, so the README remains misleading and fails the MD045 accessibility check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 1, Update the badge label and image alt/title attributes on the README header: replace the leftover "RxSwift" text in the badge markup with "Swift Package Manager" (so the header link and label match the SPM badge) and add a meaningful alt attribute to the <img> tag (e.g., alt="Swift Package Manager") and adjust the title to match, ensuring the "# A/B Smartly Swift SDK" header's badge markup reflects the correct label and includes accessible alt text.Tests/ABSmartlyTests/ContextTest.swift (1)
2263-2292:⚠️ Potential issue | 🟡 Minor
testRetryMechanismActivationstill does not exercise retry behaviour.
Context.publish()invokes the handler once per call. The closure on Line 2274-Line 2280 can fail that one invocation, but nothing in this test re-callspublish(), sopublishAttemptscannot prove a retry path. Either rename the test to the failure-path it actually covers, or add an explicit retry loop in the test harness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ContextTest.swift` around lines 2263 - 2292, The test testRetryMechanismActivation incorrectly assumes Context.publish() will retry via the handler; update the test to actually exercise retry logic by either renaming the test to reflect it only tests a single-failure path or modify it to call the retry mechanism: drive multiple publish attempts (e.g. loop calling context.publish() until success or maxAttempts) or trigger whatever higher-level retry API exists, using handler.publishEventClosure to simulate transient failures and assert publishAttempts reaches the expected retry count; reference Context.publish(), handler.publishEventClosure, publishAttempts and maxAttempts when making the change.Sources/ABSmartly/Context.swift (1)
28-29:⚠️ Potential issue | 🟠 MajorSynchronise
readyPromiseaccess inwaitUntilReady().Between Line 167 and Line 169, another thread can resolve the initialisation promise and set
readyPromise = nil. In that interleaving theelse iffalls through and the returned promise never settles. Read/copyreadyPromiseand the ready state under the same lock you use for writes.Also applies to: 116-140, 161-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Context.swift` around lines 28 - 29, Access to readyPromise in waitUntilReady() must be synchronized with promiseLock to avoid a race where another thread can resolve and nil out readyPromise between checks; while holding promiseLock, read/copy readyPromise and the ready state into local variables, then release the lock and operate on the copied promise (or create and assign a new Promise while still holding the lock) so that waitUntilReady(), any initialiser resolution paths, and other callers use the same protected snapshot. Specifically modify waitUntilReady() to acquire promiseLock, inspect/set readyPromise and isReady under that lock, store the reference into a local variable (e.g., localPromise) and return/use that localPromise after releasing the lock to ensure the returned promise cannot be lost by an interleaving thread; apply the same locking pattern wherever readyPromise is read or written.
🧹 Nitpick comments (4)
Sources/ABSmartly/Internal/Hashing/MurmurHash.swift (1)
3-3: Consider using a caseless enum instead of a class.
MurmurHashcontains only static members and no instance state. A caselessenumprevents accidental instantiation and is the idiomatic Swift pattern for namespacing utility functions.♻️ Suggested change
-class MurmurHash { +enum MurmurHash {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/Internal/Hashing/MurmurHash.swift` at line 3, MurmurHash is a utility container with only static members; replace the top-level class declaration with a caseless enum to prevent instantiation and follow Swift idioms: change the declaration of MurmurHash from "class MurmurHash" to "enum MurmurHash" (no cases), keep all existing static functions/properties (e.g., any static methods inside MurmurHash) unchanged, and remove any initializers or references that create instances of MurmurHash if present.Tests/ABSmartlyTests/PerformanceTests.swift (2)
21-30: Minor:clockis not reinitialised insetUp.Unlike other mocks,
clockis not reassigned insetUp, only itsmillisReturnValueis set. This could theoretically carry state between tests ifClockMockhas mutable state beyondmillisReturnValue. Consider addingclock = ClockMock()for consistency.♻️ Suggested fix
override func setUp() async throws { provider = ContextDataProviderMock() handler = ContextEventHandlerMock() logger = ContextEventLoggerMock() parser = DefaultVariableParser() scheduler = SchedulerMock() + clock = ClockMock() scheduler.scheduleAfterExecuteReturnValue = ScheduledHandleMock() scheduler.scheduleWithFixedDelayAfterRepeatingExecuteReturnValue = ScheduledHandleMock() clock.millisReturnValue = 1_620_000_000_000 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/PerformanceTests.swift` around lines 21 - 30, In setUp(), reinitialize the clock mock to avoid shared mutable state by adding clock = ClockMock() before setting clock.millisReturnValue; update the setUp() method (the initializer block that assigns provider, handler, logger, parser, scheduler) so it explicitly reassigns clock using ClockMock and then sets clock.millisReturnValue = 1_620_000_000_000 to match the other mock initializations.
194-210: Test name is misleading: no memory measurement is performed.
testCacheMemoryUsagesuggests memory profiling, but the test only verifiesgetPendingCount()values. This is a functional correctness test, not a memory/cache usage test. Consider renaming to better reflect its purpose, e.g.,testPendingCountAfterTreatmentsAndGoals.Additionally, this test does not use
measure {}, so it won't appear in Xcode's performance metrics.♻️ Suggested rename
-func testCacheMemoryUsage() throws { +func testPendingCountAccumulatesCorrectly() throws {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/PerformanceTests.swift` around lines 194 - 210, The test named testCacheMemoryUsage is misleading because it doesn't measure memory; rename the test method to something like testPendingCountAfterTreatmentsAndGoals (update the function declaration for testCacheMemoryUsage) and update any references/uses in the test suite; if you intended a performance benchmark, wrap the exercise (the loop creating treatments/goals and the XCTAssertEqual checks) in an XCTest measure { } block instead of just renaming so it appears in Xcode performance metrics.Tests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swift (1)
13-14: Line 13 should assert the rawJSONresult.Using
.boolValuehere can hide aJSON.nullreturn, so the null-needle behaviour is not pinned down as precisely as the null-haystack case on Line 14. Please compare the returnedJSONdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swift` around lines 13 - 14, Replace the boolean-based assertion that inspects .boolValue with a direct JSON equality check so the null-needle case is asserted precisely; specifically, change the assertion that calls inOperator.evaluate(evaluator, [JSON.null, "abcdefghijk"]) to compare the returned JSON directly (e.g., XCTAssertEqual(JSON.null, inOperator.evaluate(evaluator, [JSON.null, "abcdefghijk"]))) instead of using .boolValue, keeping the same inOperator and evaluator references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ABSmartly/Context.swift`:
- Around line 505-539: The refresh() and close() paths set the atomic flags
(refreshing/closing) before publishing their in-flight Promise
(refreshPromise/closePromise), allowing a concurrent caller to see the flag true
but get a nil promise and return Promise.value(()). Fix by creating the Promise
first, then publish it and flip the flag in one critical section (or use an
atomic compare-exchange that sets both state and stored promise together), e.g.,
construct the Promise for refresh()/close(), assign it to
refreshPromise/closePromise, then set refreshing/closing to true and return the
captured promise to other callers; ensure the same pattern is applied in both
refresh() and close() and that callers always read a captured localPromise
variable to return.
- Around line 1029-1049: The helper jsonObjectToNative currently drops nested
NSNulls; change its signature to jsonObjectToNative(_ jsonObject: Any, topLevel:
Bool = true) and make it return NSNull() for JSON nulls when topLevel is false
(only return nil when topLevel is true), update all recursive calls (inside the
dict loop and array mapping) to pass topLevel: false, replace the array
compactMap with map to preserve NSNull placeholders, and ensure the dictionary
loop assigns result[key] = nativeValue (allowing NSNull values) rather than
skipping entries when a nested null is encountered.
---
Outside diff comments:
In `@Sources/ABSmartly/Context.swift`:
- Around line 587-648: The flush() implementation clears exposures, achievements
and pendingCount before calling handler.publish(event:) which loses events if
publish fails; change the logic so buffers are not emptied until publish
succeeds (move the exposures/achievements reset and pendingCount.store(0) to
after the publish Promise resolves in the .done block) or, alternately, capture
the localExposures/localAchievements as now but restore them into
exposures/achievements and reset pendingCount in the .recover block before
rethrowing the error; adjust uses of eventLock (and contextLock where needed) to
perform clearing or restoration under the same lock to avoid races, and keep the
existing logEvent/logError and handler.publish(event:) usage.
---
Duplicate comments:
In `@README.md`:
- Around line 160-170: The README examples call Context methods that now throw
and use a removed method name; update all examples to use getContextData()
instead of getData(), add try/await or wrap calls in do-catch where Context
methods are invoked (for getContextData(), getTreatment, peekTreatment,
getVariableValue, peekVariableValue, track, publish, refresh, setAttribute,
setAttributes, setOverride, setOverrides) and fix the incorrect peekTreatment
label usage to match the current signature in Sources/ABSmartly/Context.swift;
search for usages of context.getData and any context.<method>(...) listed in the
review and update each snippet to use try (or try await) and proper error
handling consistent with surrounding examples.
- Line 1: Update the badge label and image alt/title attributes on the README
header: replace the leftover "RxSwift" text in the badge markup with "Swift
Package Manager" (so the header link and label match the SPM badge) and add a
meaningful alt attribute to the <img> tag (e.g., alt="Swift Package Manager")
and adjust the title to match, ensuring the "# A/B Smartly Swift SDK" header's
badge markup reflects the correct label and includes accessible alt text.
In `@Sources/ABSmartly/Context.swift`:
- Around line 28-29: Access to readyPromise in waitUntilReady() must be
synchronized with promiseLock to avoid a race where another thread can resolve
and nil out readyPromise between checks; while holding promiseLock, read/copy
readyPromise and the ready state into local variables, then release the lock and
operate on the copied promise (or create and assign a new Promise while still
holding the lock) so that waitUntilReady(), any initialiser resolution paths,
and other callers use the same protected snapshot. Specifically modify
waitUntilReady() to acquire promiseLock, inspect/set readyPromise and isReady
under that lock, store the reference into a local variable (e.g., localPromise)
and return/use that localPromise after releasing the lock to ensure the returned
promise cannot be lost by an interleaving thread; apply the same locking pattern
wherever readyPromise is read or written.
In `@Tests/ABSmartlyTests/ContextTest.swift`:
- Around line 2263-2292: The test testRetryMechanismActivation incorrectly
assumes Context.publish() will retry via the handler; update the test to
actually exercise retry logic by either renaming the test to reflect it only
tests a single-failure path or modify it to call the retry mechanism: drive
multiple publish attempts (e.g. loop calling context.publish() until success or
maxAttempts) or trigger whatever higher-level retry API exists, using
handler.publishEventClosure to simulate transient failures and assert
publishAttempts reaches the expected retry count; reference Context.publish(),
handler.publishEventClosure, publishAttempts and maxAttempts when making the
change.
---
Nitpick comments:
In `@Sources/ABSmartly/Internal/Hashing/MurmurHash.swift`:
- Line 3: MurmurHash is a utility container with only static members; replace
the top-level class declaration with a caseless enum to prevent instantiation
and follow Swift idioms: change the declaration of MurmurHash from "class
MurmurHash" to "enum MurmurHash" (no cases), keep all existing static
functions/properties (e.g., any static methods inside MurmurHash) unchanged, and
remove any initializers or references that create instances of MurmurHash if
present.
In `@Tests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swift`:
- Around line 13-14: Replace the boolean-based assertion that inspects
.boolValue with a direct JSON equality check so the null-needle case is asserted
precisely; specifically, change the assertion that calls
inOperator.evaluate(evaluator, [JSON.null, "abcdefghijk"]) to compare the
returned JSON directly (e.g., XCTAssertEqual(JSON.null,
inOperator.evaluate(evaluator, [JSON.null, "abcdefghijk"]))) instead of using
.boolValue, keeping the same inOperator and evaluator references.
In `@Tests/ABSmartlyTests/PerformanceTests.swift`:
- Around line 21-30: In setUp(), reinitialize the clock mock to avoid shared
mutable state by adding clock = ClockMock() before setting
clock.millisReturnValue; update the setUp() method (the initializer block that
assigns provider, handler, logger, parser, scheduler) so it explicitly reassigns
clock using ClockMock and then sets clock.millisReturnValue = 1_620_000_000_000
to match the other mock initializations.
- Around line 194-210: The test named testCacheMemoryUsage is misleading because
it doesn't measure memory; rename the test method to something like
testPendingCountAfterTreatmentsAndGoals (update the function declaration for
testCacheMemoryUsage) and update any references/uses in the test suite; if you
intended a performance benchmark, wrap the exercise (the loop creating
treatments/goals and the XCTAssertEqual checks) in an XCTest measure { } block
instead of just renaming so it appears in Xcode performance metrics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e82d2d12-2ad9-4dc2-8c7b-aca7180befe4
📒 Files selected for processing (12)
README.mdSources/ABSmartly/Context.swiftSources/ABSmartly/Internal/Hashing/MurmurHash.swiftTests/ABSmartlyTests/ContextTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/EqualsOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/GreaterThanOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/GreaterThanOrEqualOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/LessThanOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/LessThanOrEqualOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swiftTests/ABSmartlyTests/PerformanceTests.swift
| private func jsonObjectToNative(_ jsonObject: Any) -> Any? { | ||
| if jsonObject is NSNull { | ||
| return nil | ||
| } else if let dict = jsonObject as? [String: Any] { | ||
| var result: [String: Any] = [:] | ||
| for (key, value) in dict { | ||
| if let nativeValue = jsonObjectToNative(value) { | ||
| result[key] = nativeValue | ||
| } | ||
| } | ||
| return result | ||
| } else if let array = jsonObject as? [Any] { | ||
| return array.compactMap { jsonObjectToNative($0) } | ||
| } else if let string = jsonObject as? String { | ||
| return string | ||
| } else if let bool = jsonObject as? Bool { | ||
| return bool | ||
| } else if let number = jsonObject as? NSNumber { | ||
| return number | ||
| } | ||
| return jsonObject |
There was a problem hiding this comment.
Do not drop nested JSON null values when coercing custom fields.
Line 1035 and Line 1041 filter null members out of objects and arrays entirely, so {"a": null} becomes {} and [1, null, 2] becomes [1, 2]. That changes the custom-field payload rather than preserving it. Keep NSNull() for nested nulls and only map a top-level JSON null to nil.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/Context.swift` around lines 1029 - 1049, The helper
jsonObjectToNative currently drops nested NSNulls; change its signature to
jsonObjectToNative(_ jsonObject: Any, topLevel: Bool = true) and make it return
NSNull() for JSON nulls when topLevel is false (only return nil when topLevel is
true), update all recursive calls (inside the dict loop and array mapping) to
pass topLevel: false, replace the array compactMap with map to preserve NSNull
placeholders, and ensure the dictionary loop assigns result[key] = nativeValue
(allowing NSNull values) rather than skipping entries when a nested null is
encountered.
On refresh, previously all assignment exposed flags were reset causing duplicate exposure events for unchanged experiments. Now only experiments where data actually changed (id, iteration, fullOnVariant, trafficSplit) have their exposed flag reset. Also removes closed check from setOverride to match JS SDK behavior.
Store the failed error in setDataFailed and expose via readyError() returning nil on success or the Error on failure
…andardize error messages Adds finalize(), isFinalized(), and isFinalizing() as aliases for close(), isClosed(), and isClosing() to align with JS SDK terminology. Standardizes the not-ready error message to remove the extra callsite hint.
Use lowercase 'ABsmartly' prefix, standard unit error formats, and add period to UID blank error message.
- Bump swift-tools-version to 5.6 to match swift-crypto 3.0.0 minimum requirement - Fix README examples: getData() -> getContextData(), fix peekTreatment call syntax - Remove unused promiseLock declaration from Context - Fix unconditional ellipsis suffix in AudienceMatcher, DefaultVariableParser, and Context error logs - Use UInt32(truncatingIfNeeded:) in Buffers to handle negative Int values safely - Compare unit immutability guard against trimmed UID rather than raw input - Fix setUnit error message to distinguish closing vs closed state
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ABSmartly/DefaultHTTPClient.swift (1)
118-120:⚠️ Potential issue | 🟠 MajorRetry budget for 502/503 is off by one vs the generic retry helper.
With the current 1-based
attemptcounter,attempt < retriesgives one fewer retry for 502/503 than for transport errors.🐛 Suggested fix
- if (rsp.statusCode == 502 || rsp.statusCode == 503) - && (attempt < self.config.retries) + if (rsp.statusCode == 502 || rsp.statusCode == 503) + && (attempt <= self.config.retries)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 118 - 120, The retry condition for 502/503 uses a 1-based attempt counter and currently checks "attempt < self.config.retries", which yields one fewer retry than the generic transport-error path; update the conditional in DefaultHTTPClient's retry loop that references rsp.statusCode, attempt, and self.config.retries so it uses the same inclusive semantics as the generic helper (e.g., change the comparison to allow attempt <= self.config.retries or otherwise align the bounds) so 502/503 get the same retry budget.
♻️ Duplicate comments (2)
README.md (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd proper badge alt text and correct the stale RxSwift label.
Line 1 still has an image without
alt, and the surrounding link metadata still says “RxSwift”, which is misleading.🔧 Suggested fix
-# A/B Smartly Swift SDK <a href="https://github.com/apple/swift-package-manager" alt="RxSwift on Swift Package Manager" title="RxSwift on Swift Package Manager"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" /></a> +# A/B Smartly Swift SDK <a href="https://github.com/apple/swift-package-manager" title="Swift Package Manager compatible"><img src="https://img.shields.io/badge/Swift%20Package%20Manager-compatible-brightgreen.svg" alt="Swift Package Manager compatible badge" /></a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 1, The README header's badge link still references "RxSwift" and the <img> lacks alt text; update the anchor attributes and image tag so they correctly reference "Swift Package Manager" (not RxSwift) and add descriptive alt text for the badge (e.g., alt="Swift Package Manager compatible"). Locate the header line that contains the anchor and img tag and change the title/alt attributes and any visible label that mentions "RxSwift" to "Swift Package Manager" while keeping the badge URL and appearance unchanged.Sources/ABSmartly/DefaultHTTPClient.swift (1)
145-150:⚠️ Potential issue | 🟠 Major
close()promise still resolves before session invalidation completes.
finishTasksAndInvalidate()is asynchronous, butclose()resolves immediately, so callers may proceed before shutdown has actually finished.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/DefaultHTTPClient.swift` around lines 145 - 150, The close() method currently calls session?.finishTasksAndInvalidate() but returns immediately; change it to return a Promise that resolves only after the session has actually invalidated by wiring the Promise resolution into the URLSessionDelegate callback: create a pendingClose Promise/Fulfill (or reuse a stored resolver) on the class, call session?.finishTasksAndInvalidate() inside close(), and in the delegate method urlSession(_:didBecomeInvalidWithError:) fulfill that pending promise (also handle the case where session is already nil by fulfilling immediately). Keep using sessionLock when touching session and the pending resolver to avoid races and ensure close()'s returned Promise resolves after invalidation completes.
🧹 Nitpick comments (9)
Tests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swift (2)
47-53: Test name does not reflect actual behaviour.The test is named
testNoSemaphoreThreadLeakbut only performs basic regex matching without verifying thread counts, semaphore states, or resource cleanup. This could mislead future maintainers into thinking thread-safety is being verified when it is not.Either rename the test to something like
testBasicMatchingSucceeds, or implement actual thread-leak verification (e.g. run many iterations in a loop and check that thread/resource counts remain stable).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swift` around lines 47 - 53, Rename or update the test to reflect its real behavior: either rename testNoSemaphoreThreadLeak to testBasicMatchingSucceeds (or similar) and keep the two assertions that call matchOperator.evaluate(evaluator, ...) and XCTAssertTrue/XCTAssertFalse, or implement real thread/semaphore leak checks by running many iterations of matchOperator.evaluate(evaluator, ...) (using a loop) and asserting system resource stability after the loop (e.g., stable thread count or no semaphore contention). Ensure references to matchOperator.evaluate and evaluator remain correct when renaming or adding the loop-based verification.
40-45: Consider adding exact boundary tests.This test verifies values one below the limit. For completeness, consider also testing exactly at the boundary (1000 and 10000) to ensure the implementation uses
>rather than>=correctly.💡 Suggested additional test case
func testAcceptsPatternAndInputAtExactLimits() { let pattern = String(repeating: "a", count: 1000) let input = String(repeating: "a", count: 10000) let result = matchOperator.evaluate(evaluator, [input, pattern]) XCTAssertTrue(result.boolValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swift` around lines 40 - 45, Add an exact-boundary unit test in MatchOperatorTest.swift to verify the operator accepts pattern length 1000 and input length 10000 (not just one below); create a new test method (e.g., testAcceptsPatternAndInputAtExactLimits) that builds pattern = String(repeating: "a", count: 1000) and input = String(repeating: "a", count: 10000), calls matchOperator.evaluate(evaluator, [input, pattern]) and asserts result.boolValue is true to ensure the implementation uses the correct strictness.Tests/ABSmartlyTests/JsonExpr/Operators/BinaryOperatorNullSafetyTest.swift (1)
52-60: Strengthen Match null-safety assertions with concrete expected values.The
XCTAssertNotNil(result)checks at lines 54 and 59 are tautological becauseevaluate()returns non-optionalJSON, never nil. This leaves the tests verifying only that no crash occurs, not that the operator handles null arguments correctly.MatchOperator's behaviour is defined and testable: when LHS is null, it returns false; when RHS is null, it returns true. Use
XCTAssertEqualor test.boolValueto assert these concrete results instead, aligning with the pattern used by other binary operator tests in this file (e.g.testEqualsNullNull,testInWithNullHaystack).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/JsonExpr/Operators/BinaryOperatorNullSafetyTest.swift` around lines 52 - 60, The tests testMatchWithNullLhsDoesNotCrash and testMatchWithNullRhsDoesNotCrash currently only assert non-nil but should assert concrete boolean results from matchOp.evaluate(evaluator, ...). Change the assertions to check the JSON boolean value: when called with [JSON.null, "abc"] assert the result is false (e.g., compare result.boolValue or XCTAssertEqual to false), and when called with ["abc", JSON.null] assert the result is true; keep using matchOp.evaluate and evaluator as before.Tests/ABSmartlyTests/Internal/MD5Test.swift (1)
25-93: Consider consolidating duplicate test data.The individual test methods duplicate the values already defined in
testCases. Whilst having named test methods provides clearer test output, the duplication introduces maintenance overhead if expected values need updating.One alternative is to use a single parameterised test that provides context on failure:
func testAllHashCases() { for (input, expected) in testCases { XCTAssertEqual(Hashing.hash(input), expected, "Hash mismatch for input: '\(input)'") } }However, keeping the individual tests is a valid choice for test isolation and IDE integration. This is a style preference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/Internal/MD5Test.swift` around lines 25 - 93, The tests duplicate expected input/outputs already present in testCases; replace the many individual test methods with a single parameterised loop (e.g. add a testAllHashCases function) that iterates for (input, expected) in testCases and asserts XCTAssertEqual(Hashing.hash(input), expected, "Hash mismatch for input: '\(input)'") to remove duplication while preserving per-case failure context; alternatively, if you prefer to keep separate methods for IDE visibility, derive each test's expected value from testCases to avoid hard-coded duplicates.README.md (1)
382-387: Avoidtry!in README initialisation examples.Line 382 uses
try!, which models crash-prone integration patterns in production apps.♻️ Safer sample pattern
- private init() { - sdk = try! ABsmartlySDK( - endpoint: "https://your-company.absmartly.io/v1", - apiKey: ProcessInfo.processInfo.environment["ABSMARTLY_API_KEY"] ?? "", - application: "ios-app", - environment: "production" - ) - } + private init() { + do { + sdk = try ABsmartlySDK( + endpoint: "https://your-company.absmartly.io/v1", + apiKey: ProcessInfo.processInfo.environment["ABSMARTLY_API_KEY"] ?? "", + application: "ios-app", + environment: "production" + ) + } catch { + fatalError("Failed to initialise ABsmartlySDK: \(error)") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 382 - 387, The README example uses force-throwing construction with try! when creating ABsmartlySDK which encourages crashes; replace it with a safe pattern that either uses do-try-catch around ABsmartlySDK(...) and logs or surfaces initialization errors, or use a non-crashing optional pattern with let sdk = try? ABsmartlySDK(...) and handle the nil case (e.g., log an error and abort gracefully or show fallback), ensuring the ABsmartlySDK initializer call and the apiKey/application/environment values are the same identifiers referenced in the example.Tests/ABSmartlyTests/DefaultHTTPClientTest.swift (1)
11-39:MockURLProtocolis introduced but not exercised by these tests.Either wire it into the client’s
URLSessionConfiguration.protocolClassesvia injection, or remove it until a deterministic request-path test uses it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/DefaultHTTPClientTest.swift` around lines 11 - 39, MockURLProtocol is defined but never used by the tests; either register it on the URLSession used by the SUT or delete it. Fix by creating a URLSessionConfiguration (e.g., .ephemeral), set configuration.protocolClasses = [MockURLProtocol.self], build a URLSession(configuration:) and pass that session into the DefaultHTTPClient (or whatever initializer/method constructs the client) so MockURLProtocol.requestHandler is exercised during requests; alternatively remove the MockURLProtocol and its requestHandler if you don't plan to inject a custom session.Tests/ABSmartlyTests/ConcurrencyTests.swift (2)
204-231: Potential test flakiness in concurrent unit assertions.The assertion on line 230 (
XCTAssertEqual(units.count, 50)) runs after the expectation fulfils but the expectation only waits for all async blocks to start executingsetUnitorgetUnits. There's a small window wheregetUnits()in the assertion could execute before allsetUnitcalls complete.Consider adding a small delay before the final assertion or using a separate completion barrier.
💡 Consider adding a barrier to ensure all writes complete
wait(for: [expectation], timeout: 10.0) + // Allow any in-flight setUnit calls to complete + Thread.sleep(forTimeInterval: 0.1) + let units = context.getUnits() XCTAssertEqual(units.count, 50)Alternatively, use
concurrentQueue.sync(flags: .barrier) {}before the assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ConcurrencyTests.swift` around lines 204 - 231, The final assertion in testConcurrentSetAndGetUnits is flaky because the expectation only counts task starts; ensure all setUnit writes finish before calling context.getUnits() by adding a completion barrier on concurrentQueue (use concurrentQueue.sync(flags: .barrier) { } after dispatching the async tasks and before calling context.getUnits()), or alternatively increment the expectation only after each setUnit completes and wait for that — target the testConcurrentSetAndGetUnits function and synchronize around context.setUnit/context.getUnits to guarantee all writes are visible before the XCTAssertEqual.
233-260: Same potential flakiness concern for attribute assertions.Similar to the unit test, line 259's assertion could be racy if any
setAttributecalls are still in flight whengetAttributes()is called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ABSmartlyTests/ConcurrencyTests.swift` around lines 233 - 260, The final assertion in testConcurrentAttributeAccess can still race because some setAttribute(name: "attr_<i>", value:) calls might not have completed before the final getAttributes() is checked; modify the test to wait specifically for all setAttribute tasks to finish (e.g., use a DispatchGroup or a separate XCTestExpectation only fulfilled inside the setAttribute async blocks) and wait on that before calling context.getAttributes() and asserting count, ensuring setAttribute completions are the synchronization point rather than mixing them with the concurrent getAttributes() work.Sources/ABSmartly/ABSmartlySDK.swift (1)
18-31: Redundant guard in theelsebranch.The
elsebranch on line 25 is only entered when the condition on line 18 (config.contextDataProvider == nil || config.contextPublisher == nil) is false, meaning both values are non-nil. The guard on lines 26-28 can therefore never fail.♻️ Simplify by removing the redundant guard
} else { - guard let provider = config.contextDataProvider, let handler = config.contextPublisher else { - throw ABSmartlyError("Missing contextDataProvider or contextPublisher") - } - contextDataProvider = provider - contextEventHandler = handler + contextDataProvider = config.contextDataProvider! + contextEventHandler = config.contextPublisher! }Or, restructure the logic more clearly:
- if config.contextDataProvider == nil || config.contextPublisher == nil { + if let provider = config.contextDataProvider, let publisher = config.contextPublisher { + contextDataProvider = provider + contextEventHandler = publisher + } else { guard let client = client else { throw ABSmartlyError("Missing Client instance") } - contextDataProvider = config.contextDataProvider ?? DefaultContextDataProvider(client: client) - contextEventHandler = config.contextPublisher ?? DefaultContextPublisher(client: client) - } else { - guard let provider = config.contextDataProvider, let handler = config.contextPublisher else { - throw ABSmartlyError("Missing contextDataProvider or contextPublisher") - } - contextDataProvider = provider - contextEventHandler = handler + contextEventHandler = config.contextPublisher ?? DefaultContextPublisher(client: client) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ABSmartly/ABSmartlySDK.swift` around lines 18 - 31, The else branch contains a redundant guard because the outer if already ensures both config.contextDataProvider and config.contextPublisher are non-nil; remove the guard and directly assign the unwrapped values to contextDataProvider and contextEventHandler (e.g., contextDataProvider = config.contextDataProvider! and contextEventHandler = config.contextPublisher!), or better yet refactor the whole block to use a single guard-let at the top (guard let client = client else { ... }) and a separate guard-let for provider/handler when appropriate (referencing config.contextDataProvider, config.contextPublisher, contextDataProvider, contextEventHandler, and ABSmartlySDK).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 119-122: Update the README table to show contextPublisher as the
primary configuration option instead of contextEventHandler: replace or reorder
the row for `contextEventHandler` to indicate it's deprecated/legacy and add a
new row for `contextPublisher` (`ContextPublisher`) as the main API with a clear
description ("Primary publisher for SDK events (ready, exposure, goal, etc.)").
Ensure the table still documents `contextEventHandler` (or marks it as legacy)
and update any mentions of `ContextEventHandler`/`ContextEventHandler` types to
point readers to `ContextPublisher`/`ContextPublisher` for new integrations.
In `@Sources/ABSmartly/Context.swift`:
- Around line 108-138: The Context class references self.promiseLock when
accessing readyPromise but the promiseLock property is not declared; add a
private lock member (e.g., declare private let promiseLock = NSLock() alongside
the other lock properties in the Context class) so calls in the readyPromise
creation (the promise.done / .catch blocks that call promiseLock.lock() /
unlock()) compile and correctly synchronize access to readyPromise.
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Line 104: The code sets request.timeoutInterval to the resource timeout
(self.config.connectionResourceTimeout) incorrectly; update the assignment in
DefaultHTTPClient (the line assigning request.timeoutInterval) to use the
request timeout config field (e.g., self.config.requestTimeout or the
appropriate request-specific timeout property on your Config object) so
request.timeoutInterval reflects the intended request timeout rather than the
connection resource timeout.
In `@Sources/ABSmartly/Internal/Hashing/Hashing.swift`:
- Around line 44-50: Change the visibility of the Hashing utility and its
exposed method to internal: make the class declaration "internal class Hashing"
(instead of public) and change "public static func hash(_ unit: String)" to
"static func hash(_ unit: String)" so both the class and its hash(_: ) method
are internal; leave hashBytes(_:) as-is. Update any tests that reference the
now-internal method if needed (e.g., adjust test access or use the internal
`@testable` import) and confirm callers like Context that use hashBytes(_:) still
compile.
In `@Tests/ABSmartlyTests/DefaultHTTPClientTest.swift`:
- Around line 89-92: The testBadURL in DefaultHTTPClientTest.swift currently
asserts only that the error is non-nil; change this to assert the specific
URLError code (badURL) to tighten semantics by casting the received error to
URLError (or checking (error as? URLError)?.code) and using XCTAssertEqual or
XCTAssertTrue to verify the code equals .badURL, then fulfill the expectation;
update the assertion in the catch block where the closure receives `error`.
---
Outside diff comments:
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Around line 118-120: The retry condition for 502/503 uses a 1-based attempt
counter and currently checks "attempt < self.config.retries", which yields one
fewer retry than the generic transport-error path; update the conditional in
DefaultHTTPClient's retry loop that references rsp.statusCode, attempt, and
self.config.retries so it uses the same inclusive semantics as the generic
helper (e.g., change the comparison to allow attempt <= self.config.retries or
otherwise align the bounds) so 502/503 get the same retry budget.
---
Duplicate comments:
In `@README.md`:
- Line 1: The README header's badge link still references "RxSwift" and the
<img> lacks alt text; update the anchor attributes and image tag so they
correctly reference "Swift Package Manager" (not RxSwift) and add descriptive
alt text for the badge (e.g., alt="Swift Package Manager compatible"). Locate
the header line that contains the anchor and img tag and change the title/alt
attributes and any visible label that mentions "RxSwift" to "Swift Package
Manager" while keeping the badge URL and appearance unchanged.
In `@Sources/ABSmartly/DefaultHTTPClient.swift`:
- Around line 145-150: The close() method currently calls
session?.finishTasksAndInvalidate() but returns immediately; change it to return
a Promise that resolves only after the session has actually invalidated by
wiring the Promise resolution into the URLSessionDelegate callback: create a
pendingClose Promise/Fulfill (or reuse a stored resolver) on the class, call
session?.finishTasksAndInvalidate() inside close(), and in the delegate method
urlSession(_:didBecomeInvalidWithError:) fulfill that pending promise (also
handle the case where session is already nil by fulfilling immediately). Keep
using sessionLock when touching session and the pending resolver to avoid races
and ensure close()'s returned Promise resolves after invalidation completes.
---
Nitpick comments:
In `@README.md`:
- Around line 382-387: The README example uses force-throwing construction with
try! when creating ABsmartlySDK which encourages crashes; replace it with a safe
pattern that either uses do-try-catch around ABsmartlySDK(...) and logs or
surfaces initialization errors, or use a non-crashing optional pattern with let
sdk = try? ABsmartlySDK(...) and handle the nil case (e.g., log an error and
abort gracefully or show fallback), ensuring the ABsmartlySDK initializer call
and the apiKey/application/environment values are the same identifiers
referenced in the example.
In `@Sources/ABSmartly/ABSmartlySDK.swift`:
- Around line 18-31: The else branch contains a redundant guard because the
outer if already ensures both config.contextDataProvider and
config.contextPublisher are non-nil; remove the guard and directly assign the
unwrapped values to contextDataProvider and contextEventHandler (e.g.,
contextDataProvider = config.contextDataProvider! and contextEventHandler =
config.contextPublisher!), or better yet refactor the whole block to use a
single guard-let at the top (guard let client = client else { ... }) and a
separate guard-let for provider/handler when appropriate (referencing
config.contextDataProvider, config.contextPublisher, contextDataProvider,
contextEventHandler, and ABSmartlySDK).
In `@Tests/ABSmartlyTests/ConcurrencyTests.swift`:
- Around line 204-231: The final assertion in testConcurrentSetAndGetUnits is
flaky because the expectation only counts task starts; ensure all setUnit writes
finish before calling context.getUnits() by adding a completion barrier on
concurrentQueue (use concurrentQueue.sync(flags: .barrier) { } after dispatching
the async tasks and before calling context.getUnits()), or alternatively
increment the expectation only after each setUnit completes and wait for that —
target the testConcurrentSetAndGetUnits function and synchronize around
context.setUnit/context.getUnits to guarantee all writes are visible before the
XCTAssertEqual.
- Around line 233-260: The final assertion in testConcurrentAttributeAccess can
still race because some setAttribute(name: "attr_<i>", value:) calls might not
have completed before the final getAttributes() is checked; modify the test to
wait specifically for all setAttribute tasks to finish (e.g., use a
DispatchGroup or a separate XCTestExpectation only fulfilled inside the
setAttribute async blocks) and wait on that before calling
context.getAttributes() and asserting count, ensuring setAttribute completions
are the synchronization point rather than mixing them with the concurrent
getAttributes() work.
In `@Tests/ABSmartlyTests/DefaultHTTPClientTest.swift`:
- Around line 11-39: MockURLProtocol is defined but never used by the tests;
either register it on the URLSession used by the SUT or delete it. Fix by
creating a URLSessionConfiguration (e.g., .ephemeral), set
configuration.protocolClasses = [MockURLProtocol.self], build a
URLSession(configuration:) and pass that session into the DefaultHTTPClient (or
whatever initializer/method constructs the client) so
MockURLProtocol.requestHandler is exercised during requests; alternatively
remove the MockURLProtocol and its requestHandler if you don't plan to inject a
custom session.
In `@Tests/ABSmartlyTests/Internal/MD5Test.swift`:
- Around line 25-93: The tests duplicate expected input/outputs already present
in testCases; replace the many individual test methods with a single
parameterised loop (e.g. add a testAllHashCases function) that iterates for
(input, expected) in testCases and asserts XCTAssertEqual(Hashing.hash(input),
expected, "Hash mismatch for input: '\(input)'") to remove duplication while
preserving per-case failure context; alternatively, if you prefer to keep
separate methods for IDE visibility, derive each test's expected value from
testCases to avoid hard-coded duplicates.
In `@Tests/ABSmartlyTests/JsonExpr/Operators/BinaryOperatorNullSafetyTest.swift`:
- Around line 52-60: The tests testMatchWithNullLhsDoesNotCrash and
testMatchWithNullRhsDoesNotCrash currently only assert non-nil but should assert
concrete boolean results from matchOp.evaluate(evaluator, ...). Change the
assertions to check the JSON boolean value: when called with [JSON.null, "abc"]
assert the result is false (e.g., compare result.boolValue or XCTAssertEqual to
false), and when called with ["abc", JSON.null] assert the result is true; keep
using matchOp.evaluate and evaluator as before.
In `@Tests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swift`:
- Around line 47-53: Rename or update the test to reflect its real behavior:
either rename testNoSemaphoreThreadLeak to testBasicMatchingSucceeds (or
similar) and keep the two assertions that call matchOperator.evaluate(evaluator,
...) and XCTAssertTrue/XCTAssertFalse, or implement real thread/semaphore leak
checks by running many iterations of matchOperator.evaluate(evaluator, ...)
(using a loop) and asserting system resource stability after the loop (e.g.,
stable thread count or no semaphore contention). Ensure references to
matchOperator.evaluate and evaluator remain correct when renaming or adding the
loop-based verification.
- Around line 40-45: Add an exact-boundary unit test in MatchOperatorTest.swift
to verify the operator accepts pattern length 1000 and input length 10000 (not
just one below); create a new test method (e.g.,
testAcceptsPatternAndInputAtExactLimits) that builds pattern = String(repeating:
"a", count: 1000) and input = String(repeating: "a", count: 10000), calls
matchOperator.evaluate(evaluator, [input, pattern]) and asserts result.boolValue
is true to ensure the implementation uses the correct strictness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dda58f42-659c-4d2c-b477-b2202caafd2d
📒 Files selected for processing (25)
Package.swiftREADME.mdSources/ABSmartly/ABSmartlyConfig.swiftSources/ABSmartly/ABSmartlySDK.swiftSources/ABSmartly/AudienceMatcher.swiftSources/ABSmartly/ClientConfig.swiftSources/ABSmartly/Context.swiftSources/ABSmartly/DefaultContextEventHandler.swiftSources/ABSmartly/DefaultContextPublisher.swiftSources/ABSmartly/DefaultHTTPClient.swiftSources/ABSmartly/DefaultVariableParser.swiftSources/ABSmartly/Internal/Hashing/Buffers.swiftSources/ABSmartly/Internal/Hashing/Hashing.swiftSources/ABSmartly/JsonExpr/Operators/MatchOperator.swiftSources/ABSmartly/Protocols/ContextEventHandler.swiftSources/ABSmartly/Protocols/ContextPublisher.swiftTests/ABSmartlyTests/ConcurrencyTests.swiftTests/ABSmartlyTests/ContextTest.swiftTests/ABSmartlyTests/DefaultHTTPClientTest.swiftTests/ABSmartlyTests/Internal/MD5Test.swiftTests/ABSmartlyTests/JsonExpr/Operators/BinaryOperatorNullSafetyTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swiftTests/ABSmartlyTests/JsonExpr/Operators/MatchOperatorTest.swiftTests/ABSmartlyTests/PerformanceTests.swiftTests/ABSmartlyTests/VariantAssignerTest.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/ABSmartly/Internal/Hashing/Buffers.swift
- Tests/ABSmartlyTests/JsonExpr/Operators/InOperatorTest.swift
- Sources/ABSmartly/DefaultVariableParser.swift
- Sources/ABSmartly/JsonExpr/Operators/MatchOperator.swift
- Package.swift
| | contextEventLogger | `ContextEventLogger` | ❌ | `nil` | Callback to handle SDK events (ready, exposure, goal, etc.) | | ||
| | contextDataProvider | `ContextDataProvider` | ❌ | auto | Custom provider for context data (advanced usage) | | ||
| | contextEventHandler | `ContextEventHandler` | ❌ | auto | Custom handler for publishing events (advanced usage) | | ||
| | variableParser | `VariableParser` | ❌ | auto | Custom parser for variable values (advanced usage) | |
There was a problem hiding this comment.
Document the new publisher API as the primary option.
Lines 119-122 still present contextEventHandler as the main config field, but the new surface is contextPublisher. This can mislead new adopters.
📝 Suggested table update
-| contextEventHandler | `ContextEventHandler` | ❌ | auto | Custom handler for publishing events (advanced usage) |
+| contextPublisher | `ContextPublisher` | ❌ | auto | Custom publisher for context events (advanced usage) |
+| contextEventHandler | `ContextEventHandler` | ❌ | deprecated | Deprecated alias for `contextPublisher` (kept for backwards compatibility) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 119 - 122, Update the README table to show
contextPublisher as the primary configuration option instead of
contextEventHandler: replace or reorder the row for `contextEventHandler` to
indicate it's deprecated/legacy and add a new row for `contextPublisher`
(`ContextPublisher`) as the main API with a clear description ("Primary
publisher for SDK events (ready, exposure, goal, etc.)"). Ensure the table still
documents `contextEventHandler` (or marks it as legacy) and update any mentions
of `ContextEventHandler`/`ContextEventHandler` types to point readers to
`ContextPublisher`/`ContextPublisher` for new integrations.
|
|
||
| var request = URLRequest(url: requestURL) | ||
| request.httpMethod = method | ||
| request.timeoutInterval = self.config.connectionResourceTimeout |
There was a problem hiding this comment.
Line 104 uses the wrong timeout field for request timeout.
request.timeoutInterval should use the request timeout config, not the resource timeout value.
🐛 Suggested fix
- request.timeoutInterval = self.config.connectionResourceTimeout
+ request.timeoutInterval = self.config.connectionRequestTimeout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request.timeoutInterval = self.config.connectionResourceTimeout | |
| request.timeoutInterval = self.config.connectionRequestTimeout |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/DefaultHTTPClient.swift` at line 104, The code sets
request.timeoutInterval to the resource timeout
(self.config.connectionResourceTimeout) incorrectly; update the assignment in
DefaultHTTPClient (the line assigning request.timeoutInterval) to use the
request timeout config field (e.g., self.config.requestTimeout or the
appropriate request-specific timeout property on your Config object) so
request.timeoutInterval reflects the intended request timeout rather than the
connection resource timeout.
| public static func hash(_ unit: String) -> String { | ||
| return MD5Base64Url(unit) | ||
| } | ||
|
|
||
| static func hash(_ unit: String) -> [UInt8] { | ||
| static func hashBytes(_ unit: String) -> [UInt8] { | ||
| return Array(MD5Base64Url(unit).utf8) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Hashing is used outside the ABSmartly module or in public documentation
rg -n "Hashing\." --type swift -g '!*Test*' | head -30Repository: absmartly/swift-sdk
Length of output: 152
🏁 Script executed:
# First, let's check the full Hashing class definition to see if the class itself is public
cat -n Sources/ABSmartly/Internal/Hashing/Hashing.swiftRepository: absmartly/swift-sdk
Length of output: 1886
🏁 Script executed:
# Search for all uses of Hashing.hash (not hashBytes)
rg -n "Hashing\.hash\(" --type swiftRepository: absmartly/swift-sdk
Length of output: 2059
🏁 Script executed:
# Check if there's any documentation or comments about the public API
rg -n "public" Sources/ABSmartly/Internal/Hashing/Hashing.swiftRepository: absmartly/swift-sdk
Length of output: 144
🏁 Script executed:
# Check the module structure to understand what's being exported
fd -e swift -path "*/Sources/ABSmartly/*" -type f | grep -E "(module|public)" | head -20Repository: absmartly/swift-sdk
Length of output: 231
🏁 Script executed:
# Look for any public.swift or Package.swift to understand public API
fd "Package.swift|module.modulemap" Sources/Repository: absmartly/swift-sdk
Length of output: 45
🏁 Script executed:
# Check if Hashing is exported in any module or public declarations
rg -n "Hashing" --type swift --glob "**/Sources/**" -g "!*Test*" | grep -v "Hashing\." | grep -v "class Hashing" | grep -v "func hash"Repository: absmartly/swift-sdk
Length of output: 45
🏁 Script executed:
# Check for any public module files or exports
find Sources/ABSmartly -name "*.swift" -type f | xargs grep -l "public"Repository: absmartly/swift-sdk
Length of output: 1694
🏁 Script executed:
# Look for any documentation files mentioning the API
find . -name "*.md" -o -name "*.rst" | xargs grep -i "hashing\|hash" 2>/dev/null | head -20Repository: absmartly/swift-sdk
Length of output: 45
🏁 Script executed:
# Check if there's a direct import of Hashing or if it's accessed through another class
rg -n "import.*Hashing" --type swiftRepository: absmartly/swift-sdk
Length of output: 45
Make Hashing class and its methods internal.
The class is currently public despite being located in the Internal directory and never being used outside tests. The public hash(_:) method is tested but unused in production code, whilst the internal hashBytes(_:) method is the only one called in production (Context.swift). This inconsistency suggests unintentional public exposure.
Unless this is deliberate public API, change public class Hashing to internal class Hashing and public static func hash to static func hash to accurately reflect that this is an internal utility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ABSmartly/Internal/Hashing/Hashing.swift` around lines 44 - 50,
Change the visibility of the Hashing utility and its exposed method to internal:
make the class declaration "internal class Hashing" (instead of public) and
change "public static func hash(_ unit: String)" to "static func hash(_ unit:
String)" so both the class and its hash(_: ) method are internal; leave
hashBytes(_:) as-is. Update any tests that reference the now-internal method if
needed (e.g., adjust test access or use the internal `@testable` import) and
confirm callers like Context that use hashBytes(_:) still compile.
| .catch { error in | ||
| XCTAssertNotNil(error) | ||
| expectation.fulfill() | ||
| } |
There was a problem hiding this comment.
Assert specific error semantics, not just non-nil errors.
In testBadURL, checking for URLError(.badURL) would better protect the intended contract than XCTAssertNotNil(error) alone.
💡 Suggested assertion tightening
- .catch { error in
- XCTAssertNotNil(error)
+ .catch { error in
+ guard case let urlError as URLError = error else {
+ XCTFail("Expected URLError, got \(type(of: error))")
+ expectation.fulfill()
+ return
+ }
+ XCTAssertEqual(urlError.code, .badURL)
expectation.fulfill()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/ABSmartlyTests/DefaultHTTPClientTest.swift` around lines 89 - 92, The
testBadURL in DefaultHTTPClientTest.swift currently asserts only that the error
is non-nil; change this to assert the specific URLError code (badURL) to tighten
semantics by casting the received error to URLError (or checking (error as?
URLError)?.code) and using XCTAssertEqual or XCTAssertTrue to verify the code
equals .badURL, then fulfill the expectation; update the assertion in the catch
block where the closure receives `error`.
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.
…ndling) A null operand short-circuits binary operators to null — eq(null, null) is null, not true. This matches origin/main of every SDK and the collector. The branch had introduced an eq override (or removed the base null-skip) that made eq(null,null) true, diverging from the canonical behavior. Revert to the null-skip behavior and align the operator tests.
Align eq/gte/lte/match null-operand unit tests with the SDK fix (2f50e8e) where BinaryOperator short-circuits a null operand to null, matching the collector.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Approved per request. Missing PR-triggered CI and production HTTP coverage. DefaultClientTest injects HTTPClientMock; DefaultHTTPClientTest defines MockURLProtocol, but the production client constructs its own ephemeral URLSession, so the tests do not verify real GET/PUT request behavior. Please add CI and deterministic production-client HTTP tests.
Real 127.0.0.1 socket server (no URLProtocol mocks) drives the production client through GET /context and PUT /context, asserting the wire contract. Add CI workflow running tests on push + pull_request.
|
@Pedro-Revez-Silva Addressed. PR CI: the repo had no `.github` at all — added `.github/workflows/main.yml` (commit `955a1a3`) running `swift build`/`swift test` on macOS for `push` + `pull_request`. Production HTTP test (no URLProtocol mocks): added `Tests/.../HTTPIntegrationTest.swift` — a real `NWListener` HTTP server on 127.0.0.1 (ephemeral port), not a URLProtocol mock. The production `DefaultHTTPClient` builds its own URLSession and makes genuine TCP requests against it. Drives the public API (`ABsmartlySDK(...) → createContext → waitUntilReady → getTreatment + track → publish()`) and asserts the wire contract for GET /context and PUT /context. This directly addresses your note that the previous MockURLProtocol approach did not exercise the real client. |
…overy off main queue
JSON custom-field decoding used `jsonObject as? Bool`, but Foundation's
NSNumber bridges any 0/1 numeric value to Swift Bool via `as?`, not just
genuine JSON true/false literals — so {"123":1,"456":0} was decoded as
{"123":true,"456":false}. Now checks NSNumber's objCType to distinguish
real booleans from 0/1 integers.
flush()'s `.recover` handler (which restores pending exposures/goals and
rethrows on publish failure) had no explicit dispatch queue, so PromiseKit
defaulted it to DispatchQueue.main. Outside a UIKit/AppKit run loop nothing
services that queue, so the handler silently never ran and the promise
hung forever instead of surfacing the failure. Dispatches on
DispatchQueue.global() to match the preceding `.done`.
joalves
changed the title
2 participants
Summary
Bugs found and fixed during verification
jsonObjectToNative()usedjsonObject as? Bool, which Foundation's NSNumber bridging matches for any numeric value that is exactly 0 or 1, not just genuine JSON boolean literals. Fixed by checking the NSNumber'sobjCTypeinstead.flush()'s.recoverhandler (which restores pending exposures/goals on failure) had no explicit dispatch queue, so PromiseKit defaulted it toDispatchQueue.main— which nothing services outside a UIKit/AppKit run loop, so the handler never ran in server-side usage. Fixed by running it onDispatchQueue.global(), matching the existing.donehandler in the same method..valueinstead of its own.asyncValue()helper, silently converting a hung/failed promise into an apparent success. Fixed there too (cross-sdk-tests commit a312c4c).Test Results
swift buildsucceeds, exit code 0Changes (19 files, +493 -220, plus the fixes above)
MatchOperator.swift- ReDoS protection with timeout + nested quantifier detectionContext.swift- Flush only clears after success, atomic ready flag, error logging, JSON custom-field boolean-coercion fix, publish-failure recovery queue fixABSmartlySDK.swift- Comprehensive error handlingExperiment.swift- Strict decoding, removed mass try? usageContextData.swift- Proper error propagationDefaultHTTPClient.swift- Atomic retry counterBuffers.swift,MurmurHash.swift- Dead code removalOrCombinator.swift- Final annotationApplication.swift- Changed class to structContextConfig.swift- forEach to for..inTest plan
swift buildsucceeds with zero errorsSummary by CodeRabbit
New Features
Breaking Changes
Bug Fixes
Documentation
Tests
Chores