🧹 Decouple HTTP exceptions from VAD utility - #11338
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e3840d6f-32c9-432e-b77a-80cf8f24a3c8) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 921c6fcf4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except VADEmptyError: | ||
| raise HTTPException(status_code=400, detail="Audio is empty") |
There was a problem hiding this comment.
When VAD finds no speech, this commit changes the utility's exception contract and adds translations at the HTTP and batch-script boundaries, but it modifies no tests; an incorrect exception type or catch would therefore turn the upload's intended 400 into a 500 or let the maintenance thread fail unnoticed. Add a behavioral test that drives the zero-segment path and verifies VADEmptyError is translated correctly, as required for behavior-changing bug fixes.
AGENTS.md reference: AGENTS.md:L26-L28
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- In
backend/routers/speech_profile.py, theVADEmptyError→400 "Audio is empty"branch is currently untested, so a future change could silently alter this user-facing contract (for example returning a generic error instead of the expected 400). Add a focused router test that triggersVADEmptyErrorand asserts the exact status code/message to de-risk regressions.
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="backend/routers/speech_profile.py">
<violation number="1" location="backend/routers/speech_profile.py:86">
P3: The new `VADEmptyError` handling path is untested. The router branch that converts `VADEmptyError` into the `400 "Audio is empty"` response is the exact user-facing behavior this PR preserves, and the script's `except VADEmptyError: return` is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches `apply_vad_for_speech_profile` to raise `VADEmptyError` and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| apply_vad_for_speech_profile(file_path) | ||
| try: | ||
| apply_vad_for_speech_profile(file_path) | ||
| except VADEmptyError: |
There was a problem hiding this comment.
P3: The new VADEmptyError handling path is untested. The router branch that converts VADEmptyError into the 400 "Audio is empty" response is the exact user-facing behavior this PR preserves, and the script's except VADEmptyError: return is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches apply_vad_for_speech_profile to raise VADEmptyError and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/speech_profile.py, line 86:
<comment>The new `VADEmptyError` handling path is untested. The router branch that converts `VADEmptyError` into the `400 "Audio is empty"` response is the exact user-facing behavior this PR preserves, and the script's `except VADEmptyError: return` is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches `apply_vad_for_speech_profile` to raise `VADEmptyError` and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.</comment>
<file context>
@@ -81,7 +81,10 @@ def upload_profile(file: UploadFile, uid: str = Depends(auth.get_current_user_ui
- apply_vad_for_speech_profile(file_path)
+ try:
+ apply_vad_for_speech_profile(file_path)
+ except VADEmptyError:
+ raise HTTPException(status_code=400, detail="Audio is empty")
</file context>
undivisible
added
human
Git-on-my-level
removed
the
workflow-review
|
Reviewed the full three-file diff; this looks like a good separation-of-concerns cleanup.
Backend CI is green. I also removed Automated maintainer review; final merge decision remains with the maintainers. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
the
positive-signal
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_644ec186-dc8a-499c-849d-3e7693c026c9) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1665c14c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) as mock_aseg, patch.object(mod, "VADEmptyError", _FakeVADEmptyError), patch.object( | ||
| mod, "apply_vad_for_speech_profile", side_effect=_FakeVADEmptyError("Audio is empty") |
There was a problem hiding this comment.
Fresh evidence since the earlier missing-coverage comment is that this new test replaces both VADEmptyError and apply_vad_for_speech_profile with matching fakes, so it never executes the changed zero-segment branch in utils/stt/vad.py. If the real utility continued raising HTTPException or later raised another type, this test would still pass while silent uploads return 500 and the batch script fails to handle them; patch vad_is_empty to return [] and invoke the real utility through the route (or separately assert the real utility's exception) so the regression test covers the production contract.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Introduces VADEmptyError so empty speech-profile audio returns 400 instead of 500, with a unit test. CI green. Approve-only (refactor, no linked bug issue).
|
Thanks for the update — I reviewed the current four-file diff on this head, including the newly added regression test.
Validation: backend CI is green. I also ran This still looks like a clean backend separation-of-concerns cleanup, and the existing by AI on behalf of David — automated maintainer review; final merge decisions remain with the maintainers. |
4e1665c to
5466314
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_892655a0-1a99-445d-ad8c-1c9f5ad4013d) |
Refactored `apply_vad_for_speech_profile` in `backend/utils/stt/vad.py` to raise a new `VADEmptyError` instead of FastAPI's `HTTPException(400)`. This logic is now caught in the `backend/routers/speech_profile.py` router and `backend/scripts/stt/j_apply_vad_to_speech_profiles.py` script, decoupling web exceptions from core domain utilities. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
5466314 to
84043ba
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_20ce6641-379a-4e88-b712-b418987d762b) |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks — this looks like a good narrow backend cleanup. I reviewed the current head (84043baef77fa1e23442ad297539e9fc2ec8f9fa):
backend/utils/stt/vad.py: replacing the FastAPIHTTPExceptionwithVADEmptyErrorkeeps the VAD utility framework-independent while preserving the empty-audio signal before any trimming/export work.backend/routers/speech_profile.py: catchingVADEmptyErrorat the upload route preserves the user-facing400 "Audio is empty"response and still bails out before duration caching, upload, or embedding extraction.backend/scripts/stt/j_apply_vad_to_speech_profiles.py: handlingVADEmptyErrorlets the maintenance script skip empty profiles instead of crashing on a web-layer exception, which matches the decoupling goal.backend/tests/unit/test_speech_profile_wav_decode.py: the added empty-VAD test asserts the 400 response and verifies upload is not called, covering the earlier automated concern around this route behavior.
CI is green, and I do not see a blocking issue in this diff. Keeping this as a positive signal; final merge judgment can stay with the human maintainer path already in progress.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Exercise the real VAD zero-segment exception at the utility boundary and cover the HTTP and batch translations at their callers. Failure-Class: none Tests: backend/.venv/bin/python -m pytest -q tests/unit/test_speech_profile_wav_decode.py tests/unit/test_vad_onnx.py
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ceb5afbc-f18d-4c7f-bd6a-208ae658e701) |
Exercise the production VAD zero-segment exception through the upload route and batch maintenance script, asserting both user-facing translations and their side-effect guards. Failure-Class: none
Refactored `apply_vad_for_speech_profile` in `backend/utils/stt/vad.py` to raise a new `VADEmptyError` instead of FastAPI's `HTTPException(400)`. This logic is now caught in the `backend/routers/speech_profile.py` router and `backend/scripts/stt/j_apply_vad_to_speech_profiles.py` script, decoupling web exceptions from core domain utilities. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Refactored `apply_vad_for_speech_profile` in `backend/utils/stt/vad.py` to raise a new `VADEmptyError` instead of FastAPI's `HTTPException(400)`. This logic is now caught in the `backend/routers/speech_profile.py` router and `backend/scripts/stt/j_apply_vad_to_speech_profiles.py` script, decoupling web exceptions from core domain utilities. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3e4715d46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(value, str) and value: | ||
| try: | ||
| action_item_data[date_field] = datetime.fromisoformat(value.replace('Z', '+00:00')) |
There was a problem hiding this comment.
When a tool or LLM supplies a date-only ISO string such as 2024-01-01, datetime.fromisoformat produces a timezone-naive value; an already-parsed naive datetime bypasses this branch entirely. Those values are then handed to Firestore, which rejects them, causing extracted action items—potentially an entire background batch—to be lost. Convert both string and datetime inputs to timezone-aware UTC before returning.
Useful? React with 👍 / 👎.
| # Manual only: automatic push/schedule tagging was cutting a new macOS candidate | ||
| # tag on nearly every desktop-affecting main merge and every 15 minutes. Plan and | ||
| # publish a candidate deliberately via workflow_dispatch; qualification/promotion |
There was a problem hiding this comment.
After this edit, desktop_auto_release.yml has only workflow_dispatch, so ordinary merges never invoke the planner and no daily beta candidate is cut unless an operator remembers to run the workflow manually. Restore the scheduled automatic trigger required by the repository's desktop release pipeline.
AGENTS.md reference: AGENTS.md:L126-L128
Useful? React with 👍 / 👎.
| if url == '' or url == ',': | ||
| disable_user_webhook_db(uid, wtype) | ||
| else: | ||
| enable_user_webhook_db(uid, wtype) |
There was a problem hiding this comment.
When a mobile user clears the audio-bytes webhook and saves, developer_mode_provider.dart posts ,<delay> (normally ,5), which does not match either literal here. The endpoint therefore re-enables a webhook with no URL, so the toggle comes back on and audio processing remains enabled despite the user's attempt to disable it; determine emptiness from the URL portion before the comma.
Useful? React with 👍 / 👎.
| String? selectedLanguageName = selectedLanguage != null | ||
| ? homeProvider.availableLanguages.entries.firstWhere((element) => element.value == selectedLanguage).key |
There was a problem hiding this comment.
If the stored primary-language code is not in this bundled map, opening the forced language dialog throws StateError at firstWhere before it can render. This is reachable for valid server-supported values such as sw, cy, or af, which PATCH /v1/users/language accepts but the bundled map omits; use the existing safe lookup/fallback instead of assuming every stored code is present.
Useful? React with 👍 / 👎.
| setSegments((prev) => { | ||
| // Update existing segment or add new one | ||
| const existingIndex = prev.findIndex((s) => s.id === segment.id); | ||
| if (existingIndex >= 0) { | ||
| const updated = [...prev]; | ||
| updated[existingIndex] = segment; | ||
| return updated; | ||
| } | ||
| return [...prev, segment]; |
There was a problem hiding this comment.
During long browser recordings, every finalized segment takes this append path, so the React state and rendered transcript grow without limit while each update copies the entire array. Sessions reaching hundreds of segments progressively stall Chrome and can make the recording UI unusable; retain only a bounded recent window while leaving the server-side session intact for finalization.
Useful? React with 👍 / 👎.
| conversation_ids = vector_db.query_vectors(query, uid, starts_at=starts_at, ends_at=ends_at, k=limit) | ||
| if not conversation_ids: | ||
| return [] |
There was a problem hiding this comment.
This route now queries only conversation summary vectors, which do not contain phrases spoken solely in transcript segments. Consequently, MCP clients receive no result for exact names, decisions, or quotes that were omitted from the generated summary even though matching transcript chunks are indexed; merge search_transcript_chunks hits with the summary results and return the corresponding snippets.
Useful? React with 👍 / 👎.
| // Content-derived hit region: the fixed window is larger than the | ||
| // visible chrome/menu, and its transparent margins must keep passing | ||
| // clicks through to windows below (hitTest returns nil outside this). |
There was a problem hiding this comment.
In notch mode the panel is intentionally fixed at its maximum hover size, leaving a large transparent area around the visible chrome. Returning nil from the content view's hitTest does not make the NSWindow click-through—the frame view still owns the event—and this change removes the window-level ignoresMouseEvents synchronization, so the invisible panel intercepts clicks on the main window's top navigation and other apps beneath it. Restore window-level mouse interception control or stop reserving the oversized transparent frame.
Useful? React with 👍 / 👎.
| ] | ||
| for f in futures: | ||
| f.result() | ||
| ordered_chunks = [context_data[m.id] for m in memories if m.id in context_data] | ||
| context_str = '\n'.join(ordered_chunks).strip() | ||
| context_str = '\n'.join(context_data.values()).strip() |
There was a problem hiding this comment.
Each worker inserts its chunk into context_data when it finishes, so joining dict.values() orders context by nondeterministic thread completion rather than the similarity-ranked memories list established above. With multiple retrieved conversations this can present the LLM with arbitrarily reordered evidence and produce unstable or less relevant answers; rebuild the output by iterating memories and selecting matching IDs instead of deferring the known defect in an untracked TODO.
AGENTS.md reference: AGENTS.md:L93-L93
Useful? React with 👍 / 👎.
| @override | ||
| @EnviedField(varName: 'OPENAI_API_KEY', obfuscate: true) | ||
| final String? openAIAPIKey = _ProdEnv.openAIAPIKey; |
There was a problem hiding this comment.
The production Codemagic app workflows write the real OPENAI_API_KEY into .env, and this EnviedField causes build_runner to compile that value into every released Flutter binary. obfuscate: true is reversible obfuscation rather than secret storage, so an app recipient can recover the provider credential and use it outside Omi; this also directly contradicts app/config/client_env_policy.yaml, which classifies OPENAI_API_KEY as server-only. Route OpenAI calls through an authenticated backend and remove the field from the public client.
Useful? React with 👍 / 👎.
| async def initialize_stt(self) -> bool: | ||
| request = self.host.request | ||
| provider = getattr(self.host.stt_service, 'value', self.host.stt_service) | ||
| if self.host.use_custom_stt: |
There was a problem hiding this comment.
When a session starts with Parakeet but _create_stt_socket falls back to Modulate, that method updates self.host.stt_service after this local value has already been captured. The death monitor and initialization failure paths therefore report parakeet in the client failure event and provider metrics even though Modulate was serving and failed, obscuring the actual incident and misleading provider-specific diagnostics; resolve the provider after socket creation or at each failure boundary.
Useful? React with 👍 / 👎.
Git-on-my-level
added
needs-scope-reduction
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the original VAD cleanup — the small VADEmptyError part still looks directionally right (backend/utils/stt/vad.py, backend/routers/speech_profile.py, and backend/scripts/stt/j_apply_vad_to_speech_profiles.py keep the framework exception at the route/script boundary).
I need to request changes on the current head, though, because it has grown far beyond that refactor and now carries several unrelated, high-risk behavior changes:
app/lib/backend/http/openai.dartadds direct client calls tohttps://api.openai.com/v1/...withAuthorization: Bearer ${Env.openAIAPIKey}, andapp/.env.template/app/lib/env/*.dartadd an app-side OpenAI key. That moves LLM calls and user content from the backend-managed/gateway path into the shipped app surface, which needs an explicit security/privacy/product decision rather than being bundled into a VAD utility cleanup.backend/routers/desktop_chat.pyremoves the structured managed lane (CHAT_STRUCTURED_AUTO_LANE_ID,_MANAGED_STRUCTURED_ALIASES, lane-specific accounting) and routes managed gateway requests through the chat-agent lane. That can change model routing/personality/accounting for non-conversational extraction/planner calls.backend/route_policy_manifest.yamlplus routers such asbackend/routers/conversations.py,backend/routers/knowledge_graph.py,backend/routers/integrations.py,backend/routers/memories.py, andbackend/routers/users.pyremove multiple first-party extraction/synthesis/language endpoints and their policy entries, while many corresponding tests are deleted. That is a broad API/product contract change, not a decoupling refactor..github/workflows/desktop_auto_release.yml,.github/scripts/check-desktop-changelog.py,.github/scripts/plan-desktop-release.py, and their tests remove scheduled release-train/changelog/fallback behavior. This is release infrastructure and needs focused workflow review on its own.desktop/macos/AGENTS.mdchanges AI/coding-agent release-pipeline guidance, but the new text says candidates are cut on every macOS-affecting merge plus a 15-minute schedule, while the workflow diff removes the schedule and leavesworkflow_dispatchonly. That instruction file can directly mislead future coding/review agents about how desktop releases work, so it should not land in this state.backend/database/vector_db.pyremoves the injectablequery_vectorpath and weakens transcript date filtering to only apply when both bounds are present. That changes retrieval behavior and testability independently of the VAD work.web/app/package.json/web/app/package-lock.jsonremove Vitest and test scripts while web transcript tests are deleted, which further broadens the risk surface.
Please reduce this PR back to the VAD exception cleanup, or split/rework these unrelated backend API, desktop release, client-LLM, retrieval, web dependency, and agent-instruction changes into separately reviewed PRs with their own validation. The current head needs security-sensitive client/API-key review and release-workflow maintainer sign-off before it can be considered.
by AI on behalf of David — automated maintainer review; blocking here because the current head bundles unrelated security-sensitive client LLM/API-key changes, release workflow changes, and inaccurate agent-facing release guidance.
3 participants
🎯 What: The
apply_vad_for_speech_profileutility function inbackend/utils/stt/vad.pywas directly raising a FastAPIHTTPException(400)when a processed audio file was empty. This has been replaced with a domain-specificVADEmptyError. TheHTTPExceptionlogic has been moved up to the routing layer inbackend/routers/speech_profile.py. The background scriptbackend/scripts/stt/j_apply_vad_to_speech_profiles.pywas also updated to catchVADEmptyError.💡 Why: This change separates concerns and decouples domain/utility logic from web framework-specific exceptions.
HTTPExceptionshould only be thrown from routing layers. This makes the utility function safer to reuse across different contexts, such as thej_apply_vad_to_speech_profiles.pybackground script which previously would have crashed upon encountering an empty audio file.✅ Verification: Ran pytest tests against the updated files (
backend/tests/unit/test_speech_profile_wav_decode.py,backend/tests/unit/test_user_speaker_embedding.py,backend/tests/unit/test_vad_onnx.py) to confirm no regressions and tested API compatibility. Code Review was completed.✨ Result: A cleaner architectural separation of concerns between core utility code and web layers, and a more robust background script that won't crash when encountering empty voice segments.
PR created automatically by Jules for task 5064561204901573843 started by @undivisible
Note
Low Risk
Small layering refactor on speech-profile upload validation; HTTP 400 for empty audio is preserved and covered by tests.
Overview
apply_vad_for_speech_profileno longer raises FastAPIHTTPExceptionwhen VAD finds no speech. It now raises a domainVADEmptyError, andvad.pydrops its FastAPI dependency.The
/v3/upload-audiohandler catchesVADEmptyErrorand still returns 400 with "Audio is empty", so API behavior for empty speech uploads is unchanged. The batchj_apply_vad_to_speech_profilesscript catches the same error, logs, and skips re-upload instead of failing the job.Unit tests cover the new exception in
apply_vad_for_speech_profile, the upload route’s 400 mapping, and the batch script’s skip path.Reviewed by Cursor Bugbot for commit e4f12d7. Configure here.