Fixing the n/w layer #259
Fixing the n/w layer #259
Conversation
…ment mode This commit implements a hybrid build mode where runanywhere-core is consumed as pre-built binaries (remote) while only runanywhere-commons is built locally. Key Changes: 1. Pre-built Core Library Support: - Added download-core-prebuilt.sh to download pre-built core static libraries - Created FindRunAnywhereCorePrebuilt.cmake to locate pre-built libraries - Falls back to extracting libraries from XCFramework if static libs unavailable 2. CMake Configuration: - Added USE_PREBUILT_CORE option to CMakeLists.txt - Updated main CMakeLists.txt to support both pre-built and source compilation - Modified backend CMakeLists.txt (llamacpp, onnx) to link pre-built libraries - Updated include directories to work with both modes 3. Build Script Updates: - Updated build-ios.sh to auto-detect and use pre-built libraries - Modified combine_*_libs() functions to use pre-built core libraries - Updated build_and_run.sh to download pre-built core in --local mode - Removed core compilation step in local mode 4. Package.swift Configuration: - Added testLocal flag to switch between local and remote binary targets - Local mode: Uses frameworks from Binaries/ directory - Remote mode: Downloads all frameworks from GitHub releases - Updated binary target selection logic 5. Build Mode Behavior: - WITHOUT --local: 100% remote (all frameworks from GitHub releases) - WITH --local: Hybrid mode (commons built locally, core from remote pre-built) This ensures that runanywhere-core is never compiled locally, only consumed as pre-built binaries, while allowing local development of runanywhere-commons. Files Changed: - sdk/runanywhere-commons/cmake/FindRunAnywhereCorePrebuilt.cmake (new) - sdk/runanywhere-commons/scripts/download-core-prebuilt.sh (new) - sdk/runanywhere-commons/CMakeLists.txt - sdk/runanywhere-commons/backends/llamacpp/CMakeLists.txt - sdk/runanywhere-commons/backends/onnx/CMakeLists.txt - sdk/runanywhere-commons/scripts/build-ios.sh - sdk/runanywhere-swift/Package.swift - examples/ios/RunAnywhereAI/scripts/build_and_run.sh
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds local vs remote prebuilt-core build mode, CMake support for using/downloaded prebuilt core libraries, expanded model/voice load APIs to accept path/id/name, richer device telemetry schema and batching, streaming token-count reporting, and corresponding Swift bridge and build script updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build Script
participant CMake as CMake System
participant Git as runanywhere-binaries
participant Prebuilt as Prebuilt Artifacts
participant Source as Source Tree
alt Local Mode (LOCAL_MODE=true)
Build->>Build: copy_core_to_commons()
Build->>CMake: USE_PREBUILT_CORE=ON
CMake->>Git: download-core-prebuilt.sh (fetch zip)
Git-->>Prebuilt: provide .a libs + includes
CMake->>Prebuilt: FindRunAnywhereCorePrebuilt sets RUNANYWHERE_CORE_* vars
CMake->>CMake: link prebuilt libs
Build->>Build: copy_frameworks_to_swift()
Build->>Build: set_package_mode(true)
else Remote Mode (LOCAL_MODE=false)
Build->>CMake: USE_PREBUILT_CORE=OFF
CMake->>Source: FindRunAnywhereCore (use source tree)
Source-->>CMake: source paths
CMake->>CMake: build from source
end
Note over Build,Source: Final artifacts produced for Swift/iOS build
sequenceDiagram
participant Client as iOS App
participant HTTP as HTTPService
participant DeviceMgr as DeviceManager
participant Backend as Backend API
participant Telemetry as TelemetryManager
Client->>DeviceMgr: registerIfNeeded()
DeviceMgr->>DeviceMgr: check env & registered state
alt Development (already registered)
DeviceMgr->>DeviceMgr: perform UPSERT flow
else Production (already registered)
DeviceMgr->>DeviceMgr: skip registration
end
DeviceMgr->>HTTP: POST /rest/v1/sdk_devices?on_conflict=device_id (Prefer: resolution=merge-duplicates)
HTTP->>Backend: send request
Backend-->>HTTP: 200 or 409
HTTP->>DeviceMgr: treat 409 as success
DeviceMgr->>Telemetry: emit registration event
sequenceDiagram
participant SDK as RunAnywhere SDK
participant Telemetry as TelemetryManager
participant Queue as EventQueue
participant HTTP as HTTP callback
SDK->>Telemetry: recordEvent(event)
Telemetry->>Queue: enqueue event
alt Development
Telemetry->>HTTP: flush immediately
else Production
Telemetry->>Telemetry: accumulate batch (size=10 or timeout=5000ms)
alt Completion event or batch full
Telemetry->>HTTP: flush batch now
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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. Comment |
|
@coderabbitai review please |
|
@shubhammalhotra28, I'll review the changes in this PR right away. 🧠 Learnings used✅ Actions performedReview triggered.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 7cef222 in 2 minutes and 21 seconds. Click for details.
- Reviewed
3143lines of code in47files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sdk/runanywhere-commons/src/infrastructure/telemetry/rac_llm_component.h:116
- Draft comment:
The loadModel API now takes model_path, model_id, and model_name. Ensure that the documentation and any client code are updated to reflect these parameter changes consistently. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. sdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/Models/RunAnywhere+ModelManagement.swift:27
- Draft comment:
For built‑in models, the call uses the modelId as both the path and identifier. Verify that using the modelId as the file path is intentional and that the documentation clearly explains this behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify their intention and ensure documentation clarity, which violates the rules. It doesn't provide a specific code suggestion or ask for a specific test to be written.
3. sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+State.swift:249
- Draft comment:
In loadStoredAuth, the use of try? for keychain retrieval can hide underlying errors. Consider adding more detailed logging or error handling if keychain access fails unexpectedly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. sdk/runanywhere-swift/Sources/RunAnywhere/Public/RunAnywhere.swift:250
- Draft comment:
In the async Phase 2 services initialization, network authentication and service setup errors are logged as warnings. Consider whether critical failures here should trigger a retry or more robust error reporting. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. sdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/TTS/RunAnywhere+TTS.swift:110
- Draft comment:
The PCM-to-WAV conversion calls a C++ utility (rac_audio_float32_to_wav) and frees memory with rac_free. Double-check that the conversion handles all error cases and that memory is managed safely. Adding extra checks could help prevent malformed data. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swift:153
- Draft comment:
The telemetry HTTP callback dispatches requests via an async Task. While the pattern is correct, ensure that cases where HTTPService is not configured are handled gracefully across all execution paths. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp:266
- Draft comment:
Typographical concern: The event string 'storage.cache.clear_failed' uses an underscore in between 'clear' and 'failed'. Consider using a dot for consistency (e.g., 'storage.cache.clear.failed') to match the convention used in other event strings. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a style/consistency comment about event naming conventions. Looking at the code, there is indeed a mix of patterns - most use dots exclusively (like "device.registration.failed"), but there are exceptions like "first_token" and "clear_failed". The comment is technically correct that most events use dots, but this is a minor stylistic issue. The rules say comments should be about clear code changes required, not minor style issues. This is borderline - it's about consistency, which could be considered a code quality issue. However, it's not a bug, and the current naming is functional. The inconsistency already exists in the codebase (e.g., "first_token"), so this isn't introducing a new pattern. This seems like a minor nitpick that doesn't clearly require a change. While consistency is valuable, this is a very minor naming issue that doesn't affect functionality. The codebase already has some inconsistencies (like "first_token"), so this isn't introducing a new problem. This might be considered too minor to warrant a PR comment. The comment is about a minor style issue in a string constant. While consistency is good, this doesn't rise to the level of "clearly a code change required" - it's more of a suggestion. Given the rules emphasize not making obvious or unimportant comments, and this is purely cosmetic with no functional impact, it should probably be deleted. This comment is about a minor naming consistency issue in an event string. It's not a bug or functional issue, just a stylistic suggestion. Given the rules state not to make obvious or unimportant comments, and this is purely cosmetic, the comment should be deleted.
Workflow ID: wflow_QWytT6h4CRaExl4P
You can customize 
| event_data.data.tts_synthesis.synthesis_id = synthesis_id.c_str(); | ||
| event_data.data.tts_synthesis.model_id = voice_id; | ||
| event_data.data.tts_synthesis.model_name = voice_name; | ||
| event_data.data.tts_synthesis.model_name = voice_name; |
There was a problem hiding this comment.
Typo: Duplicate assignment for model_name. It appears that voice_name is being assigned twice consecutively; please remove one of the duplicate lines.
| event_data.data.tts_synthesis.model_name = voice_name; |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Device.swift, line 93 (link)style: hardcoded literal key duplicates
registeredKeyconstant definitionuse the
registeredKeyconstant instead of the literal string to avoid inconsistency -
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Device.swift, line 100 (link)style: hardcoded literal key duplicates
registeredKeyconstant definitionuse the
registeredKeyconstant instead of the literal string to avoid inconsistency
47 files reviewed, 4 comments
sdk/runanywhere-swift/Package.swift
Outdated
| // | ||
| // ============================================================================= | ||
| let testLocal = false | ||
| let testLocal = true // Local development mode: use local frameworks from Binaries/ |
There was a problem hiding this comment.
logic: testLocal hardcoded to true will break production builds
| let testLocal = true // Local development mode: use local frameworks from Binaries/ | |
| let testLocal = false // Local development mode: use local frameworks from Binaries/ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-swift/Package.swift
Line: 25:25
Comment:
**logic:** `testLocal` hardcoded to `true` will break production builds
```suggestion
let testLocal = false // Local development mode: use local frameworks from Binaries/
```
How can I resolve this? If you propose a fix, please make it concise.| if path.contains("/rest/v1/sdk_devices") { | ||
| // Add on_conflict query parameter to the path | ||
| let upsertPath = path.contains("?") ? "\(path)&on_conflict=device_id" : "\(path)?on_conflict=device_id" | ||
| return try await postRawWithHeaders(upsertPath, payload, requiresAuth: requiresAuth, additionalHeaders: ["Prefer": "resolution=merge-duplicates"]) | ||
| } |
There was a problem hiding this comment.
style: hardcoded Supabase path check for UPSERT logic
use endpoint constant or configuration instead of hardcoded path string to avoid breaking if the API path changes
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift
Line: 96:100
Comment:
**style:** hardcoded Supabase path check for UPSERT logic
use endpoint constant or configuration instead of hardcoded path string to avoid breaking if the API path changes
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_stt_component.h (1)
63-71: Documentation mismatch with function signature.The documentation block still references only
model_id, but the function signature now includesmodel_path,model_id, andmodel_name. Compare withsdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.hwhich has the updated documentation.Proposed fix
/** * @brief Load a model * * @param handle Component handle - * @param model_id Model ID to load + * @param model_path File path to the model (used for loading) + * @param model_id Model identifier (used for telemetry, e.g., "sherpa-onnx-whisper-tiny.en") + * @param model_name Human-readable model name (e.g., "Sherpa Whisper Tiny (ONNX)") * @return RAC_SUCCESS or error code */ RAC_API rac_result_t rac_stt_component_load_model(rac_handle_t handle, const char* model_path, const char* model_id, const char* model_name);sdk/runanywhere-swift/Sources/CRACommons/include/rac_tts_component.h (1)
63-71: Documentation mismatch with function signature.The documentation block still only describes
voice_id, but the function signature now includesvoice_path,voice_id, andvoice_name. This mirrors the same issue in the STT component header.Proposed fix
/** * @brief Load a voice * * @param handle Component handle - * @param voice_id Voice ID to load + * @param voice_path File path to the voice model (used for loading) + * @param voice_id Voice identifier (used for telemetry) + * @param voice_name Human-readable voice name * @return RAC_SUCCESS or error code */ RAC_API rac_result_t rac_tts_component_load_voice(rac_handle_t handle, const char* voice_path, const char* voice_id, const char* voice_name);sdk/runanywhere-swift/Sources/CRACommons/include/rac_llm_component.h (1)
107-117: Documentation does not match the updated signature.The docstring at lines 107-115 only mentions
model_idbut the function now acceptsmodel_path,model_id, andmodel_name. Update the documentation to describe all parameters:Suggested documentation update
/** * @brief Load a model * * Mirrors Swift's LLMCapability.loadModel(_:) * * @param handle Component handle - * @param model_id Model ID to load + * @param model_path File path to the model (used for loading) + * @param model_id Model identifier (used for telemetry) + * @param model_name Human-readable model name (e.g., "Llama 3.2 1B") * @return RAC_SUCCESS or error code */ RAC_API rac_result_t rac_llm_component_load_model(rac_handle_t handle, const char* model_path, const char* model_id, const char* model_name);sdk/runanywhere-commons/backends/onnx/CMakeLists.txt (1)
94-110: Dead code: duplicateelseifblock is unreachable.Lines 108-110 contain an
elseif(DEFINED RUNANYWHERE_CORE_ONNX_LIB)block that will never execute because the same condition is already checked on line 95. This appears to be leftover from a merge or refactoring.🔎 Proposed fix - remove dead code
# Link against runanywhere-core's onnx backend # Use pre-built library if available, otherwise use compiled target if(DEFINED RUNANYWHERE_CORE_ONNX_LIB) # Pre-built library mode (USE_PREBUILT_CORE=ON) target_link_libraries(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_ONNX_LIB}) # Include directories are set globally via RUNANYWHERE_CORE_INCLUDE_DIRS elseif(TARGET runanywhere_onnx) # Source compilation mode (legacy) target_link_libraries(rac_backend_onnx PRIVATE runanywhere_onnx) # Include runanywhere-core headers for backend APIs target_include_directories(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_DIR}/src ${RUNANYWHERE_CORE_DIR}/src/capabilities ${RUNANYWHERE_CORE_DIR}/src/backends/onnx ) -elseif(DEFINED RUNANYWHERE_CORE_ONNX_LIB) - target_link_libraries(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_ONNX_LIB}) endif()
🧹 Nitpick comments (15)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Security/KeychainManager.swift (1)
169-176: Consider extracting shared query logic.The query setup and execution code (lines 170-176) is duplicated from
retrieveData(lines 146-152). Consider extracting this into a private helper method that returns(status: OSStatus, data: Data?)to improve maintainability.🔎 Example refactor to reduce duplication
Add a private helper method:
private func executeDataQuery(for key: String) -> (status: OSStatus, data: Data?) { var query = baseQuery(for: key) query[kSecReturnData as String] = true query[kSecMatchLimit as String] = kSecMatchLimitOne var result: AnyObject? let status = SecItemCopyMatching(query as CFDictionary, &result) let data = result as? Data return (status, data) }Then simplify both methods:
public func retrieveData(for key: String) throws -> Data { - var query = baseQuery(for: key) - query[kSecReturnData as String] = true - query[kSecMatchLimit as String] = kSecMatchLimitOne - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) + let (status, data) = executeDataQuery(for: key) - guard status == errSecSuccess, - let data = result as? Data else { + guard status == errSecSuccess, let data = data else { if status == errSecItemNotFound { throw SDKError.security(.keychainError, "Item not found in keychain") } throw SDKError.security(.keychainError, "Failed to retrieve item from keychain: OSStatus \(status)") } return data } public func retrieveDataIfExists(for key: String) throws -> Data? { - var query = baseQuery(for: key) - query[kSecReturnData as String] = true - query[kSecMatchLimit as String] = kSecMatchLimitOne - - var result: AnyObject? - let status = SecItemCopyMatching(query as CFDictionary, &result) + let (status, data) = executeDataQuery(for: key) if status == errSecItemNotFound { return nil } - guard status == errSecSuccess, - let data = result as? Data else { + guard status == errSecSuccess, let data = data else { throw SDKError.security(.keychainError, "Failed to retrieve item from keychain: OSStatus \(status)") } return data }sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+State.swift (1)
250-274: Inconsistent error handling for optional fields could hide Keychain errors.Lines 255-262 use explicit
do/catchto distinguish between missing items and actual Keychain errors for required tokens. However, lines 272-274 usetry?for optional fields, which suppresses both missing-item cases and genuine Keychain errors (e.g., corruption, access denied).This inconsistency makes it harder to debug Keychain issues. Consider using the same pattern for optional fields:
🔎 Proposed fix for consistent error handling
- // Load additional fields (optional - these may not exist) - let userId = try? KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.userId") - let orgId = try? KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.organizationId") - let deviceIdStored = try? KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.deviceId") + // Load additional fields (optional - these may not exist) + // If Keychain error occurs (not just missing), log it + let userId: String? + let orgId: String? + let deviceIdStored: String? + + do { + userId = try KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.userId") + orgId = try KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.organizationId") + deviceIdStored = try KeychainManager.shared.retrieveIfExists(for: "com.runanywhere.sdk.deviceId") + } catch { + SDKLogger(category: "CppBridge.State").debug("Keychain error loading optional fields: \(error.localizedDescription)") + userId = nil + orgId = nil + deviceIdStored = nil + }sdk/runanywhere-swift/Package.swift (1)
208-210: Consider inlining the helper if no future extensibility is planned.The
onnxBackendDependencies()helper currently returns a hardcoded array without any conditional logic. If dynamic dependency resolution isn't anticipated, consider inlining it directly in the target definition (line 115) for simplicity.sdk/runanywhere-swift/Sources/RunAnywhere/Public/RunAnywhere.swift (1)
326-330: Good placement for telemetry flush.Flushing queued telemetry events immediately after HTTP configuration ensures events accumulated during initialization are sent promptly. The logic is sound.
Note: The
CppBridge.Telemetry.flush()implementation (inCppBridge+Telemetry.swift) usesNSLock. As per coding guidelines, Swift 6 concurrency primitives should be preferred overNSLock. Consider refactoring the telemetry bridge to use Swift actors orMutexfrom the Swift Synchronization module when updating that file.sdk/runanywhere-commons/src/core/events.cpp (1)
12-12: Remove the unusedrac_logger.hinclude.The
rac_logger.hheader is included on line 12 but contains no logging calls or references anywhere in this 653-line file. If logging is intended for future use, defer the include until needed to keep the code clean.sdk/runanywhere-commons/include/rac/features/llm/rac_llm_metrics.h (1)
250-263: Consider documenting edge-case behavior for the new token counting API.The new function
rac_streaming_metrics_set_token_countsis well-structured, but the documentation could be more explicit about:
- When this function should be called relative to
mark_complete()ormark_failed()- Behavior if called multiple times (does the last call win?)
- Whether negative values are rejected
- Thread-safety guarantees (if any)
📝 Suggested documentation enhancement
/** * @brief Set actual token counts from backend. * * Call this with actual token counts from the LLM backend's tokenizer * to get accurate telemetry instead of character-based estimation. + * + * This should be called after mark_start() and before mark_complete(). + * If called multiple times, the last values are used. + * Negative values are treated as 0 (estimation fallback). * * @param handle Collector handle * @param input_tokens Actual input/prompt token count (0 to use estimation) * @param output_tokens Actual output/completion token count (0 to use estimation) * @return RAC_SUCCESS or error code */sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swift (1)
69-77: ReplaceNSLockwith Swift 6 concurrency primitives.The coding guidelines specify not to use
NSLockas it is outdated. Consider refactoringTelemetryto anactor(similar toTTS,LLM,STTcomponents) or using Swift'sMutextype from the Synchronization framework.Option 1: Convert to actor (recommended, consistent with other bridges)
- public enum Telemetry { - - private static var manager: OpaquePointer? - private static let lock = NSLock() + public actor Telemetry { + public static let shared = Telemetry() + + private var manager: OpaquePointer? + + private init() {}This would require adjusting method signatures and call sites to use
await.Option 2: Use Swift 6 Mutex (if synchronous access is required)
+import Synchronization + extension CppBridge { public enum Telemetry { private static var manager: OpaquePointer? - private static let lock = NSLock() + private static let lock = Mutex<Void>(())Note:
Mutexrequires iOS 18+ / macOS 15+.Based on coding guidelines.
sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift (2)
193-208: Verify handling of edge cases in query string parsing.The path splitting using
split(separator:maxSplits:)could produce unexpected results if the path contains only?with no query content (e.g.,"/path?"), ascomponents[1]would be an empty string. While this is unlikely in practice, consider adding a guard.Also, the
URLComponentsapproach correctly preserves the path and query, but if the base URL already has a query string, this code would overwrite it rather than merging.🔎 Suggested improvement
if path.contains("?") { // Split path and query parameters let components = path.split(separator: "?", maxSplits: 1) + guard components.count == 2, !components[1].isEmpty else { + return base.appendingPathComponent(path) + } let pathPart = String(components[0]) let queryPart = String(components[1])
236-253: Consider potential information disclosure in logging.The 409 conflict handling is correct for device registration. However, logging that includes URL paths could potentially leak device IDs or other identifiers if they appear in the URL. The current logging at line 246 (
request.url?.absoluteString) and line 252 is appropriate for debugging but ensure this doesn't expose sensitive data in production logs.sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp (2)
626-639: Potential double flush for completion events.The
rac_telemetry_manager_trackfunction (called at line 626) already has auto-flush logic that may flush the queue. Then lines 630-636 trigger an additional flush for completion events. This could result in redundant flush calls where the second one finds an empty queue.While functionally safe (empty queue returns early at line 663), this is inefficient and the double flush is conceptually confusing.
Consider consolidating the flush logic, perhaps by having
rac_telemetry_manager_trackaccept a hint about whether immediate flush is needed:- rac_result_t result = rac_telemetry_manager_track(manager, &payload); - - // For completion/failure events in production, trigger immediate flush - // This ensures important terminal events are captured before app exits - if (result == RAC_SUCCESS && - manager->environment != RAC_ENV_DEVELOPMENT && - is_completion_event(event_type) && - manager->http_callback) { - log_debug("Telemetry", "Completion event detected, triggering immediate flush"); - rac_telemetry_manager_flush(manager); - } - - return result; + // For completion events, force immediate flush in production + bool force_flush = is_completion_event(event_type) && + manager->environment != RAC_ENV_DEVELOPMENT; + + return rac_telemetry_manager_track_with_flush_hint(manager, &payload, force_flush);
46-48: Consider making batch configuration configurable.The batch size (10) and timeout (5000ms) are hardcoded as
static constexpr. For a production SDK, these might need tuning based on network conditions or app requirements. Consider making these configurable via the manager configuration.sdk/runanywhere-commons/scripts/download-core-prebuilt.sh (1)
52-61: Consider adding a timeout to the version fetch.The
curlcommand for fetching the latest version has no timeout, which could cause the script to hang indefinitely if the network is slow or unresponsive.🔎 Proposed fix
- LATEST=$(curl -sL "${RAW_URL}/LATEST_CORE_VERSION" 2>/dev/null || echo "") + LATEST=$(curl -sL --connect-timeout 10 --max-time 30 "${RAW_URL}/LATEST_CORE_VERSION" 2>/dev/null || echo "")examples/ios/RunAnywhereAI/scripts/build_and_run.sh (1)
316-317: Declare and assign separately to avoid masking return values.As flagged by static analysis, combining
localwith command substitution can mask the return value of the command.🔎 Proposed fix
+ local end_time - local end_time=$(date +%s) + end_time=$(date +%s)Note: This pattern also appears in other functions (e.g.,
build_core,build_swift_sdk,build_app,deploy_and_run). Consider applying the same fix consistently.sdk/runanywhere-commons/CMakeLists.txt (2)
111-114: Redundant variable self-assignments.Lines 113-114 assign
RUNANYWHERE_CORE_HAS_LLAMACPPandRUNANYWHERE_CORE_HAS_ONNXto themselves, which has no effect in CMake. IfFindRunAnywhereCorePrebuilt.cmakealready sets these variables, they're available without reassignment. If it doesn't, this won't create them.Consider removing these lines or documenting the intent if they serve as compatibility markers:
🔎 Proposed fix
# Set variables for compatibility with existing code set(RUNANYWHERE_CORE_INCLUDE_DIRS ${RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRS}) - set(RUNANYWHERE_CORE_HAS_LLAMACPP ${RUNANYWHERE_CORE_HAS_LLAMACPP}) - set(RUNANYWHERE_CORE_HAS_ONNX ${RUNANYWHERE_CORE_HAS_ONNX}) + # Note: RUNANYWHERE_CORE_HAS_LLAMACPP and RUNANYWHERE_CORE_HAS_ONNX + # are already set by FindRunAnywhereCorePrebuilt.cmake else()
247-267: Duplicate include directory blocks.Lines 247-256 add core includes as
PRIVATE, then lines 258-267 add the same directories asPUBLIC. This is redundant—PUBLICincludes are already visible to the target itself (in addition to consumers), making thePRIVATEblock unnecessary.🔎 Proposed fix - remove redundant PRIVATE block
# Private includes for implementation target_include_directories(rac_commons PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src ) -# Core includes (pre-built or source) -if(USE_PREBUILT_CORE AND DEFINED RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRS) - target_include_directories(rac_commons PRIVATE - ${RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRS} - ) -elseif(NOT USE_PREBUILT_CORE AND DEFINED RUNANYWHERE_CORE_INCLUDE_DIRS) - target_include_directories(rac_commons PRIVATE - ${RUNANYWHERE_CORE_INCLUDE_DIRS} - ) -endif() -# Include runanywhere-core headers (for pre-built or source mode) +# Core includes (pre-built or source) - PUBLIC for consumers if(USE_PREBUILT_CORE AND DEFINED RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRS) target_include_directories(rac_commons PUBLIC ${RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRS} ) elseif(NOT USE_PREBUILT_CORE AND DEFINED RUNANYWHERE_CORE_INCLUDE_DIRS) target_include_directories(rac_commons PUBLIC ${RUNANYWHERE_CORE_INCLUDE_DIRS} ) endif()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
examples/ios/RunAnywhereAI/scripts/build_and_run.shsdk/runanywhere-commons/CMakeLists.txtsdk/runanywhere-commons/backends/llamacpp/CMakeLists.txtsdk/runanywhere-commons/backends/llamacpp/src/rac_llm_llamacpp.cppsdk/runanywhere-commons/backends/onnx/CMakeLists.txtsdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cppsdk/runanywhere-commons/backends/onnx/src/rac_tts_onnx.cppsdk/runanywhere-commons/cmake/FindRunAnywhereCorePrebuilt.cmakesdk/runanywhere-commons/exports/RACommons.exportssdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.hsdk/runanywhere-commons/include/rac/core/rac_analytics_events.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_metrics.hsdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.hsdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.hsdk/runanywhere-commons/include/rac/infrastructure/network/rac_endpoints.hsdk/runanywhere-commons/include/rac/infrastructure/telemetry/rac_telemetry_types.hsdk/runanywhere-commons/scripts/build-ios.shsdk/runanywhere-commons/scripts/download-core-prebuilt.shsdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cppsdk/runanywhere-commons/src/core/events.cppsdk/runanywhere-commons/src/features/llm/llm_component.cppsdk/runanywhere-commons/src/features/llm/streaming_metrics.cppsdk/runanywhere-commons/src/features/stt/stt_component.cppsdk/runanywhere-commons/src/features/tts/tts_component.cppsdk/runanywhere-commons/src/features/voice_agent/voice_agent.cppsdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cppsdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cppsdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cppsdk/runanywhere-swift/Package.swiftsdk/runanywhere-swift/Sources/CRACommons/include/rac_llm_component.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_llm_metrics.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_structured_error.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_stt_component.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_telemetry_types.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_tts_component.hsdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Device.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+State.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+TTS.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Security/KeychainManager.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/Models/RunAnywhere+ModelManagement.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/TTS/RunAnywhere+TTS.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/RunAnywhere.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use the latest Swift 6 APIs always.
Do not use NSLock as it is outdated. Use Swift 6 concurrency primitives instead.
Files:
sdk/runanywhere-swift/Sources/RunAnywhere/Public/RunAnywhere.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+State.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Security/KeychainManager.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+TTS.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Device.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/TTS/RunAnywhere+TTS.swiftsdk/runanywhere-swift/Package.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/Models/RunAnywhere+ModelManagement.swift
sdk/runanywhere-swift/**/Package.swift
📄 CodeRabbit inference engine (CLAUDE.md)
sdk/runanywhere-swift/**/Package.swift: iOS minimum deployment target must be 13.0 or later for Swift SDK.
Swift minimum version must be 5.9 or later for iOS SDK.
Files:
sdk/runanywhere-swift/Package.swift
🧠 Learnings (3)
📚 Learning: 2025-11-26T22:49:40.358Z
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-swift/**/Package.swift : Swift minimum version must be 5.9 or later for iOS SDK.
Applied to files:
sdk/runanywhere-swift/Package.swift
📚 Learning: 2025-11-26T22:49:40.358Z
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-swift/**/Package.swift : iOS minimum deployment target must be 13.0 or later for Swift SDK.
Applied to files:
sdk/runanywhere-swift/Package.swift
📚 Learning: 2025-11-26T22:49:40.358Z
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Use `./scripts/sdk.sh` for all Kotlin SDK operations instead of calling Gradle directly. It handles configuration and build complexity.
Applied to files:
examples/ios/RunAnywhereAI/scripts/build_and_run.sh
🧬 Code graph analysis (26)
sdk/runanywhere-swift/Sources/RunAnywhere/Public/RunAnywhere.swift (3)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swift (1)
flush(133-140)sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift (2)
flush(247-252)debug(343-348)sdk/runanywhere-commons/include/rac/core/rac_logger.h (1)
debug(325-330)
sdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.h (1)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (4)
rac_lifecycle_load(147-221)rac_lifecycle_load(147-149)rac_lifecycle_get_model_name(317-329)rac_lifecycle_get_model_name(317-317)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+State.swift (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Environment.swift (1)
toC(20-26)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Security/KeychainManager.swift (1)
retrieveIfExists(196-206)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_tts_component.h (1)
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift (3)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swift (2)
loadModel(67-81)getHandle(31-45)sdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/Models/RunAnywhere+ModelManagement.swift (1)
loadModel(9-39)sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (1)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (4)
rac_lifecycle_load(147-221)rac_lifecycle_load(147-149)rac_lifecycle_get_model_name(317-329)rac_lifecycle_get_model_name(317-317)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Security/KeychainManager.swift (1)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Errors/SDKError.swift (1)
security(286-292)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+TTS.swift (5)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift (1)
getHandle(31-45)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swift (1)
getHandle(31-45)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+VAD.swift (1)
getHandle(30-44)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+VoiceAgent.swift (1)
getHandle(31-52)sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_llm_metrics.h (1)
sdk/runanywhere-commons/src/features/llm/streaming_metrics.cpp (2)
rac_streaming_metrics_set_token_counts(278-289)rac_streaming_metrics_set_token_counts(278-280)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (3)
sdk/runanywhere-commons/src/core/rac_core.cpp (3)
lock(83-83)lock(113-113)lock(228-228)sdk/runanywhere-commons/src/core/sdk_state.cpp (13)
lock(44-44)lock(55-58)lock(56-56)lock(60-80)lock(61-61)lock(82-105)lock(83-83)lock(111-114)lock(112-112)lock(116-119)lock(117-117)lock(121-124)lock(122-122)sdk/runanywhere-commons/src/infrastructure/events/event_publisher.cpp (2)
current_time_ms(47-50)current_time_ms(47-47)
sdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.h (1)
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (1)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (4)
rac_lifecycle_load(147-221)rac_lifecycle_load(147-149)rac_lifecycle_get_model_name(317-329)rac_lifecycle_get_model_name(317-317)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_llm_component.h (1)
sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Device.swift (2)
sdk/runanywhere-kotlin/src/commonMain/kotlin/com/runanywhere/sdk/data/network/models/AuthModels.kt (1)
architecture(97-117)sdk/runanywhere-kotlin/src/commonMain/kotlin/com/runanywhere/sdk/capabilities/device/ProcessorInfo.kt (1)
chipName(50-139)
sdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cpp (1)
sdk/runanywhere-commons/src/core/sdk_state.cpp (2)
env(42-53)env(42-43)
sdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.h (1)
sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_stt_component.h (1)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (2)
sdk/runanywhere-commons/src/core/sdk_state.cpp (2)
env(42-53)env(42-43)sdk/runanywhere-commons/src/infrastructure/model_management/model_assignment.cpp (1)
json(127-127)
sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp (3)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Telemetry.swift (4)
sdk/runanywhere-react-native/src/Foundation/Logging/Logger/SDKLogger.ts (1)
SDKLogger(15-67)sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift (2)
post(142-147)post(150-155)sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (2)
json(88-91)json(88-88)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+Environment.swift (1)
requiresAuth(39-41)
sdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.h (1)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)
sdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/TTS/RunAnywhere+TTS.swift (1)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+TTS.swift (1)
loadVoice(61-75)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swift (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift (2)
loadModel(61-75)getHandle(31-45)sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)
sdk/runanywhere-swift/Sources/RunAnywhere/Public/Extensions/Models/RunAnywhere+ModelManagement.swift (3)
sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+LLM.swift (1)
loadModel(61-75)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+STT.swift (1)
loadModel(67-81)sdk/runanywhere-swift/Sources/RunAnywhere/Foundation/Bridge/Extensions/CppBridge+TTS.swift (1)
loadVoice(61-75)
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp (1)
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (2)
json(88-91)json(88-88)
sdk/runanywhere-commons/src/features/llm/llm_component.cpp (1)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (6)
rac_lifecycle_load(147-221)rac_lifecycle_load(147-149)rac_lifecycle_get_model_id(303-315)rac_lifecycle_get_model_id(303-303)rac_lifecycle_get_model_name(317-329)rac_lifecycle_get_model_name(317-317)
🪛 Shellcheck (0.11.0)
sdk/runanywhere-commons/scripts/download-core-prebuilt.sh
[warning] 164-164: Use "${var:?}" to ensure this never expands to / .
(SC2115)
examples/ios/RunAnywhereAI/scripts/build_and_run.sh
[warning] 316-316: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (2)
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
| // Register ONNX backend in runanywhere-core's BackendRegistry | ||
| // This must be called before ra_create_backend("onnx") can work | ||
| // NOTE: ensure_backends_registered() in runanywhere_bridge.cpp also registers | ||
| // backends, but we call this explicitly to ensure it happens early | ||
| try { | ||
| runanywhere::register_onnx_backend(); | ||
| RAC_LOG_INFO(LOG_CAT, "ONNX backend registered in runanywhere-core BackendRegistry"); | ||
| } catch (const std::exception& e) { | ||
| RAC_LOG_ERROR(LOG_CAT, "Failed to register ONNX backend in core: %s", e.what()); | ||
| // Continue anyway - ensure_backends_registered() might handle it | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ensure_backends_registered and check its relationship with this registration
rg -n -C5 'ensure_backends_registered|register_onnx_backend' --type=cppRepository: RunanywhereAI/runanywhere-sdks
Length of output: 3516
🏁 Script executed:
#!/bin/bash
# Find ensure_backends_registered() function and its implementation
rg -n -B3 -A10 'ensure_backends_registered' --type=cpp
# Also find the containing function's signature and return path
rg -n -B20 'Register ONNX backend in runanywhere-core' --type=cpp | grep -E '(^[^:]+:[0-9]+:\s*(rac_|RAC_|void|int|bool|auto)|RAC_SUCCESS|return)'Repository: RunanywhereAI/runanywhere-sdks
Length of output: 2791
🏁 Script executed:
#!/bin/bash
# Find the containing function signature
rg -n -B50 'Register ONNX backend in runanywhere-core' sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp | grep -E '(^[^:]+:[0-9]+:\s*(rac_|RAC_|^[a-zA-Z_].*\()|return RAC_SUCCESS)' | head -20
# Find runanywhere_bridge.cpp
fd -type f 'runanywhere_bridge' --type f
# Also search for ensure_backends_registered in all files
rg -n 'ensure_backends_registered' --type=cppRepository: RunanywhereAI/runanywhere-sdks
Length of output: 598
🏁 Script executed:
#!/bin/bash
# Get the containing function by looking backwards from line 494
sed -n '400,510p' sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp | head -110
# Search for ensure_backends_registered in all file types
rg -n 'ensure_backends_registered'
# Check what exceptions might be thrown
rg -n 'throw' sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cppRepository: RunanywhereAI/runanywhere-sdks
Length of output: 4863
🏁 Script executed:
#!/bin/bash
# Get the end of the rac_backend_onnx_register function
sed -n '489,600p' sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp
# Search all files for ensure_backends_registered to verify it exists
rg -r 'ensure_backends_registered' --no-headingRepository: RunanywhereAI/runanywhere-sdks
Length of output: 4319
🏁 Script executed:
#!/bin/bash
# Search for ensure_backends_registered in all files
grep -r 'ensure_backends_registered' . 2>/dev/null | head -20
# Also check what exceptions runanywhere::register_onnx_backend might throw
grep -r 'register_onnx_backend' . --include="*.cpp" --include="*.h" -B5 -A5 | head -40Repository: RunanywhereAI/runanywhere-sdks
Length of output: 3101
Address incomplete exception handling and silent failure in backend registration.
Two substantive concerns with this registration code:
-
Incomplete exception handling: Only
std::exceptionis caught (line 501). Ifrunanywhere::register_onnx_backend()throws a non-standard exception type, it will propagate uncaught and crash the registration process. Consider catching all exceptions withcatch (...)instead. -
Silent failure masks initialization issues: The function logs an error but continues execution and ultimately returns
RAC_SUCCESS(line 597) even when core backend registration fails. This masks initialization problems and could cause runtime failures later when attempting to use the ONNX backend.
The comment referencing ensure_backends_registered() in runanywhere_bridge.cpp should be clarified or removed if that function doesn't handle fallback registration.
There was a problem hiding this comment.
// Continue anyway - this is best-effort early registration.
The backend will be registered by ensure_backends_registered() in runanywhere_bridge.cpp during first use if this fails.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ensure_backends_registered more comprehensively
echo "=== Searching for ensure_backends_registered ==="
rg -n 'ensure_backends_registered' --no-heading
echo -e "\n=== Searching for runanywhere_bridge files ==="
fd -t f 'runanywhere_bridge' --exec echo {}
echo -e "\n=== Searching for register_onnx_backend calls ==="
rg -n 'register_onnx_backend\(\)' --type=cpp -C 3
echo -e "\n=== Searching for backend registration patterns in bridge files ==="
fd -t f 'bridge' --exec rg -l 'register.*backend|ensure.*backend' {} \; | head -10
Length of output: 2442
@shubhammalhotra28, I searched the codebase for the ensure_backends_registered() function you mentioned, but it doesn't appear to exist. The function is only referenced in comments within this file itself, and there's no runanywhere_bridge.cpp file in the repository (only runanywhere_bridge.h in the React Native SDK).
Without a verified fallback mechanism, the silent failure could mask initialization issues. Two options to consider:
- If failure is truly acceptable: Update the comment to accurately explain why (e.g., "Backend will be lazily registered on first use via [actual mechanism]")
- If failure should propagate: Return an error code when core registration fails
Additionally, the exception handler should catch all exceptions, not just std::exception:
} catch (...) {
RAC_LOG_ERROR(LOG_CAT, "Failed to register ONNX backend in core (unknown exception)");
// Document why continuing is safe, or return an error
}Could you clarify where the actual fallback registration happens, or confirm if registration failure should be treated as a fatal error?
🧬 Code Graph Analysis Results
-
File: sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp, lines 30-36
Forward declaration for runanywhere ONNX backend registration:
namespace runanywhere {
void register_onnx_backend();
} -
File: sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp, lines 400-480
rac_result_t rac_backend_onnx_register(void) {
if (g_registered) {
return RAC_ERROR_MODULE_ALREADY_REGISTERED;
}// Register ONNX backend in runanywhere-core's BackendRegistry // This must be called before ra_create_backend("onnx") can work // NOTE: ensure_backends_registered() in runanywhere_bridge.cpp also registers // backends, but we call this explicitly to ensure it happens early try { runanywhere::register_onnx_backend(); RAC_LOG_INFO(LOG_CAT, "ONNX backend registered in runanywhere-core BackendRegistry"); } catch (const std::exception& e) { RAC_LOG_ERROR(LOG_CAT, "Failed to register ONNX backend in core: %s", e.what()); // Continue anyway - ensure_backends_registered() might handle it } // ... rest of registration logic for module, strategies, and providers}
-
File: sdk/runanywhere-commons/backends/onnx/src/rac_backend_onnx_register.cpp, lines 480-640
(Continuation of rac_backend_onnx_register)
// The function continues with module registration, strategy registrations,
// and provider registrations, concluding with g_registered = true on success.
Note: If you need exact line-numbered excerpts, please provide a source with line numbers or your preferred ranges, and I can adjust precisely.
| * @param voice_path File path to the voice (used for loading) | ||
| * @param voice_id Voice identifier (used for telemetry, e.g., "vits-piper-en_GB-alba-medium") | ||
| * @param voice_name Human-readable voice name (e.g., "Piper TTS (British English)") | ||
| * @return RAC_SUCCESS or error code | ||
| */ | ||
| RAC_API rac_result_t rac_tts_component_load_voice(rac_handle_t handle, const char* voice_id); | ||
| RAC_API rac_result_t rac_tts_component_load_voice(rac_handle_t handle, const char* voice_path, | ||
| const char* voice_id, const char* voice_name); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to rac_tts_component_load_voice to verify all have been updated
echo "=== Searching for rac_tts_component_load_voice calls in C/C++ ==="
rg -nP --type=cpp --type=c -C3 'rac_tts_component_load_voice\s*\('
echo -e "\n=== Searching for rac_tts_component_load_voice calls in Swift ==="
rg -nP --type=swift -C3 'rac_tts_component_load_voice'
echo -e "\n=== Searching for rac_tts_component_load_voice in all file types ==="
rg -nP -C3 'rac_tts_component_load_voice\s*\(' --max-count=100Repository: RunanywhereAI/runanywhere-sdks
Length of output: 10707
Fix argument mapping in voice_agent.cpp call sites.
All callers have been updated to the new 4-parameter signature, but two call sites in voice_agent.cpp are semantically incorrect: line 331 passes voice_id for all three parameters, and lines 445-446 pass cfg->tts_config.voice for all three. The Swift implementation correctly passes distinct values (pathPtr, idPtr, namePtr). Update the C++ call sites to properly map the parameters:
voice_path: appropriate file path or voice identifiervoice_id: telemetry identifier (e.g., "vits-piper-en_GB-alba-medium")voice_name: human-readable name (e.g., "Piper TTS (British English)")
🤖 Prompt for AI Agents
In sdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.h around
voice_agent.cpp call sites (voice_agent.cpp lines ~331 and ~445-446), the
rac_tts_component_load_voice calls currently pass the same value for all three
parameters; update those calls to match the intended 4-parameter mapping used by
the Swift implementation: pass the voice file path (or voice identifier used for
loading) as the first argument (voice_path), the telemetry identifier (e.g.,
"vits-piper-en_GB-alba-medium") as the second (voice_id), and the human-readable
voice name (e.g., "Piper TTS (British English)") as the third (voice_name).
Concretely, replace the repeated identical arguments with the correct trio
(path, id, name) — mirroring the Swift call (pathPtr, idPtr, namePtr) or using
the corresponding C++ variables (e.g., voice_path, voice_id, voice_name or
cfg->tts_config.voice_path / cfg->tts_config.voice_id /
cfg->tts_config.voice_name) so each parameter receives the correct value.
| # Clean up XCFramework | ||
| rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME}" |
There was a problem hiding this comment.
Protect against accidental deletion of parent directory.
The static analysis tool flagged this correctly. If XCFW_NAME were empty (which shouldn't happen, but defensive coding is good), this could delete ${THIRD_PARTY_DIR}/ contents.
🔎 Proposed fix
- rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME}"
+ rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME:?XCFW_NAME must be set}"📝 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.
| # Clean up XCFramework | |
| rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME}" | |
| # Clean up XCFramework | |
| rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME:?XCFW_NAME must be set}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 164-164: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🤖 Prompt for AI Agents
In sdk/runanywhere-commons/scripts/download-core-prebuilt.sh around lines
163-164, the rm -rf "${THIRD_PARTY_DIR}/${XCFW_NAME}" call can delete the parent
directory if XCFW_NAME is empty; add a defensive guard that verifies XCFW_NAME
is non-empty and that the resolved target path is not identical to
THIRD_PARTY_DIR (or “/”) before running rm -rf, and if the checks fail, abort
with an error message and non-zero exit code so deletion never happens on an
unexpected empty or invalid variable.
sdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cpp
Outdated
Show resolved
Hide resolved
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp
Outdated
Show resolved
Hide resolved
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp
Outdated
Show resolved
Hide resolved
|
@coderabbitai please do a detailed review |
|
@greptile please do a detailed review |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 97c59f6 in 1 minute and 14 seconds. Click for details.
- Reviewed
266lines of code in7files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift:97
- Draft comment:
Consider using a stricter check than using 'contains' on the URL string to match the device registration endpoint. A more robust comparison (e.g. checking if the path exactly equals the dev endpoint or using URLComponents) can help avoid false positives. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift:192
- Draft comment:
The buildURL() method manually splits the path to handle query parameters. Consider leveraging URLComponents for constructing the full URL to ensure proper encoding and avoid potential issues with duplicated or missing slashes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. sdk/runanywhere-swift/Package.swift:25
- Draft comment:
Good fix: The testLocal flag is now set to false for production. Ensure that this remains false in CI/CD to prevent local framework loading. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure a specific behavior in CI/CD, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or ask for confirmation of an intentional change.
4. sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift:259
- Draft comment:
In resolveToken(), consider synchronizing token refresh to avoid initiating multiple simultaneous refreshes when several requests detect an expired token. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift:289
- Draft comment:
The error parsing in parseHTTPError() currently checks a few keys. Consider expanding this logic or logging additional response data to aid in diagnosing server-side issues. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_Hq45uvfLpE6b7d1N
You can customize
|
Tip For best results, initiate chat on the files or code changes. @shubhammalhotra28, I'll provide a detailed review of this substantial PR. Let me break down my analysis by functional area: 🏗️ Build System & Local DevelopmentStrengths:
Concerns:
🔄 API Signature ChangesMajor breaking changes - all model/voice loading APIs now require three parameters instead of one: // Before
rac_llm_component_load_model(handle, model_id)
// After
rac_llm_component_load_model(handle, model_path, model_id, model_name)Concerns:
📱 Device RegistrationGood additions:
Critical Issues:
📊 Telemetry & MetricsExcellent improvements:
Issues:
🌐 Network LayerGood additions:
Concerns:
🔒 Security & KeychainExcellent improvement:
Minor suggestion: public func retrieveIfExists(for key: String) throws -> String? {
guard let data = try retrieveDataIfExists(for: key) else {
return nil
}
guard let string = String(data: data, encoding: .utf8) else {
throw KeychainError.invalidData // Better than returning nil
}
return string
}🧪 Testing RecommendationsThis PR needs extensive testing:
📋 SummaryHigh Priority Issues:
Medium Priority:
Positive Aspects:
Recommendation: Address the high-priority issues before merge, especially the 🧠 Learnings used |
There was a problem hiding this comment.
Additional Comments (1)
-
sdk/runanywhere-commons/backends/onnx/CMakeLists.txt, line 108-110 (link)logic: duplicate conditional check - this code is unreachable since line 95 handles the same condition
50 files reviewed, 1 comment
- Add clear documentation that model_id and model_name are optional - model_id defaults to model_path if NULL - model_name defaults to model_id if NULL - Fix Swift-side rac_lifecycle.h signature (was out of sync) Updated headers: - rac_lifecycle.h (C++ and Swift) - rac_llm_component.h - rac_stt_component.h - rac_tts_component.h
The elseif(DEFINED RUNANYWHERE_CORE_ONNX_LIB) at line 108 was unreachable because the same condition is already checked at line 95.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed dbffa3b in 50 seconds. Click for details.
- Reviewed
95lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.h:143
- Draft comment:
Clarify fallback chain: if model_id defaults to model_path and model_name defaults to model_id, then if model_id is NULL, model_name remains NULL. Confirm intended behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. sdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.h:116
- Draft comment:
Verify the default chain: model_id defaults to model_path and model_name defaults to model_id – consider if this is the desired cascading behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. sdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.h:68
- Draft comment:
Revisit fallback defaults: model_id is optional and defaults to model_path, and model_name defaults to model_id. Ensure this order is intended for STT models. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. sdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.h:68
- Draft comment:
Ensure fallback logic for voice parameters is as intended: if voice_id defaults to voice_path and voice_name defaults to voice_id, then a missing voice_id may lead to a NULL voice_name. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. sdk/runanywhere-swift/Sources/CRACommons/include/rac_lifecycle.h:142
- Draft comment:
Double-check the fallback descriptions: with model_id defaulting to model_path and model_name defaulting to model_id, confirm if a NULL model_id results in a meaningful model_name. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_jGMJEx3AYbIAffnA
You can customize
|
All comments are addressed, just need to do one more testing again. Thanks! |
|
@coderabbitai please do a detailed review. |
|
@shubhammalhotra28, I'll perform a detailed review of the updated changes now that you've addressed the previous feedback. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2cabdaf in 1 minute and 51 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sdk/runanywhere-commons/backends/onnx/CMakeLists.txt:105
- Draft comment:
Ensure that the removal of the fallback branch linking RUNANYWHERE_CORE_ONNX_LIB is intentional. This change may break setups that rely on linking a pre-built ONNX library when the target is not available. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment violates multiple rules: 1) It asks the author to "ensure" something is intentional, which is explicitly mentioned as not useful in the rules. 2) It's speculative - "may break setups" is a conditional statement without strong evidence. 3) Looking at the full file, the RUNANYWHERE_CORE_ONNX_LIB case is still handled at lines 95-98, so the comment appears to be factually incorrect about what was removed. The diff shows removal of an elseif branch, but that same logic exists earlier in the file structure. The comment doesn't provide actionable feedback and asks for confirmation rather than pointing to a definite issue. I might be misreading the structure - perhaps there were two separate places where RUNANYWHERE_CORE_ONNX_LIB was handled and one was intentionally removed as redundant. The comment could be valid if this was a fallback case that served a different purpose than the one at lines 95-98. Even if there were two separate cases, the comment still violates the rule about asking the author to "ensure" something is intentional. The comment is phrased as asking for confirmation rather than identifying a definite problem. Additionally, it's speculative ("may break") without providing strong evidence of an actual issue. The RUNANYWHERE_CORE_ONNX_LIB functionality is clearly still present in the file. This comment should be deleted. It asks the author to confirm their intention ("Ensure that..."), which is explicitly prohibited. It's also speculative ("may break") without strong evidence of a problem. The RUNANYWHERE_CORE_ONNX_LIB case is still handled in the file at lines 95-98, so the concern appears unfounded.
Workflow ID: wflow_t3Am5p7dCngyEHYN
You can customize
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_lifecycle.h (2)
106-107: Callback signature is inconsistent with the new load API.The
rac_lifecycle_create_service_fncallback only receivesmodel_id, but the documentation states thatmodel_pathis "used for loading" and is the REQUIRED parameter (line 144). If the callback is responsible for creating/loading the service, it needs access tomodel_path.🔎 Proposed fix: Update callback signature to receive all three parameters
/** * @brief Service creation callback * * Called by the lifecycle manager to create a service for a given model ID. * + * @param model_path The file path to load * @param model_id The model ID to load + * @param model_name The human-readable model name * @param user_data User-provided context * @param out_service Output: Handle to the created service * @return RAC_SUCCESS or error code */ -typedef rac_result_t (*rac_lifecycle_create_service_fn)(const char* model_id, void* user_data, +typedef rac_result_t (*rac_lifecycle_create_service_fn)(const char* model_path, + const char* model_id, + const char* model_name, + void* user_data, rac_handle_t* out_service);
196-214: Missingrac_lifecycle_get_model_nameaccessor API.The AI summary states that
rac_lifecycle_get_model_namewas added as a companion to the existingrac_lifecycle_get_model_id(line 204), but this accessor is not declared in the header file. Sincemodel_nameis now a first-class parameter in the load API, a corresponding getter should be provided for consistency.🔎 Proposed addition: Add missing accessor
Add after line 214:
RAC_API rac_handle_t rac_lifecycle_get_service(rac_handle_t handle); +/** + * @brief Get current model name + * + * Mirrors Swift's ManagedLifecycle.currentModelName + * + * @param handle Lifecycle manager handle + * @return Current model name (may be NULL if not loaded) + */ +RAC_API const char* rac_lifecycle_get_model_name(rac_handle_t handle); + /** * @brief Require service or return errorsdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift (1)
237-254: Validate 409 conflict details before treating as success.Line 239 treats all 409 responses for device registration as success without examining the response body or conflict details. This assumes every conflict means "device already exists with identical data," but 409 could indicate:
- Constraint violations on unexpected fields
- Data mismatches that merge-duplicates couldn't resolve
- Concurrent registration race conditions with different payloads
The Supabase
resolution=merge-duplicatesbehavior should be validated by checking the response body to confirm the conflict was successfully resolved and the expected device state was achieved.🔎 Suggested validation approach
let isDeviceRegistration = request.url?.absoluteString.contains(RAC_ENDPOINT_DEV_DEVICE_REGISTER) ?? false let isSuccess = (200...299).contains(httpResponse.statusCode) || (isDeviceRegistration && httpResponse.statusCode == 409) guard isSuccess else { let error = parseHTTPError( statusCode: httpResponse.statusCode, data: data, url: request.url?.absoluteString ?? "unknown" ) logger.error("HTTP \(httpResponse.statusCode): \(request.url?.absoluteString ?? "unknown")") throw error } if isDeviceRegistration && httpResponse.statusCode == 409 { - logger.info("Device already registered (409) - treating as success") + // Validate that Supabase successfully merged the device record + // The response should contain the merged device record + if data.isEmpty { + logger.warning("Device registration 409 with empty response - conflict may not be resolved") + } else { + logger.info("Device already registered (409) - treating as success") + } }Consider also parsing the response to confirm it contains the expected device record structure.
♻️ Duplicate comments (2)
sdk/runanywhere-swift/Package.swift (1)
25-25: Good fix:testLocal = falseresolves the previous critical issue.Setting
testLocal = falseensures that production builds download XCFrameworks from GitHub releases, preventing build failures for other developers. This directly addresses the concern raised in previous reviews.sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift (1)
92-103: Use more robust endpoint matching instead of substring check.The
contains()check on line 97 will match any path containing the device registration substring, which could trigger UPSERT logic on unintended endpoints. This duplicates the fragile check at line 238.Consider:
- Using
hasSuffix()or exact path matching instead ofcontains()- Centralizing the endpoint check in a helper method to avoid duplication
- Comparing against a canonical path constant rather than substring matching
🔎 Suggested refactor
+ private func isDeviceRegistrationPath(_ path: String) -> Bool { + let normalizedPath = path.split(separator: "?").first.map(String.init) ?? path + return normalizedPath.hasSuffix(RAC_ENDPOINT_DEV_DEVICE_REGISTER) + } + public func postRaw( _ path: String, _ payload: Data, requiresAuth: Bool ) async throws -> Data { - if path.contains(RAC_ENDPOINT_DEV_DEVICE_REGISTER) { + if isDeviceRegistrationPath(path) { let upsertPath = path.contains("?") ? "\(path)&on_conflict=device_id" : "\(path)?on_conflict=device_id" return try await postRawWithHeaders(upsertPath, payload, requiresAuth: requiresAuth, additionalHeaders: ["Prefer": "resolution=merge-duplicates"]) }Then reuse the helper at line 238.
🧹 Nitpick comments (9)
sdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cpp (1)
164-165: Debug logging for diagnostics is helpful.The endpoint and truncated payload logging at DEBUG level aids troubleshooting. The null check on
json_ptr(line 165) is defensive but technically redundant since the early return at line 150 ensuresjson_ptris non-null at this point.🔎 Optional: Remove redundant null check
- RAC_LOG_DEBUG(LOG_CAT, "Registration JSON payload (first 200 chars): %.200s", json_ptr ? json_ptr : "(null)"); + RAC_LOG_DEBUG(LOG_CAT, "Registration JSON payload (first 200 chars): %.200s", json_ptr);sdk/runanywhere-swift/Package.swift (1)
111-111: Consider clarifying the dependency comment.The comment states "RABackendONNX requires RunAnywhereCore for ra_create_backend symbols," which might suggest that RunAnywhereCore needs to be added as a dependency. However, the helper function comment (lines 206-207) clarifies that these symbols are already included in RABackendONNXBinary. Consider rewording to avoid potential confusion, e.g., "Note: RABackendONNX provides ra_create_backend symbols (included in binary)."
sdk/runanywhere-commons/backends/onnx/CMakeLists.txt (2)
78-86: Consider using value check instead ofDEFINEDfor robustness.
if(DEFINED RUNANYWHERE_CORE_BRIDGE_LIB)evaluates true even if the variable is set to an empty string. Consider usingif(RUNANYWHERE_CORE_BRIDGE_LIB)which checks both existence and non-empty value. Additionally, if neither condition is met, linkage is silently skipped, which could lead to confusing linker errors.🔎 Suggested improvement
# Link runanywhere_bridge directly to ensure ra_create_backend symbols are available # Use pre-built library if available, otherwise use compiled target -if(DEFINED RUNANYWHERE_CORE_BRIDGE_LIB) +if(RUNANYWHERE_CORE_BRIDGE_LIB) # Pre-built library mode (USE_PREBUILT_CORE=ON) target_link_libraries(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_BRIDGE_LIB}) elseif(TARGET runanywhere_bridge) # Source compilation mode (legacy) target_link_libraries(rac_backend_onnx PRIVATE runanywhere_bridge) +else() + message(WARNING "No runanywhere_bridge available for rac_backend_onnx - symbols may be missing") endif()
94-108: Add backend-specific include directories in prebuilt mode for consistency and maintainability.The prebuilt include setup (
RUNANYWHERE_CORE_PREBUILT_INCLUDE_DIRSin FindRunAnywhereCorePrebuilt.cmake) does not include backend-specific paths likesrc/backends/onnx, whereas source mode explicitly includes them. Whilerac_backend_onnxreceives transitive includes via its public link torac_commons, the comment on line 98 suggests includes are "set globally," which is misleading. For clarity and to match source mode behavior, explicitly add backend-specific includes in prebuilt mode:🔎 Suggested improvement
# Link against runanywhere-core's onnx backend # Use pre-built library if available, otherwise use compiled target -if(DEFINED RUNANYWHERE_CORE_ONNX_LIB) +if(RUNANYWHERE_CORE_ONNX_LIB) # Pre-built library mode (USE_PREBUILT_CORE=ON) target_link_libraries(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_ONNX_LIB}) - # Include directories are set globally via RUNANYWHERE_CORE_INCLUDE_DIRS + # Include backend-specific headers from prebuilt core + if(DEFINED RUNANYWHERE_CORE_PREBUILT_DIR) + target_include_directories(rac_backend_onnx PRIVATE + ${RUNANYWHERE_CORE_PREBUILT_DIR}/src/backends/onnx + ) + endif() elseif(TARGET runanywhere_onnx) # Source compilation mode (legacy) target_link_libraries(rac_backend_onnx PRIVATE runanywhere_onnx) # Include runanywhere-core headers for backend APIs target_include_directories(rac_backend_onnx PRIVATE ${RUNANYWHERE_CORE_DIR}/src ${RUNANYWHERE_CORE_DIR}/src/capabilities ${RUNANYWHERE_CORE_DIR}/src/backends/onnx ) +else() + message(WARNING "No runanywhere_onnx backend available for rac_backend_onnx") endif()sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp (2)
442-447: Note: Timeout flush is passive, not timer-driven.The timeout check only triggers when a new event arrives, meaning events could sit in the queue longer than 5 seconds if no new events are enqueued. This is acceptable for telemetry since:
- Completion events trigger immediate flush anyway
- Manager destruction flushes remaining events
- A truly idle app doesn't need frequent flushes
If stricter timing guarantees are needed in the future, consider adding an active timer thread.
601-606: Consider removing verbose debug logging in production.While helpful during development, logging every TTS event's voice info at debug level may be noisy. The null-check logging is useful, but the else-branch logging the voice value on every event might be excessive for production telemetry code.
sdk/runanywhere-swift/Sources/CRACommons/include/rac_voice_agent.h (1)
362-370: Consider documenting nullability of optional parameters.The function parameters
model_idandmodel_nameappear to be optional based on the implementation (onlymodel_pathis validated). Consider adding explicit nullability notes to help API consumers understand which parameters are required vs optional.🔎 Suggested documentation enhancement
/** * @brief Load an STT model into the voice agent. * * @param handle Voice agent handle * @param model_path File path to the model (used for loading) - * @param model_id Model identifier (used for telemetry, e.g., "whisper-base") - * @param model_name Human-readable model name (e.g., "Whisper Base") + * @param model_id Model identifier for telemetry (can be NULL, defaults to model_path) + * @param model_name Human-readable model name (can be NULL, defaults to model_id) * @return RAC_SUCCESS or error code */Apply similar changes to
rac_voice_agent_load_llm_modelandrac_voice_agent_load_tts_voice.sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (2)
426-428: Consider distinguishing between zero and unset for last_seen_at.The code adds
last_seen_atonly when> 0, which treats zero as "unset". If zero is a valid timestamp (Unix epoch, January 1, 1970), this could silently skip a legitimate value. However, for UPSERT device registration, this is likely acceptable since timestamps are typically current time.If zero is a valid timestamp in your domain, consider using a sentinel value (e.g.,
-1) or a separate boolean flag to distinguish "not set" from "epoch time".Also applies to: 477-479
384-480: Significant schema divergence between development and production - validate with backend teams.The development path uses a flattened structure while production uses nested
device_info. Key differences include:
Field Development Production Structure Flattened Nested device_infodevice_id Top-level (line 392) Absent at top-level build_token Included (lines 408-410) Omitted Required fields Conditionally added Always added with defaults While the comments indicate these schemas match their respective backends (Supabase vs FastAPI), ensure both backend teams are aligned on:
- Field mappings and required vs optional constraints
- UPSERT semantics with
last_seen_attimestamp- How
device_idis passed in production (if not in the JSON body)Consider adding integration tests that validate the JSON output against JSON schemas for both environments to catch schema drift early.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
sdk/runanywhere-commons/backends/onnx/CMakeLists.txtsdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.hsdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.hsdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.hsdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.hsdk/runanywhere-commons/include/rac/features/voice_agent/rac_voice_agent.hsdk/runanywhere-commons/src/features/tts/tts_component.cppsdk/runanywhere-commons/src/features/voice_agent/voice_agent.cppsdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cppsdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cppsdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cppsdk/runanywhere-swift/Package.swiftsdk/runanywhere-swift/Sources/CRACommons/include/rac_endpoints.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_lifecycle.hsdk/runanywhere-swift/Sources/CRACommons/include/rac_voice_agent.hsdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/runanywhere-commons/include/rac/core/capabilities/rac_lifecycle.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use the latest Swift 6 APIs always.
Do not use NSLock as it is outdated. Use Swift 6 concurrency primitives instead.
Files:
sdk/runanywhere-swift/Package.swiftsdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift
sdk/runanywhere-swift/**/Package.swift
📄 CodeRabbit inference engine (CLAUDE.md)
sdk/runanywhere-swift/**/Package.swift: iOS minimum deployment target must be 13.0 or later for Swift SDK.
Swift minimum version must be 5.9 or later for iOS SDK.
Files:
sdk/runanywhere-swift/Package.swift
🧠 Learnings (2)
📚 Learning: 2025-11-26T22:49:40.358Z
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-swift/**/Package.swift : iOS minimum deployment target must be 13.0 or later for Swift SDK.
Applied to files:
sdk/runanywhere-swift/Package.swift
📚 Learning: 2025-11-26T22:49:40.358Z
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-swift/**/Package.swift : Swift minimum version must be 5.9 or later for iOS SDK.
Applied to files:
sdk/runanywhere-swift/Package.swift
🧬 Code graph analysis (9)
sdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cpp (1)
sdk/runanywhere-commons/src/core/sdk_state.cpp (2)
env(42-53)env(42-43)
sdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.h (1)
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.h (1)
sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)
sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp (3)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)sdk/runanywhere-commons/src/features/llm/llm_component.cpp (2)
rac_llm_component_load_model(230-241)rac_llm_component_load_model(230-231)sdk/runanywhere-commons/src/features/tts/tts_component.cpp (2)
rac_tts_component_load_voice(204-214)rac_tts_component_load_voice(204-205)
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (1)
sdk/runanywhere-commons/src/core/capabilities/lifecycle_manager.cpp (4)
rac_lifecycle_load(147-221)rac_lifecycle_load(147-149)rac_lifecycle_get_model_name(317-329)rac_lifecycle_get_model_name(317-317)
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (1)
sdk/runanywhere-commons/src/core/sdk_state.cpp (2)
env(42-53)env(42-43)
sdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.h (1)
sdk/runanywhere-commons/src/features/stt/stt_component.cpp (2)
rac_stt_component_load_model(225-235)rac_stt_component_load_model(225-226)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_voice_agent.h (1)
sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp (6)
rac_voice_agent_load_stt_model(254-287)rac_voice_agent_load_stt_model(254-257)rac_voice_agent_load_llm_model(289-322)rac_voice_agent_load_llm_model(289-292)rac_voice_agent_load_tts_voice(324-357)rac_voice_agent_load_tts_voice(324-327)
sdk/runanywhere-commons/include/rac/features/voice_agent/rac_voice_agent.h (1)
sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp (6)
rac_voice_agent_load_stt_model(254-287)rac_voice_agent_load_stt_model(254-257)rac_voice_agent_load_llm_model(289-322)rac_voice_agent_load_llm_model(289-292)rac_voice_agent_load_tts_voice(324-357)rac_voice_agent_load_tts_voice(324-327)
🔇 Additional comments (33)
sdk/runanywhere-swift/Sources/CRACommons/include/rac_endpoints.h (1)
37-37: Endpoint rename looks good; verify backend and all callers are aligned.The rename from
device_registrationstosdk_devicesis cleaner and matches the updated endpoint in the commons header. Ensure the Supabase table/view has been updated accordingly and that all HTTP clients targeting this development endpoint are consuming this macro rather than hardcoding the path.sdk/runanywhere-commons/src/infrastructure/device/rac_device_manager.cpp (2)
104-117: Environment-aware registration logic is well-designed.The distinction between production (skip if registered) and development (always UPSERT for
last_seen_attracking) is sound. The inline comments clearly explain the rationale, which aids maintainability.
126-126: Duplicate log removed.The duplicate
RAC_LOG_INFOfor device ID (previously flagged) has been addressed.sdk/runanywhere-swift/Package.swift (2)
201-210: Nice refactor: centralized dependency management.The
onnxBackendDependencies()helper function effectively centralizes the dependency list for ONNXBackend, making it easier to maintain and update. The implementation is correct, returning the requiredRABackendONNXBinaryandONNXRuntimeBinarydependencies.
216-243: Clear documentation and correct implementation.The updated comments clearly explain the binary target selection behavior for both local development (
testLocal = true) and production (testLocal = false) modes. The implementation correctly uses local paths for commons frameworks and remote URLs for onnxruntime in local mode, which aligns with the documented behavior.sdk/runanywhere-swift/Sources/CRACommons/include/rac_lifecycle.h (2)
144-149: Documentation is clear and well-structured.The parameter documentation clearly distinguishes required vs. optional parameters and documents the defaulting chain (model_name → model_id → model_path). The examples help clarify intended usage.
Ensure the implementation correctly handles the cascading defaults, especially when model_id is NULL (model_name should default to the resolved model_path, not to NULL).
152-154: Breaking API change—all call sites correctly updated.This is a breaking signature change, and the codebase has been properly migrated. All three call sites (tts_component.cpp, stt_component.cpp, llm_component.cpp) pass distinct values for model_path, model_id, and model_name. The implementation includes defensive fallback logic: if model_id is null, it uses model_path; if model_name is null, it uses model_id. No incomplete migrations remain.
sdk/runanywhere-swift/Sources/RunAnywhere/Data/Network/Services/HTTPService.swift (2)
214-221: LGTM - Clean header propagation.The
additionalHeadersparameter and propagation logic is well-implemented, enabling flexible header customization for specific request types.
259-281: Confirm this dual-auth design is intentional.Lines 272-276 implement a documented fallback to API key when
requiresAuth=truebut no OAuth token is available. The code comment explicitly states this "supports API key-only authentication for production mode."This creates a dual-auth model where
requiresAuth=trueendpoints accept either:
- OAuth tokens (user-level authentication), or
- API keys (service-level authentication) as fallback
Confirm with the team that:
- This dual-auth behavior for
requiresAuth=trueendpoints is intentional, not an oversight- Downstream services are designed to accept and correctly handle both OAuth and API-key Bearer tokens
- The fallback is acceptable for your compliance/audit requirements (API key requests won't have user attribution)
sdk/runanywhere-commons/include/rac/features/stt/rac_stt_component.h (1)
67-75: LGTM! Consistent API pattern with LLM component.The signature and documentation mirror the LLM component changes, maintaining a consistent API pattern across all capability components. The parameter semantics and defaults are identical, which is good for developer experience.
sdk/runanywhere-commons/include/rac/features/tts/rac_tts_component.h (1)
67-75: LGTM! Consistent API pattern with voice-appropriate naming.The signature follows the same pattern as LLM/STT with appropriate voice-specific terminology (voice_path, voice_id, voice_name). Documentation is clear and consistent.
sdk/runanywhere-commons/src/features/tts/tts_component.cpp (3)
204-213: LGTM! Implementation correctly delegates to lifecycle manager.The updated signature correctly passes all three parameters to
rac_lifecycle_load, maintaining consistency with the header declaration.
273-280: Good defensive logging for debugging telemetry issues.The null-check with appropriate log levels (warning for null, debug for success) helps diagnose cases where voice telemetry data might be missing without being overly noisy in production.
288-288: LGTM! Telemetry enrichment with voice_name.All event emission points (started, failed, completed) now consistently include
model_name(voice_name) for richer telemetry data.sdk/runanywhere-commons/src/features/voice_agent/voice_agent.cpp (4)
254-270: LGTM! STT model loading updated correctly.The signature is updated to match the new API, validation correctly checks
model_path(the required parameter), and all parameters are forwarded to the component.
289-305: LGTM! LLM model loading updated correctly.Consistent implementation pattern with STT loading.
324-340: LGTM! TTS voice loading updated correctly.Consistent implementation pattern with STT/LLM loading, using voice-specific terminology.
427-454: LGTM! Initialization path updated to use new config fields.The initialization correctly:
- Checks
model_path/voice_pathpresence (the required fields)- Passes all three config fields to the respective load functions
- Maintains the same fallback behavior when no path is specified
sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_manager.cpp (4)
64-76: Good fix: Thread-safe RNG seeding withstd::call_once.This addresses the previously flagged race condition where multiple threads could simultaneously call
srand(). The use ofstd::call_onceensures the seeding happens exactly once in a thread-safe manner.
44-48: LGTM! Batching configuration with sensible defaults.The batching constants (10 events, 5 second timeout) provide a reasonable balance between reducing HTTP overhead and ensuring timely telemetry delivery.
131-144: LGTM! Completion event detection for immediate flush.Identifying completion/failure events for immediate flush ensures that important terminal telemetry data (generation completed, transcription failed, etc.) is captured before the app might exit.
493-495: LGTM! model_name fallback to model_id.The pattern
payload.model_name = llm.model_name ? llm.model_name : llm.model_idensures telemetry always has a meaningful name even when the human-readable name isn't explicitly provided.sdk/runanywhere-commons/include/rac/features/voice_agent/rac_voice_agent.h (5)
113-120: LGTM! STT config extended with path and name fields.The structure now captures the full loading context: path for file location, id for telemetry, and name for human-readable display. Documentation is clear about when each field is used.
126-133: LGTM! LLM config follows same pattern as STT.Consistent structure across capability configurations.
139-146: LGTM! TTS config uses voice-appropriate naming.Consistent pattern with voice-specific terminology (voice_path, voice_id, voice_name).
169-173: LGTM! Default config properly initializes new fields.All new pointer fields are initialized to
RAC_NULL, ensuring safe defaults and allowing callers to selectively set only the fields they need.
362-398: LGTM! API declarations updated with comprehensive documentation.The load function declarations now clearly document:
model_path/voice_path: File path for loadingmodel_id/voice_id: Identifier for telemetrymodel_name/voice_name: Human-readable display nameThis is a breaking API change; all callers must be updated to the new signature.
sdk/runanywhere-commons/include/rac/features/llm/rac_llm_component.h (1)
113-121: LGTM! Well-documented API extension.The expanded signature with clear parameter semantics (path for loading, id for telemetry, name for human-readable display) is well-documented. The fallback defaults (model_id → model_path, model_name → model_id) are correctly implemented in the lifecycle layer and sensibly reduce burden on callers who don't need all parameters.
This is a breaking API change; all discovered callers in voice_agent.cpp have been correctly updated to the new signature.
sdk/runanywhere-swift/Sources/CRACommons/include/rac_voice_agent.h (2)
113-146: LGTM on config struct design.The enriched configuration structs cleanly separate concerns: path for loading, ID for telemetry, and human-readable name for display. The naming convention is consistent across STT, LLM, and TTS components.
169-173: LGTM on default configuration.Initializing all new string fields to
RAC_NULLis appropriate and allows callers to selectively provide values.sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp (3)
47-110: Excellent additions to JsonBuilder - security vulnerability resolved.The new helper methods (
add_string_always,add_string_or_null,add_int_or_null,add_bool_always,start_nested) provide the missing functionality needed to safely construct device registration JSON with proper escaping and nullable field support. These methods directly address the critical JSON injection vulnerability flagged in the previous review.
430-472: Production path now uses safe JsonBuilder - critical security issue resolved.The production environment path has been completely refactored to use JsonBuilder methods with proper escaping throughout, eliminating the JSON injection vulnerability flagged in the previous review. The nested
device_infostructure is built safely using the new helper methods.
430-480: Verify production backend schema expects nested device_info structure.The production path intentionally uses a nested
device_infoobject per its schema alignment comment, while development uses a flattened structure. However,device_idis not completely omitted—it's present in the payload as a fallback fordevice_fingerprint(line 474:info->device_fingerprint ? info->device_fingerprint : (info->device_id ? info->device_id : "")).The concern remains valid only if the production backend (schemas/device.py) expects
device_idat the top level rather than nested. Since this backend schema is external to this repository, the structure cannot be verified without access to backend code. Confirm with the backend team that the nested structure aligns with their FastAPI endpoint expectations.The
sdk_versionconcern is not an issue—it's guaranteed to have a value ("unknown" fallback in rac_device_manager.cpp line 141).
| // For development mode (Supabase), flatten the structure to match Supabase schema | ||
| // For production/staging, use nested device_info structure | ||
| if (env == RAC_ENV_DEVELOPMENT) { | ||
| // Flattened structure for Supabase (matches Kotlin SDK DevDeviceRegistrationRequest) | ||
| const rac_device_registration_info_t* info = &request->device_info; | ||
|
|
||
| // Required fields (matching Supabase schema) | ||
| if (info->device_id) { | ||
| json.add_string("device_id", info->device_id); | ||
| } | ||
| if (info->platform) { | ||
| json.add_string("platform", info->platform); | ||
| } | ||
| if (info->os_version) { | ||
| json.add_string("os_version", info->os_version); | ||
| } | ||
| if (info->device_model) { | ||
| json.add_string("device_model", info->device_model); | ||
| } | ||
| if (request->sdk_version) { | ||
| json.add_string("sdk_version", request->sdk_version); | ||
| } | ||
|
|
||
| // Optional fields | ||
| if (request->build_token) { | ||
| json.add_string("build_token", request->build_token); | ||
| } | ||
| if (info->total_memory > 0) { | ||
| json.add_int("total_memory", info->total_memory); | ||
| } | ||
| if (info->architecture) { | ||
| json.add_string("architecture", info->architecture); | ||
| } | ||
| if (info->chip_name) { | ||
| json.add_string("chip_name", info->chip_name); | ||
| } | ||
| if (info->form_factor) { | ||
| json.add_string("form_factor", info->form_factor); | ||
| } | ||
| // has_neural_engine is always set (rac_bool_t), so we can always include it | ||
| json.add_bool("has_neural_engine", info->has_neural_engine, RAC_TRUE); | ||
| // Add last_seen_at timestamp for UPSERT to update existing records | ||
| if (request->last_seen_at_ms > 0) { | ||
| json.add_timestamp("last_seen_at", request->last_seen_at_ms); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Supabase schema definitions and related files
fd -e sql -e json -e md | head -50 | xargs rg -l "device_registration|device_id" 2>/dev/null || echo "Searching with different pattern..."
# More targeted search for schema files
find . -type f \( -name "*.sql" -o -name "*schema*" -o -name "*migration*" \) | head -20Repository: RunanywhereAI/runanywhere-sdks
Length of output: 109
🏁 Script executed:
# Search for Kotlin SDK DevDeviceRegistrationRequest
rg -i "devdeviceregistration|device.*registration.*request" --type kotlin --type java | head -30Repository: RunanywhereAI/runanywhere-sdks
Length of output: 2898
🏁 Script executed:
# Also search in any documentation or comments referencing Supabase schema
rg -B 3 -A 10 "supabase.*schema|device_id.*platform" | head -50Repository: RunanywhereAI/runanywhere-sdks
Length of output: 3144
🏁 Script executed:
# Look for test files or examples that might show expected schema
fd -e json | xargs rg -l "device_id.*platform.*os_version" 2>/dev/null | head -10Repository: RunanywhereAI/runanywhere-sdks
Length of output: 56
🏁 Script executed:
# Read the Kotlin DevDeviceRegistrationRequest definition
cat sdk/runanywhere-kotlin/src/commonMain/kotlin/com/runanywhere/sdk/data/network/models/DevAnalyticsModels.kt | head -100Repository: RunanywhereAI/runanywhere-sdks
Length of output: 2405
🏁 Script executed:
# Find the C struct definition for device registration
rg -B 5 -A 20 "rac_device_registration_info_t|device_registration_request_t" --type c --type h | head -80Repository: RunanywhereAI/runanywhere-sdks
Length of output: 8929
🏁 Script executed:
# Search for Supabase migrations or schema files
find . -type f \( -name "*migration*" -o -name "*schema*" -o -path "*supabase*" \) | head -30Repository: RunanywhereAI/runanywhere-sdks
Length of output: 178
🏁 Script executed:
# Check the C header file for device registration structures
cat sdk/runanywhere-commons/include/rac/data/models/rac_device_info.h 2>/dev/null || find . -name "rac_device_info.h" -o -name "*device*registration*.h" | head -5Repository: RunanywhereAI/runanywhere-sdks
Length of output: 56
🏁 Script executed:
# Read complete Kotlin DevDeviceRegistrationRequest definition
cat sdk/runanywhere-kotlin/src/commonMain/kotlin/com/runanywhere/sdk/data/network/models/DevAnalyticsModels.kt | grep -A 50 "data class DevDeviceRegistrationRequest"Repository: RunanywhereAI/runanywhere-sdks
Length of output: 1235
🏁 Script executed:
# Read SupabaseClient.kt to see how DevDeviceRegistrationRequest is used
cat sdk/runanywhere-kotlin/src/commonMain/kotlin/com/runanywhere/sdk/foundation/supabase/SupabaseClient.kt | head -150Repository: RunanywhereAI/runanywhere-sdks
Length of output: 7042
🏁 Script executed:
# Search for any schema documentation or comments about Supabase device registration
rg -B 5 -A 10 "DevDeviceRegistrationRequest|device_registration.*table|supabase.*table" | head -100Repository: RunanywhereAI/runanywhere-sdks
Length of output: 11352
🏁 Script executed:
# Check if there are any other C SDK implementations to compare
rg -B 3 -A 15 "last_seen_at_ms|app_version" sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cppRepository: RunanywhereAI/runanywhere-sdks
Length of output: 1768
🏁 Script executed:
# Verify the complete C code section to see all fields being added
sed -n '384,429p' sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cppRepository: RunanywhereAI/runanywhere-sdks
Length of output: 1970
Critical schema incompatibilities between C and Kotlin implementations - fix before merging.
The flattened Supabase schema does NOT match the Kotlin SDK DevDeviceRegistrationRequest as claimed. Verification reveals:
- Field mismatch: C code sends
last_seen_atbut Kotlin model has no such field. Supabase table schema validation will reject this. - Missing field: Kotlin model includes
app_version(optional), but C code never sends it. - Type violation: C code always sends
has_neural_engineas boolean, but Kotlin model defines it asBoolean?(nullable). - Required fields too permissive: All five required Kotlin fields (
device_id,platform,os_version,device_model,sdk_version) have null checks in C code, allowing incomplete requests to be sent.
The JSON output will fail Supabase UPSERT operations due to unexpected last_seen_at column and missing expected fields. Align the C struct serialization with the actual DevDeviceRegistrationRequest schema before this can be merged.
🤖 Prompt for AI Agents
In sdk/runanywhere-commons/src/infrastructure/telemetry/telemetry_json.cpp
around lines 384-429, the flattened DevDeviceRegistrationRequest JSON is not
aligned with the Kotlin model: stop emitting last_seen_at (remove the
json.add_timestamp("last_seen_at", ...)), add app_version when
request->app_version is present, make has_neural_engine optional by only adding
it when a sentinel indicates presence (do not always add a boolean), and enforce
the five required fields (device_id, platform, os_version, device_model,
sdk_version) by validating all are non-null before serializing (fail/return an
error or skip sending if any are missing) so the generated JSON matches the
Kotlin schema and Supabase UPSERT expectations.
There was a problem hiding this comment.
Kotlin is getting an update soon, this just affects swift and c++ altogether - we're still going to bring the changes for kotlin and toher sdks soon
There was a problem hiding this comment.
@shubhammalhotra28, thanks for clarifying! A phased rollout makes sense when coordinating schema changes across multiple SDKs.
Since the Swift/C++ implementation is introducing new fields (last_seen_at, expanded device info) that Kotlin doesn't yet support, would you like me to create a follow-up issue to track the Kotlin schema alignment? This would help ensure the Kotlin SDK gets updated with:
- The new
last_seen_atfield for UPSERT behavior - The expanded device info fields (architecture, chip_name, form_factor, neural engine details, etc.)
- Removal of deprecated
app_versionif no longer needed
Let me know if you'd like me to open an issue with the schema changes that need to be ported!
🧠 Learnings used
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/src/**/*.kt : NEVER make assumptions when implementing KMP code. ALWAYS refer to the iOS implementation as the definitive source of truth. Check the iOS implementation, copy the logic exactly, and adapt only for Kotlin syntax.
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/src/**/*.kt : Always use data classes and structured types (enums, sealed classes) instead of strings for models and configuration.
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/src/**/*.kt : Use coroutines and suspend functions instead of async/await patterns for asynchronous operations.
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/*.gradle.kts : Kotlin version must be 2.1.21 or later for the Kotlin Multiplatform SDK.
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/src/androidMain/**/*.kt : Use platform-specific optimizations in their respective source directories: `androidMain/` for Android optimizations, `jvmMain/` for JVM optimizations.
Learnt from: CR
Repo: RunanywhereAI/runanywhere-sdks PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T22:49:40.358Z
Learning: Applies to sdk/runanywhere-kotlin/**/*.gradle.kts : JVM target must be 17 or later for Kotlin Multiplatform SDK.
| /** Model path - file path used for loading (can be NULL to use already-loaded model) */ | ||
| const char* model_path; |
There was a problem hiding this comment.
Documentation inconsistency: config says path can be NULL, but implementation rejects NULL.
The config struct comments state that model_path/voice_path "can be NULL to use already-loaded model", but the implementation in voice_agent.cpp explicitly rejects NULL paths:
if (!handle || !model_path) {
return RAC_ERROR_INVALID_ARGUMENT;
}Either update the documentation to indicate paths are required, or update the implementation to handle NULL paths gracefully when a model is already loaded.
🔎 Suggested documentation fix if paths are required
typedef struct rac_voice_agent_stt_config {
- /** Model path - file path used for loading (can be NULL to use already-loaded model) */
+ /** Model path - file path used for loading (required) */
const char* model_path;Apply similar changes to rac_voice_agent_llm_config and rac_voice_agent_tts_config.
Also applies to: 127-128, 140-141
🤖 Prompt for AI Agents
In sdk/runanywhere-swift/Sources/CRACommons/include/rac_voice_agent.h around
lines 114-115 (and similarly for 127-128 and 140-141), the comments say
model_path/voice_path "can be NULL to use already-loaded model" but the C++
implementation rejects NULL; update the header comments to state these paths are
required (non-NULL) and cannot be NULL, and apply the same documentation change
to rac_voice_agent_llm_config and rac_voice_agent_tts_config so the docs match
the implementation.
|
|
||
| // Check if path contains query parameters | ||
| if path.contains("?") { | ||
| // Split path and query parameters | ||
| let components = path.split(separator: "?", maxSplits: 1) | ||
| let pathPart = String(components[0]) | ||
| let queryPart = String(components[1]) | ||
|
|
||
| // Build URL with query parameters using URLComponents | ||
| guard var urlComponents = URLComponents(url: base, resolvingAgainstBaseURL: true) else { | ||
| return base.appendingPathComponent(path) | ||
| } | ||
| let existingPath = urlComponents.path | ||
| urlComponents.path = existingPath + pathPart | ||
| urlComponents.query = queryPart | ||
|
|
||
| return urlComponents.url ?? base.appendingPathComponent(path) | ||
| } |
There was a problem hiding this comment.
Improve error handling for URL construction failures.
The fallback at lines 202 and 208 silently returns base.appendingPathComponent(path) when URLComponents creation or URL building fails. This will produce an incorrect URL when the path contains query parameters (the ? and query string will be percent-encoded as part of the path component).
Consider:
- Logging a warning when URLComponents creation fails to help diagnose issues
- Returning the original simple concatenation only for paths without query parameters
- Or throwing an error if URL construction fails, as this indicates a configuration or input problem
🔎 Suggested improvement
if path.contains("?") {
let components = path.split(separator: "?", maxSplits: 1)
let pathPart = String(components[0])
let queryPart = String(components[1])
guard var urlComponents = URLComponents(url: base, resolvingAgainstBaseURL: true) else {
+ logger.warning("Failed to create URLComponents for base URL: \(base)")
return base.appendingPathComponent(path)
}
let existingPath = urlComponents.path
urlComponents.path = existingPath + pathPart
urlComponents.query = queryPart
- return urlComponents.url ?? base.appendingPathComponent(path)
+ guard let finalURL = urlComponents.url else {
+ logger.warning("Failed to build URL with query parameters: \(path)")
+ return base.appendingPathComponent(path)
+ }
+ return finalURL
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
minor, can be skipped for now
There was a problem hiding this comment.
@shubhammalhotra28, understood! I've noted this as a minor issue to revisit later if needed.
Description
Script making sure that we can do local development.
Additionally addressing the network layer changes needed to make the SDK invoke the API's
Type of Change
Testing
Labels
Please add the appropriate label(s):
iOS SDK- Changes to iOS/Swift SDKAndroid SDK- Changes to Android/Kotlin SDKiOS Sample- Changes to iOS example appAndroid Sample- Changes to Android example appChecklist
Screenshots - Attach all the relevant UI changes screenshots for iOS/Android and MacOS/Tablet/large screen sizes
Important
Enhances local development workflow, updates device registration logic, and improves telemetry handling in the SDK.
--localflag inbuild_and_run.shfor local development builds, copying frameworks toBinaries/.CMakeLists.txtto support pre-built core libraries withUSE_PREBUILT_COREoption.HTTPService.swiftfor device registration using Supabase.on_conflict=device_idquery parameter andPrefer: resolution=merge-duplicatesheader.telemetry_manager.cpp.rac_lifecycle.handrac_llm_component.h.Package.swiftby settingtestLocal = falsefor production builds.download-core-prebuilt.shscript for downloading pre-built core libraries.This description was created by
for 2cabdaf. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
API Updates
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Summary
--localflag forbuild_and_run.shthat copies runanywhere-commons frameworks to Swift SDK'sBinaries/directoryon_conflict=device_idquery parameter andPrefer: resolution=merge-duplicatesheader to handle existing devices gracefullyImportant Files Changed
testLocal = falseto ensure production builds use remote GitHub releases instead of local frameworkson_conflictquery parameter andresolution=merge-duplicatesheader--localflag for local development builds with framework copying and Package.swift configuration managementUSE_PREBUILT_COREoption to support linking against pre-built core libraries instead of compiling from sourceConfidence score: 4/5
testLocal = trueissue that was previously identified has been resolved by setting it tofalsein Package.swift, ensuring production safetyContext used:
dashboard- CLAUDE.md (source)