Conversation
There was a problem hiding this comment.
Code Review
The pull request introduces several significant improvements, including the unification of AI chat bridges into a single ACPBridge supporting both Omi and user Claude accounts, local-first task management with background synchronization, and enhanced resource monitoring to prevent system degradation under memory pressure. However, there are critical security concerns regarding SQL injection in the cloud agent and synchronization service, as well as performance issues related to blocking the main thread during initialization. These issues should be addressed to ensure the application's stability and security.
I am having trouble creating individual review comments. Click here to see my feedback.
desktop/agent-cloud/agent.mjs (830)
Critical Security Risk: SQL Injection. The code directly interpolates the table name and column names (col) into SQL queries. Since these values are derived from the request body, a malicious client could execute arbitrary SQL commands. Identifiers like table and column names cannot be parameterized in SQLite; they must be strictly validated against a whitelist of allowed names or properly escaped using a utility function to prevent injection attacks.
desktop/Desktop/Sources/AgentSyncService.swift (234)
High Security Risk: SQL Injection. The table name is interpolated directly into the PRAGMA table_info query. While the current specs are hardcoded within the service, this pattern is inherently unsafe and violates best practices for database interactions. It is recommended to validate the table name against a known set of allowed identifiers before executing the query.
desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift (272)
High Performance Issue: Blocking Main Thread. Thread.sleep(forTimeInterval: 1.5) blocks the current thread. Since this initialization sequence is typically triggered from the main thread (e.g., during app launch or onboarding), it will cause the entire application UI to hang and potentially trigger a watchdog termination. This delay should be handled asynchronously using Task.sleep within an asynchronous context to keep the UI responsive.
desktop/Desktop/Sources/Stores/TasksStore.swift (1200-1202)
High Correctness/Thread Safety Issue. Modifying the @Published property incompleteTasks from a background Task without ensuring it runs on the Main Actor can lead to race conditions or runtime crashes in SwiftUI. Additionally, replacing a task by index after an asynchronous network call is unreliable as the task's position or presence in the array may have changed. It is safer to perform the update on the main actor and handle cases where the task might have moved to the completed list.
Automated sync from omi-desktop at 474e1e1f.
This PR was created by the sync pipeline.