Omi windows#9333
Open
krVatsal wants to merge 66 commits into
Open
Conversation
…s and knowledge base
…ting bar position
… actions, rewind-thumb-selected
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 189 files
Confidence score: 1/5
mcp/backend/auth/dependencies.pyandmcp/backend/api/routes/tools.pytogether create a critical auth gap: JWTs are accepted without signature/claim validation and/tools/executecan run tools without aCurrentUser, so forged tokens or unauthenticated callers could execute privileged actions—restore full JWT verification (signature/exp/iss/aud) and require auth on tool execution before merging.mcp/backend/mcp_servers/google_drive_server.pyexposes arbitrary server-local file read/write paths through Drive tools, which can leak secrets (for example.env/credentials) or overwrite local files when downloading—restrict operations to an allowlisted workspace and validate/canonicalize paths before merging.mcp/backend/auth/mcp_token_bridge.pyis implemented againstlangchain-mcp-adapters0.1.0APIs (client.session()), butmcp/backend/requirements.txtpins0.0.10; this is a likely runtime break on first use—align the dependency version or refactor to the pinned API before merging.- The Windows Rust changes in
omi-windows/crates/omi-db/src/clipboard.rs,omi-windows/crates/omi-app/src/app_tracker.rs, andomi-windows/crates/omi-capture/src/video_chunk.rshave concrete crash/corruption risks (UTF-8 byte slicing panics and ffmpeg frame-size/buffer mismatch on odd dimensions), so user-facing instability is likely—switch to char-safe truncation and make encoded frame buffers match declared padded dimensions before merging.
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcp/backend/auth/mcp_token_bridge.py">
<violation number="1" location="mcp/backend/auth/mcp_token_bridge.py:1">
P0: This new module is written against `langchain-mcp-adapters 0.1.0` (`client.session()` API), but `mcp/backend/requirements.txt` pins `langchain-mcp-adapters==0.0.10`. The `client.session()` method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise `AttributeError` at runtime on a clean install. Either bump the pinned dependency to `>=0.1.0` or rewrite the client usage to match the 0.0.10 API.</violation>
</file>
<file name="mcp/backend/auth/dependencies.py">
<violation number="1" location="mcp/backend/auth/dependencies.py:36">
P0: The Bearer JWT is decoded with signature verification explicitly disabled (`verify_signature=False`) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary `sub` claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing `backend/routers/auth.py` already demonstrates proper RS256 verification — this dependency should follow the same pattern.</violation>
</file>
<file name="omi-windows/crates/omi-db/src/clipboard.rs">
<violation number="1" location="omi-windows/crates/omi-db/src/clipboard.rs:90">
P0: Slicing `e.content` by byte index `[..120]` can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from `.chars().take(120)` and appending the ellipsis only when the original `.chars().count()` exceeds 120.</violation>
</file>
<file name="mcp/backend/api/routes/tools.py">
<violation number="1" location="mcp/backend/api/routes/tools.py:62">
P1: The tools router calls `agent_manager.get_tools_info()` and `agent_manager.tools`, but `AgentManager` does not define either. This means `/tools`, `/tools/execute`, and `/tools/{tool_name}` will all crash with `AttributeError` at runtime. Add the missing delegation on `AgentManager` or point `tools.py` at the orchestrator's own tools interface.</violation>
<violation number="2" location="mcp/backend/api/routes/tools.py:128">
P0: The `/tools/execute` endpoint allows arbitrary tool execution without authentication. It directly calls `tool.ainvoke(request.arguments)` without a `CurrentUser` dependency, while the same file correctly requires auth on `/tools/status`. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add `user: CurrentUser` to the endpoint signature to align with the rest of the API.</violation>
</file>
<file name="mcp/backend/mcp_servers/google_drive_server.py">
<violation number="1" location="mcp/backend/mcp_servers/google_drive_server.py:205">
P1: These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. `upload_drive_file` can read any file the backend can access (including `.env` or credentials) and upload it to Drive. `download_drive_file` can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with `Path.resolve()` so each operation is constrained to an allowed base directory.</violation>
</file>
<file name="mcp/backend/knowledge_engine/club/chunker.py">
<violation number="1" location="mcp/backend/knowledge_engine/club/chunker.py:112">
P1: The chunking loop in `_split_text` trims `chunk` to a sentence boundary but advances `start` using the original fixed-width `end`. When a boundary is found early (trimmed by more than `chunk_overlap`), the text between the trimmed chunk end and `end - chunk_overlap` is permanently skipped. This causes irreversible data loss in embeddings and retrieval.
Recommend basing the next `start` on the actual emitted chunk length: advance from the trimmed chunk's end minus overlap, or at minimum ensure `start` does not exceed the trimmed chunk's end.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/app_tracker.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/app_tracker.rs:174">
P1: The fallback title-capping logic in `extract_app_name` slices by byte index (`&title[..40]`), which panics if byte 40 is not a UTF-8 character boundary. Window titles with multi-byte characters (CJK, emoji, Cyrillic) are common, and since this runs in a 10-second polling loop, one bad title can either crash the tracker task or cause repeated panics. Use character-aware truncation (e.g., `char_indices`) to cap safely.</violation>
</file>
<file name="omi-windows/crates/omi-capture/src/video_chunk.rs">
<violation number="1" location="omi-windows/crates/omi-capture/src/video_chunk.rs:112">
P1: The encoder tells ffmpeg to expect even-dimension frames (`ew`/`eh`) via `-video_size`, but writes the original unpadded `rgba.as_raw()` buffer. When width or height is odd, ffmpeg reads `ew * eh * 4` bytes per frame while only `width * height * 4` bytes are sent, misaligning every subsequent frame and corrupting or hanging the video stream. The frame data needs to match the declared dimensions — either resize/pad the image to `ew x eh` before writing, or pass the original dimensions to ffmpeg and handle padding through a filter instead.</violation>
</file>
<file name="mcp/backend/api/routes/health.py">
<violation number="1" location="mcp/backend/api/routes/health.py:44">
P1: The `/status` endpoint references `agent_manager.llm.model_name`, but `AgentManager` has no `llm` attribute. Calling this endpoint will raise an `AttributeError` instead of returning status. Consider removing the `llm_model` field from the response or exposing the model info through a safe property on `AgentManager` that the endpoint can call.</violation>
</file>
<file name="mcp/backend/hitl/confirmation.py">
<violation number="1" location="mcp/backend/hitl/confirmation.py:231">
P1: Calendar date normalization hard-codes the IST timezone (`+05:30`) in `_parse_natural_datetime`, `_normalize_datetime`, and the LLM system prompt inside `_draft_calendar_fields`. This silently schedules events at incorrect local times for any user outside IST. Since the orchestration layer is user-oriented, a fixed timezone is a functional correctness risk. Consider deriving the user's timezone from their profile or settings, or at minimum defaulting to UTC (`Z`) so downstream calendar APIs interpret the time consistently.</violation>
</file>
<file name="mcp/backend/requirements.txt">
<violation number="1" location="mcp/backend/requirements.txt:98">
P1: The new `mcp/backend/requirements.txt` is missing runtime dependencies that the backend code directly imports. `supabase` is imported in `auth/supabase_client.py` and three other files, `jwt` is imported in `auth/dependencies.py`, and `EmailStr` in `api/routes/auth.py` requires the optional `email-validator` package in pydantic v2. Without these entries, a fresh install will fail on first import. Adding `supabase`, `PyJWT`, and `email-validator` to this requirements file would fix the issue.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/context_watcher.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/context_watcher.rs:129">
P1: Byte-index slicing `&ocr_text[..ocr_text.len().min(1200)]` can panic at runtime when OCR text contains multi-byte UTF-8 characters (e.g. CJK, emoji). `str.len()` counts bytes, not characters, so the 1200 byte boundary may fall mid-character. Since this runs in a background watcher loop, a panic here would crash the task on common multilingual content. The codebase already handles this correctly in `capture.rs` using `char_indices().nth()` to truncate at a safe boundary — the same pattern should be used here.</violation>
<violation number="2" location="omi-windows/crates/omi-app/src/context_watcher.rs:243">
P2: The loop computes `interval_secs` from `cfg.context_watcher_interval_secs` but never applies it to the `tokio::time::Interval` timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the `Interval` when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.</violation>
</file>
<file name="mcp/backend/api/routes/auth.py">
<violation number="1" location="mcp/backend/api/routes/auth.py:149">
P1: The OAuth callback handlers interpolate `error`, `google_email`, `github_username`, and exception messages directly into redirect query strings without URL-encoding. If any of these values contain `&`, `=`, `?`, `#`, or other reserved characters, the query string is malformed and can even inject unintended parameters. Encode dynamic values with `urllib.parse.quote` (or `urllib.parse.urlencode`) before constructing the redirect URL.</violation>
</file>
<file name="omi-windows/crates/omi-capture/src/lib.rs">
<violation number="1" location="omi-windows/crates/omi-capture/src/lib.rs:74">
P1: When `capture_tick("all")` captures multiple monitors, all frames are pushed into the same `VideoChunkEncoder` stream. `VideoChunkEncoder::add_frame()` only checks for aspect-ratio changes when splitting chunks, not exact dimension equality. Two monitors with the same aspect ratio but different resolutions (e.g. 1920×1080 and 3840×2160) will therefore be written into a single ffmpeg rawvideo pipe sized for the first frame, corrupting the encoded output. Additionally, interleaving unrelated monitor frames into one video stream produces fragmented, jump-cut footage rather than a usable recording per monitor. Consider either maintaining a separate encoder per monitor or skipping video encoding when `frames.len() > 1`.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/clipboard_watcher.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/clipboard_watcher.rs:45">
P1: The clipboard preview truncation uses byte-indexed slicing (`&text[..60]`) which will panic at runtime if byte 60 falls inside a multi-byte UTF-8 character. Clipboard content commonly includes emoji, CJK text, and other non-ASCII content, making this a realistic crash path in a long-running watcher loop. Consider using character-aware truncation instead, such as `text.chars().take(60).collect::<String>()` to build the preview safely.</violation>
</file>
<file name="mcp/backend/core/client.py">
<violation number="1" location="mcp/backend/core/client.py:45">
P1: This test script calls `agent_manager.get_latest_gmail_messages()`, `get_upcoming_events()`, and `rag_retrieve()`, but `AgentManager` no longer exposes these methods — it only routes through the SmartOrchestrator. Running the script will throw `AttributeError`. Consider removing or updating the direct MCP test path to match the current `AgentManager` API.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/chat.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/chat.rs:167">
P1: Byte-based string slicing (`[..body_text.len().min(N)]`) can panic when the truncation point lands inside a multi-byte UTF-8 character. Since the text comes from an external LLM API, non-ASCII content is common. Switch to character-based truncation so the app doesn't crash on international or Unicode responses.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/mcp_bridge.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/mcp_bridge.rs:309">
P1: `confirm_mcp` posts to `/api/message/confirm` without the conditional `Authorization` header that `query_mcp` adds for the same MCP backend. If the backend requires auth for the message endpoint, confirmations will fail even though the initial query succeeds.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/config.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/config.rs:416">
P1: Persisting Firebase tokens, Google OAuth tokens, and API keys as plaintext JSON to `%APPDATA%/omi/config.json` is a credential-leakage risk on Windows — any process running as the same user can read this file. The `omi-app` crate already includes `aes-gcm` in `Cargo.toml` and implements a `dpapi_decrypt` helper using Windows DPAPI in `google_calendar.rs`; consider using DPAPI (or `CryptProtectData`) to encrypt the config file before writing it, or split sensitive credentials into an encrypted credential store instead of raw JSON.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/settings.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/settings.rs:15">
P1: The Wearable BLE section presents fake scan results and mock connection state as real functionality. `scan_ble_devices` hardcodes two fake devices with synthetic addresses, and the Connect button only updates local UI signals rather than establishing an actual BLE connection. This makes the settings UI misleading since a user can appear "Connected" without any hardware interaction. Consider either wiring this to a real BLE library (e.g., `btleplug` on Windows) or clearly marking the section as a placeholder with a TODO so users are not misled.</violation>
</file>
<file name="mcp/backend/api/main.py">
<violation number="1" location="mcp/backend/api/main.py:90">
P1: The `api/routes/tools.py` router is never imported or included in `main.py`, so every tools endpoint (`/api/tools`, `/api/tools/status`, `/api/tools/execute`, etc.) is unreachable even though the route implementations, models, and agent integration are all present. Add the missing import and `app.include_router` call.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/recording.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/recording.rs:252">
P1: After a conversation completes, a brand-new non-running `AgentRuntime` is created inline and passed to `ProactiveEngine::on_conversation_ended()`. Inside the proactive engine, the memory-insight branch calls `runtime.query()`, which requires a live Node.js process. The app intentionally no longer spawns Node.js (`app.rs`: "We no longer spawn Node.js! All execution paths route through `agent_runtime.query_native()`"), so this call will always silently fail because its result is ignored with `let _ =`. When `proactive_agent_enabled` is true, users will never see memory-based proactive insights and the failure will be invisible. The proactive engine should either use the native LLM path (`query_native`) or receive a properly initialized runtime.</violation>
</file>
<file name="omi-windows/crates/omi-audio/src/mixer.rs">
<violation number="1" location="omi-windows/crates/omi-audio/src/mixer.rs:24">
P1: The mixer task can leak forever after its input broadcast channels close. Both receiver branches use `Ok(chunk) = recv()` patterns, which silently ignore `Err(Closed)` results. When both channels close, only the `flush_interval.tick()` branch remains active, `out_len` stays zero, and the loop `continue`s forever rather than stopping. This leaks the mixer task after capture shutdown. Consider matching the full `Result` on each receiver and tracking channel closures so the loop exits when no sources remain.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/hotkey.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/hotkey.rs:115">
P1: The `Ctrl+Shift+R` hotkey is registered as `toggle_rec` and documented as “start / stop recording”, but the event listener maps key press to `StartRecord` and key release to `StopRecord`. This turns the shortcut into a momentary trigger (recording stops as soon as the keys are released), which is the same behavior as the separate PTT hotkey (`Ctrl+Space`). It should probably behave like the other toggles (`ToggleBar`, `ToggleVoiceChat`) by reacting only to the `Pressed` event and letting the app layer toggle the recording state.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/components/floating_bar.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/components/floating_bar.rs:43">
P1: The voice-history sync effect re-appends every item from `voice_history` onto the existing `chat_history` on each update, which duplicates all previously imported turns every time a new voice turn arrives. Consider either rebuilding `chat_history` from `voice_history` on each change or tracking a last-synced index so only new items are appended.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/app.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/app.rs:764">
P1: Clipboard preview truncation slices a Rust `String` by byte offset (`&c.content[..120]`), which panics at runtime if byte 120 falls inside a multibyte UTF-8 character. Because `c.content` comes from the OS clipboard and can contain emoji, CJK text, or other multibyte codepoints, this guard is not sufficient. The panic occurs inside the async agent-query context builder and silently aborts the query.</violation>
</file>
<file name="omi-windows/crates/omi-transcription/src/streaming.rs">
<violation number="1" location="omi-windows/crates/omi-transcription/src/streaming.rs:200">
P1: The transcription service can panic at runtime when Deepgram returns non-ASCII text. The logging statements on lines 200, 206, and 254 slice `String` using byte indices (`&text[..text.len().min(500)]` and `&text[..text.len().min(400)]`), but in Rust `str` indexing panics if the byte offset is not a UTF-8 character boundary. Because Deepgram transcripts can contain Unicode (e.g., multi-byte CJK, emoji, or accented characters), any logged message whose 400th or 500th byte falls inside a multi-byte code point will cause a thread panic and abort the transcription stream.
Consider using character-aware truncation instead, for example a small helper that does `text.chars().take(limit).collect::<String>()` or similar, so logging is safe regardless of the transcript language.</violation>
<violation number="2" location="omi-windows/crates/omi-transcription/src/streaming.rs:283">
P1: After `tokio::select!` returns when either the send or receive task finishes, the remaining task's `JoinHandle` is dropped but the task continues running in the background. This泄漏s resources and can emit stale transcript segments after the stream function has returned. You should explicitly abort the surviving task and optionally await it so it yields and shuts down cleanly. A minimal pattern is: store whichever handle finishes, then call `other_handle.abort()` and `let _ = other_handle.await;` after the `select!`.</violation>
</file>
<file name="mcp/backend/tests/test_rag.py">
<violation number="1" location="mcp/backend/tests/test_rag.py:6">
P1: This file is structured as a standalone script but named `test_rag.py` under the `tests/` directory. Because pytest collects `test_*.py` files by importing them, running `pytest` will immediately execute the module-level code — loading embedding models, creating temporary directories, making network requests to Supabase, and more — during *collection*, well before any test runs. There are no `test_*` functions or pytest-compatible assertions anywhere in the file, so pytest won't find anything to run after the import side effects finish. The sibling `test_mcp_servers.py` shows the correct pattern: wrap script logic in a `main()` function behind an `if __name__ == '__main__':` guard so it only runs when explicitly invoked. Consider either (a) adding a `__name__` guard and renaming the file without the `test_` prefix if it's meant to be run directly, or (b) refactoring into proper pytest test functions if it should participate in automated test runs.</violation>
</file>
<file name="omi-windows/crates/omi-db/src/unified_search.rs">
<violation number="1" location="omi-windows/crates/omi-db/src/unified_search.rs:160">
P1: The snippet-truncation logic in `search_memories_text` (and mirrored in three other search methods) uses byte-index slicing (`&content[..150]`) to truncate snippets after 150 bytes. In Rust this panics at runtime when index 150 falls inside a multi-byte UTF-8 character. Because these fields contain user-generated text, Unicode characters are expected and the search call can crash.
Use `char_indices()` to find the byte boundary at the 150th character instead, and apply the same fix to the other three matching truncation sites (`search_screenshots_unified`, `search_clipboard_unified`, `search_conversations_unified`).</violation>
</file>
<file name="backend/utils/app_integrations.py">
<violation number="1" location="backend/utils/app_integrations.py:419">
P1: The critic (`validate_notification`) only vets `notification_text` and `draft.reasoning`, but the new code stages `draft.proposed_action` to the database immediately after approval without validating the action's `description`, `action_type`, or `due_at`. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in `validate_notification` or adding a separate validation step before `staged_tasks_db.create_staged_task`.</violation>
<violation number="2" location="backend/utils/app_integrations.py:429">
P1: The `due_at` value extracted from `draft.proposed_action` is read but never passed to `create_staged_task`. Since the staged-task DB function already accepts `due_at` via kwargs, this omission means calendar/event times and task deadlines generated by the LLM are silently discarded. The frontend then has no structured datetime to schedule or promote, making the proposed-action feature ineffective for time-sensitive items. Pass `due_at=due_at` into the `create_staged_task` call.</violation>
</file>
<file name="mcp/backend/core/agent.py">
<violation number="1" location="mcp/backend/core/agent.py:249">
P1: The `resume` method accepts `user_id` as a parameter but never uses it, instead passing `thread_id` as the orchestrator's `user_id`. This makes the API contract misleading and could break token lookups or resume state matching if the orchestrator expects the actual user UUID. Consider either using `user_id` (if the orchestrator should receive it) or removing the unused parameter from the signature.</violation>
</file>
<file name="omi-windows/crates/omi-ble/src/protocol.rs">
<violation number="1" location="omi-windows/crates/omi-ble/src/protocol.rs:181">
P1: The `DeviceType::from_name` implementation has unreachable classification branches that will select the wrong audio codec.
Any BLE device name starting with "friend" — including "Friend Pendant" — is captured by the first `starts_with("friend")` branch and classified as `DeviceType::Omi`, so `DeviceType::FriendPendant` is never returned for those names. This causes Friend Pendant hardware to use the Omi `Opus` default instead of its intended `LC3` codec.
Likewise, "limitless" is mapped to `FriendPendant`, making `DeviceType::Limitless` completely unreachable from name detection even though `Limitless` has its own distinct `OpusFS320` default. This means Limitless devices will use `LC3` with the wrong frame size, which will break audio frame assembly.
Reordering the checks so more specific patterns are matched first — or narrowing the broad `starts_with("friend")` condition — would fix the misclassification.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/llm.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/llm.rs:258">
P1: When `complete_streaming` falls back to the next provider after a streaming failure, it may already have emitted partial tokens via `on_token` from the failed provider. Downstream callers like `agent_runtime.rs` push each token into the live UI, so a subsequent provider's output gets concatenated with the partial output — resulting in garbled text visible to the user. Consider tracking whether any tokens were emitted and avoiding downstream fallback when the stream already started, or send a reset/clear signal before retrying.</violation>
</file>
<file name="mcp/backend/orchestration/red_flag_node.py">
<violation number="1" location="mcp/backend/orchestration/red_flag_node.py:12">
P1: `red_flag_node` is defined as a module-level function but takes `self` and accesses instance attributes (`self.llm`, `self._get_fallback_response()`). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for `AgentState` from different paths. Consider refactoring this node into a plain function that accepts only `state`, or wrapping it in a class whose instance method can be bound and passed to `add_node`.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/rewind.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/rewind.rs:158">
P1: The rewind auto-refresh effect captures the `db` value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because `use_effect` reruns when `db` changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels `spawn` tasks on component drop, not on effect rerun. Read the `db` Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with `use_signal` holding the `Task` and calling `.cancel()`).</violation>
</file>
<file name="omi-windows/crates/omi-ble/src/connection.rs">
<violation number="1" location="omi-windows/crates/omi-ble/src/connection.rs:161">
P2: When `audio_tx.send(frame)` fails because the receiver was dropped, the `break` exits only the inner `for` loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., `'stream: loop { ... }`) and `break 'stream;` so the receiver-drop condition terminates the stream as intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,178 @@ | |||
| """ | |||
There was a problem hiding this comment.
P0: This new module is written against langchain-mcp-adapters 0.1.0 (client.session() API), but mcp/backend/requirements.txt pins langchain-mcp-adapters==0.0.10. The client.session() method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise AttributeError at runtime on a clean install. Either bump the pinned dependency to >=0.1.0 or rewrite the client usage to match the 0.0.10 API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/auth/mcp_token_bridge.py, line 1:
<comment>This new module is written against `langchain-mcp-adapters 0.1.0` (`client.session()` API), but `mcp/backend/requirements.txt` pins `langchain-mcp-adapters==0.0.10`. The `client.session()` method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise `AttributeError` at runtime on a clean install. Either bump the pinned dependency to `>=0.1.0` or rewrite the client usage to match the 0.0.10 API.</comment>
<file context>
@@ -0,0 +1,178 @@
+"""
+backend/auth/mcp_token_bridge.py - FIXED for langchain-mcp-adapters 0.1.0
+
</file context>
|
|
||
| try: | ||
| # Decode Firebase JWT without signature verification for local backend use | ||
| payload = jwt.decode(token, options={"verify_signature": False}) |
There was a problem hiding this comment.
P0: The Bearer JWT is decoded with signature verification explicitly disabled (verify_signature=False) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary sub claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing backend/routers/auth.py already demonstrates proper RS256 verification — this dependency should follow the same pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/auth/dependencies.py, line 36:
<comment>The Bearer JWT is decoded with signature verification explicitly disabled (`verify_signature=False`) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary `sub` claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing `backend/routers/auth.py` already demonstrates proper RS256 verification — this dependency should follow the same pattern.</comment>
<file context>
@@ -0,0 +1,68 @@
+
+ try:
+ # Decode Firebase JWT without signature verification for local backend use
+ payload = jwt.decode(token, options={"verify_signature": False})
+
+ firebase_uid = payload.get("sub")
</file context>
Comment on lines
+90
to
+92
| let preview = if e.content.len() > 120 { | ||
| format!("{}…", &e.content[..120]) | ||
| } else { |
There was a problem hiding this comment.
P0: Slicing e.content by byte index [..120] can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from .chars().take(120) and appending the ellipsis only when the original .chars().count() exceeds 120.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-db/src/clipboard.rs, line 90:
<comment>Slicing `e.content` by byte index `[..120]` can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from `.chars().take(120)` and appending the ellipsis only when the original `.chars().count()` exceeds 120.</comment>
<file context>
@@ -0,0 +1,114 @@
+ .iter()
+ .map(|e| {
+ let app = e.source_app.as_deref().unwrap_or("unknown");
+ let preview = if e.content.len() > 120 {
+ format!("{}…", &e.content[..120])
+ } else {
</file context>
Suggested change
| let preview = if e.content.len() > 120 { | |
| format!("{}…", &e.content[..120]) | |
| } else { | |
| let preview = if e.content.chars().count() > 120 { | |
| format!("{}…", e.content.chars().take(120).collect::<String>()) | |
| } else { | |
| e.content.clone() | |
| }; |
|
|
||
|
|
||
| @router.post("/tools/execute") | ||
| async def execute_tool(request: ToolExecutionRequest): |
There was a problem hiding this comment.
P0: The /tools/execute endpoint allows arbitrary tool execution without authentication. It directly calls tool.ainvoke(request.arguments) without a CurrentUser dependency, while the same file correctly requires auth on /tools/status. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add user: CurrentUser to the endpoint signature to align with the rest of the API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/api/routes/tools.py, line 128:
<comment>The `/tools/execute` endpoint allows arbitrary tool execution without authentication. It directly calls `tool.ainvoke(request.arguments)` without a `CurrentUser` dependency, while the same file correctly requires auth on `/tools/status`. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add `user: CurrentUser` to the endpoint signature to align with the rest of the API.</comment>
<file context>
@@ -0,0 +1,211 @@
+
+
+@router.post("/tools/execute")
+async def execute_tool(request: ToolExecutionRequest):
+ """
+ Manually execute a specific tool.
</file context>
|
|
||
|
|
||
| @mcp.tool() | ||
| def download_drive_file(file_id: str, destination_path: str) -> str: |
There was a problem hiding this comment.
P1: These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. upload_drive_file can read any file the backend can access (including .env or credentials) and upload it to Drive. download_drive_file can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with Path.resolve() so each operation is constrained to an allowed base directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/mcp_servers/google_drive_server.py, line 205:
<comment>These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. `upload_drive_file` can read any file the backend can access (including `.env` or credentials) and upload it to Drive. `download_drive_file` can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with `Path.resolve()` so each operation is constrained to an allowed base directory.</comment>
<file context>
@@ -0,0 +1,422 @@
+
+
+@mcp.tool()
+def download_drive_file(file_id: str, destination_path: str) -> str:
+ """
+ Download a file from Google Drive to local storage.
</file context>
| from orchestration.state import AgentState # Your state definition | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| def red_flag_node(self, state: AgentState) -> AgentState: |
There was a problem hiding this comment.
P1: red_flag_node is defined as a module-level function but takes self and accesses instance attributes (self.llm, self._get_fallback_response()). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for AgentState from different paths. Consider refactoring this node into a plain function that accepts only state, or wrapping it in a class whose instance method can be bound and passed to add_node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/orchestration/red_flag_node.py, line 12:
<comment>`red_flag_node` is defined as a module-level function but takes `self` and accesses instance attributes (`self.llm`, `self._get_fallback_response()`). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for `AgentState` from different paths. Consider refactoring this node into a plain function that accepts only `state`, or wrapping it in a class whose instance method can be bound and passed to `add_node`.</comment>
<file context>
@@ -0,0 +1,60 @@
+from orchestration.state import AgentState # Your state definition
+logger = logging.getLogger(__name__)
+
+def red_flag_node(self, state: AgentState) -> AgentState:
+ """Detect unethical/destructive queries"""
+ query = state["user_query"].lower()
</file context>
|
|
||
| # ── Send ───────────────────────────────────────────────────────────── | ||
| # ── Handle Proposed Action ─────────────────────────────────────────── | ||
| if draft.proposed_action and draft.proposed_action.action_type != 'none': |
There was a problem hiding this comment.
P1: The critic (validate_notification) only vets notification_text and draft.reasoning, but the new code stages draft.proposed_action to the database immediately after approval without validating the action's description, action_type, or due_at. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in validate_notification or adding a separate validation step before staged_tasks_db.create_staged_task.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils/app_integrations.py, line 419:
<comment>The critic (`validate_notification`) only vets `notification_text` and `draft.reasoning`, but the new code stages `draft.proposed_action` to the database immediately after approval without validating the action's `description`, `action_type`, or `due_at`. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in `validate_notification` or adding a separate validation step before `staged_tasks_db.create_staged_task`.</comment>
<file context>
@@ -414,6 +415,26 @@ def _process_mentor_proactive_notification(uid: str, conversation_messages: list
# ── Send ─────────────────────────────────────────────────────────────
+ # ── Handle Proposed Action ───────────────────────────────────────────
+ if draft.proposed_action and draft.proposed_action.action_type != 'none':
+ action = draft.proposed_action
+ desc = action.description or notification_text
</file context>
| // Auto-refresh every 5s to pick up new frames from the capture task | ||
| let db_refresh = db.clone(); | ||
| use_effect(move || { | ||
| let db_snap = db_refresh.read().clone(); |
There was a problem hiding this comment.
P1: The rewind auto-refresh effect captures the db value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because use_effect reruns when db changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels spawn tasks on component drop, not on effect rerun. Read the db Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with use_signal holding the Task and calling .cancel()).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-app/src/pages/rewind.rs, line 158:
<comment>The rewind auto-refresh effect captures the `db` value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because `use_effect` reruns when `db` changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels `spawn` tasks on component drop, not on effect rerun. Read the `db` Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with `use_signal` holding the `Task` and calling `.cancel()`).</comment>
<file context>
@@ -0,0 +1,410 @@
+ // Auto-refresh every 5s to pick up new frames from the capture task
+ let db_refresh = db.clone();
+ use_effect(move || {
+ let db_snap = db_refresh.read().clone();
+ spawn(async move {
+ loop {
</file context>
| Some(n) if n.uuid == audio_uuid => { | ||
| let frames = assembler.process_notification(&n.value); | ||
| for frame in frames { | ||
| if audio_tx.send(frame).await.is_err() { |
There was a problem hiding this comment.
P2: When audio_tx.send(frame) fails because the receiver was dropped, the break exits only the inner for loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., 'stream: loop { ... }) and break 'stream; so the receiver-drop condition terminates the stream as intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-ble/src/connection.rs, line 161:
<comment>When `audio_tx.send(frame)` fails because the receiver was dropped, the `break` exits only the inner `for` loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., `'stream: loop { ... }`) and `break 'stream;` so the receiver-drop condition terminates the stream as intended.</comment>
<file context>
@@ -0,0 +1,305 @@
+ Some(n) if n.uuid == audio_uuid => {
+ let frames = assembler.process_notification(&n.value);
+ for frame in frames {
+ if audio_tx.send(frame).await.is_err() {
+ info!("[BLE] Audio receiver dropped, stopping stream");
+ break;
</file context>
| @@ -0,0 +1,388 @@ | |||
| /// Context Watcher — analyzes screen content and emits proactive suggestions. | |||
There was a problem hiding this comment.
P2: The loop computes interval_secs from cfg.context_watcher_interval_secs but never applies it to the tokio::time::Interval timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the Interval when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-app/src/context_watcher.rs, line 243:
<comment>The loop computes `interval_secs` from `cfg.context_watcher_interval_secs` but never applies it to the `tokio::time::Interval` timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the `Interval` when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.</comment>
<file context>
@@ -0,0 +1,388 @@
+ continue;
+ }
+
+ // Set the tick interval from config
+ let interval_secs = cfg.context_watcher_interval_secs.max(10).min(300);
+
</file context>
There was a problem hiding this comment.
40 issues found across 189 files
Confidence score: 1/5
mcp/backend/auth/dependencies.pyandmcp/backend/api/routes/tools.pytogether create a critical auth gap: JWTs are accepted without signature/claim validation and/tools/executecan run tools without aCurrentUser, so forged tokens or unauthenticated callers could execute privileged actions—restore full JWT verification (signature/exp/iss/aud) and require auth on tool execution before merging.mcp/backend/mcp_servers/google_drive_server.pyexposes arbitrary server-local file read/write paths through Drive tools, which can leak secrets (for example.env/credentials) or overwrite local files when downloading—restrict operations to an allowlisted workspace and validate/canonicalize paths before merging.mcp/backend/auth/mcp_token_bridge.pyis implemented againstlangchain-mcp-adapters0.1.0APIs (client.session()), butmcp/backend/requirements.txtpins0.0.10; this is a likely runtime break on first use—align the dependency version or refactor to the pinned API before merging.- The Windows Rust changes in
omi-windows/crates/omi-db/src/clipboard.rs,omi-windows/crates/omi-app/src/app_tracker.rs, andomi-windows/crates/omi-capture/src/video_chunk.rshave concrete crash/corruption risks (UTF-8 byte slicing panics and ffmpeg frame-size/buffer mismatch on odd dimensions), so user-facing instability is likely—switch to char-safe truncation and make encoded frame buffers match declared padded dimensions before merging.
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcp/backend/auth/mcp_token_bridge.py">
<violation number="1" location="mcp/backend/auth/mcp_token_bridge.py:1">
P0: This new module is written against `langchain-mcp-adapters 0.1.0` (`client.session()` API), but `mcp/backend/requirements.txt` pins `langchain-mcp-adapters==0.0.10`. The `client.session()` method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise `AttributeError` at runtime on a clean install. Either bump the pinned dependency to `>=0.1.0` or rewrite the client usage to match the 0.0.10 API.</violation>
</file>
<file name="mcp/backend/auth/dependencies.py">
<violation number="1" location="mcp/backend/auth/dependencies.py:36">
P0: The Bearer JWT is decoded with signature verification explicitly disabled (`verify_signature=False`) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary `sub` claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing `backend/routers/auth.py` already demonstrates proper RS256 verification — this dependency should follow the same pattern.</violation>
</file>
<file name="omi-windows/crates/omi-db/src/clipboard.rs">
<violation number="1" location="omi-windows/crates/omi-db/src/clipboard.rs:90">
P0: Slicing `e.content` by byte index `[..120]` can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from `.chars().take(120)` and appending the ellipsis only when the original `.chars().count()` exceeds 120.</violation>
</file>
<file name="mcp/backend/api/routes/tools.py">
<violation number="1" location="mcp/backend/api/routes/tools.py:62">
P1: The tools router calls `agent_manager.get_tools_info()` and `agent_manager.tools`, but `AgentManager` does not define either. This means `/tools`, `/tools/execute`, and `/tools/{tool_name}` will all crash with `AttributeError` at runtime. Add the missing delegation on `AgentManager` or point `tools.py` at the orchestrator's own tools interface.</violation>
<violation number="2" location="mcp/backend/api/routes/tools.py:128">
P0: The `/tools/execute` endpoint allows arbitrary tool execution without authentication. It directly calls `tool.ainvoke(request.arguments)` without a `CurrentUser` dependency, while the same file correctly requires auth on `/tools/status`. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add `user: CurrentUser` to the endpoint signature to align with the rest of the API.</violation>
</file>
<file name="mcp/backend/mcp_servers/google_drive_server.py">
<violation number="1" location="mcp/backend/mcp_servers/google_drive_server.py:205">
P1: These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. `upload_drive_file` can read any file the backend can access (including `.env` or credentials) and upload it to Drive. `download_drive_file` can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with `Path.resolve()` so each operation is constrained to an allowed base directory.</violation>
</file>
<file name="mcp/backend/knowledge_engine/club/chunker.py">
<violation number="1" location="mcp/backend/knowledge_engine/club/chunker.py:112">
P1: The chunking loop in `_split_text` trims `chunk` to a sentence boundary but advances `start` using the original fixed-width `end`. When a boundary is found early (trimmed by more than `chunk_overlap`), the text between the trimmed chunk end and `end - chunk_overlap` is permanently skipped. This causes irreversible data loss in embeddings and retrieval.
Recommend basing the next `start` on the actual emitted chunk length: advance from the trimmed chunk's end minus overlap, or at minimum ensure `start` does not exceed the trimmed chunk's end.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/app_tracker.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/app_tracker.rs:174">
P1: The fallback title-capping logic in `extract_app_name` slices by byte index (`&title[..40]`), which panics if byte 40 is not a UTF-8 character boundary. Window titles with multi-byte characters (CJK, emoji, Cyrillic) are common, and since this runs in a 10-second polling loop, one bad title can either crash the tracker task or cause repeated panics. Use character-aware truncation (e.g., `char_indices`) to cap safely.</violation>
</file>
<file name="omi-windows/crates/omi-capture/src/video_chunk.rs">
<violation number="1" location="omi-windows/crates/omi-capture/src/video_chunk.rs:112">
P1: The encoder tells ffmpeg to expect even-dimension frames (`ew`/`eh`) via `-video_size`, but writes the original unpadded `rgba.as_raw()` buffer. When width or height is odd, ffmpeg reads `ew * eh * 4` bytes per frame while only `width * height * 4` bytes are sent, misaligning every subsequent frame and corrupting or hanging the video stream. The frame data needs to match the declared dimensions — either resize/pad the image to `ew x eh` before writing, or pass the original dimensions to ffmpeg and handle padding through a filter instead.</violation>
</file>
<file name="mcp/backend/api/routes/health.py">
<violation number="1" location="mcp/backend/api/routes/health.py:44">
P1: The `/status` endpoint references `agent_manager.llm.model_name`, but `AgentManager` has no `llm` attribute. Calling this endpoint will raise an `AttributeError` instead of returning status. Consider removing the `llm_model` field from the response or exposing the model info through a safe property on `AgentManager` that the endpoint can call.</violation>
</file>
<file name="mcp/backend/hitl/confirmation.py">
<violation number="1" location="mcp/backend/hitl/confirmation.py:231">
P1: Calendar date normalization hard-codes the IST timezone (`+05:30`) in `_parse_natural_datetime`, `_normalize_datetime`, and the LLM system prompt inside `_draft_calendar_fields`. This silently schedules events at incorrect local times for any user outside IST. Since the orchestration layer is user-oriented, a fixed timezone is a functional correctness risk. Consider deriving the user's timezone from their profile or settings, or at minimum defaulting to UTC (`Z`) so downstream calendar APIs interpret the time consistently.</violation>
</file>
<file name="mcp/backend/requirements.txt">
<violation number="1" location="mcp/backend/requirements.txt:98">
P1: The new `mcp/backend/requirements.txt` is missing runtime dependencies that the backend code directly imports. `supabase` is imported in `auth/supabase_client.py` and three other files, `jwt` is imported in `auth/dependencies.py`, and `EmailStr` in `api/routes/auth.py` requires the optional `email-validator` package in pydantic v2. Without these entries, a fresh install will fail on first import. Adding `supabase`, `PyJWT`, and `email-validator` to this requirements file would fix the issue.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/context_watcher.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/context_watcher.rs:129">
P1: Byte-index slicing `&ocr_text[..ocr_text.len().min(1200)]` can panic at runtime when OCR text contains multi-byte UTF-8 characters (e.g. CJK, emoji). `str.len()` counts bytes, not characters, so the 1200 byte boundary may fall mid-character. Since this runs in a background watcher loop, a panic here would crash the task on common multilingual content. The codebase already handles this correctly in `capture.rs` using `char_indices().nth()` to truncate at a safe boundary — the same pattern should be used here.</violation>
<violation number="2" location="omi-windows/crates/omi-app/src/context_watcher.rs:243">
P2: The loop computes `interval_secs` from `cfg.context_watcher_interval_secs` but never applies it to the `tokio::time::Interval` timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the `Interval` when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.</violation>
</file>
<file name="mcp/backend/api/routes/auth.py">
<violation number="1" location="mcp/backend/api/routes/auth.py:149">
P1: The OAuth callback handlers interpolate `error`, `google_email`, `github_username`, and exception messages directly into redirect query strings without URL-encoding. If any of these values contain `&`, `=`, `?`, `#`, or other reserved characters, the query string is malformed and can even inject unintended parameters. Encode dynamic values with `urllib.parse.quote` (or `urllib.parse.urlencode`) before constructing the redirect URL.</violation>
</file>
<file name="omi-windows/crates/omi-capture/src/lib.rs">
<violation number="1" location="omi-windows/crates/omi-capture/src/lib.rs:74">
P1: When `capture_tick("all")` captures multiple monitors, all frames are pushed into the same `VideoChunkEncoder` stream. `VideoChunkEncoder::add_frame()` only checks for aspect-ratio changes when splitting chunks, not exact dimension equality. Two monitors with the same aspect ratio but different resolutions (e.g. 1920×1080 and 3840×2160) will therefore be written into a single ffmpeg rawvideo pipe sized for the first frame, corrupting the encoded output. Additionally, interleaving unrelated monitor frames into one video stream produces fragmented, jump-cut footage rather than a usable recording per monitor. Consider either maintaining a separate encoder per monitor or skipping video encoding when `frames.len() > 1`.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/clipboard_watcher.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/clipboard_watcher.rs:45">
P1: The clipboard preview truncation uses byte-indexed slicing (`&text[..60]`) which will panic at runtime if byte 60 falls inside a multi-byte UTF-8 character. Clipboard content commonly includes emoji, CJK text, and other non-ASCII content, making this a realistic crash path in a long-running watcher loop. Consider using character-aware truncation instead, such as `text.chars().take(60).collect::<String>()` to build the preview safely.</violation>
</file>
<file name="mcp/backend/core/client.py">
<violation number="1" location="mcp/backend/core/client.py:45">
P1: This test script calls `agent_manager.get_latest_gmail_messages()`, `get_upcoming_events()`, and `rag_retrieve()`, but `AgentManager` no longer exposes these methods — it only routes through the SmartOrchestrator. Running the script will throw `AttributeError`. Consider removing or updating the direct MCP test path to match the current `AgentManager` API.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/chat.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/chat.rs:167">
P1: Byte-based string slicing (`[..body_text.len().min(N)]`) can panic when the truncation point lands inside a multi-byte UTF-8 character. Since the text comes from an external LLM API, non-ASCII content is common. Switch to character-based truncation so the app doesn't crash on international or Unicode responses.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/mcp_bridge.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/mcp_bridge.rs:309">
P1: `confirm_mcp` posts to `/api/message/confirm` without the conditional `Authorization` header that `query_mcp` adds for the same MCP backend. If the backend requires auth for the message endpoint, confirmations will fail even though the initial query succeeds.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/config.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/config.rs:416">
P1: Persisting Firebase tokens, Google OAuth tokens, and API keys as plaintext JSON to `%APPDATA%/omi/config.json` is a credential-leakage risk on Windows — any process running as the same user can read this file. The `omi-app` crate already includes `aes-gcm` in `Cargo.toml` and implements a `dpapi_decrypt` helper using Windows DPAPI in `google_calendar.rs`; consider using DPAPI (or `CryptProtectData`) to encrypt the config file before writing it, or split sensitive credentials into an encrypted credential store instead of raw JSON.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/settings.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/settings.rs:15">
P1: The Wearable BLE section presents fake scan results and mock connection state as real functionality. `scan_ble_devices` hardcodes two fake devices with synthetic addresses, and the Connect button only updates local UI signals rather than establishing an actual BLE connection. This makes the settings UI misleading since a user can appear "Connected" without any hardware interaction. Consider either wiring this to a real BLE library (e.g., `btleplug` on Windows) or clearly marking the section as a placeholder with a TODO so users are not misled.</violation>
</file>
<file name="mcp/backend/api/main.py">
<violation number="1" location="mcp/backend/api/main.py:90">
P1: The `api/routes/tools.py` router is never imported or included in `main.py`, so every tools endpoint (`/api/tools`, `/api/tools/status`, `/api/tools/execute`, etc.) is unreachable even though the route implementations, models, and agent integration are all present. Add the missing import and `app.include_router` call.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/recording.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/recording.rs:252">
P1: After a conversation completes, a brand-new non-running `AgentRuntime` is created inline and passed to `ProactiveEngine::on_conversation_ended()`. Inside the proactive engine, the memory-insight branch calls `runtime.query()`, which requires a live Node.js process. The app intentionally no longer spawns Node.js (`app.rs`: "We no longer spawn Node.js! All execution paths route through `agent_runtime.query_native()`"), so this call will always silently fail because its result is ignored with `let _ =`. When `proactive_agent_enabled` is true, users will never see memory-based proactive insights and the failure will be invisible. The proactive engine should either use the native LLM path (`query_native`) or receive a properly initialized runtime.</violation>
</file>
<file name="omi-windows/crates/omi-audio/src/mixer.rs">
<violation number="1" location="omi-windows/crates/omi-audio/src/mixer.rs:24">
P1: The mixer task can leak forever after its input broadcast channels close. Both receiver branches use `Ok(chunk) = recv()` patterns, which silently ignore `Err(Closed)` results. When both channels close, only the `flush_interval.tick()` branch remains active, `out_len` stays zero, and the loop `continue`s forever rather than stopping. This leaks the mixer task after capture shutdown. Consider matching the full `Result` on each receiver and tracking channel closures so the loop exits when no sources remain.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/hotkey.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/hotkey.rs:115">
P1: The `Ctrl+Shift+R` hotkey is registered as `toggle_rec` and documented as “start / stop recording”, but the event listener maps key press to `StartRecord` and key release to `StopRecord`. This turns the shortcut into a momentary trigger (recording stops as soon as the keys are released), which is the same behavior as the separate PTT hotkey (`Ctrl+Space`). It should probably behave like the other toggles (`ToggleBar`, `ToggleVoiceChat`) by reacting only to the `Pressed` event and letting the app layer toggle the recording state.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/components/floating_bar.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/components/floating_bar.rs:43">
P1: The voice-history sync effect re-appends every item from `voice_history` onto the existing `chat_history` on each update, which duplicates all previously imported turns every time a new voice turn arrives. Consider either rebuilding `chat_history` from `voice_history` on each change or tracking a last-synced index so only new items are appended.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/app.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/app.rs:764">
P1: Clipboard preview truncation slices a Rust `String` by byte offset (`&c.content[..120]`), which panics at runtime if byte 120 falls inside a multibyte UTF-8 character. Because `c.content` comes from the OS clipboard and can contain emoji, CJK text, or other multibyte codepoints, this guard is not sufficient. The panic occurs inside the async agent-query context builder and silently aborts the query.</violation>
</file>
<file name="omi-windows/crates/omi-transcription/src/streaming.rs">
<violation number="1" location="omi-windows/crates/omi-transcription/src/streaming.rs:200">
P1: The transcription service can panic at runtime when Deepgram returns non-ASCII text. The logging statements on lines 200, 206, and 254 slice `String` using byte indices (`&text[..text.len().min(500)]` and `&text[..text.len().min(400)]`), but in Rust `str` indexing panics if the byte offset is not a UTF-8 character boundary. Because Deepgram transcripts can contain Unicode (e.g., multi-byte CJK, emoji, or accented characters), any logged message whose 400th or 500th byte falls inside a multi-byte code point will cause a thread panic and abort the transcription stream.
Consider using character-aware truncation instead, for example a small helper that does `text.chars().take(limit).collect::<String>()` or similar, so logging is safe regardless of the transcript language.</violation>
<violation number="2" location="omi-windows/crates/omi-transcription/src/streaming.rs:283">
P1: After `tokio::select!` returns when either the send or receive task finishes, the remaining task's `JoinHandle` is dropped but the task continues running in the background. This泄漏s resources and can emit stale transcript segments after the stream function has returned. You should explicitly abort the surviving task and optionally await it so it yields and shuts down cleanly. A minimal pattern is: store whichever handle finishes, then call `other_handle.abort()` and `let _ = other_handle.await;` after the `select!`.</violation>
</file>
<file name="mcp/backend/tests/test_rag.py">
<violation number="1" location="mcp/backend/tests/test_rag.py:6">
P1: This file is structured as a standalone script but named `test_rag.py` under the `tests/` directory. Because pytest collects `test_*.py` files by importing them, running `pytest` will immediately execute the module-level code — loading embedding models, creating temporary directories, making network requests to Supabase, and more — during *collection*, well before any test runs. There are no `test_*` functions or pytest-compatible assertions anywhere in the file, so pytest won't find anything to run after the import side effects finish. The sibling `test_mcp_servers.py` shows the correct pattern: wrap script logic in a `main()` function behind an `if __name__ == '__main__':` guard so it only runs when explicitly invoked. Consider either (a) adding a `__name__` guard and renaming the file without the `test_` prefix if it's meant to be run directly, or (b) refactoring into proper pytest test functions if it should participate in automated test runs.</violation>
</file>
<file name="omi-windows/crates/omi-db/src/unified_search.rs">
<violation number="1" location="omi-windows/crates/omi-db/src/unified_search.rs:160">
P1: The snippet-truncation logic in `search_memories_text` (and mirrored in three other search methods) uses byte-index slicing (`&content[..150]`) to truncate snippets after 150 bytes. In Rust this panics at runtime when index 150 falls inside a multi-byte UTF-8 character. Because these fields contain user-generated text, Unicode characters are expected and the search call can crash.
Use `char_indices()` to find the byte boundary at the 150th character instead, and apply the same fix to the other three matching truncation sites (`search_screenshots_unified`, `search_clipboard_unified`, `search_conversations_unified`).</violation>
</file>
<file name="backend/utils/app_integrations.py">
<violation number="1" location="backend/utils/app_integrations.py:419">
P1: The critic (`validate_notification`) only vets `notification_text` and `draft.reasoning`, but the new code stages `draft.proposed_action` to the database immediately after approval without validating the action's `description`, `action_type`, or `due_at`. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in `validate_notification` or adding a separate validation step before `staged_tasks_db.create_staged_task`.</violation>
<violation number="2" location="backend/utils/app_integrations.py:429">
P1: The `due_at` value extracted from `draft.proposed_action` is read but never passed to `create_staged_task`. Since the staged-task DB function already accepts `due_at` via kwargs, this omission means calendar/event times and task deadlines generated by the LLM are silently discarded. The frontend then has no structured datetime to schedule or promote, making the proposed-action feature ineffective for time-sensitive items. Pass `due_at=due_at` into the `create_staged_task` call.</violation>
</file>
<file name="mcp/backend/core/agent.py">
<violation number="1" location="mcp/backend/core/agent.py:249">
P1: The `resume` method accepts `user_id` as a parameter but never uses it, instead passing `thread_id` as the orchestrator's `user_id`. This makes the API contract misleading and could break token lookups or resume state matching if the orchestrator expects the actual user UUID. Consider either using `user_id` (if the orchestrator should receive it) or removing the unused parameter from the signature.</violation>
</file>
<file name="omi-windows/crates/omi-ble/src/protocol.rs">
<violation number="1" location="omi-windows/crates/omi-ble/src/protocol.rs:181">
P1: The `DeviceType::from_name` implementation has unreachable classification branches that will select the wrong audio codec.
Any BLE device name starting with "friend" — including "Friend Pendant" — is captured by the first `starts_with("friend")` branch and classified as `DeviceType::Omi`, so `DeviceType::FriendPendant` is never returned for those names. This causes Friend Pendant hardware to use the Omi `Opus` default instead of its intended `LC3` codec.
Likewise, "limitless" is mapped to `FriendPendant`, making `DeviceType::Limitless` completely unreachable from name detection even though `Limitless` has its own distinct `OpusFS320` default. This means Limitless devices will use `LC3` with the wrong frame size, which will break audio frame assembly.
Reordering the checks so more specific patterns are matched first — or narrowing the broad `starts_with("friend")` condition — would fix the misclassification.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/llm.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/llm.rs:258">
P1: When `complete_streaming` falls back to the next provider after a streaming failure, it may already have emitted partial tokens via `on_token` from the failed provider. Downstream callers like `agent_runtime.rs` push each token into the live UI, so a subsequent provider's output gets concatenated with the partial output — resulting in garbled text visible to the user. Consider tracking whether any tokens were emitted and avoiding downstream fallback when the stream already started, or send a reset/clear signal before retrying.</violation>
</file>
<file name="mcp/backend/orchestration/red_flag_node.py">
<violation number="1" location="mcp/backend/orchestration/red_flag_node.py:12">
P1: `red_flag_node` is defined as a module-level function but takes `self` and accesses instance attributes (`self.llm`, `self._get_fallback_response()`). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for `AgentState` from different paths. Consider refactoring this node into a plain function that accepts only `state`, or wrapping it in a class whose instance method can be bound and passed to `add_node`.</violation>
</file>
<file name="omi-windows/crates/omi-app/src/pages/rewind.rs">
<violation number="1" location="omi-windows/crates/omi-app/src/pages/rewind.rs:158">
P1: The rewind auto-refresh effect captures the `db` value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because `use_effect` reruns when `db` changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels `spawn` tasks on component drop, not on effect rerun. Read the `db` Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with `use_signal` holding the `Task` and calling `.cancel()`).</violation>
</file>
<file name="omi-windows/crates/omi-ble/src/connection.rs">
<violation number="1" location="omi-windows/crates/omi-ble/src/connection.rs:161">
P2: When `audio_tx.send(frame)` fails because the receiver was dropped, the `break` exits only the inner `for` loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., `'stream: loop { ... }`) and `break 'stream;` so the receiver-drop condition terminates the stream as intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -0,0 +1,178 @@ | |||
| """ | |||
There was a problem hiding this comment.
P0: This new module is written against langchain-mcp-adapters 0.1.0 (client.session() API), but mcp/backend/requirements.txt pins langchain-mcp-adapters==0.0.10. The client.session() method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise AttributeError at runtime on a clean install. Either bump the pinned dependency to >=0.1.0 or rewrite the client usage to match the 0.0.10 API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/auth/mcp_token_bridge.py, line 1:
<comment>This new module is written against `langchain-mcp-adapters 0.1.0` (`client.session()` API), but `mcp/backend/requirements.txt` pins `langchain-mcp-adapters==0.0.10`. The `client.session()` method does not exist in 0.0.10, so Google/GitHub MCP sessions will raise `AttributeError` at runtime on a clean install. Either bump the pinned dependency to `>=0.1.0` or rewrite the client usage to match the 0.0.10 API.</comment>
<file context>
@@ -0,0 +1,178 @@
+"""
+backend/auth/mcp_token_bridge.py - FIXED for langchain-mcp-adapters 0.1.0
+
</file context>
|
|
||
| try: | ||
| # Decode Firebase JWT without signature verification for local backend use | ||
| payload = jwt.decode(token, options={"verify_signature": False}) |
There was a problem hiding this comment.
P0: The Bearer JWT is decoded with signature verification explicitly disabled (verify_signature=False) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary sub claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing backend/routers/auth.py already demonstrates proper RS256 verification — this dependency should follow the same pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/auth/dependencies.py, line 36:
<comment>The Bearer JWT is decoded with signature verification explicitly disabled (`verify_signature=False`) and no claim validation (expiration, issuer, audience). This allows anyone to forge a valid token by crafting a JWT with an arbitrary `sub` claim and authenticate as any user. Firebase tokens should be verified with the project's public keys and validated against the expected audience and issuer. The existing `backend/routers/auth.py` already demonstrates proper RS256 verification — this dependency should follow the same pattern.</comment>
<file context>
@@ -0,0 +1,68 @@
+
+ try:
+ # Decode Firebase JWT without signature verification for local backend use
+ payload = jwt.decode(token, options={"verify_signature": False})
+
+ firebase_uid = payload.get("sub")
</file context>
Comment on lines
+90
to
+92
| let preview = if e.content.len() > 120 { | ||
| format!("{}…", &e.content[..120]) | ||
| } else { |
There was a problem hiding this comment.
P0: Slicing e.content by byte index [..120] can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from .chars().take(120) and appending the ellipsis only when the original .chars().count() exceeds 120.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-db/src/clipboard.rs, line 90:
<comment>Slicing `e.content` by byte index `[..120]` can panic when a clipboard entry contains multibyte UTF-8 characters (e.g., emoji, CJK). If byte 120 falls inside a character boundary, Rust will panic at runtime. Clipboard content is user-controlled, so this is a real crash risk. Consider using a Unicode-safe truncation instead, such as collecting from `.chars().take(120)` and appending the ellipsis only when the original `.chars().count()` exceeds 120.</comment>
<file context>
@@ -0,0 +1,114 @@
+ .iter()
+ .map(|e| {
+ let app = e.source_app.as_deref().unwrap_or("unknown");
+ let preview = if e.content.len() > 120 {
+ format!("{}…", &e.content[..120])
+ } else {
</file context>
Suggested change
| let preview = if e.content.len() > 120 { | |
| format!("{}…", &e.content[..120]) | |
| } else { | |
| let preview = if e.content.chars().count() > 120 { | |
| format!("{}…", e.content.chars().take(120).collect::<String>()) | |
| } else { | |
| e.content.clone() | |
| }; |
|
|
||
|
|
||
| @router.post("/tools/execute") | ||
| async def execute_tool(request: ToolExecutionRequest): |
There was a problem hiding this comment.
P0: The /tools/execute endpoint allows arbitrary tool execution without authentication. It directly calls tool.ainvoke(request.arguments) without a CurrentUser dependency, while the same file correctly requires auth on /tools/status. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add user: CurrentUser to the endpoint signature to align with the rest of the API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/api/routes/tools.py, line 128:
<comment>The `/tools/execute` endpoint allows arbitrary tool execution without authentication. It directly calls `tool.ainvoke(request.arguments)` without a `CurrentUser` dependency, while the same file correctly requires auth on `/tools/status`. If this router is registered, any unauthenticated caller could trigger write-capable MCP tools (Gmail, Calendar, Drive, GitHub). Add `user: CurrentUser` to the endpoint signature to align with the rest of the API.</comment>
<file context>
@@ -0,0 +1,211 @@
+
+
+@router.post("/tools/execute")
+async def execute_tool(request: ToolExecutionRequest):
+ """
+ Manually execute a specific tool.
</file context>
|
|
||
|
|
||
| @mcp.tool() | ||
| def download_drive_file(file_id: str, destination_path: str) -> str: |
There was a problem hiding this comment.
P1: These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. upload_drive_file can read any file the backend can access (including .env or credentials) and upload it to Drive. download_drive_file can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with Path.resolve() so each operation is constrained to an allowed base directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/mcp_servers/google_drive_server.py, line 205:
<comment>These Drive MCP tools expose arbitrary server-local filesystem paths to tool callers. `upload_drive_file` can read any file the backend can access (including `.env` or credentials) and upload it to Drive. `download_drive_file` can write to any local path, potentially overwriting server files. Consider adding a sandbox directory restriction and normalizing/validating paths with `Path.resolve()` so each operation is constrained to an allowed base directory.</comment>
<file context>
@@ -0,0 +1,422 @@
+
+
+@mcp.tool()
+def download_drive_file(file_id: str, destination_path: str) -> str:
+ """
+ Download a file from Google Drive to local storage.
</file context>
| from orchestration.state import AgentState # Your state definition | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| def red_flag_node(self, state: AgentState) -> AgentState: |
There was a problem hiding this comment.
P1: red_flag_node is defined as a module-level function but takes self and accesses instance attributes (self.llm, self._get_fallback_response()). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for AgentState from different paths. Consider refactoring this node into a plain function that accepts only state, or wrapping it in a class whose instance method can be bound and passed to add_node.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/backend/orchestration/red_flag_node.py, line 12:
<comment>`red_flag_node` is defined as a module-level function but takes `self` and accesses instance attributes (`self.llm`, `self._get_fallback_response()`). LangGraph node callbacks receive only the state object, so this function cannot be wired into a graph as-is. There are also duplicate/conflicting imports for `AgentState` from different paths. Consider refactoring this node into a plain function that accepts only `state`, or wrapping it in a class whose instance method can be bound and passed to `add_node`.</comment>
<file context>
@@ -0,0 +1,60 @@
+from orchestration.state import AgentState # Your state definition
+logger = logging.getLogger(__name__)
+
+def red_flag_node(self, state: AgentState) -> AgentState:
+ """Detect unethical/destructive queries"""
+ query = state["user_query"].lower()
</file context>
|
|
||
| # ── Send ───────────────────────────────────────────────────────────── | ||
| # ── Handle Proposed Action ─────────────────────────────────────────── | ||
| if draft.proposed_action and draft.proposed_action.action_type != 'none': |
There was a problem hiding this comment.
P1: The critic (validate_notification) only vets notification_text and draft.reasoning, but the new code stages draft.proposed_action to the database immediately after approval without validating the action's description, action_type, or due_at. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in validate_notification or adding a separate validation step before staged_tasks_db.create_staged_task.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils/app_integrations.py, line 419:
<comment>The critic (`validate_notification`) only vets `notification_text` and `draft.reasoning`, but the new code stages `draft.proposed_action` to the database immediately after approval without validating the action's `description`, `action_type`, or `due_at`. This means an LLM can pass the notification quality gate while emitting an incorrect or misleading task/calendar event that gets persisted for the user. Consider including the proposed action in `validate_notification` or adding a separate validation step before `staged_tasks_db.create_staged_task`.</comment>
<file context>
@@ -414,6 +415,26 @@ def _process_mentor_proactive_notification(uid: str, conversation_messages: list
# ── Send ─────────────────────────────────────────────────────────────
+ # ── Handle Proposed Action ───────────────────────────────────────────
+ if draft.proposed_action and draft.proposed_action.action_type != 'none':
+ action = draft.proposed_action
+ desc = action.description or notification_text
</file context>
| // Auto-refresh every 5s to pick up new frames from the capture task | ||
| let db_refresh = db.clone(); | ||
| use_effect(move || { | ||
| let db_snap = db_refresh.read().clone(); |
There was a problem hiding this comment.
P1: The rewind auto-refresh effect captures the db value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because use_effect reruns when db changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels spawn tasks on component drop, not on effect rerun. Read the db Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with use_signal holding the Task and calling .cancel()).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-app/src/pages/rewind.rs, line 158:
<comment>The rewind auto-refresh effect captures the `db` value once before spawning an infinite async loop. This causes two problems: (1) the loop observes a stale snapshot, so if the database becomes available after mount it is never read, and (2) because `use_effect` reruns when `db` changes, each rerun spawns another uncancelled 5-second polling task while the old ones keep running. Dioxus only cancels `spawn` tasks on component drop, not on effect rerun. Read the `db` Signal fresh inside the loop body on each iteration, and consider cancelling the previous task handle before spawning a new one (for example with `use_signal` holding the `Task` and calling `.cancel()`).</comment>
<file context>
@@ -0,0 +1,410 @@
+ // Auto-refresh every 5s to pick up new frames from the capture task
+ let db_refresh = db.clone();
+ use_effect(move || {
+ let db_snap = db_refresh.read().clone();
+ spawn(async move {
+ loop {
</file context>
| Some(n) if n.uuid == audio_uuid => { | ||
| let frames = assembler.process_notification(&n.value); | ||
| for frame in frames { | ||
| if audio_tx.send(frame).await.is_err() { |
There was a problem hiding this comment.
P2: When audio_tx.send(frame) fails because the receiver was dropped, the break exits only the inner for loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., 'stream: loop { ... }) and break 'stream; so the receiver-drop condition terminates the stream as intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-ble/src/connection.rs, line 161:
<comment>When `audio_tx.send(frame)` fails because the receiver was dropped, the `break` exits only the inner `for` loop, not the outer notification-processing loop. The function will continue consuming BLE notifications indefinitely, repeatedly trying to send on a closed channel. The log message says "stopping stream," but the stream does not actually stop. To fix, use an outer loop label (e.g., `'stream: loop { ... }`) and `break 'stream;` so the receiver-drop condition terminates the stream as intended.</comment>
<file context>
@@ -0,0 +1,305 @@
+ Some(n) if n.uuid == audio_uuid => {
+ let frames = assembler.process_notification(&n.value);
+ for frame in frames {
+ if audio_tx.send(frame).await.is_err() {
+ info!("[BLE] Audio receiver dropped, stopping stream");
+ break;
</file context>
| @@ -0,0 +1,388 @@ | |||
| /// Context Watcher — analyzes screen content and emits proactive suggestions. | |||
There was a problem hiding this comment.
P2: The loop computes interval_secs from cfg.context_watcher_interval_secs but never applies it to the tokio::time::Interval timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the Interval when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At omi-windows/crates/omi-app/src/context_watcher.rs, line 243:
<comment>The loop computes `interval_secs` from `cfg.context_watcher_interval_secs` but never applies it to the `tokio::time::Interval` timer, so the configured polling interval is completely ignored and the watcher always polls every 15 seconds. Consider recreating the `Interval` when the configured period changes, or remove the unused config field if dynamic adjustment isn't intended yet.</comment>
<file context>
@@ -0,0 +1,388 @@
+ continue;
+ }
+
+ // Set the tick interval from config
+ let interval_secs = cfg.context_watcher_interval_secs.max(10).min(300);
+
</file context>
Git-on-my-level
added
needs-maintainer-review
Git-on-my-level
requested changes
Jul 9, 2026
Git-on-my-level
left a comment
Git-on-my-level
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the substantial work here. I’m going to request changes rather than approve because this PR touches security-sensitive auth/tooling surfaces, dependency files, agent instructions, and a very large new Windows/MCP implementation, and there are several concrete blockers to resolve before maintainer review can proceed.
Blocking issues I found:
mcp/backend/auth/dependencies.pydecodes bearer JWTs withverify_signature=Falseand no issuer/audience/expiry validation. That would let a caller forge a token with an arbitrarysuband authenticate as another user. Please use Firebase/project key verification and validate the expected claims before trusting the token.mcp/backend/api/routes/tools.pyexposes/tools/executewithout aCurrentUserdependency while it can invoke the agent/MCP tool layer. Since these tools include Gmail/Calendar/Drive/GitHub-style capabilities, this needs authentication and per-user authorization before any execution endpoint is exposed.mcp/backend/auth/mcp_token_bridge.pyis written for the newerlangchain-mcp-adaptersclient.session()API, butmcp/backend/requirements.txtpinslangchain-mcp-adapters==0.0.10. A fresh install is likely to fail at runtime unless the dependency and code are aligned.mcp/backend/requirements.txtappears incomplete for direct imports in the new backend (supabase,PyJWT, andemail-validatorare needed by the added code). Please make the new service installable from a clean environment.app/pubspec.yamlchangeswhisper_flutter_newfrom the pinned Git dependency to a local path (../whisper_flutter_new), which will break normal checkout/CI/release builds unless that sibling directory exists.app/release.shchanges the production release target fromlib/main_prod.darttolib/main.dart. That is a release-behavior change and needs a clear justification or should be reverted.backend/utils/llm/chat.pyadds an agent prompt statement granting broad permission to access personal emails/calendar/health data. That changes user-data/agent behavior semantics and needs careful product/privacy review, narrower wording, and alignment with actual user consent/tool authorization.desktop/Backend-Rust/src/routes/auth.rsexpands Google OAuth scopes from basic profile scopes to Gmail read/compose/send/labels and Calendar scopes. That is a sensitive consent expansion and needs explicit product/privacy review plus UI/consent copy and tests.
Agent-instruction/documentation impact:
CLAUDE.mdadds a long Windows implementation plan to the repository’s coding-agent instruction file without updatingAGENTS.md, even though the existing instructions say to keep them synced. Because coding/review agents read this file as repo-level guidance, adding a large feature plan here can steer future AI agents toward this Windows/MCP architecture regardless of whether maintainers have accepted it..windsurf/workflows/review.mdadds review-agent workflow instructions. The content is mostly generic, but it changes automated coding/review-agent behavior in the repo and should be intentionally reviewed by maintainers.mcp/backend/README.mddescribes a new agentic chatbot/MCP backend and setup path. Several claims depend on code that currently has auth/dependency/runtime blockers, so the docs should be corrected after the implementation is made safe and installable.
On scope: I’m not asking for a split just because the PR is large, but this currently mixes separable risk surfaces: a Windows desktop app, a new MCP backend, OAuth/token storage, broad Google/GitHub tool access, Flutter release/dependency changes, and repo-level AI-agent instructions. Splitting or at least narrowing the PR to one reviewable surface would materially help security review, testing, and rollback.
Please add focused validation for the new auth/tool execution paths and clean install/build checks for the new backend and app changes. Human maintainer review is needed before merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Complete build for windows in Dioxus(Rust) :)