[AIT-569] Groundwork for investigation of LocalDevice-related issues

[lawrence-forooghian]

A few commits that improve logging around LocalDevice storage, and which also provide a new private client option to tell a client not to touch the LocalDevice. See commit messages for more details.

I'm using these in my investigation of push registration issues (https://ably.atlassian.net/browse/AIT-569). See #2204 for that WIP investigation.

Summary by CodeRabbit

Release Notes

• New Features

  • Added test configuration options to disable local device functionality and control storage value logging for enhanced debugging capabilities.

• Refactor

  • Improved local device storage architecture and serialization mechanisms for better code maintainability and testability.

[coderabbitai]

Walkthrough

This PR introduces test configuration options for disabling local device functionality in Ably's iOS SDK. It adds a new ARTThrowingLocalDeviceStorage class that raises exceptions on storage operations, updates ARTTestClientOptions with disableLocalDevice and logLocalDeviceStorageValues properties, refactors persistence APIs to use internal archiving methods, and enhances storage logging with conditional value redaction.

Changes

Cohort / File(s)	Summary
Project Configuration & New Storage Ably.xcodeproj/project.pbxproj, Source/PrivateHeaders/Ably/ARTThrowingLocalDeviceStorage.h, Source/ARTThrowingLocalDeviceStorage.m, Source/Ably.modulemap, Source/include/module.modulemap	Added new ARTThrowingLocalDeviceStorage class that implements ARTDeviceStorage protocol by raising NSInternalInconsistencyException on all storage operations. Integrated into Xcode project build graph and module maps as a private header.
Test Configuration Source/PrivateHeaders/Ably/ARTTestClientOptions.h, Source/ARTTestClientOptions.m	Added two new boolean properties to ARTTestClientOptions: logLocalDeviceStorageValues (controls value logging in storage operations) and disableLocalDevice (prevents local device access). Updated copyWithZone: to include these properties.
Storage Implementation & Logging Source/PrivateHeaders/Ably/ARTLocalDeviceStorage.h, Source/ARTLocalDeviceStorage.m	Extended initializer signatures to accept logValues parameter. Added conditional value redaction for logging via valueToLogForValue: helper. Enhanced all storage operations (objectForKey:, setObject:forKey:, secretForDevice:, setSecret:forDevice:) with detailed debug/warn logging including keychain/user defaults operation context.
Persistence API Foundation Source/PrivateHeaders/Ably/ARTTypes+Private.h, Source/ARTTypes.m	Added new art_unarchiveFromStorage:key:withLogger: method to NSObject (ARTArchive) category that retrieves persisted NSData via storage interface and unarchives using existing internal path. Includes forward declaration for ARTDeviceStorage protocol.
Archive/Unarchive API Removals Source/PrivateHeaders/Ably/ARTDeviceIdentityTokenDetails+Private.h, Source/ARTDeviceIdentityTokenDetails.m, Source/PrivateHeaders/Ably/ARTPushActivationEvent.h, Source/ARTPushActivationEvent.m, Source/PrivateHeaders/Ably/ARTPushActivationState.h, Source/ARTPushActivationState.m	Removed public/private archive/unarchive method declarations and implementations from device identity token, push activation event, and push activation state classes. These methods previously forwarded to internal art_archive and art_unarchiveFromData: variants.
Local Device & State Restoration Source/ARTLocalDevice.m, Source/ARTPushActivationStateMachine.m	Updated device initialization and state machine restoration to use new art_unarchiveFromStorage:key:withLogger: method instead of manual objectForKey: + art_unarchiveFromData: flows. Changed identity token persistence from archiveWithLogger: to art_archiveWithLogger:.
Conditional Storage Selection & Auth Source/ARTRest.m, Source/ARTAuth.m	Updated ARTRest initialization to conditionally select storage implementation: uses ARTThrowingLocalDeviceStorage when testOptions.disableLocalDevice is enabled, otherwise uses ARTLocalDeviceStorage with logLocalDeviceStorageValues setting. Added guard in ARTAuth.setLocalDeviceClientId_nosync: to early-return when local device is disabled. Added conditional imports for test configuration headers.
Test Utilities Test/AblyTests/Test Utilities/MockDeviceStorage.swift, Test/AblyTests/Tests/PushTests.swift	Updated test utilities to use art_archive(withLogger:) instead of removed public archive methods, with force-unwraps of results for test data generation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 With storage that throws and configs that glow,
Logging turned on—or off, gentle and slow,
Device access controlled by a test flag's command,
The state flows through archives, all carefully planned,
A refactor of persistence—bold, wise, and grand! 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main purpose of the changeset: adding groundwork (infrastructure, logging, and test options) to support investigation of LocalDevice-related issues (AIT-569), which is evident from the commit messages and all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch AIT-569-keychain-investigation-groundwork

---

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: 2

🧹 Nitpick comments (3)

Source/ARTLocalDeviceStorage.m (2)

22-33: Consider redacting keys as well, or at least normalising the helper name.

valueToLogForValue: only redacts the value; the log lines still emit key / deviceId verbatim (e.g. Lines 41, 52, 67, 95). The device ID is arguably not secret, but it is a stable per-install identifier and previously wasn't logged by default. If the intent is that logs are safe to share by default, consider either (a) documenting that keys/device IDs are always logged, or (b) gating the deviceId through valueToLogForValue: too when _logValues is NO. Non-blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Source/ARTLocalDeviceStorage.m` around lines 22 - 33, The current helper
valueToLogForValue: only redacts values when _logValues is NO, leaving
keys/deviceId exposed; update logging to either (A) pass both keys and values
through a single normalized helper (rename valueToLogForValue: to
loggableRepresentationForObject: and call it for keys and values wherever logs
are emitted) or (B) add a companion method keyToLogForKey: and use it for
deviceId/key logging paths so that when _logValues is NO the key/deviceId is
also replaced with a redacted token; ensure you update all call sites that log
key/deviceId (those emitting key / deviceId) to use the new/renamed helper and
keep the existing NSData base64 behavior for values.

---

46-54: Minor: setObject:forKey: logs "deleted" for any nil write, even when nothing was stored.

NSUserDefaults setObject:nil forKey: is effectively removeObjectForKey:, which is a no-op if the key was absent. The "UserDefaults deleted for key …" log will therefore fire on first-run cleanup paths where nothing existed. Consider reading first and logging deleted vs delete no-op, or softening the wording to "UserDefaults cleared for key %@". Non-blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Source/ARTLocalDeviceStorage.m` around lines 46 - 54, The current
setObject:forKey: method always logs "UserDefaults deleted for key …" when value
is nil even if the key wasn't present; update -[ARTLocalDeviceStorage
setObject:forKey:] to first check NSUserDefaults.standardUserDefaults
objectForKey:key (or objectIsPresent) before calling setObject:, then call
ARTLogDebug(_logger, ...) with "deleted" only if an existing value was removed
and use a softer message like "cleared" or "delete no-op" when there was nothing
to remove; keep the existing ARTLogDebug usage and valueToLogForValue: for
non-nil writes.

Source/ARTThrowingLocalDeviceStorage.m (1)

1-27: Interface conformance is correctly declared. ✓

Consider marking the throwing helper __attribute__((noreturn)) (optional). +[NSException raise:format:] is not declared noreturn, which is why you need the trailing return nil; in objectForKey: / secretForDevice:. You could DRY the four call sites with a small static helper:

♻️ Optional refactor

+static void ARTThrowDisableLocalDevice(SEL sel, NSString *detail) __attribute__((noreturn));
+static void ARTThrowDisableLocalDevice(SEL sel, NSString *detail) {
+    [NSException raise:NSInternalInconsistencyException
+                format:@"%@ invoked %@ on a client configured with disableLocalDevice",
+                       NSStringFromSelector(sel), detail];
+    __builtin_unreachable();
+}

Then the return nil; lines become unnecessary and the exception messages stay consistent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Source/ARTThrowingLocalDeviceStorage.m` around lines 1 - 27, The four methods
in ARTThrowingLocalDeviceStorage (objectForKey:, setObject:forKey:,
secretForDevice:, setSecret:forDevice:) duplicate the NSException raise logic
and keep trailing return nil; because +[NSException raise:format:] isn't
declared noreturn; add a small static helper (e.g. static void
ARTThrowingLocalDeviceStorageRaise(SEL sel, id info) __attribute__((noreturn));)
that calls +[NSException raise:format:] with a consistent message and use that
helper from each of the four methods, then remove the unnecessary return
statements in objectForKey: and secretForDevice:.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Source/ARTLocalDevice.m`:
- Around line 78-85: Update the error log message in the branch where
deviceSecret is missing to accurately reflect that the stored deviceId will be
discarded and replaced: locate the ARTLogError call that references deviceId in
ARTLocalDevice.m and change its wording to something like "Discarding stored
device ID %@ and generating a new ID/secret pair: no secret found
(UserDefaults/Keychain out of sync?)" so it matches the behavior of
generateAndPersistPairOfDeviceIdAndSecret and the PR description.

In `@Test/AblyTests/Test` Utilities/MockDeviceStorage.swift:
- Line 17: Replace the force-unwrap of the nullable result from
art_archive(withLogger:) with a safe unwrap and a test failure on nil: call
art_archive(withLogger:) into a guard/if let, on nil call XCTFail with a clear
message and return, otherwise pass the unwrapped NSData into
simulateOnNextRead(data:for:) for ARTPushActivationCurrentStateKey; do the same
replacement for the identical force-unwrap occurrences in the PushTests tests
where art_archive(withLogger:) is used.

---

Nitpick comments:
In `@Source/ARTLocalDeviceStorage.m`:
- Around line 22-33: The current helper valueToLogForValue: only redacts values
when _logValues is NO, leaving keys/deviceId exposed; update logging to either
(A) pass both keys and values through a single normalized helper (rename
valueToLogForValue: to loggableRepresentationForObject: and call it for keys and
values wherever logs are emitted) or (B) add a companion method keyToLogForKey:
and use it for deviceId/key logging paths so that when _logValues is NO the
key/deviceId is also replaced with a redacted token; ensure you update all call
sites that log key/deviceId (those emitting key / deviceId) to use the
new/renamed helper and keep the existing NSData base64 behavior for values.
- Around line 46-54: The current setObject:forKey: method always logs
"UserDefaults deleted for key …" when value is nil even if the key wasn't
present; update -[ARTLocalDeviceStorage setObject:forKey:] to first check
NSUserDefaults.standardUserDefaults objectForKey:key (or objectIsPresent) before
calling setObject:, then call ARTLogDebug(_logger, ...) with "deleted" only if
an existing value was removed and use a softer message like "cleared" or "delete
no-op" when there was nothing to remove; keep the existing ARTLogDebug usage and
valueToLogForValue: for non-nil writes.

In `@Source/ARTThrowingLocalDeviceStorage.m`:
- Around line 1-27: The four methods in ARTThrowingLocalDeviceStorage
(objectForKey:, setObject:forKey:, secretForDevice:, setSecret:forDevice:)
duplicate the NSException raise logic and keep trailing return nil; because
+[NSException raise:format:] isn't declared noreturn; add a small static helper
(e.g. static void ARTThrowingLocalDeviceStorageRaise(SEL sel, id info)
__attribute__((noreturn));) that calls +[NSException raise:format:] with a
consistent message and use that helper from each of the four methods, then
remove the unnecessary return statements in objectForKey: and secretForDevice:.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 915b67ae-dcf0-40f3-8c4d-9b74b6803c1a

📥 Commits

Reviewing files that changed from the base of the PR and between 745e7b7 and 7c3e73a.

📒 Files selected for processing (23)

• Ably.xcodeproj/project.pbxproj
• Source/ARTAuth.m
• Source/ARTDeviceIdentityTokenDetails.m
• Source/ARTLocalDevice.m
• Source/ARTLocalDeviceStorage.m
• Source/ARTPushActivationEvent.m
• Source/ARTPushActivationState.m
• Source/ARTPushActivationStateMachine.m
• Source/ARTRest.m
• Source/ARTTestClientOptions.m
• Source/ARTThrowingLocalDeviceStorage.m
• Source/ARTTypes.m
• Source/Ably.modulemap
• Source/PrivateHeaders/Ably/ARTDeviceIdentityTokenDetails+Private.h
• Source/PrivateHeaders/Ably/ARTLocalDeviceStorage.h
• Source/PrivateHeaders/Ably/ARTPushActivationEvent.h
• Source/PrivateHeaders/Ably/ARTPushActivationState.h
• Source/PrivateHeaders/Ably/ARTTestClientOptions.h
• Source/PrivateHeaders/Ably/ARTThrowingLocalDeviceStorage.h
• Source/PrivateHeaders/Ably/ARTTypes+Private.h
• Source/include/module.modulemap
• Test/AblyTests/Test Utilities/MockDeviceStorage.swift
• Test/AblyTests/Tests/PushTests.swift

💤 Files with no reviewable changes (6)

• Source/PrivateHeaders/Ably/ARTPushActivationEvent.h
• Source/PrivateHeaders/Ably/ARTDeviceIdentityTokenDetails+Private.h
• Source/ARTPushActivationState.m
• Source/ARTDeviceIdentityTokenDetails.m
• Source/ARTPushActivationEvent.m
• Source/PrivateHeaders/Ably/ARTPushActivationState.h

[maratal]

LGTM