[ECO-5633] Refactor executeRequest

[maratal]

Closes #2135

Summary by CodeRabbit

• Chores

  • Per-request HTTP timeouts and a unified request pipeline for consistent request preparation and handling.
  • Normalized error responses, clarified token-renewal retry behavior, and improved host fallback/retry resilience.
  • Callbacks now receive consistent, structured error information across REST flows.

• Bug Fixes

  • Prevented post-failure continuation in a push registration error path to avoid duplicate processing.

• Tests

  • Updated tests for the new request pipeline and added assertions for specific auth error descriptions.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Centralizes and renames ARTRest HTTP execution: introduces per-request timeouts, new entry points executeAblyRequest and executeExternalRequest, and a unified completion handler with token-renew single-retry and fallback-host retrying; updates all internal call sites to the new APIs.

Changes

Cohort / File(s)	Summary
Core REST surface & internals Source/ARTRest.m, PrivateHeaders/Ably/ARTRest+Private.h	Introduces executeAblyRequest and executeExternalRequest entry points; adds handleAblyRestRequestCompletion: for content-type validation, token-renew single-retry, HTTP error decoding, fallback retry/host switching, and request normalization (Accept, timeout, Ably-Agent, client id, request_id).
Auth & token flows Source/ARTAuth.m	Sets per-request timeout on built requests and switches auth-related REST calls to the new executeExternalRequest / executeAblyRequest paths.
Pagination & paginated executor Source/ARTHTTPPaginatedResponse.m, Source/ARTPaginatedResult.m	Replaces executeRequest usages with executeAblyRequest; introduces ARTPaginatedResult class method executePaginated:withRequest:andResponseProcessor:... to drive paginated calls through the Ably request pipeline.
Push subsystem Source/ARTPushActivationStateMachine.m, Source/ARTPushAdmin.m, Source/ARTPushChannel.m, Source/ARTPushChannelSubscriptions.m, Source/ARTPushDeviceRegistrations.m	Replaces executeRequest with executeAblyRequest across registration, update, sync, unregistration, publish and subscription flows; preserves status handling. Adds an early return after emitting failure in deviceRegistration.
Channel / Annotations / Message paths Source/ARTRestChannel.m, Source/ARTRestAnnotations.m	Swaps calls to use executeAblyRequest while keeping parameters and completion handling unchanged.
Tests Test/AblyTests/Tests/... RealtimeClientConnectionTests.swift, RestClientTests.swift, UtilitiesTests.swift, AuthTests.swift	Updated test call sites from execute/executeRequest to executeAblyRequest; added two assertions in auth tests checking error description content.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client
    participant ARTRest as ARTRestInternal
    participant Auth as Authentication
    participant Network as NetworkLayer
    participant Fallback as ARTFallback

    Client->>ARTRest: executeAblyRequest(request, authOption, wrapperSDKAgents)
    alt authOption == Off
        ARTRest->>Network: executeExternalRequest(request)
        Network-->>ARTRest: response / data / error
    else needs auth
        ARTRest->>Auth: obtain token or sign request
        Auth-->>ARTRest: token / signedRequest
        ARTRest->>Network: executeExternalRequest(signedRequest)
        Network-->>ARTRest: response / data / error
    end

    ARTRest->>ARTRest: handleAblyRestRequestCompletion(response, data, error, requestId)
    alt invalid content-type or decode error
        ARTRest-->>Client: error (with requestId)
    else HTTP error and shouldRenewToken
        ARTRest->>Auth: renew token (single retry)
        Auth-->>ARTRest: new token / error
        ARTRest->>Network: retry request with new token
        Network-->>ARTRest: response / data / error
        ARTRest-->>Client: response or error
    else HTTP error and shouldRetryWithFallback
        ARTRest->>Fallback: choose alternate host
        ARTRest->>Network: retry request on fallback host
        Network-->>ARTRest: response / data / error
        ARTRest-->>Client: response or error
    else success
        ARTRest-->>Client: response / data (update prioritized host)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through headers, timeout in paw,
Swapped old calls for Ably's new law.
Tokens retried once, fallbacks take flight,
I nibbled bugs away by lantern light.
🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'ECO-5633 Refactor executeRequest' is vague and doesn't clearly convey the primary objective to fix JSON error handling in HTTP responses.	Consider updating the title to be more specific about the fix, such as 'ECO-5633 Fix JSON error handling in HTTP responses' to better communicate the intent to fix JSON error decoding issues.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The PR refactors the REST request execution layer, centralizing error handling logic including JSON parsing and validation, which addresses the core issue of handling JSON error responses that are strings or non-dictionary formats.
Out of Scope Changes check	✅ Passed	All changes focus on refactoring the executeRequest method and updating call sites to use new Ably-specific wrappers; changes are scoped to REST execution logic and error handling directly related to fixing JSON error handling.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/2135-refactor-execute-request

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Source/ARTHTTPPaginatedResponse.m (1)

96-127: Ably HTTP errors are now likely swallowed in paginated responses

With executeAblyRequest now surfacing Ably HTTP errors via the error parameter (as ARTErrorInfo), the existing logic here will treat those as successful paginated responses with empty items:

• For Ably errors: error is non-nil and error.domain == ARTAblyErrorDomain, so the early if (error && ![error.domain isEqualToString:ARTAblyErrorDomain]) branch is skipped.
• items becomes @[] because error is non-nil.
• The method then builds an ARTHTTPPaginatedResponse and calls callback(result, nil), losing the actual error.

That makes paginated Ably calls (e.g. stats/history) report success with an empty page when the server actually returned an error.

You probably want to treat any non-nil error from executeAblyRequest as a failure, and only decode items when error == nil. For example:

-    [rest executeAblyRequest:request withAuthOption:ARTAuthenticationOn wrapperSDKAgents:wrapperSDKAgents completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) {
-        if (error && ![error.domain isEqualToString:ARTAblyErrorDomain]) {
-            callback(nil, [ARTErrorInfo createFromNSError:error]);
-            return;
-        }
+    [rest executeAblyRequest:request withAuthOption:ARTAuthenticationOn wrapperSDKAgents:wrapperSDKAgents completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) {
+        if (error) {
+            ARTErrorInfo *errorInfo;
+            if ([error isKindOfClass:[ARTErrorInfo class]]) {
+                errorInfo = (ARTErrorInfo *)error;
+            } else {
+                errorInfo = [ARTErrorInfo createFromNSError:error);
+            }
+            callback(nil, errorInfo);
+            return;
+        }

         ARTLogDebug(logger, @"HTTP Paginated response: %@", response);
         ARTLogDebug(logger, @"HTTP Paginated response data: %@", [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding]);

         NSError *decodeError = nil;

         ARTPaginatedResultResponseProcessor responseProcessor = ^(NSHTTPURLResponse *response, NSData *data, NSError **errorPtr) {
             id<ARTEncoder> encoder = [rest.encoders objectForKey:response.MIMEType];
             return [encoder decodeToArray:data error:errorPtr];
         };
-        NSArray *items = error ? @[] : responseProcessor(response, data, &decodeError);
+        NSArray *items = responseProcessor(response, data, &decodeError);

This preserves decode-error handling (still returning callback(nil, ARTErrorInfo-from-decodeError) below) while ensuring Ably HTTP errors from executeAblyRequest are surfaced to callers instead of being turned into empty pages.

🧹 Nitpick comments (7)

Test/AblyTests/Tests/UtilitiesTests.swift (1)

260-263: Align closure parameters with usage to satisfy SwiftLint

The migration to rest.internal.executeAblyRequest and the explanatory comment on using a placeholder URL both look fine for these simulated-error tests.

Given that neither test uses the response parameter, you can simplify the closure signatures to avoid the unused_closure_parameter warnings mentioned in static analysis:

-        rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { response, _, error in
+        rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { _, _, error in
             guard let error = error as? ARTErrorInfo else {
                 fail("Should be ARTErrorInfo"); done(); return
             }
             ...
         })

Apply the same pattern to the second call at Line 297.

Also applies to: 295-298

Test/AblyTests/Tests/RestClientTests.swift (1)

179-181: Confirm executor choice for non‑Ably URL in HTML/plaintext error test

This test now calls rest.internal.executeAblyRequest for a request whose URL host is www.example.com. If executeAblyRequest is intended only for Ably API hosts (with fallback, auth, and Ably‑specific error shaping), you may want to route this via the new executeExternalRequest instead so this test more closely mirrors how external endpoints (like the one in ECO‑5633) are handled in production. If executeAblyRequest is explicitly designed to support arbitrary hosts as well, keeping it here is fine but worth confirming that assumption.

Source/ARTPushActivationStateMachine.m (1)

192-215: Push activation HTTP calls now use executeAblyRequest; device registration error path is safer

Switching the four push‑activation HTTP calls to executeAblyRequest keeps semantics aligned with other Ably REST interactions.

In deviceRegistration, the new return after emitting GettingDeviceRegistrationFailed on error prevents attempting to decode data on a failed request, which is the right control‑flow fix.

Minor follow‑up you may consider while you’re here: in the decode‑error branch you still log error.localizedDescription even though the failure is in decodeError; swapping to decodeError.localizedDescription would make the log more useful.

Also applies to: 255-274, 304-322, 363-383

Source/ARTPushChannelSubscriptions.m (1)

93-108: executeAblyRequest migration for push channel subscriptions looks fine; relies on non‑nil responses

Both save and _removeWhere now route via executeAblyRequest with ARTAuthenticationOn, preserving the existing status‑code‑based success logic and error wrapping. These completions still assume response is non‑nil (they read response.statusCode before consulting error), so just ensure the new executor preserves the previous guarantee of always providing an NSHTTPURLResponse on completion.

Also applies to: 211-226

Source/ARTRestChannel.m (1)

258-305: Status request now uses executeAblyRequest; verify response non‑nil invariant

Routing the channel status call through executeAblyRequest with ARTAuthenticationOn is consistent with the rest of the REST layer refactor. This method, however, dereferences response.statusCode unconditionally and only later considers error. Please confirm that executeAblyRequest still guarantees a non‑nil NSHTTPURLResponse in all completion paths (including network failures); otherwise, it would be safer to guard for response == nil before accessing statusCode and synthesize an ARTErrorInfo from error in that case.

Source/PrivateHeaders/Ably/ARTRest+Private.h (1)

74-76: Internal REST APIs are well-factored; consider aligning nullability annotations

The separation between executeExternalRequest (raw HTTP) and the two executeAblyRequest overloads (Ably-aware + auth) is clear and matches the implementation in ARTRest.m.

One small polish point: in the header, the completion: parameter for

- (nullable NSObject<ARTCancellable> *)executeAblyRequest:(NSURLRequest *)request
                                         wrapperSDKAgents:(nullable NSDictionary<NSString *, NSString *> *)wrapperSDKAgents
                                               completion:(nullable ARTURLRequestCallback)callback;

is nullable, whereas the implementation declares it as nonnull. It would be good to make these consistent (either allow nil in both, or require it in both) to avoid confusion and potential nullability warnings.

Also applies to: 79-83, 84-86

Source/ARTRest.m (1)

247-268: Centralised auth/REST execution is solid; consider improving cancellable semantics

The new flow:

• executeAblyRequest:withAuthOption:... normalising auth modes to either:
  • a plain Ably request (ARTAuthenticationOff), or
  • executeAblyRequestWithAuthentication:... with the appropriate method / force flag; and
• _timeWithWrapperSDKAgents now going through executeAblyRequest(..., ARTAuthenticationOff, ...)

is a nice consolidation of the various call sites into a single pipeline.

One thing to be aware of is the __block NSObject<ARTCancellable> *task pattern in executeAblyRequestWithAuthentication:...:

__block NSObject<ARTCancellable> *task;

if (method == ARTAuthMethodBasic) {
    ...
    task = [self executeAblyRequest:request wrapperSDKAgents:wrapperSDKAgents completion:callback];
}
else {
    if (!force && [self.auth tokenRemainsValid]) {
        ...
        task = [self executeAblyRequest:request wrapperSDKAgents:wrapperSDKAgents completion:callback];
    }
    else {
        task = [self.auth _authorize:nil options:self.options callback:^(ARTTokenDetails *tokenDetails, NSError *error) {
            ...
            task = [self executeAblyRequest:request wrapperSDKAgents:wrapperSDKAgents completion:callback];
        }];
    }
}
return task;

For the “new token then send request” path, the ARTCancellable returned to the caller represents the auth request, not the eventual Ably HTTP call (reassigning task inside the callback does not update what the caller already received). If you want executeAblyRequestWithAuthentication to be cancellable as a single operation, you may want a small composite ARTCancellable that coordinates cancellation of both the auth and (once created) the subsequent HTTP task, instead of returning just the auth task.

Also applies to: 270-281, 318-341, 616-616

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between baad166 and ffb92af.

📒 Files selected for processing (16)

• Source/ARTAuth.m (3 hunks)
• Source/ARTHTTPPaginatedResponse.m (1 hunks)
• Source/ARTPaginatedResult.m (1 hunks)
• Source/ARTPushActivationStateMachine.m (4 hunks)
• Source/ARTPushAdmin.m (1 hunks)
• Source/ARTPushChannel.m (4 hunks)
• Source/ARTPushChannelSubscriptions.m (2 hunks)
• Source/ARTPushDeviceRegistrations.m (4 hunks)
• Source/ARTRest.m (6 hunks)
• Source/ARTRestAnnotations.m (1 hunks)
• Source/ARTRestChannel.m (4 hunks)
• Source/PrivateHeaders/Ably/ARTRest+Private.h (1 hunks)
• Test/AblyTests/Tests/AuthTests.swift (1 hunks)
• Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4 hunks)
• Test/AblyTests/Tests/RestClientTests.swift (1 hunks)
• Test/AblyTests/Tests/UtilitiesTests.swift (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-10T14:14:47.456Z

Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.

Applied to files:

• Source/ARTPushChannel.m
• Source/ARTAuth.m
• Source/ARTPushChannelSubscriptions.m
• Source/ARTPushAdmin.m
• Source/ARTRestAnnotations.m
• Source/ARTPushDeviceRegistrations.m
• Source/ARTPushActivationStateMachine.m
• Source/ARTRestChannel.m
• Source/ARTPaginatedResult.m
• Source/ARTHTTPPaginatedResponse.m
• Source/ARTRest.m

🧬 Code graph analysis (1)

Test/AblyTests/Tests/UtilitiesTests.swift (1)

Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)

• rest (41-49)

🪛 SwiftLint (0.57.0)

Test/AblyTests/Tests/UtilitiesTests.swift

[Warning] 262-262: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

[Warning] 297-297: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: check (iOS, test_iOS18_4)
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: check
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: build
• GitHub Check: check

🔇 Additional comments (13)

Source/ARTRestAnnotations.m (1)

205-208: executeAblyRequest migration for annotation publish looks correct

Using executeAblyRequest:withAuthOption:ARTAuthenticationOn wrapperSDKAgents:nil here is consistent with other internal REST calls and preserves the existing error-wrapping behavior around ARTErrorInfo. No further changes needed.

Test/AblyTests/Tests/AuthTests.swift (1)

595-595: Additional assertion on error description strengthens regression coverage

Asserting that errorInfo.description() contains "Invalid request: body param is required" complements the existing checks on error code and message and directly guards against regressions for the #2135 error-shape case. Looks good as written.

Source/ARTPushAdmin.m (1)

87-94: Push admin now correctly uses executeAblyRequest

Switching to executeAblyRequest:withAuthOption:ARTAuthenticationOn wrapperSDKAgents:... here is consistent with the rest of the refactor and retains the existing logging and ARTErrorInfo propagation semantics for push publishes.

Source/ARTPushChannel.m (1)

132-138: executeAblyRequest migration for push channel subscribe/unsubscribe looks correct

All four push subscribe/unsubscribe paths now call executeAblyRequest with ARTAuthenticationOn, preserving the existing request construction, logging, and callback behavior. No functional issues spotted.

Also applies to: 166-172, 203-210, 237-244

Source/ARTAuth.m (2)

259-275: AuthUrl requests now respect client HTTP timeout

Setting request.timeoutInterval from _options.httpRequestTimeout brings authUrl token requests in line with the client’s HTTP timeout configuration. This assumes _options is always an ARTClientOptions (or subclass of ARTAuthOptions) that actually defines httpRequestTimeout; if there’s any code path where _options could be a bare ARTAuthOptions, it’d be good to double‑check that before merging.

---

369-382: Executor split between external authUrl and Ably token endpoint is consistent

Using _rest executeExternalRequest for authUrl and _rest executeAblyRequest with ARTAuthenticationOff for /keys/{keyName}/requestToken matches the intended separation of concerns (generic external HTTP vs Ably API with key‑based auth). Error propagation and decoding logic around these calls remains unchanged, which should keep existing behavior intact apart from the centralized request handling in the new executors.

Also applies to: 512-525

Source/ARTRestChannel.m (3)

393-404: Message publish now goes via executeAblyRequest; error wrapping preserved

internalPostMessages now uses executeAblyRequest and still wraps any NSError into ARTErrorInfo, optionally prepending the request URL when addRequestIds is enabled. The behavior and threading model (callback re‑dispatched onto _userQueue) are preserved.

---

517-528: Message update/delete HTTP call migration is consistent

The _updateMessage helper now calls executeAblyRequest with ARTAuthenticationOn, keeping the constructed URL, body, and error wrapping unchanged. The higher‑level updateMessage/deleteMessage entry points retain their semantics.

---

572-621: getMessageWithSerial now uses executeAblyRequest with unchanged decoding behavior

The getMessageWithSerial path is now routed through executeAblyRequest, but still:

• wraps transport errors into ARTErrorInfo (respecting addRequestIds), and
• on HTTP 200, decodes a single ARTMessage then re‑decodes its data via dataEncoder.

No regressions are apparent in the control flow.

Source/ARTPushDeviceRegistrations.m (1)

94-117: executeAblyRequest migration for push device registrations is consistent

All device registration CRUD operations now call executeAblyRequest with ARTAuthenticationOn, while preserving existing request construction, status‑code checks (200/204), decoding, and error‑to‑ARTErrorInfo mapping. This keeps behavior aligned with the refactored REST stack.

Also applies to: 143-168, 215-230, 261-274

Source/ARTPaginatedResult.m (1)

147-183: executePaginated now uses executeAblyRequest with auth; callback threading preserved

executePaginated now routes through rest executeAblyRequest with ARTAuthenticationOn, centralizing Ably‑specific handling (auth, retries, error shaping) for all paginated resources. first/next still wrap the callback to hop onto _userQueue, so caller‑visible threading remains the same. This looks correct assuming all paginated endpoints in use are authenticated Ably API calls, which matches current call sites (channel history, push/device lists, etc.).

Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)

5354-5354: Tests correctly migrated to executeAblyRequest

Switching these helpers to client.internal.rest.executeAblyRequest(..., withAuthOption: .on, wrapperSDKAgents: nil, ...) keeps the semantics of the original REST calls while exercising the new internal pipeline. The changes are consistent and look good.

Also applies to: 5380-5380, 5444-5444, 5511-5511

Source/ARTRest.m (1)

318-341: executeExternalRequest cleanly isolates non-Ably HTTP calls

The new executeExternalRequest:completion: method provides a straightforward, Ably-agnostic path for arbitrary NSURLRequests:

• Uses httpExecutor directly, bypassing Ably-specific headers, token renewal, and fallback logic.
• Maps statusCode >= 400 to an ARTErrorInfo (if no error already present) using the raw body as a string.

This clear separation between external and Ably-internal requests is appropriate and should make callers’ intent and behavior much easier to reason about.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Test/AblyTests/Tests/AuthTests.swift (1)

593-605: Fix description() call – use description property instead

ARTErrorInfo/NSError exposes description as a property in Swift, not a method. XCTAssertTrue(errorInfo.description().contains(...)) will not compile; use the property instead.

-                XCTAssertTrue(errorInfo.description().contains("Invalid request: body param is required")) // see https://github.com/ably/ably-cocoa/issues/2135
+                XCTAssertTrue(errorInfo.description.contains("Invalid request: body param is required")) // see https://github.com/ably/ably-cocoa/issues/2135

The subsequent errorInfo.message assertion can remain as is.

🧹 Nitpick comments (3)

Source/ARTPushActivationStateMachine.m (2)

192-214: Minor: log uses wrong error variable on decode failure

In the decode‑failure branch you log error.localizedDescription, but error is the network error parameter; the failure at this point is decodeError.

if (decodeError != nil) {
    ARTLogError(self->_logger,
                @"%@: decode identity token details failed (%@)",
                NSStringFromClass(self.class),
-               error.localizedDescription);
+               decodeError.localizedDescription);
    [self sendEvent:[ARTPushActivationEventGettingDeviceRegistrationFailed newWithError:[ARTErrorInfo createFromNSError:decodeError]]];
    return;
}

This keeps the emitted error unchanged while making the log accurate.

---

363-383: Consider guarding against response == nil in deregistration error path

The deregistration completion handler assumes that when error is non‑nil, response is also non‑nil:

if (error) {
    // ...
    if (response.statusCode == 401 || error.code == 40005) {
        ...

If executeAblyRequest ever calls this completion with a transport‑level error (e.g. network failure) and response == nil, this would dereference a nil response. Today this may be prevented by the internal contract, but adding a defensive check would make this code more robust:

if (error) {
    const BOOL unauthorized =
        (response && response.statusCode == 401) || error.code == 40005;
    if (unauthorized) {
        ...
    } else {
        ...
    }
    return;
}

Behaviour for existing 401 / 40005 cases would remain unchanged.

Source/ARTRest.m (1)

318-341: Suggest using art_shortString for consistency and safety.

The error message construction at line 327 converts the full response data to a string without truncation. For consistency with handleAblyRestRequestCompletion (line 396) and to prevent potentially large error messages, consider using art_shortString:

         if (response.statusCode >= 400) {
             if (!error) {
+                NSString *plain = [[NSString alloc] initWithData:data ?: [NSData data] encoding:NSUTF8StringEncoding];
                 // Return error with HTTP StatusCode if ARTErrorStatusCode does not exist
                 error = [ARTErrorInfo createWithCode:response.statusCode * 100
                                               status:response.statusCode
-                                             message:[[NSString alloc] initWithData:data ?: [NSData data] encoding:NSUTF8StringEncoding]];
+                                             message:[plain art_shortString]];
             }
         }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ffb92af and 9d6fd3f.

📒 Files selected for processing (16)

• Source/ARTAuth.m (3 hunks)
• Source/ARTHTTPPaginatedResponse.m (1 hunks)
• Source/ARTPaginatedResult.m (1 hunks)
• Source/ARTPushActivationStateMachine.m (4 hunks)
• Source/ARTPushAdmin.m (1 hunks)
• Source/ARTPushChannel.m (4 hunks)
• Source/ARTPushChannelSubscriptions.m (2 hunks)
• Source/ARTPushDeviceRegistrations.m (4 hunks)
• Source/ARTRest.m (6 hunks)
• Source/ARTRestAnnotations.m (1 hunks)
• Source/ARTRestChannel.m (4 hunks)
• Source/PrivateHeaders/Ably/ARTRest+Private.h (1 hunks)
• Test/AblyTests/Tests/AuthTests.swift (1 hunks)
• Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (4 hunks)
• Test/AblyTests/Tests/RestClientTests.swift (1 hunks)
• Test/AblyTests/Tests/UtilitiesTests.swift (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (8)

• Source/ARTPushAdmin.m
• Source/ARTRestChannel.m
• Source/ARTPushChannelSubscriptions.m
• Source/ARTRestAnnotations.m
• Test/AblyTests/Tests/RealtimeClientConnectionTests.swift
• Source/ARTPushDeviceRegistrations.m
• Test/AblyTests/Tests/RestClientTests.swift
• Source/PrivateHeaders/Ably/ARTRest+Private.h

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-10T14:14:47.456Z

Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.

Applied to files:

• Source/ARTPushActivationStateMachine.m
• Source/ARTHTTPPaginatedResponse.m
• Source/ARTAuth.m
• Source/ARTPushChannel.m
• Source/ARTPaginatedResult.m
• Source/ARTRest.m

🧬 Code graph analysis (1)

Test/AblyTests/Tests/UtilitiesTests.swift (1)

Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)

• rest (41-49)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: check (iOS, test_iOS18_4)
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: check
• GitHub Check: build
• GitHub Check: check

🔇 Additional comments (11)

Source/ARTAuth.m (3)

259-275: Per‑request timeout for authUrl token requests looks correct

Using _options.httpRequestTimeout on the auth URL NSMutableURLRequest aligns these token requests with the client-level HTTP timeout; no functional issues spotted.

---

369-382: Confirm executeExternalRequest contract matches handleAuthUrlResponse expectations

Switching authUrl token fetching to _rest executeExternalRequest:completion: makes sense for non‑Ably endpoints, but this code assumes:

• response and data are still provided for non‑2xx HTTP statuses, and
• HTTP errors are not converted into an ARTErrorInfo via the error parameter and short‑circuited before handleAuthUrlResponse runs.

If executeExternalRequest were to start surfacing HTTP errors only via error (with empty/omitted data), JSON/plain‑text token responses on error paths would no longer be decodable here. Please double‑check the new implementation’s contract.

---

493-525: Token request now goes through executeAblyRequest – call site looks sound

Routing /keys/{keyName}/requestToken via _rest executeAblyRequest:… withAuthOption:ARTAuthenticationOff keeps the previous auth option and centralises Ably error handling; the completion still decodes ARTTokenDetails from the configured encoder with the same behaviour as before.

Test/AblyTests/Tests/UtilitiesTests.swift (1)

246-305: Tests correctly updated to use executeAblyRequest for REST error handling

Both tests now invoke rest.internal.executeAblyRequest(_:wrapperSDKAgents:completion:) against the simulated error responses, keeping the same URL, executor wiring, and ARTErrorInfo assertions. This keeps the tests aligned with the new internal REST surface without changing their semantics.

Source/ARTHTTPPaginatedResponse.m (1)

89-128: Ensure executeAblyRequest preserves the expected error contract for paginated responses

executePaginated assumes that:

• error is only non‑nil for transport/client errors (e.g. network issues), and
• Ably HTTP error responses (non‑2xx with Ably error headers/body) are not surfaced via error but instead via response/data.

That’s why it only wraps non‑Ably errors:

if (error && ![error.domain isEqualToString:ARTAblyErrorDomain]) {
    callback(nil, [ARTErrorInfo createFromNSError:error]);
    return;
}

and then calls:

NSArray *items = error ? @[] : responseProcessor(response, data, &decodeError);
...
callback(result, nil);

after constructing the ARTHTTPPaginatedResponse.

With the switch to executeAblyRequest, please confirm that this contract is still honoured; otherwise Ably HTTP errors could end up being hidden (non‑nil error with Ably domain, empty items, and callback(result, nil)).

Source/ARTPushChannel.m (1)

106-245: Push channel subscribe/unsubscribe now correctly use executeAblyRequest

All four push channel subscription endpoints (device/client subscribe/unsubscribe) now go through _rest executeAblyRequest:… withAuthOption:ARTAuthenticationOn, still:

• constructing the same requests (paths, methods, bodies, headers),
• logging failures, and
• wrapping any NSError into ARTErrorInfo for the user callback on _userQueue.

This keeps the public behaviour intact while aligning these calls with the new centralised Ably request pipeline.

Source/ARTPaginatedResult.m (1)

147-150: LGTM! Correct authentication option for paginated requests.

The change from executeRequest to executeAblyRequest with ARTAuthenticationOn correctly ensures paginated requests are authenticated, which is the expected behavior for Ably API requests.

Source/ARTRest.m (4)

247-316: LGTM! Authentication routing correctly refactored.

The authentication routing logic correctly handles the different authentication modes (Off, On, NewToken, TokenRetry, UseBasic) and properly reuses valid tokens before requesting new ones. The refactoring maintains the existing authentication behavior while improving code organization.

---

368-476: LGTM! Excellent centralization of completion logic.

The new handleAblyRestRequestCompletion method effectively centralizes:

• Content-type validation (380-400): Correctly constructs artificial errors for invalid content-types using art_shortString, addressing the PR objective for handling non-JSON error responses
• Token renewal (408-416): Properly implements single-retry token refresh logic
• Fallback retry (441-463): Correctly implements host fallback with proper request cloning using mutableCopy
• Error wrapping (464-474): Ensures non-ARTErrorInfo errors are wrapped with requestId for traceability

The refactoring improves maintainability by consolidating error handling logic that was previously scattered.

---

481-544: LGTM! Request preparation and execution correctly implemented.

The method properly:

• Configures request headers (Accept, Ably-Agent, X-Ably-ClientId, etc.)
• Handles request_id query parameter, correctly reusing originalRequestId for fallback attempts (line 505)
• Manages fallback retry timeout expiration (516-522)
• Delegates completion handling to handleAblyRestRequestCompletion

The implementation correctly centralizes request preparation and execution logic.

---

616-616: LGTM! Time method correctly updated to new API.

The time method now uses executeAblyRequest with ARTAuthenticationOff, which is correct since time requests don't require authentication.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

Test/AblyTests/Tests/UtilitiesTests.swift (2)

262-262: Replace unused closure parameter with _.

The response parameter in the completion handler is not used and should be replaced with _ following Swift best practices:

🔎 Proposed fix

-            rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { response, _, error in
+            rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { _, _, error in

---

297-297: Replace unused closure parameter with _.

The response parameter in the completion handler is not used and should be replaced with _ following Swift best practices:

🔎 Proposed fix

-            rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { response, _, error in
+            rest.internal.executeAblyRequest(request, wrapperSDKAgents:nil, completion: { _, _, error in

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9d6fd3f and 9546629.

📒 Files selected for processing (16)

• Source/ARTAuth.m
• Source/ARTHTTPPaginatedResponse.m
• Source/ARTPaginatedResult.m
• Source/ARTPushActivationStateMachine.m
• Source/ARTPushAdmin.m
• Source/ARTPushChannel.m
• Source/ARTPushChannelSubscriptions.m
• Source/ARTPushDeviceRegistrations.m
• Source/ARTRest.m
• Source/ARTRestAnnotations.m
• Source/ARTRestChannel.m
• Source/PrivateHeaders/Ably/ARTRest+Private.h
• Test/AblyTests/Tests/AuthTests.swift
• Test/AblyTests/Tests/RealtimeClientConnectionTests.swift
• Test/AblyTests/Tests/RestClientTests.swift
• Test/AblyTests/Tests/UtilitiesTests.swift

🚧 Files skipped from review as they are similar to previous changes (8)

• Source/ARTRestAnnotations.m
• Source/ARTPushChannelSubscriptions.m
• Test/AblyTests/Tests/RestClientTests.swift
• Source/ARTPushActivationStateMachine.m
• Source/ARTRestChannel.m
• Test/AblyTests/Tests/RealtimeClientConnectionTests.swift
• Source/ARTPushChannel.m
• Source/ARTPushDeviceRegistrations.m

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-10T14:14:47.456Z

Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.

Applied to files:

• Source/ARTPushAdmin.m
• Source/ARTHTTPPaginatedResponse.m
• Source/ARTPaginatedResult.m
• Source/ARTAuth.m
• Source/ARTRest.m

🪛 SwiftLint (0.57.0)

Test/AblyTests/Tests/UtilitiesTests.swift

[Warning] 262-262: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

[Warning] 297-297: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: build
• GitHub Check: check
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: check
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: check (iOS, test_iOS18_4)

🔇 Additional comments (16)

Test/AblyTests/Tests/AuthTests.swift (1)

595-595: LGTM! Error description validation aligns with PR objectives.

The assertion correctly verifies that error descriptions contain the expected message when authUrl requests fail. This addresses the issue referenced (#2135) about handling JSON error responses properly. The combination of checking both description() (line 595) and message (line 604) ensures complete error information is populated.

Source/ARTPushAdmin.m (1)

87-87: LGTM! Correct method rename for Ably API call.

The rename from executeRequest to executeAblyRequest correctly distinguishes this as an Ably API invocation (push publish endpoint) rather than an external request. The parameters and control flow remain unchanged.

Source/ARTHTTPPaginatedResponse.m (1)

96-96: LGTM! Pagination request correctly uses Ably-specific path.

The method rename appropriately routes paginated Ably API requests through executeAblyRequest, maintaining consistency with the refactored execution flow.

Source/ARTAuth.m (3)

263-263: Good addition: Per-request timeout support.

Setting the timeout interval from client options enables per-request timeout control, which aligns with the PR's stated objectives. This applies to authUrl requests built by this method.

---

374-374: Correct distinction: External request for authUrl.

The switch to executeExternalRequest is appropriate here since authUrl can point to any external authentication server (not Ably's API). This architectural separation improves clarity.

---

512-512: LGTM! Token request correctly uses Ably API path.

The rename to executeAblyRequest is correct for token requests, which target Ably's /keys/{keyName}/requestToken endpoint. The authentication option parameter is properly maintained.

Source/ARTPaginatedResult.m (1)

150-150: LGTM! Consistent rename for paginated Ably requests.

The method rename correctly routes paginated result requests through executeAblyRequest, maintaining consistency with the refactored execution architecture.

Source/PrivateHeaders/Ably/ARTRest+Private.h (1)

74-86: Excellent API evolution: Clear separation of concerns.

The new API surface provides good architectural improvements:

• executeExternalRequest: For non-Ably requests (e.g., authUrl), with a simpler signature using immutable NSURLRequest
• executeAblyRequest overloads: For Ably API calls, with variants that allow full control over authentication or use defaults

This separation makes the API more self-documenting and reduces the chance of misconfiguration (e.g., accidentally applying authentication to external URLs or vice versa).

Source/ARTRest.m (8)

247-268: LGTM! Clear authentication routing.

The authentication option routing logic correctly delegates to the appropriate execution paths, with proper token retry counter management.

---

270-316: LGTM! Robust authentication flow.

The authentication methods correctly handle both Basic and Token auth, with proper token reuse logic and new token acquisition when needed.

---

318-341: LGTM! Appropriate handling for external requests.

The simplified error handling is correct for external requests — no request ID tracking, no token renewal, and no fallback retries, which is appropriate since these aren't Ably API calls.

---

343-347: LGTM!

Clean entry point that delegates to the internal execution method with initial state.

---

363-476: LGTM! Robust centralized completion handling.

This completion handler correctly implements:

• Content-type validation with appropriate fallback error creation
• Error decoding with proper handling of decode failures (addresses the PR objective of handling non-dictionary JSON error responses)
• Token renewal with single-retry limit
• Fallback host retry logic with proper host switching and retry counting
• Consistent error wrapping to ensure requestId propagation

The error handling chain (decode → decodeError → raw fallback) ensures that even malformed JSON responses are handled gracefully.

---

478-523: LGTM! Comprehensive request preparation.

The request preparation logic correctly sets all necessary headers and handles request ID generation, fallback retry timeout expiration per RSC15f, and allows header overrides where appropriate.

---

527-544: Verify cancellation behavior for retry tasks.

The task returned to the caller is the initial HTTP request task. If a retry occurs (line 538-540), the retry task is assigned to the local task variable, but this happens asynchronously after the method has already returned. This means:

• The caller holds a reference to the original request task only
• Canceling the original task won't cancel any retry tasks initiated by the completion handler
• Retry tasks will continue even if the caller attempts to cancel the operation

Is this the intended behavior, or should cancellation propagate to retry attempts? If propagation is desired, a parent cancellable wrapper managing all retry tasks would be needed.

---

592-632: LGTM! Correctly integrated with refactored execution path.

The timeWithWrapperSDKAgents method correctly uses the new executeAblyRequest API with ARTAuthenticationOff, and error handling remains appropriate.