Address Greg's review feedback

[PatrickCroninMM]

Links to feedback comments (because there was no PR)

Ongoing discussion

• Should we log if conversion to UTF-8 fails?
• Tracking token != Stored ID

Addressed in this PR

• consistent logging for keychain storage errors
• deinit doesn't need a lock
• Encodable and Sendable conformance

Addressed in response to Jeremy's feedback in another PR

• prefer "whitespacesAndNewlines"
• "can be private"
• "update the version?"
• "Crosses async boundaries"

[gemini-code-assist]

Code Review

This pull request updates logging for IDFV and tracking token caching, removes locking in the DeviceTracker deinitializer, and adds Sendable conformance to DeviceData. A logic error was identified in DeviceDataCollector.swift where the logging condition for caching the IDFV was inverted, causing success messages to be logged on failure and vice versa.

[oschwald]

🤖 Claude Code Review — PR #7

I’ve reviewed this PR using 5 specialized analysis passes (general code quality, test coverage, error handling, type design, and comment accuracy). Here’s the consolidated review.

Summary

This PR is well-executed overall. The ServerResponse redesign (making fields non-optional with construction-time validation) is a standout improvement — it eliminated missingTrackingToken entirely by making illegal states unrepresentable. The naming convention split (“tracking token” public / “stored ID” internal) is applied thoroughly across all 22 files. The ObjC wrapper layer is clean and minimal.

See inline comments below for specific issues. A few additional items that couldn’t be placed inline:

---

TrackingResult doc reference

Sources/MinFraudDevice/Model/TrackingResult.swift:6 references /device/tracking_token as the minFraud API field name. Since the wire format now uses stored_id, verify this still refers to the correct minFraud API field (not the SDK wire format). If correct, consider adding a brief clarification to preempt confusion.

Monotonic clock comment

Sources/MinFraudDevice/Network/DeviceAPIClient.swift:72 says “monotonic approach” but doesn’t explain why ProcessInfo.systemUptime was chosen over Date(). Consider: “Use ProcessInfo.systemUptime instead of Date() to avoid wall-clock skew from NTP adjustments.”

---

Strengths

• ServerResponse redesign makes illegal states unrepresentable — textbook improvement
• Naming convention applied consistently across all 22 files
• CustomNSError conformances give ObjC consumers meaningful error domains/codes
• Else branch for keychain save failure directly addresses the original review’s critical finding
• Redacted description on both TrackingResult and ObjCTrackingResult prevents accidental token leakage
• deinit lock removal is correct (ARC guarantees sole ownership)
• Table-driven tests with clear case labels are easy to extend
• ObjC test coverage in both Swift (ObjCWrapperTests.swift) and actual ObjC (ObjCWrapperTests.m)

[oschwald]

Overall, this looks good to me. Claude had a few thoughts. I'll let you decide what to do with them.

[oschwald]

🤖 Claude Code Review

Five-pass automated review (code quality, test coverage, silent failures, type design, comment accuracy). All of Greg's inline comments have been addressed. No new critical issues. Five important findings and a handful of suggestions below.

Important Issues

See inline comments for #1-#3. Issues #4-#5 are posted as separate comments since the lines are not in the diff.

1. responseDecodingFailed uses error.localizedDescription, which loses structural DecodingError info (coding path, key name) — DeviceAPIClient.swift:130
2. ObjCDeviceTracker silently drops the completion handler if self is deallocated mid-flight (distinct from the documented shutdown behavior) — MMDeviceTracker.swift:32
3. testMinFraudDeviceErrorBridgesToNSError doesn't verify errorUserInfo — the whole point of commit 72cda92 — ObjCWrapperTests.swift:140
4. Monotonic clock comment states the what but not the why — DeviceAPIClient.swift:81 (see separate comment)
5. CLAUDE.md Network Layer description omits APIError.responseDecodingFailed — CLAUDE.md:112 (see separate comment)

Suggestions (lower priority)

• Shared validation: ObjCSDKConfig duplicates SDKConfig's validation logic. If SDKConfig adds constraints, the ObjC wrapper must be updated in lockstep or callers hit a precondition crash. Consider extracting a static func isValid(accountID:collectionIntervalSeconds:) -> Bool.
• IPVersion enum: Replacing ipVersion: Int on ServerResponse with enum IPVersion: Int, Sendable, Codable { case v4 = 4; case v6 = 6 } would make the constraint compile-time enforceable and the == 6 check more readable.
• Specific test assertion: testSendDeviceDataThrowsOnInvalidResponse catches is APIError without checking for .responseDecodingFailed specifically. A future refactor that accidentally threw .serverError would still pass.
• Force-unwrap inconsistency: ObjCWrapperTests.swift:23,38 uses config!.config.loggingEnabled while surrounding lines use optional chaining. XCTAssertEqual(config?.config.loggingEnabled, false) would be safer.
• Early shutdown guard: A guard !isShutDown at the top of ObjCDeviceTracker.collectAndSend (before launching the Task) would avoid a wasted network request whose result will be discarded.

Strengths

• ServerResponse redesign makes illegal states unrepresentable — textbook improvement
• CustomNSError conformances give ObjC consumers meaningful error domains/codes
• Naming convention applied consistently across all 22 files
• DecodingError wrapping into APIError.responseDecodingFailed is the right pattern
• ObjC test coverage in both Swift and actual Objective-C
• Dynamic API baseline generation elegantly solves the stale-baseline problem

[oschwald]

🤖 Claude — Important: Monotonic clock comment states the what but not the why

DeviceAPIClient.swift:81

This was flagged in Greg's original review. The current comment:

// Use a monotonic approach for calculating request duration.

ProcessInfo.systemUptime was chosen over Date() to avoid wall-clock skew from NTP adjustments — a non-obvious engineering decision worth capturing.

Suggested:

// Use ProcessInfo.systemUptime (monotonic clock) instead of Date()
// to avoid wall-clock drift from NTP adjustments.

✅ Resolved in 3982377 — comment now explains NTP adjustment immunity.

[oschwald]

🤖 Claude — Suggestion: CLAUDE.md Network Layer description omits responseDecodingFailed

CLAUDE.md:112

This PR added APIError.responseDecodingFailed as a new public error case, but the Network Layer description still only mentions serverError.

Suggested:

   - Throws `APIError.serverError` on non-success responses, `APIError.responseDecodingFailed` if the response body cannot be decoded

✅ Resolved in 4655aad — Network Layer description now includes responseDecodingFailed.

[oschwald]

🤖 Claude — Critical: NSLock deadlock when completion handler re-enters the tracker

The completion handler is invoked while self.lock is held (the defer { self.lock.unlock() } on line 38 runs after the completion returns). Since NSLock is non-reentrant, an ObjC caller that calls shutdown() or starts another collectAndSend() from inside the completion block will deadlock the main thread:

1. @MainActor Task acquires lock (line 37)
2. completion(...) called on main thread while lock is held (line 40)
3. Inside completion, caller calls shutdown() → lock.lock() (line 56) → deadlock

The same issue exists on the error path at line 45.

Fix: Read isShutDown under the lock, then call the completion after releasing:

do {
    let result = try await self.tracker.collectAndSend()
    self.lock.lock()
    let isShutDown = self.isShutDown
    self.lock.unlock()
    guard !isShutDown else { return }
    completion(ObjCTrackingResult(result: result), nil)
} catch {
    self.lock.lock()
    let isShutDown = self.isShutDown
    self.lock.unlock()
    guard !isShutDown else { return }
    completion(nil, error as NSError)
}

The race where shutdown() is called between unlock and completion invocation is benign — we already made our decision based on the state at check time.

[oschwald]

👨 Greg - I think this is a real issue that should be fixed.

[oschwald]

Looks good!