Skip to content

Sync desktop/ from omi-desktop (474e1e1f)#4967

Closed
m13v wants to merge 1 commit intomainfrom
sync/desktop-20260222-173356
Closed

Sync desktop/ from omi-desktop (474e1e1f)#4967
m13v wants to merge 1 commit intomainfrom
sync/desktop-20260222-173356

Conversation

@m13v
Copy link
Copy Markdown
Contributor

@m13v m13v commented Feb 22, 2026

Automated sync from omi-desktop at 474e1e1f.

This PR was created by the sync pipeline.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces significant improvements to the desktop application, including a unified AI chat bridge (ACP), local-first task management with background synchronization, and enhanced drag-and-drop capabilities. It also addresses several stability issues, such as the cursor jump bug in click-through views and improved OOM detection for the AI subprocess. However, there are a few high-severity concerns regarding security and performance that should be addressed: a potential SQL injection vulnerability in the cloud agent's sync logic, the use of blocking thread sleep in the proactive assistants plugin, and the aggressive use of killall on a system daemon.

I am having trouble creating individual review comments. Click here to see my feedback.

desktop/agent-cloud/agent.mjs (824-832)

security-critical critical

The dynamic schema migration logic is vulnerable to SQL injection. The table and col variables are interpolated directly into SQL strings used in db.prepare(). Since identifiers like table and column names cannot be parameterized in SQLite, they must be strictly validated against an allowlist or a strict regex pattern (e.g., /^[a-zA-Z0-9_]+$/) before being used in a query. An attacker could potentially manipulate these values to execute arbitrary SQL or corrupt the database schema.

desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift (272)

high

Using Thread.sleep(forTimeInterval:) is discouraged in modern Swift, especially within the context of structured concurrency. It blocks the current thread, which, if running on the cooperative thread pool, can degrade application performance and potentially lead to deadlocks. Use await Task.sleep(nanoseconds:) instead to yield the thread while waiting.

desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift (262-268)

high

Restarting the system-wide usernoted daemon via killall is an extremely intrusive operation. While it may force the system to pick up fresh notification registrations, it affects all running applications and can lead to a poor user experience (e.g., missing notifications from other apps, UI glitches). This should be a last resort, and ideally, a more standard API for refreshing notification settings should be used if available.

desktop/Desktop/Sources/AgentSyncService.swift (234)

security-high high

The PRAGMA table_info query uses string interpolation for the table name. While spec.name is likely derived from internal configuration, it is best practice to avoid interpolation in SQL strings to prevent potential injection or syntax errors if a table name contains special characters (like single quotes).

@m13v m13v closed this Feb 23, 2026
@m13v m13v deleted the sync/desktop-20260222-173356 branch February 23, 2026 03:31
@github-actions
Copy link
Copy Markdown
Contributor

Hey @m13v 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our roadmap and vision

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community! 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant