Conversation
📝 WalkthroughWalkthroughAdds a real‑time voice extension: server REST/WebSocket endpoints, in‑memory and Redis session managers, STT/TTS service factories, a pipecat AgentBridge processor, frontend voice client/UI and stores, example voice agent, docs, tests, and supporting config/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant WS as WebSocket Endpoint
participant Pipeline as Voice Pipeline
participant STT as STT Service
participant Agent as Agent Bridge
participant TTS as TTS Service
Browser->>WS: Connect (optional session_token)
WS->>Pipeline: build_voice_pipeline(stt, bridge, tts)
rect rgba(200, 150, 255, 0.5)
Browser->>WS: Send PCM16 audio frames
WS->>Pipeline: forward audio frames
Pipeline->>STT: stream audio
STT->>Pipeline: emit transcription
end
rect rgba(150, 200, 255, 0.5)
Pipeline->>Agent: deliver transcription
Agent->>Agent: run manifest_run (agent logic)
Agent->>Pipeline: emit text response (streamed/final)
end
rect rgba(200, 200, 150, 0.5)
Pipeline->>TTS: stream text → TTS
TTS->>Pipeline: emit PCM16 audio
Pipeline->>WS: send audio frames
WS->>Browser: playback audio
end
Browser->>WS: Send control (mute/stop)
WS->>Pipeline: handle interruption / cancel agent task
WS->>Browser: send state updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
♻️ Duplicate comments (1)
frontend/src/lib/types/Session.ts (1)
1-1:⚠️ Potential issue | 🔴 CriticalSame library import change found here.
This file has the identical
bson→bson-objectidimport change asConvSidebar.ts. The same concerns apply: this is a code change that contradicts the documentation-only PR scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/types/Session.ts` at line 1, The import of ObjectId in Session.ts was changed to "bson-objectid", which duplicates the same unintended library swap from ConvSidebar.ts and violates the docs-only PR scope; revert the change so Session.ts uses the original bson import used across the project by restoring the previous import for the ObjectId type (i.e., match the original "bson" import pattern) so the symbol referencing ObjectId in Session.ts aligns with the rest of the codebase.
🟠 Major comments (22)
.pre-commit-config.yaml-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorGlobal
.agents/exclusion bypasses security hooksLine 1 sets a top-level exclude, so hooks like
detect-secrets/banditwon’t run on.agents/**. That weakens repo-wide security scanning. Prefer per-hook excludes for only the expensive/style hooks instead of a global exclusion.Suggested adjustment
-exclude: ^\.agents/ repos: @@ - repo: https://github.com/astral-sh/ruff-pre-commit @@ - exclude: ^(examples/|bindu/grpc/generated/) + exclude: ^(examples/|bindu/grpc/generated/|\.agents/) @@ - id: ruff-format - exclude: ^(examples/|bindu/grpc/generated/) + exclude: ^(examples/|bindu/grpc/generated/|\.agents/) @@ - id: ty @@ - exclude: ^(examples/|bindu/grpc/generated/) + exclude: ^(examples/|bindu/grpc/generated/|\.agents/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 1, The top-level "exclude: ^\.agents/" entry disables all pre-commit hooks for .agents/**; remove this global exclude and instead add per-hook "exclude" patterns only to the specific expensive/style hooks you want to skip (e.g., add excludes to the individual hook entries for tools like detect-secrets, bandit, black, prettier, etc.), leaving security hooks such as detect-secrets and bandit without the .agents/ exclusion so they still run on that path.frontend/src/lib/components/chat/ChatMessage.svelte-70-84 (1)
70-84:⚠️ Potential issue | 🟠 MajorDon't let the wrapper match its own interactive guard.
At Line 271 the assistant container becomes
role="button", so theclosest(... [role='button'] ...)check at Line 75 now matchese.currentTargetfor every click inside the message.handleMessageClicktherefore returns before toggling or opening the image lightbox, which regresses the new tap behavior. The key handler should also ignore bubbled events from nested controls.Suggested fix
function handleMessageClick(e: MouseEvent) { - const target = e.target as HTMLElement | null; - if ( - target && - target.closest( - "button, a, input, textarea, select, [role='button'], [data-no-toggle]" - ) - ) { + const target = e.target; + const interactiveAncestor = + target instanceof Element + ? target.closest( + "button, a, input, textarea, select, [role='button'], [data-no-toggle]" + ) + : null; + if (interactiveAncestor && interactiveAncestor !== e.currentTarget) { return; } handleContentClick(e); - if (!(e.target instanceof HTMLImageElement)) { + if (!(target instanceof HTMLImageElement)) { isTapped = !isTapped; } }onkeydown={(event) => { + if (event.target !== event.currentTarget) return; if (event.key === "Enter" || event.key === " ") { event.preventDefault(); isTapped = !isTapped; } }}Also applies to: 271-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatMessage.svelte` around lines 70 - 84, The click guard in handleMessageClick is accidentally matching the message wrapper itself (which has role="button") and also returns for bubbled events from nested controls; update the guard to only bail out when the closest interactive ancestor is present AND is not the message container itself by checking the matched element !== e.currentTarget (or comparing to e.currentTarget via target.closest(...) !== e.currentTarget), and also ensure you ignore bubbled events by checking e.target !== e.currentTarget before toggling/opening; keep calls to handleContentClick and toggling isTapped unchanged otherwise.bindu/extensions/did/did_agent_extension.py-220-226 (1)
220-226:⚠️ Potential issue | 🟠 MajorUse a temporary file with atomic replacement to eliminate permission window.
When
recreate_keys=Trueand the private key file already exists,os.open(..., O_TRUNC, 0o600)truncates the file but preserves its existing permissions until the subsequentos.chmod()call. If the pre-existing file had overly-permissive permissions (0o644 or worse), the newly-written private key content is briefly exposed with those permissions. Replace the in-place truncation with a secure temp file creation andos.replace()to ensure the new key is never readable with incorrect permissions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/did/did_agent_extension.py` around lines 220 - 226, The current logic writing private_pem to self.private_key_path using os.open with O_TRUNC can expose the key during the short window before os.chmod; instead, create a secure temporary file in the same directory (e.g., using tempfile.NamedTemporaryFile or os.open on a temp name) with mode 0o600, write and flush/fsync the private_pem to that temp file, close it, then atomically replace the target file with os.replace(temp_path, str(self.private_key_path)), and finally ensure permissions on the final file are 0o600; update the code paths in the method that uses self.private_key_path and the recreate_keys=True branch to follow this temp-file + os.replace pattern.bindu/utils/notifications.py-62-69 (1)
62-69:⚠️ Potential issue | 🟠 MajorNormalize IPv4-mapped IPv6 addresses before checking against blocked networks.
IPv4-mapped IPv6 addresses like
::ffff:127.0.0.1bypass the block checks because they don't match any IPv4 CIDR in_BLOCKED_NETWORKS. A caller can use these forms to target internal services, defeating the SSRF protection.Add a helper function to unwrap IPv4-mapped addresses before the membership test in both locations (lines 62–69 and 165–171):
Suggested change
+def _normalize_checked_address( + addr: ipaddress.IPv4Address | ipaddress.IPv6Address, +) -> ipaddress.IPv4Address | ipaddress.IPv6Address: + if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None: + return addr.ipv4_mapped + return addr + for addr in addrs: + addr = _normalize_checked_address(addr) for blocked in _BLOCKED_NETWORKS: if addr in blocked: raise ValueError(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/notifications.py` around lines 62 - 69, Normalize IPv4-mapped IPv6 addresses before blocked-network checks by adding a small helper (e.g., _normalize_ip(ip_obj)) that detects IPv4-mapped IPv6 addresses (addr.ipv4_mapped) and returns the corresponding IPv4Address, otherwise returns the original ip object; call this helper on resolved_ip (and the other IP variable used in the second check) before iterating over _BLOCKED_NETWORKS so the membership test compares like-for-like and IPv4-mapped forms (e.g., "::ffff:127.0.0.1") are treated as 127.0.0.1 when checking containment.frontend/src/lib/server/database.ts-147-159 (1)
147-159:⚠️ Potential issue | 🟠 MajorPipeline updates using field references and aggregation syntax are not properly supported.
The migration in
10-update-reports-assistantid.tsuses aggregation-pipeline semantics (contentId: "$assistantId"for field reference and$unset: "assistantId"as a string), butapplyUpdate()only handles update-document operators. Specifically:
contentId: "$assistantId"will set the literal string"$assistantId"instead of copying the field value$unset: "assistantId"(string) won't work because the code callsObject.keys()on it, expecting an objectThis makes local migrations and in-memory tests produce incorrect results compared to production MongoDB behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/database.ts` around lines 147 - 159, The in-memory pipeline update logic must emulate MongoDB aggregation semantics so field references and string-form $unset work: modify applyUpdate (and the pipeline application in the update loop) to resolve string values that start with "$" as field references by reading the value from the current document (e.g., treat "contentId: \"$assistantId\"" as contentId = currentDoc.assistantId), and handle $unset when its operand is a string or array by deleting those keys (not calling Object.keys on a string). Ensure the pipeline reducer passes the current working document (not the original raw object) into applyUpdate and that applyUpdate recognizes pipeline-stage operators ($set, $unset, etc.), supporting $unset: "fieldName" and $unset: ["a","b"] as well as object forms..github/workflows/ci.yml-43-43 (1)
43-43:⚠️ Potential issue | 🟠 MajorUnit-test job now runs broader test scope than intended.
Line 43 switches from unit-only targeting to
tests/, which can include non-unit suites in a job explicitly labeled “Unit Tests.”Proposed fix
- uv run pytest tests/ \ + uv run pytest tests/unit/ \Based on learnings: "Applies to tests/unit/**/*.py : Place unit tests in
tests/unit/directory with pytest andpytest.mark.asynciodecorator for async tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 43, The CI unit-test step currently invokes "uv run pytest tests/" which runs the entire test tree; change this to run only the unit tests (e.g., "uv run pytest tests/unit/" or "uv run pytest 'tests/unit/**/*.py'") so the job labeled Unit Tests only executes tests in the tests/unit directory; update the pytest invocation in the unit-test job to the unit-only path pattern.bindu/extensions/voice/agent_bridge.py-297-310 (1)
297-310:⚠️ Potential issue | 🟠 MajorCancel the pending first-chunk task on timeout before returning the fallback.
If the overall response timeout fires while
first_taskis still awaitinganext(chunks), this method returns the fallback without cancelling that child task. The agent stream can keep running in the background after the session has already moved back tolistening.Proposed fix
chunks = self._iter_text_chunks(raw) + first_task: asyncio.Task[str] | None = None try: async with asyncio.timeout(self._total_response_timeout_seconds): if emit_frames: try: - first_task = asyncio.create_task(anext(chunks)) + first_task = asyncio.create_task(anext(chunks)) timeout_seconds = max( 0.0, self._first_token_timeout_seconds ) if timeout_seconds > 0: done, _pending = await asyncio.wait( @@ except TimeoutError: + if first_task and not first_task.done(): + first_task.cancel() + await asyncio.gather(first_task, return_exceptions=True) if emit_frames: self._set_state("error") fallback = DEFAULT_TIMEOUT_FALLBACK_TEXT if self._on_agent_transcript: self._safe_callback(self._on_agent_transcript, fallback, True) await self.push_frame(TextFrame(text=fallback)) self._set_state("listening") return fallback return NoneAlso applies to: 330-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/agent_bridge.py` around lines 297 - 310, The timeout branch around creating first_task (first_task = asyncio.create_task(anext(chunks))) should cancel and await that pending task before returning the fallback: when timeout_seconds > 0 and not done, call first_task.cancel(), await it (catch asyncio.CancelledError/asyncio.InvalidState as needed) to ensure the child coroutine is cleaned up, then push the DEFAULT_THINKING_TEXT via push_frame; apply the same cancellation-and-await pattern to the equivalent block handling the overall response timeout (the code referenced at lines 330-339) so no anext(chunks) task continues running after returning the fallback.examples/document-analyzer/document_analyzer.py-76-83 (1)
76-83:⚠️ Potential issue | 🟠 MajorDon't swallow DOCX extraction failures here.
Returning
""makes corrupt DOCX files look like successfully extracted documents, sohandler()can call the model with an empty document body instead of reporting a parse error.Proposed fix
def extract_text_from_docx(file_bytes): """Extract text from docx bytes""" try: doc = Document(io.BytesIO(file_bytes)) return "\n".join([p.text for p in doc.paragraphs]) except Exception as e: - logger.error(f"Error extracting DOCX text: {e}") - return "" + logger.exception("Error extracting DOCX text") + raise ValueError(f"Invalid DOCX file: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` around lines 76 - 83, The extract_text_from_docx function currently swallows all exceptions and returns an empty string, which hides parse failures from handler(); instead log the error then let the failure propagate so handler() can report a parse error—modify extract_text_from_docx to log the exception (using logger.error) and then re-raise the exception (or raise a specific ParseError) rather than returning "", so callers like handler() receive the error and can handle/report it.bindu/extensions/voice/audio_config.py-32-34 (1)
32-34:⚠️ Potential issue | 🟠 MajorInclude channel count in generic frame-size calculations.
get_frame_size()ignores the number of channels, so any non-mono configuration will under-allocate frame bytes and mis-size the stream.Proposed fix
-def get_frame_size(sample_rate: int, duration_ms: int, encoding: str) -> int: +def get_frame_size( + sample_rate: int, + duration_ms: int, + encoding: str, + channels: int = DEFAULT_CHANNELS, +) -> int: """Return a frame size in bytes for the given encoding.""" - return sample_rate * duration_ms // 1000 * get_bytes_per_sample(encoding) + if channels < 1: + raise ValueError(f"Channel count must be positive, got {channels}") + return ( + sample_rate + * duration_ms + // 1000 + * channels + * get_bytes_per_sample(encoding) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/audio_config.py` around lines 32 - 34, get_frame_size currently computes bytes per frame without accounting for multi-channel audio, causing under-allocation when channels != 1; update the function signature get_frame_size(sample_rate: int, duration_ms: int, encoding: str, channels: int = 1) to accept a channels parameter (default 1) and multiply the returned frame byte count by channels (i.e., sample_rate * duration_ms // 1000 * get_bytes_per_sample(encoding) * channels); also search for and update any callers of get_frame_size to pass the appropriate channels value (or rely on the default) so mono/stereo/etc. are handled correctly.bindu/utils/worker/parts.py-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorRuntime behavior changed in a docs-scoped PR.
Line 38 modifies worker fallback payload construction, which is production behavior. The stated scope says documentation-only; please split runtime changes into a separate PR (or explicitly update scope and include targeted tests/release notes).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/worker/parts.py` at line 38, The change to runtime behavior in parts.py modifies how a worker fallback payload is constructed by returning DataPart(kind="data", data=data); revert this runtime change in the docs-scoped PR and move the DataPart construction into a separate functional PR (or alternatively update this PR's scope to include runtime changes and add targeted unit tests and release notes). Locate the code that returns the fallback payload (the return that constructs DataPart and any surrounding function handling worker fallbacks) and restore the original behavior in this PR, then create a new PR that introduces DataPart(kind="data", data=data) along with tests for the worker fallback path and a release note describing the behavior change.frontend/src/routes/conversation/[id]/+page.svelte-252-257 (1)
252-257:⚠️ Potential issue | 🟠 MajorPass file attachments to direct-agent sendAgentMessage call.
The
base64Filesarray is prepared at lines 139–145 and correctly passed to the backend flow at line 270, but when using the direct agent API (lines 249–257),filePartsis omitted from thesendAgentMessageoptions. File uploads will be silently dropped in the direct-agent path while working correctly via the backend. Pass the prepared files:fileParts: base64Files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/conversation/`[id]/+page.svelte around lines 252 - 257, The direct-agent call to sendAgentMessage is missing the prepared file attachments, so add fileParts: base64Files to the options object passed to sendAgentMessage (the same object that currently includes abortSignal: messageUpdatesAbortController.signal, currentTaskId, taskState, and replyToTaskId). Locate the sendAgentMessage invocation and include the fileParts property referencing the base64Files variable so file uploads are forwarded in the direct-agent path.bindu/server/scheduler/memory_scheduler.py-31-33 (1)
31-33:⚠️ Potential issue | 🟠 MajorMove queue size and enqueue timeout to
app_settings(no local constants).Line 33 and Line 80 hardcode operational config in
bindu/**/*.py. This should be sourced frombindu.settings.app_settingsto keep runtime behavior centrally configurable.As per coding guidelines "Use
bindu.settings.app_settingsfor all configuration - never hardcode values such as URLs, ports, timeouts, API keys, or feature flags" and "NEVER create local config constants - useapp_settingsfrombindu.settings."Also applies to: 80-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/scheduler/memory_scheduler.py` around lines 31 - 33, Replace the local hardcoded config in memory_scheduler.py with values from bindu.settings.app_settings: remove _TASK_QUEUE_BUFFER_SIZE and any local enqueue timeout constant and instead import app_settings from bindu.settings and reference app_settings.task_queue_buffer_size (or the appropriate names used in app_settings) where the queue is created, and app_settings.enqueue_timeout (or configured timeout name) where enqueue/put with timeout is used; ensure you update the module-level import to "from bindu.settings import app_settings" and adjust any references in functions like the queue creation and the enqueue call so no local config constants remain.bindu/penguin/bindufy.py-498-499 (1)
498-499:⚠️ Potential issue | 🟠 MajorFail fast when
voiceis present but invalid instead of silently skipping.Line 498 currently ignores mis-typed
voicevalues (and empty dicts), which can hide configuration errors and make voice appear enabled but inactive.Proposed fix
- voice_config = validated_config.get("voice") - if voice_config and isinstance(voice_config, dict): + voice_config = validated_config.get("voice") + if voice_config is not None: + if not isinstance(voice_config, dict): + raise ValueError("Invalid voice configuration: 'voice' must be a dictionary") from bindu.extensions.voice import VoiceAgentExtension🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/penguin/bindufy.py` around lines 498 - 499, If a user provides a voice configuration but it's not a non-empty dict, fail fast instead of silently skipping: check the voice_config variable and if it's present but not an instance of dict or is an empty dict, raise a clear error (e.g., ValueError) describing the invalid/missing voice configuration; only proceed to import and instantiate VoiceAgentExtension when voice_config is a non-empty dict. Use the existing voice_config symbol and the VoiceAgentExtension import/usage in bindufy.py to locate where to add the validation and the exception.examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml-28-31 (1)
28-31:⚠️ Potential issue | 🟠 Major
application/pdfMIME type is incorrect for file path input.The implementation accepts text strings for both inputs: literal paths (e.g.,
/path/to/paper.pdf) and raw document text. The_read_content()function treats both as text and identifies file paths by string suffix (.endswith(".pdf")), not by MIME type. The skill.yaml should listtext/plainfor both input modes, as no binary PDF payload is ever processed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml` around lines 28 - 31, The input_modes in skill.yaml incorrectly lists application/pdf even though the code (see _read_content) treats inputs as text strings and detects PDF paths by suffix; update skill.yaml so both accepted inputs are declared as text/plain (remove application/pdf) to reflect that the skill expects text strings for raw document text and literal file paths, ensuring the manifest matches the _read_content behavior.examples/multilingual-collab-agent/main.py-60-69 (1)
60-69:⚠️ Potential issue | 🟠 MajorDon’t advertise memory when Mem0 is unavailable.
When
MEM0_API_KEYis missing orMem0Tools(...)fails, the tool list drops memory support, but the static instructions still tell the model it can remember previous conversations. That makes the example over-promise a capability it may not have. Build the memory section conditionally or add an explicit “memory disabled” instruction in the fallback path.Also applies to: 115-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multilingual-collab-agent/main.py` around lines 60 - 69, The example currently always tells the model it can remember even when Mem0 is not available; update the code that builds the static instructions so the memory-related instructions are only added when mem0_api_key / _mem0_enabled is true (i.e., after Mem0Tools(...) succeeds and tools includes Mem0Tools), or alternatively append an explicit “memory disabled” instruction in the fallback branch where _mem0_enabled is set to False and Mem0Tools initialization fails; adjust the logic around mem0_api_key, Mem0Tools, _mem0_enabled and the place that constructs the model instructions (also update the other occurrence noted around lines 115-118) so the model prompt accurately reflects whether memory is enabled.bindu/utils/worker/messages.py-21-21 (1)
21-21: 🛠️ Refactor suggestion | 🟠 MajorMove the file-size limit into
app_settings.
MAX_FILE_SIZEis a local config constant inside abindu/**/*.pymodule. That makes the upload limit harder to tune per deployment and conflicts with the repo rule to keep configuration inbindu.settings.app_settings.As per coding guidelines, "Use
bindu.settings.app_settingsfor all configuration - never hardcode values such as URLs, ports, timeouts, API keys, or feature flags" and "NEVER create local config constants - useapp_settingsfrombindu.settings."Also applies to: 114-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/worker/messages.py` at line 21, The MAX_FILE_SIZE constant in bindu.utils.worker.messages must be removed and the code should read the value from bindu.settings.app_settings instead; import app_settings from bindu.settings at top, replace usages of MAX_FILE_SIZE (including the other occurrences around the 114-119 block) with app_settings.MAX_FILE_SIZE, and add a default to app_settings (e.g., DEFAULT_UPLOAD_MAX_FILE_SIZE) if it doesn't exist so deployments can override it. Ensure any tests or callers referencing MAX_FILE_SIZE are updated to use app_settings.MAX_FILE_SIZE and remove the local constant declaration.bindu/extensions/voice/service_factory.py-87-103 (1)
87-103:⚠️ Potential issue | 🟠 MajorRestrict fallback to specific expected exceptions from the TTS provider setup.
The broad
except Exceptioncatches unexpected errors during service instantiation (e.g., parameter errors, bugs in pipecat) alongside recoverable config failures, causing them to be silently masked by fallback. The function explicitly raisesImportError(missing modules/attributes) andValueError(missing configuration), so only these should trigger fallback. Additionally, when both attempts fail, chain from bothprimary_errorandfallback_errorto preserve diagnostic context instead of discarding the primary error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/service_factory.py` around lines 87 - 103, Limit the broad exception handling in the TTS factory: only catch ImportError and ValueError from _create_tts_service_for_provider to decide to try the fallback_provider; do not catch all Exception types so parameter/bug errors aren't masked. In the fallback branch, log using logger.warning as before, attempt _create_tts_service_for_provider(fallback_provider, config), and if that also fails, set fallback_error.__context__ = primary_error (or otherwise attach primary_error to the fallback error) and raise a RuntimeError("TTS setup failed for primary and fallback providers") from fallback_error so both the primary_error and fallback_error are preserved in the exception chain.frontend/src/lib/services/voice-client.ts-349-356 (1)
349-356:⚠️ Potential issue | 🟠 MajorFlush the partial PCM buffer before sending
commit_turn.The worklet only posts audio when its 4096-sample buffer fills. On stop, you destroy the graph and immediately send
commit_turn, so the last partial chunk never reaches the server and the final words of a turn can get clipped.Also applies to: 399-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/services/voice-client.ts` around lines 349 - 356, The worklet currently only posts audio when its full 4096-sample buffer fills, so partial audio in flushBuffer() (method flushBuffer in voice-client worklet) must be sent before the app sends commit_turn; update the shutdown/stop flow that triggers commit_turn (see the stop/close logic around the commit_turn send and the other flush call at the 399-402 region) to call flushBuffer() and wait for the postMessage to complete if necessary whenever bufferIndex > 0 so the last partial PCM chunk is delivered; ensure flushBuffer() handles bufferIndex==0 as a no-op and that the caller resets bufferIndex only after the buffer has been posted.bindu/extensions/voice/redis_session_manager.py-262-272 (1)
262-272:⚠️ Potential issue | 🟠 MajorDon’t collapse Redis outages into “session not found”.
get_session()returnsNoneonRedisError, and the callers treat that exactly like a missing session. A transient Redis failure will therefore show up as 404/invalid-session instead of a backend failure, which is very hard to diagnose during active voice calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/redis_session_manager.py` around lines 262 - 272, In get_session(), don't treat redis.RedisError the same as a missing key: keep the existing logic that returns None only when data is None (after computing key via _session_key and reading via _redis_client.get and deserializing with _deserialize_session), but on redis.RedisError log the error with logger.error and re-raise (or wrap and raise a specific session storage exception) instead of returning None so callers can distinguish Redis outages from "session not found"; update the except block to not swallow the exception and ensure callers can catch the new/raised exception type.bindu/server/endpoints/voice_endpoints.py-462-470 (1)
462-470:⚠️ Potential issue | 🟠 MajorPush a disconnect into the filtered queue when handling
stop.This branch closes the real socket and returns, but it never forwards a disconnect message to
inbound_queue. The transport reading from_FilteredWebSocket.receive()can stay blocked untilsession_timeout, so stopped sessions linger instead of tearing down immediately.Suggested fix
if frame_type == "stop": control.stopped = True await _send_json( websocket, {"type": "state", "state": "ended"}, send_lock ) - try: - await websocket.close() - finally: - return + await inbound_queue.put({"type": "websocket.disconnect", "code": 1000}) + try: + await websocket.close() + except Exception: + logger.exception("Failed to close voice websocket after stop") + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 462 - 470, When handling the "stop" frame in the branch that sets control.stopped and calls _send_json, also enqueue a disconnect message into the inbound/filtered queue used by _FilteredWebSocket.receive() so the transport unblocks immediately; specifically, after setting control.stopped and sending the {"type":"state","state":"ended"} message, push a disconnect (e.g. {"type":"disconnect"} or whatever the consumer expects) into inbound_queue/filtered_queue before awaiting websocket.close(), ensuring the consumer reading from _FilteredWebSocket.receive() sees the disconnect and tears down rather than waiting for session_timeout.scripts/test_voice_providers.sh-58-60 (1)
58-60:⚠️ Potential issue | 🟠 MajorMake failed smoke checks affect the process exit code.
fail()only logs, so the Deepgram and optional--binduchecks can printFAILwhile the script still exits 0. That makes CI and reviewer smoke tests look green even when the providers are broken.Suggested fix
+FAILED=0 + pass() { echo -e "${GREEN}PASS${NC} - $1"; } -fail() { echo -e "${RED}FAIL${NC} - $1"; } +fail() { + FAILED=1 + echo -e "${RED}FAIL${NC} - $1" +} warn() { echo -e "${YELLOW}WARN${NC} - $1"; }save_artifacts echo "" echo "Done." +exit "$FAILED"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_voice_providers.sh` around lines 58 - 60, The fail() helper currently only prints a message so failing smoke checks don't affect the script exit code; update fail() to mark the run as failed (e.g., set a global FAILED=1 inside fail()) while still printing the failure, and ensure the script exits with a non-zero code at the end (check if FAILED is set and exit 1). Reference the pass(), fail(), warn() helpers and any final exit/cleanup block to implement the FAILED flag and final conditional exit.frontend/src/lib/components/chat/ChatWindow.svelte-150-168 (1)
150-168:⚠️ Potential issue | 🟠 MajorAllow attachment-only submissions again.
The new guards require non-empty text in both
handleSubmit()andsubmit(), so users can no longer send just files. That regresses multimodal flows where the attachment is the entire prompt.Suggested fix
async function handleSubmit() { - if (!draft) { - return; - } try { - const fileParts = (sources ? await Promise.all(sources) : []).filter(Boolean) as MessageFile[]; - await submit(draft, { fileParts }); + const message = draft.trim(); + const fileParts = (sources ? await Promise.all(sources) : []).filter(Boolean) as MessageFile[]; + if (!message && fileParts.length === 0) { + return; + } + await submit(message, { fileParts }); } catch (err) { console.error("Error preparing chat submission:", err); $error = err instanceof Error ? err.message : String(err); } } async function submit( message: string, options: { fileParts?: MessageFile[] } = {} ) { const fileParts = options.fileParts ?? []; - if (!message || loading || isReadOnly) return; + const trimmedMessage = message.trim(); + if ((!trimmedMessage && fileParts.length === 0) || loading || isReadOnly) return; if (requireAuthUser()) return; const contextId = agentContextId ?? undefined; let success = false; try { - for await (const _update of sendAgentMessage(message, contextId, { fileParts })) { + for await (const _update of sendAgentMessage(trimmedMessage, contextId, { fileParts })) { // Process updates if needed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatWindow.svelte` around lines 150 - 168, The guards in handleSubmit and submit prevent attachment-only sends; update handleSubmit to allow submission when there is no draft but there are resolved sources (i.e., proceed if sources produce fileParts) instead of returning immediately when draft is falsy, and change submit's guard to only block when both message is empty and fileParts is empty (preserve blocking when loading or isReadOnly). Refer to handleSubmit, draft, sources, fileParts, submit, loading, and isReadOnly when making the checks.
🟡 Minor comments (12)
bindu/extensions/did/did_agent_extension.py-243-261 (1)
243-261:⚠️ Potential issue | 🟡 MinorConsider using the full domain-qualified username for domain environments.
USERNAMEis reliably set by Windows for the current process, but in domain environments it returns only the username portion (notDOMAIN\username). Foricacls, this may resolve to an ambiguous or incorrect principal if the domain context is unclear. The current code is safe—it will raise an error if the ACL command fails—but using Windows APIs likeGetUserNameEx(NameSamCompatible)or the process SID directly viawin32securitywould be more robust for cross-environment reliability. As per coding guidelines, consider whether environment variables should be validated more strictly before use in security operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/did/did_agent_extension.py` around lines 243 - 261, The code currently reads USERNAME from the environment and passes it to icacls (see variable username and the cmd list used with subprocess.run), which can be ambiguous in domain environments; replace the env lookup with a robust retrieval of the full domain-qualified account (e.g., call the Windows API GetUserNameEx(NameSamCompatible) or obtain the process token/owner SID via win32security and translate it to a full account name) and use that result when building the icacls arguments, falling back to raising the existing RuntimeError if the API call fails; ensure the variable name stays username and the rest of the icacls command construction (cmd list and subprocess.run usage) reuses this value so authorization targets are unambiguous.examples/voice-agent/.env.example-7-10 (1)
7-10:⚠️ Potential issue | 🟡 MinorReorder keys to satisfy current dotenv-linter ordering checks.
Line 7–10, Line 18–25, and Line 34 are flagged as out-of-order. Sorting keys within each section will remove lint warnings and keep the template CI-friendly.
Proposed reorder
# Optional fallback TTS (recommended for resiliency) -VOICE__TTS_FALLBACK_PROVIDER=azure VOICE__AZURE_TTS_API_KEY=your-azure-speech-key VOICE__AZURE_TTS_REGION=eastus VOICE__AZURE_TTS_VOICE=en-US-SaraNeural +VOICE__TTS_FALLBACK_PROVIDER=azure # Optional tuning -VOICE__ENABLED=true -VOICE__STT_MODEL=nova-3 -VOICE__STT_LANGUAGE=en -VOICE__TTS_MODEL=piper-local -VOICE__SAMPLE_RATE=16000 -VOICE__AUDIO_CHANNELS=1 -VOICE__AUDIO_ENCODING=linear16 -VOICE__VAD_ENABLED=true VOICE__ALLOW_INTERRUPTIONS=true +VOICE__AUDIO_CHANNELS=1 +VOICE__AUDIO_ENCODING=linear16 +VOICE__ENABLED=true +VOICE__SAMPLE_RATE=16000 VOICE__SESSION_TIMEOUT=300 +VOICE__STT_LANGUAGE=en +VOICE__STT_MODEL=nova-3 +VOICE__TTS_MODEL=piper-local +VOICE__VAD_ENABLED=true # Optional LLM backend for smarter responses OPENROUTER_API_KEY=your-openrouter-api-key -OPENROUTER_MODEL=openai/gpt-oss-120b:free OPENROUTER_MEMORY_TURNS=4 +OPENROUTER_MODEL=openai/gpt-oss-120b:free VOICE_MAX_SENTENCES=2Also applies to: 18-25, 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/voice-agent/.env.example` around lines 7 - 10, The dotenv keys in the voice-agent template are out of alphabetical order (e.g., VOICE__TTS_FALLBACK_PROVIDER, VOICE__AZURE_TTS_API_KEY, VOICE__AZURE_TTS_REGION, VOICE__AZURE_TTS_VOICE) and are triggering dotenv-linter; fix by sorting the variables alphabetically within each logical section (the Azure TTS block and the other flagged blocks around lines 18–25 and the single key at line 34) so that each group follows lexicographic order, preserving values and comments but reordering the keys to satisfy the linter.examples/cerina_bindu/cbt/README.md-106-108 (1)
106-108:⚠️ Potential issue | 🟡 MinorFix relative paths in “Related Examples” links.
Line 106–108 currently use
../..., which resolves underexamples/cerina_bindu/. These links should point to sibling directories underexamples/, so use../../....Proposed fix
-- [`examples/collaborative-agents/`](../collaborative-agents/) — multi-agent A2A communication -- [`examples/agent_swarm/`](../agent_swarm/) — agent swarm patterns -- [`examples/beginner/`](../beginner/) — simple single-agent examples +- [`examples/collaborative-agents/`](../../collaborative-agents/) — multi-agent A2A communication +- [`examples/agent_swarm/`](../../agent_swarm/) — agent swarm patterns +- [`examples/beginner/`](../../beginner/) — simple single-agent examples🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/cerina_bindu/cbt/README.md` around lines 106 - 108, Update the three "Related Examples" markdown links that currently use ../ to point to the sibling directories under examples by changing their paths to use ../../; specifically update the links for `examples/collaborative-agents/` (`multi-agent A2A communication`), `examples/agent_swarm/` (`agent swarm patterns`), and `examples/beginner/` (`simple single-agent examples`) so each `../` becomes `../../` in the README.examples/multilingual-collab-agent/.env.example-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorReorder keys to satisfy dotenv-linter.
Current key order triggersUnorderedKeywarnings; sorting keeps the example lint-clean.Suggested diff
-OPENROUTER_API_KEY=your_openrouter_api_key_here MEM0_API_KEY=your_mem0_api_key_here MODEL_NAME=openai/gpt-4o-mini +OPENROUTER_API_KEY=your_openrouter_api_key_here🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multilingual-collab-agent/.env.example` around lines 1 - 3, Reorder the keys in the .env example so they are alphabetically sorted to satisfy dotenv-linter UnorderedKey warnings: change the sequence in examples/multilingual-collab-agent/.env.example from OPENROUTER_API_KEY, MEM0_API_KEY, MODEL_NAME to MEM0_API_KEY, MODEL_NAME, OPENROUTER_API_KEY so the keys are in lexical order.examples/pdf_research_agent/README.md-227-241 (1)
227-241:⚠️ Potential issue | 🟡 MinorTest example missing required message fields.
The simplified curl example is missing required fields (
kind,messageId,contextId,taskId) that are present in the more complete Postman example (lines 62-84). Users following this simplified example may encounter validation errors.Consider using a complete example or noting that the simplified form may not work with strict validation:
📝 Suggested complete example
curl -X POST http://localhost:3773 \ -H "Content-Type: application/json" \ -d '{ "jsonrpc": "2.0", "method": "message/send", "params": { "message": { "role": "user", - "parts": [{"kind": "text", "text": "Sample document text for testing..."}] - } + "kind": "message", + "messageId": "00000000-0000-0000-0000-000000000001", + "contextId": "00000000-0000-0000-0000-000000000002", + "taskId": "00000000-0000-0000-0000-000000000003", + "parts": [{"kind": "text", "text": "Sample document text for testing..."}] + }, + "configuration": {"acceptedOutputModes": ["application/json"]} }, - "id": "test-1" + "id": "00000000-0000-0000-0000-000000000004" }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/README.md` around lines 227 - 241, The curl example invoking the "message/send" JSON-RPC method omits required message fields used by validation (e.g., kind, messageId, contextId, taskId); update the example payload in README.md to include those fields inside the "message" object (matching the fuller Postman example's structure) or add a clear note that the simplified payload may fail strict validation, referencing the "message/send" method and the message parts format so readers can copy a working payload.examples/medical_agent/agent.py-26-28 (1)
26-28:⚠️ Potential issue | 🟡 MinorGuard against empty/malformed message payloads before invoking the model.
If the last message lacks
content, this can throw at runtime; if it is empty, you still incur an unnecessary model call.Proposed fix
def medical_handler(messages: list[dict[str, str]]) -> str: @@ - user_query = messages[-1]["content"] if messages else "" - response = medical_agent.run(user_query) + if not messages or "content" not in messages[-1]: + return "Please share your medical question." + user_query = messages[-1]["content"].strip() + if not user_query: + return "Please share your medical question." + response = medical_agent.run(user_query) return response.content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/medical_agent/agent.py` around lines 26 - 28, The code extracts messages[-1]["content"] into user_query and unconditionally calls medical_agent.run, which can throw if the last message is missing "content" or waste a model call if content is empty. Before calling medical_agent.run, validate the messages list and the last message: check that messages is non-empty, that messages[-1] is a dict-like object with a non-empty "content" string, and return an appropriate error/empty response (or skip the model call) if validation fails. Update the logic around user_query, messages, and the call to medical_agent.run to perform these checks and only invoke medical_agent.run(user_query) when user_query is a non-empty string.examples/multilingual-collab-agent/skills/research/skill.yaml-18-24 (1)
18-24:⚠️ Potential issue | 🟡 MinorClarify or drop the advertised JSON contract.
Lines 18-24 say this skill accepts and returns
application/json, but Lines 88-97 only describe plain-text input/output. That leaves callers without a concrete JSON shape to implement against. Either remove JSON here or document the request/response schema explicitly.Also applies to: 88-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multilingual-collab-agent/skills/research/skill.yaml` around lines 18 - 24, The skill.yaml currently advertises application/json in input_modes and output_modes but the implementation/docs only describe plain-text I/O; either remove the application/json entries from input_modes/output_modes to avoid promising a JSON contract, or add an explicit JSON request/response schema (e.g., top-level fields and types for the incoming request and the outgoing response) next to the plain-text description so callers know the exact JSON shape; update the input_modes/output_modes section and/or add a clear request_schema/response_schema block in skill.yaml so the JSON contract is unambiguous.examples/multilingual-collab-agent/skills/collaborate/skill.yaml-20-26 (1)
20-26:⚠️ Potential issue | 🟡 MinorClarify or drop the advertised JSON contract.
This manifest exposes
application/jsonin both directions, but Lines 100-110 only document free-form text requests and drafted text responses. That ambiguity will leak into callers and tooling. Either remove JSON from the modes or define the JSON payloads explicitly.Also applies to: 100-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multilingual-collab-agent/skills/collaborate/skill.yaml` around lines 20 - 26, The manifest lists application/json in input_modes and output_modes but the skill only documents free-form text requests and drafted text responses, creating an ambiguous contract; either remove application/json from the input_modes/output_modes entries or explicitly define the JSON request/response schema used by this skill (add concrete field names/types and examples) and update the free-form text documentation to reference that schema so callers/tools know which payload to send/expect; look for the input_modes/output_modes keys and the free-form text request/response documentation block and make the change consistently.bindu/extensions/voice/pipeline_builder.py-64-71 (1)
64-71:⚠️ Potential issue | 🟡 MinorUse consistent error handling for VAD imports.
The VAD block (lines 65-66) lacks the try/except wrapper used for STT/TTS imports in
service_factory.py. If a user enablesapp_settings.voice.vad_enabledbut the silero component is unavailable, they receive a rawImportErrorinstead of an actionable message. Wrap the imports like the STT/TTS factory paths:Example pattern
if app_settings.voice.vad_enabled: try: from pipecat.audio.vad.silero import SileroVADAnalyzer from pipecat.processors.audio.vad_processor import VADProcessor except (ImportError, AttributeError) as e: raise ImportError( "Voice VAD requires pipecat[silero]. " "Install with: pip install 'bindu[voice]'" ) from e vad_analyzer = SileroVADAnalyzer( sample_rate=app_settings.voice.sample_rate, ) vad_component = VADProcessor(vad_analyzer=vad_analyzer)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/pipeline_builder.py` around lines 64 - 71, The VAD import block triggered when app_settings.voice.vad_enabled is missing a try/except like the STT/TTS factory; wrap the imports of SileroVADAnalyzer and VADProcessor in a try/except (catch ImportError and AttributeError), and on failure raise a clear ImportError with actionable text (e.g., "Voice VAD requires pipecat[silero]. Install with: pip install 'bindu[voice]'") re-raising from the caught exception; keep the subsequent creation of vad_analyzer = SileroVADAnalyzer(...) and vad_component = VADProcessor(...) unchanged.docs/VOICE_AGENT_PLAN.md-40-40 (1)
40-40:⚠️ Potential issue | 🟡 MinorMinor formatting inconsistency in ASCII diagram.
There's a trailing hyphen (
-) that breaks the box alignment on line 40.📝 Fix
-└───────────────────────────────────────────┼────────────────────────-┘ +└───────────────────────────────────────────┼─────────────────────────┘🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/VOICE_AGENT_PLAN.md` at line 40, The ASCII diagram line contains a trailing hyphen character (the segment "└───────────────────────────────────────────┼────────────────────────-┘") that breaks box alignment; edit the diagram in VOICE_AGENT_PLAN.md to remove the stray '-' so the line ends with the proper box corner ("┘") and matches the other border characters, ensuring the box vertical and corner characters align with the rest of the diagram.examples/pdf_research_agent/pdf_research_agent.py-83-86 (1)
83-86:⚠️ Potential issue | 🟡 MinorUse
raise ... from Nonefor cleaner exception replacement.When raising a new exception to replace the original (providing a clearer user-facing message), use
from Noneto suppress the chained traceback, orfrom eif you want to preserve it.📝 Fix
except ImportError: - raise DocumentReadError( + raise DocumentReadError( f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" - ) + ) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 83 - 86, The ImportError handler in the except ImportError block that raises DocumentReadError for "'{resolved_path}'" should suppress exception chaining; change the except to capture the ImportError (e.g., except ImportError as e:) and re-raise DocumentReadError(...) using "from None" (or "from e" if you want the original traceback), i.e., update the raise of DocumentReadError in the pdf reading code to use "from None" to avoid the chained traceback while keeping the same message referencing resolved_path.bindu/server/endpoints/voice_endpoints.py-569-581 (1)
569-581:⚠️ Potential issue | 🟡 MinorReject malformed
context_idvalues instead of stringifying them.
str(body["context_id"])turnsnull, objects, and arrays into synthetic IDs like"None"or"{}". That hides bad client input and can bind the session to the wrong context.Suggested fix
if isinstance(body, dict) and "context_id" in body: - context_id = str(body["context_id"]) + raw_context_id = body["context_id"] + if not isinstance(raw_context_id, str) or not raw_context_id.strip(): + raise HTTPException( + status_code=400, + detail="context_id must be a non-empty string", + ) + context_id = raw_context_id.strip()As per coding guidelines, "Validate all external input and use type hints for input validation in Python files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 569 - 581, The current parsing block assigns context_id = str(body["context_id"]) which silently accepts nulls/objects/arrays; instead validate body["context_id"] is a proper string (and optionally a UUID) before accepting it: after loading body, if "context_id" in body check isinstance(body["context_id"], str) and that it is non-empty (or parse with uuid.UUID to enforce UUID format) and if validation fails raise HTTPException(status_code=400, detail="Invalid context_id"); only then set context_id = body["context_id"]. Reference the variables/symbols context_id, raw_body, body and the use of request.body() in the existing block to locate where to apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e3703db-45ea-4604-b894-231ed393bbf9
⛔ Files ignored due to path filters (4)
bindu/grpc/generated/agent_handler_pb2.pyis excluded by!**/generated/**bindu/grpc/generated/agent_handler_pb2.pyiis excluded by!**/generated/**bindu/grpc/generated/agent_handler_pb2_grpc.pyis excluded by!**/generated/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (131)
.github/workflows/ci.yml.gitignore.pre-commit-config.yamlREADME.bn.mdREADME.de.mdREADME.es.mdREADME.fr.mdREADME.hi.mdREADME.mdREADME.nl.mdREADME.ta.mdREADME.zh.mdSPLIT_PLAN.mdbindu/common/protocol/types.pybindu/extensions/__init__.pybindu/extensions/did/did_agent_extension.pybindu/extensions/voice/__init__.pybindu/extensions/voice/agent_bridge.pybindu/extensions/voice/audio_config.pybindu/extensions/voice/pipeline_builder.pybindu/extensions/voice/redis_session_manager.pybindu/extensions/voice/service_factory.pybindu/extensions/voice/session_factory.pybindu/extensions/voice/session_manager.pybindu/extensions/voice/voice_agent_extension.pybindu/penguin/bindufy.pybindu/penguin/config_validator.pybindu/server/applications.pybindu/server/endpoints/utils.pybindu/server/endpoints/voice_endpoints.pybindu/server/handlers/message_handlers.pybindu/server/metrics.pybindu/server/scheduler/memory_scheduler.pybindu/server/workers/base.pybindu/settings.pybindu/utils/__init__.pybindu/utils/capabilities.pybindu/utils/config/settings.pybindu/utils/logging.pybindu/utils/notifications.pybindu/utils/retry.pybindu/utils/task_telemetry.pybindu/utils/worker/messages.pybindu/utils/worker/parts.pydocs/VOICE.mddocs/VOICE_AGENT_PLAN.mdexamples/README.mdexamples/ag2_research_team/skills/research-team-skill/skill.yamlexamples/beginner/README.mdexamples/beginner/motivational_agent.pyexamples/cerina_bindu/cbt/README.mdexamples/document-analyzer/document_analyzer.pyexamples/medical_agent/.env.exampleexamples/medical_agent/agent.pyexamples/multilingual-collab-agent/.env.exampleexamples/multilingual-collab-agent/README.mdexamples/multilingual-collab-agent/agent_config.jsonexamples/multilingual-collab-agent/main.pyexamples/multilingual-collab-agent/skills/collaborate/skill.yamlexamples/multilingual-collab-agent/skills/research/skill.yamlexamples/multilingual-collab-agent/skills/translate/skill.yamlexamples/pdf_research_agent/.env.exampleexamples/pdf_research_agent/README.mdexamples/pdf_research_agent/pdf_research_agent.pyexamples/pdf_research_agent/skills/pdf-research-skill/skill.yamlexamples/typescript-langchain-agent/quiz-agent.tsexamples/typescript-langchain-quiz-agent/.env.exampleexamples/typescript-langchain-quiz-agent/quiz-agent.tsexamples/voice-agent/.env.exampleexamples/voice-agent/README.mdexamples/voice-agent/main.pyexamples/voice-agent/skills/voice-brief-response-skill/skill.yamlfrontend/.env.examplefrontend/.gitignorefrontend/src/lib/buildPrompt.tsfrontend/src/lib/components/ShareConversationModal.sveltefrontend/src/lib/components/chat/ChatInput.sveltefrontend/src/lib/components/chat/ChatMessage.sveltefrontend/src/lib/components/chat/ChatWindow.sveltefrontend/src/lib/components/chat/ContextList.sveltefrontend/src/lib/components/voice/LiveTranscript.sveltefrontend/src/lib/components/voice/VoiceCallButton.sveltefrontend/src/lib/components/voice/VoiceCallPanel.sveltefrontend/src/lib/constants/mime.tsfrontend/src/lib/jobs/refresh-conversation-stats.tsfrontend/src/lib/migrations/lock.tsfrontend/src/lib/migrations/routines/02-update-assistants-models.tsfrontend/src/lib/migrations/routines/10-update-reports-assistantid.tsfrontend/src/lib/server/config.tsfrontend/src/lib/server/database.tsfrontend/src/lib/server/files/downloadFile.tsfrontend/src/lib/server/files/uploadFile.tsfrontend/src/lib/server/models.tsfrontend/src/lib/services/voice-client.tsfrontend/src/lib/stores/chat.tsfrontend/src/lib/stores/voice.tsfrontend/src/lib/types/ConvSidebar.tsfrontend/src/lib/types/Model.tsfrontend/src/lib/types/Session.tsfrontend/src/lib/utils/agentMessageHandler.tsfrontend/src/lib/utils/tree/addChildren.spec.tsfrontend/src/lib/utils/tree/addSibling.spec.tsfrontend/src/lib/utils/tree/buildSubtree.spec.tsfrontend/src/lib/utils/tree/convertLegacyConversation.spec.tsfrontend/src/routes/+layout.sveltefrontend/src/routes/+page.sveltefrontend/src/routes/api/v2/conversations/[id]/message/[messageId]/+server.tsfrontend/src/routes/api/v2/export/+server.tsfrontend/src/routes/conversation/[id]/+page.sveltefrontend/src/routes/conversation/[id]/+server.tsfrontend/src/routes/settings/(nav)/authentication/+page.sveltefrontend/src/routes/settings/(nav)/negotiation/+page.sveltepyproject.tomlpytest.iniscripts/test_voice_providers.shtests/conftest_stubs.pytests/integration/grpc/test_grpc_e2e.pytests/unit/extensions/__init__.pytests/unit/extensions/voice/__init__.pytests/unit/extensions/voice/test_agent_bridge.pytests/unit/extensions/voice/test_service_factory.pytests/unit/extensions/voice/test_session_manager.pytests/unit/extensions/voice/test_voice_endpoints.pytests/unit/extensions/voice/test_voice_extension.pytests/unit/extensions/voice/test_voice_websocket_integration.pytests/unit/penguin/test_bindufy.pytests/unit/penguin/test_config_validator.pytests/unit/server/scheduler/test_memory_scheduler.pytests/unit/test_minimax_example.pytests/unit/utils/test_notifications.pytests/unit/utils/worker/test_messages.py
| if platform.system() == "Windows": | ||
| # Windows does not enforce POSIX permissions — write directly | ||
| self.private_key_path.write_bytes(private_pem) | ||
| self.public_key_path.write_bytes(public_pem) | ||
| self._harden_windows_key_file_acl(self.private_key_path) | ||
| self._harden_windows_key_file_acl(self.public_key_path) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'did_agent_extension.py' bindu/extensions/Repository: GetBindu/Bindu
Length of output: 101
🏁 Script executed:
wc -l bindu/extensions/did/did_agent_extension.pyRepository: GetBindu/Bindu
Length of output: 105
🏁 Script executed:
sed -n '200,280p' bindu/extensions/did/did_agent_extension.pyRepository: GetBindu/Bindu
Length of output: 2967
Avoid creating the private key with inherited ACLs on Windows.
On Windows, write_bytes() creates the file with inherited parent directory ACLs before _harden_windows_key_file_acl() executes. This creates a window where the private key is readable under inherited permissions. Additionally, if icacls fails and raises RuntimeError (line 260), the unprotected key file remains on disk.
The POSIX approach (lines 219–230) demonstrates the safer pattern: use os.open() with 0o600 mode to create the file with restricted permissions atomically from the start. Apply an equivalent approach on Windows—either create the file with restricted security descriptors using Windows APIs, or write to a pre-hardened temporary directory and atomically rename it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/extensions/did/did_agent_extension.py` around lines 213 - 218, The
Windows branch currently uses
self.private_key_path.write_bytes()/self.public_key_path.write_bytes() then
calls _harden_windows_key_file_acl(), which leaves a window where inherited ACLs
expose the key and may leave an unprotected file if hardening fails; change this
to create the files already protected (or write into a pre-hardened temp file
and atomically rename) instead of writing then hardening: replace the
write_bytes + _harden_windows_key_file_acl sequence by either (a) using Windows
API CreateFile with an explicit SECURITY_DESCRIPTOR/SECURITY_ATTRIBUTES to set
restrictive ACLs at creation time for private_key_path (and similarly for public
if needed), or (b) write to a temp file inside a directory whose ACLs are
already hardened and then atomically move/rename the temp file to
self.private_key_path/self.public_key_path; ensure _harden_windows_key_file_acl
remains as a fallback but that any failure cleans up the unprotected temp file
and does not leave an exposed key (referencing private_key_path,
public_key_path, write_bytes, and _harden_windows_key_file_acl to locate the
change).
|
Hey @Co-vengers For PR #450 (Core): For PR #451 (Examples): For PR #452 (Docs): Let me know once you get the base branches updated and the conflicts on #450 resolved. |
|
@chandan-1427 I understand. I'll fix the branches as directed. |
|
Hi @chandan-1427 I resolved the merge conflicts in PR #450 (Core) but when I am trying to change base branch from main to feat(voice-agent)/code-fixes in PR #451 I can't find the branch feat(voice-agent)/code-fixes in the dropdown menu. Do i need to rebase example branch on to core branch instead of main. |
Co-vengers
force-pushed
the
feat(voice-agent)/documentation
branch
from
e703881 to
dc8a54d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/lib/server/database.ts (1)
1-10:⚠️ Potential issue | 🟡 MinorScope mismatch with PR objectives.
The PR is described as documentation-only ("no runtime code changes; no dependency changes; no API changes"), but this file contains substantive runtime changes to the in-memory DB mock (sort option, insertOne id preservation, stream event registry, generic
withTransaction, etc.). Either the PR description should be updated to reflect these runtime changes, or these changes belong in the upstream core/stacked PR (#450) rather than the docs PR (#452).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/database.ts` around lines 1 - 10, PR is marked documentation-only but frontend/src/lib/server/database.ts contains runtime changes (sort option behavior, insertOne id preservation, stream event registry, generic withTransaction) causing a scope mismatch; either revert those runtime edits in this file or update the PR metadata and target the upstream core/stacked PR (`#450`). To fix: if this must stay doc-only, remove or revert modifications to the in-memory DB functions (restore original behavior for insertOne, eliminate added sort option handling, remove stream event registry logic, and revert the generic withTransaction change so no runtime semantics change in database.ts); otherwise update the PR description and retarget branch so these runtime changes are included in the appropriate core/stacked PR (`#450`). Ensure references to the modified symbols (insertOne, withTransaction, any stream registry code, and sort handling) are corrected accordingly.frontend/src/lib/server/config.ts (1)
135-157:⚠️ Potential issue | 🟠 MajorDon’t drop DB-backed public config keys.
keysFromDBis merged intoconfig, but the final filter only keeps keys present inpublicEnvplusPUBLIC_AGENT_BASE_URL. Any otherPUBLIC_*value managed only through the config manager will be visible server-side but missing from the client config.Possible fix
- const publicEnvKeys = Object.keys(publicEnv); - return Object.fromEntries( Object.entries(configWithDefaults).filter( - ([key]) => publicEnvKeys.includes(key) || key === "PUBLIC_AGENT_BASE_URL" + ([key]) => key.startsWith("PUBLIC_") ) ) as Record<PublicConfigKey, string>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/config.ts` around lines 135 - 157, The final client config currently filters configWithDefaults by publicEnv (plus PUBLIC_AGENT_BASE_URL), which drops DB-only PUBLIC_* keys merged earlier via keysFromDB when ConfigManagerEnabled; update the filter logic in the function that builds the return value to also include any keys from this.keysFromDB that startWith("PUBLIC_") so DB-backed public keys are preserved — e.g., compute a set from Object.keys(publicEnv) extended with Object.keys(this.keysFromDB).filter(k => k.startsWith("PUBLIC_")) and use that set when filtering configWithDefaults (symbols to update: keysFromDB, ConfigManagerEnabled, configWithDefaults, publicEnv, PUBLIC_AGENT_BASE_URL).bindu/common/protocol/types.py (1)
150-168:⚠️ Potential issue | 🟠 MajorRemove
bytesrequirement from URI file parts.
FileWithUriinherits fromFileWithBytes, forcing URI variants to require thebytesfield. This contradicts the schema documentation stating content can be "either directly as bytes or as a URI". URI-only file payloads fail validation due to the inherited requiredbytesfield.Recommended fix
`@pydantic.with_config`(ConfigDict(alias_generator=to_camel)) -class FileWithUri(FileWithBytes): +class FileWithUri(TypedDict): """File representation with URI reference.""" uri: Required[str] """The URI of the file.""" + + mimeType: NotRequired[str] + """The MIME type of the file.""" + + name: NotRequired[str] + """The name of the file.""" + + embeddings: NotRequired[list[float]] + """The embeddings of File. <NotPartOfA2A>"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/common/protocol/types.py` around lines 150 - 168, The FileWithUri class should not inherit the required bytes field from FileWithBytes; change FileWithUri so it defines its own fields (keep `@pydantic.with_config`(ConfigDict(alias_generator=to_camel))) with uri: Required[str] and make any bytes field optional (e.g., bytes: Optional[bytes] = None) or omit it entirely, so URI-only variants validate; update references to FileWithUri and FileWithBytes (and FilePart which uses FileWithBytes | FileWithUri) accordingly.frontend/src/routes/conversation/[id]/+page.svelte (1)
247-258:⚠️ Potential issue | 🟠 MajorForward attachments in the direct-agent path.
base64Filesis built at lines 139–148, and the non-agent backend path forwards files at line 272 withfiles: isRetry ? userMessage?.files : base64Files. However, the direct-agent path (lines 247–258) passes onlypromptand task options, silently dropping attachments for Bindu model conversations.The
sendAgentMessagefunction infrontend/src/lib/utils/agentMessageHandler.tsaccepts afilePartsoption (lines 202–206) that matches the structure ofbase64Files. Add it to the options object:Fix
messageUpdatesIterator = sendAgentMessage( prompt ?? '', page.params.id!, { abortSignal: messageUpdatesAbortController.signal, currentTaskId: currentTaskId ?? undefined, taskState: currentTaskState ?? undefined, replyToTaskId: replyToTaskId ?? undefined, + fileParts: isRetry ? userMessage?.files : base64Files, } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/conversation/`[id]/+page.svelte around lines 247 - 258, The direct-agent path is dropping attachments: when useDirectAgentAPI is true the sendAgentMessage call (in +page.svelte, where messageUpdatesIterator is set) currently only passes prompt and task options and omits base64Files; update the options object passed to sendAgentMessage to include fileParts: isRetry ? userMessage?.files : base64Files (matching the non-agent path) so attachments are forwarded to the agent; locate sendAgentMessage usage around messageUpdatesIterator and add the fileParts property to the options.frontend/src/lib/components/chat/ChatInput.svelte (1)
162-169:⚠️ Potential issue | 🟠 MajorAllow file-only submissions.
The new file-part flow still requires non-empty text, so selecting a file and pressing Enter does nothing unless the user also types a message. Include
files.length > 0in the submit guard.Proposed fix
- value.trim() !== "" + (value.trim() !== "" || files.length > 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatInput.svelte` around lines 162 - 169, The Enter-key submit guard in handleKeydown currently blocks submission when value.trim() is empty even if files are attached; update the condition in handleKeydown to allow submit when there are files by replacing the value check with (value.trim() !== "" || files.length > 0), keeping the other guards (event.key === "Enter", !event.shiftKey, !isCompositionOn, !isVirtualKeyboard()) unchanged so Enter will submit when at least one file is present.
♻️ Duplicate comments (1)
bindu/extensions/did/did_agent_extension.py (1)
213-229:⚠️ Potential issue | 🟠 MajorTOCTOU window on Windows is only partially mitigated — same concern as prior review.
The rollback improves the failure path, but the root issue from the previous review still stands:
self.private_key_path.write_bytes(private_pem)materializes the private key on disk under the parent directory's inherited ACLs before_harden_windows_key_file_aclruns. Anyone with inherited read on the key dir has a window to read the key during that interval, regardless of later cleanup.unlink(missing_ok=True)after the fact also does not guarantee data destruction (NTFS may retain the content until the clusters are reused, and USN/Journal/VSS copies are unaffected).For true confidentiality, create the file already restricted, e.g. one of:
- Write to a pre-hardened temp path inside an ACL-locked subdirectory, then
os.replace()to the final name.- Use
win32security/CreateFilewith an explicitSECURITY_ATTRIBUTESowned-by-current-user-only DACL at creation time.- As a weaker fallback: harden the parent directory's ACL first, then write.
Also note the rollback's inner
except OSErroronly logs — ifunlinkfails, the outerraisestill propagates, leaving an unprotected key on disk. Consider escalating that log tologger.errorand including the underlying exception.
🟠 Major comments (21)
examples/pdf_research_agent/pdf_research_agent.py-38-70 (1)
38-70:⚠️ Potential issue | 🟠 Major
ALLOWED_BASE_DIRrestriction likely breaks the documented usage.
ALLOWED_BASE_DIRis hard-coded to the directory containingpdf_research_agent.py, and_read_contentrejects any PDF whose resolved path is not under it. The module docstring and the example print ({"role": "user", "content": "/path/to/paper.pdf"}) still advertise arbitrary paths, so any realistic invocation (e.g.~/Documents/paper.pdf) will now fail with "PDF path is outside the allowed document directory".If the intent is path-traversal hardening, consider either (a) making the allowed directory configurable via env var (e.g.
PDF_RESEARCH_DOCS_DIR) with a sensible default, or (b) usingPath.is_relative_to/resolved_path.relative_to(ALLOWED_BASE_DIR)instead ofos.path.commonpath(which can produce surprising results across drives/roots on Windows and with symlinks). At minimum, update the docstring/example to reflect the new constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 38 - 70, ALLOWED_BASE_DIR being hard-coded breaks passing arbitrary user PDF paths; make the allowed directory configurable and perform a robust containment check in _read_content: read an env var PDF_RESEARCH_DOCS_DIR (fallback to Path(__file__).parent.resolve()) to set ALLOWED_BASE_DIR, resolve both ALLOWED_BASE_DIR and resolved_path with Path.resolve(), then use Path.is_relative_to(resolved_path, ALLOWED_BASE_DIR) (or try resolved_path.relative_to(ALLOWED_BASE_DIR) in a try/except for older Python) to validate containment and still raise DocumentReadError on failure; also update any docstring/example to mention the PDF_RESEARCH_DOCS_DIR override.examples/document-analyzer/document_analyzer.py-76-83 (1)
76-83:⚠️ Potential issue | 🟠 MajorSilent DOCX failures now yield empty documents instead of surfacing as errors.
The new
try/exceptreturns""on failure, which_collect_prompt_and_documentsthen appends toextracted_docsas a valid (empty) document. As a result,if not extracted_docs:is never true for a broken DOCX, theerrorsbranch is skipped, and the agent is asked to analyze an empty string — the user gets a hallucinated/empty analysis instead of the "Failed to process documents" message.Consider re-raising (or raising a dedicated error) so the caller in
_collect_prompt_and_documentsrecords it inerrors, and also addresses the Ruff BLE001 hint by catching a narrower exception where possible.🛠️ Proposed fix
def extract_text_from_docx(file_bytes): """Extract text from docx bytes""" try: doc = Document(io.BytesIO(file_bytes)) - return "\n".join([p.text for p in doc.paragraphs]) - except Exception as e: - logger.error(f"Error extracting DOCX text: {e}") - return "" + except Exception as e: + logger.error("Error extracting DOCX text: %s", e) + raise ValueError(f"Invalid DOCX file: {e}") from e + return "\n".join(p.text for p in doc.paragraphs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` around lines 76 - 83, The function extract_text_from_docx currently swallows all errors and returns an empty string, causing callers like _collect_prompt_and_documents to treat failed DOCX parses as valid empty documents; change it to not return "" on failure but instead re-raise (or raise a dedicated error) so the caller can record it in errors: replace the broad except block in extract_text_from_docx with a narrower exception catch if possible (or except Exception as e), log the error with details via logger.error, then raise a new DocumentProcessingError (or re-raise e) so _collect_prompt_and_documents can handle it; ensure you add the DocumentProcessingError class (or appropriate exception) and update any imports/typing as needed.frontend/src/routes/api/v2/export/+server.ts-129-139 (1)
129-139:⚠️ Potential issue | 🟠 MajorMirror the stream cleanup pattern from
downloadFile.tsto avoid resource leaks.Lines 129–139 reject on invalid chunks but leave the stream flowing and listeners attached. When rejection occurs on line 134, the stream should be destroyed and all handlers removed. Use named handlers with a cleanup function and
once()for terminal events, matching the proven pattern atfrontend/src/lib/server/files/downloadFile.ts:28–57.Suggested fix
const fileBuffer = await new Promise<Buffer>((resolve, reject) => { const chunks: Uint8Array[] = []; - fileStream.on("data", (chunk) => { + const onData = (chunk: unknown) => { if (chunk instanceof Uint8Array) { chunks.push(chunk); return; } + + const err = new Error("Unexpected chunk type from avatar download stream"); + fileStream.destroy(); + cleanup(); - reject(new Error("Unexpected chunk type from avatar download stream")); - }); - fileStream.on("error", (err) => + reject(err); + }; + + const onError = (err: Error | null | undefined) => { + cleanup(); reject(err ?? new Error("Avatar download failed")) - ); - fileStream.on("end", () => resolve(Buffer.concat(chunks))); + }; + + const onEnd = () => { + cleanup(); + resolve(Buffer.concat(chunks)); + }; + + const cleanup = () => { + fileStream.off("data", onData); + fileStream.off("error", onError); + fileStream.off("end", onEnd); + }; + + fileStream.on("data", onData); + fileStream.once("error", onError); + fileStream.once("end", onEnd); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/api/v2/export/`+server.ts around lines 129 - 139, The avatar download promise currently attaches anonymous "data", "error", and "end" handlers to fileStream and rejects on invalid chunks without cleaning up; update the logic to use named handlers for onData, onError, and onEnd, attach terminal handlers with once(), implement a cleanup function that removes all three listeners and calls fileStream.destroy() when rejecting or resolving, and call that cleanup from both onError (and on invalid chunk rejection) and onEnd before resolving the Buffer.concat(chunks); reference the existing fileStream variable and the Buffer.concat(chunks) resolve path to mirror the downloadFile.ts pattern.frontend/src/lib/components/chat/ChatMessage.svelte-70-84 (1)
70-84:⚠️ Potential issue | 🟠 MajorExclude the wrapper itself from the interactive-target guard.
The wrapper element now has
role="button"(line 271). SinceElement.closest()includes ancestors, any click inside the message matches the wrapper and returns early (line 76), preventinghandleContentClick()andisTappedfrom executing. This breaks image lightbox and message toggling. Apply the same guard to theonkeydownhandler to prevent bubbled keyboard events from child controls from toggling the message.Proposed fix
function handleMessageClick(e: MouseEvent) { - const target = e.target as HTMLElement | null; - if ( - target && - target.closest( - "button, a, input, textarea, select, [role='button'], [data-no-toggle]" - ) - ) { + const target = e.target; + const root = e.currentTarget; + if (!(target instanceof Element) || !(root instanceof Element)) { + return; + } + + const interactiveTarget = target.closest( + "button, a, input, textarea, select, [role='button'], [data-no-toggle]" + ); + if (interactiveTarget && interactiveTarget !== root) { return; } handleContentClick(e); - if (!(e.target instanceof HTMLImageElement)) { + if (!(target instanceof HTMLImageElement)) { isTapped = !isTapped; } }onkeydown={(event) => { + if (event.target !== event.currentTarget) { + return; + } if (event.key === "Enter" || event.key === " ") { event.preventDefault(); isTapped = !isTapped; } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatMessage.svelte` around lines 70 - 84, handleMessageClick currently returns early because target.closest matches the wrapper's role="button" ancestor; change the interactive-target guard in handleMessageClick to ignore the wrapper itself by comparing the matched ancestor against e.currentTarget (i.e., only return if closest(...) finds an ancestor that is not the message wrapper/currentTarget). Apply the same guard logic to the onkeydown handler (the keydown toggle handler) so keyboard events originating from child interactive controls (but not the wrapper itself) don't toggle the message; reference handleMessageClick and the onkeydown toggle handler and use e.currentTarget to distinguish the wrapper from ancestor matches.bindu/extensions/did/did_agent_extension.py-251-287 (1)
251-287:⚠️ Potential issue | 🟠 MajorReplace localized group names with well-known SIDs and narrow overly broad exception handlers.
This code will fail on non-English Windows systems:
Localized built-in principal names (critical).
"Users","Authenticated Users", and"Everyone"are localized on non-English Windows (e.g., German "Benutzer", French "Tout le monde"). Theicacls /remove:gcommand will return non-zero, which deletes freshly written keys and breaks agent startup. Use well-known SIDs instead (locale-independent):
- Users →
*S-1-5-32-545- Authenticated Users →
*S-1-5-11- Everyone →
*S-1-1-0Overly broad
except Exception(lines 258, 263). Narrow toexcept OSError:to avoid masking unrelated errors.os.getlogin()specifically raisesOSErrorwhen there is no controlling terminal.User-sourced string in command. Resolve the username to a SID (e.g., via
win32security.LookupAccountNameorwhoami /user) to avoid ambiguity with homonymous domain accounts and exotic characters.Suggested diff
# Remove inherited ACLs and grant full control to current user only. + # Use well-known SIDs so this works on non-English Windows. cmd = [ "icacls", str(path), "/inheritance:r", "/grant:r", f"{username}:F", "/remove:g", - "Users", - "Authenticated Users", - "Everyone", + "*S-1-5-32-545", # BUILTIN\Users + "*S-1-5-11", # NT AUTHORITY\Authenticated Users + "*S-1-1-0", # Everyone ] result = subprocess.run(cmd, capture_output=True, text=True, check=False)And at lines 258 and 263:
except Exception: + except OSError:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/did/did_agent_extension.py` around lines 251 - 287, In _harden_windows_key_file_acl, replace the localized group names ("Users", "Authenticated Users", "Everyone") in the icacls /remove:g args with the locale-independent well-known SIDs (*S-1-5-32-545, *S-1-5-11, *S-1-1-0), narrow the broad except Exception handlers around os.getlogin() and getpass.getuser() to except OSError, and resolve the current username to a SID before calling icacls (e.g., via win32security.LookupAccountName or by parsing whoami /user output) so the command uses the SID for granting/removing ACLs rather than a potentially ambiguous or localized account name. Ensure the RuntimeError messages remain but only raise after these narrower checks and SID resolution fail.frontend/src/lib/server/database.ts-471-553 (1)
471-553:⚠️ Potential issue | 🟠 MajorEvent ordering in
openDownloadStreamis registration-order dependent and fragile.Initial
data/end/erroremissions are scheduled viasetTimeout(..., 0)per event insidescheduleInitialEmit. Because eachsetTimeoutenqueues independently, the execution order depends entirely on the order listeners are registered. A consumer registering.on("end", ...).on("data", ...)will observeendbeforedata, violating readable-stream semantics.While current consumers (downloadFile.ts, export/+server.ts) happen to register listeners in the correct order (
data→error→end), this is coincidental fragility. If a future consumer registers in a different order, it breaks.Additionally, if the file is missing and a consumer only subscribes to
data/endwithouterror, nothing gets scheduled and the stream hangs.Decouple the emission sequence from listener registration order by scheduling the full sequence (
data→endorerror) once on first subscription, regardless of event name:♻️ Sketch
- const scheduleInitialEmit = (event: string) => { - if (event === "error" && !scheduled.error && !file) { - scheduled.error = true; - setTimeout(() => emit("error", new Error("File not found")), 0); - } - if (event === "data" && !scheduled.data && file) { - scheduled.data = true; - setTimeout(() => emit("data", file.data), 0); - } - if (event === "end" && !scheduled.end && file) { - scheduled.end = true; - setTimeout(() => emit("end"), 0); - } - }; + let emissionScheduled = false; + const scheduleInitialEmit = () => { + if (emissionScheduled) return; + emissionScheduled = true; + setTimeout(() => { + if (!file) { + emit("error", new Error("File not found")); + return; + } + emit("data", file.data); + emit("end"); + }, 0); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/database.ts` around lines 471 - 553, The current openDownloadStream schedules each initial event separately in scheduleInitialEmit, making emission order depend on listener registration and leaving no error emitted if only data/end are subscribed; instead, change it to schedule a single initial emission sequence on first subscription: in openDownloadStream (functions: addListener, scheduleInitialEmit, downloadStream) add a single scheduledInitial boolean and when the first listener is added call setTimeout once to run a deterministic sequence that if file exists emits "data" then "end" (in that order) and if file is missing emits "error", remove the per-event setTimeouts and per-event scheduled flags, and ensure destroy prevents further emits; this guarantees correct ordering regardless of registration order and still emits error when file is missing even if only data/end listeners were attached.frontend/src/routes/+layout.svelte-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorRestore the error-store subscription or remove the toast path.
After dropping the
errorstore wiring,currentErroris never assigned, so<Toast>no longer appears for updates fromfrontend/src/lib/stores/errors.ts.Possible fix
- import { onMount, untrack } from "svelte"; + import { onDestroy, onMount, untrack } from "svelte"; + import { error } from "$lib/stores/errors"; @@ let isNavCollapsed = $state(false); let currentError: string | undefined = $state(); + let errorToastTimeout: ReturnType<typeof setTimeout> | undefined; + + const unsubscribeError = error.subscribe((message) => { + currentError = message; + if (errorToastTimeout) clearTimeout(errorToastTimeout); + if (message) { + errorToastTimeout = setTimeout(() => { + currentError = undefined; + error.set(undefined); + }, 5000); + } + }); + + onDestroy(() => { + unsubscribeError(); + if (errorToastTimeout) clearTimeout(errorToastTimeout); + });Also applies to: 105-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/`+layout.svelte at line 4, The currentError used by the <Toast> component is never updated because the subscription to the error store was removed; restore a subscription to the error store (from frontend/src/lib/stores/errors.ts) in the +layout.svelte component (use onMount to subscribe and set currentError, and unsubscribe on destroy) so that updates to the store assign currentError and trigger <Toast>, or alternatively remove the <Toast> path and any references to currentError; make sure to reference the currentError variable, the Toast component, and the error store subscription/unsubscribe logic when applying the fix.frontend/src/lib/services/agent-api.ts-216-220 (1)
216-220:⚠️ Potential issue | 🟠 MajorDon’t expose the unconfigured singleton as the agent base URL.
agentAPIis constructed without a base URL, sogetAgentBaseUrl()returns thehttp://localhost:3773fallback in deployed browsers unless something else mutates this instance. That will route users to their own machine instead of the configured agent service. Initialize the singleton from the public config, or make this helper resolve/acceptPUBLIC_AGENT_BASE_URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/services/agent-api.ts` around lines 216 - 220, The exported singleton agentAPI is created without a configured base URL so getAgentBaseUrl() returns the local fallback; fix by wiring the public config into the singleton or by making the getter read the configured env var: initialize agentAPI using the public config value (e.g., pass PUBLIC_AGENT_BASE_URL into new AgentAPI(...)) or change getAgentBaseUrl() to return the resolved PUBLIC_AGENT_BASE_URL when available and otherwise fall back to agentAPI.getBaseUrl(); update references to the symbols agentAPI, getAgentBaseUrl, AgentAPI and PUBLIC_AGENT_BASE_URL accordingly so deployed clients get the real agent service URL rather than localhost.bindu/extensions/voice/session_manager.py-125-144 (1)
125-144:⚠️ Potential issue | 🟠 MajorRoute session limits and cleanup timing through
app_settings.
max_sessions,session_timeout, and the 30-second cleanup interval are runtime configuration. Hardcoding them here makes voice capacity/timeout tuning drift from deployment settings.Shape of the change
+from bindu.settings import app_settings @@ - def __init__(self, max_sessions: int = 10, session_timeout: int = 300): + def __init__( + self, + max_sessions: int | None = None, + session_timeout: int | None = None, + cleanup_interval: int | None = None, + ): """Initialize in-memory voice session manager limits and cleanup state.""" + max_sessions = max_sessions if max_sessions is not None else app_settings.voice.max_sessions + session_timeout = ( + session_timeout + if session_timeout is not None + else app_settings.voice.session_timeout + ) + self._cleanup_interval = ( + cleanup_interval + if cleanup_interval is not None + else app_settings.voice.cleanup_interval + ) @@ - await asyncio.sleep(30) # check every 30 s + await asyncio.sleep(self._cleanup_interval)As per coding guidelines,
bindu/**/*.py: “Usebindu.settings.app_settingsfor all configuration - never hardcode values such as URLs, ports, timeouts, API keys, or feature flags” and “Settings imports should usefrom bindu.settings import app_settings”.Also applies to: 263-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/session_manager.py` around lines 125 - 144, Use bindu.settings.app_settings for runtime config: import "from bindu.settings import app_settings" and in VoiceSessionManager.__init__ pull default values from app_settings instead of hardcoding—if max_sessions or session_timeout are not passed (or are None) set them from app_settings (e.g. app_settings.VOICE_MAX_SESSIONS and app_settings.VOICE_SESSION_TIMEOUT), then validate them as before; also replace the hardcoded 30-second cleanup interval used when scheduling or running the _cleanup_task (see the cleanup logic around the current _cleanup_task usage at ~lines 263-268) with a value from app_settings (e.g. app_settings.VOICE_CLEANUP_INTERVAL), preserving the existing validation and lock usage.bindu/utils/notifications.py-252-260 (1)
252-260:⚠️ Potential issue | 🟠 MajorReject CR/LF before writing raw HTTPS headers.
The HTTPS path manually interpolates
request_headersinto the wire request. SinceAuthorizationcan come from webhook config, CR/LF in a header value can inject additional headers or corrupt the request.Possible fix
context = ssl.create_default_context() request_headers = {"Host": host_header, "Connection": "close", **headers} request_headers["Content-Length"] = str(len(payload)) + for key, value in request_headers.items(): + if "\r" in key or "\n" in key or "\r" in value or "\n" in value: + raise NotificationDeliveryError( + None, "Invalid push notification header value" + ) try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/notifications.py` around lines 252 - 260, The code builds raw HTTP request lines from request_headers into request_lines and directly concatenates payload, which is vulnerable to header injection if any header value (e.g., Authorization) contains CR/LF; sanitize or reject such values before writing them to the wire by validating every header key/value in request_headers (and host_header) to ensure they contain no '\r' or '\n' (either raise an error or strip/encode them), and only after validation construct request_lines and request_bytes; update the logic that assembles request_headers/request_lines/payload to perform this check (or safe encoding) so CR/LF cannot be injected into the raw request.bindu/server/applications.py-118-120 (1)
118-120:⚠️ Potential issue | 🟠 MajorGuard voice capability detection when
manifestis absent.
manifestis optional inBinduApplication.__init__, butget_voice_extension_from_capabilities()dereferencesmanifest.capabilities. Instantiating withmanifest=Nonecan now fail before the later assertion.🐛 Proposed fix
- x402_ext = get_x402_extension_from_capabilities(manifest) - voice_ext = get_voice_extension_from_capabilities(manifest) + x402_ext = get_x402_extension_from_capabilities(manifest) + voice_ext = ( + get_voice_extension_from_capabilities(manifest) + if manifest is not None + else None + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/applications.py` around lines 118 - 120, The constructor BinduApplication.__init__ currently calls get_voice_extension_from_capabilities(manifest) and get_x402_extension_from_capabilities(manifest) unguarded, which will dereference manifest.capabilities when manifest is None; change the init to first check if manifest is not None (or manifest and manifest.capabilities) and only call get_voice_extension_from_capabilities and get_x402_extension_from_capabilities when manifest is present, otherwise set voice_ext and x402_ext to None (or their safe defaults) so instantiation with manifest=None won't raise before the later assertion.frontend/src/lib/stores/chat.ts-21-29 (1)
21-29:⚠️ Potential issue | 🟠 MajorWiden
FilePart.file.bytestype to matchnormalizeFileBytes()signature.The
normalizeFileBytes()function acceptsstring | ArrayBuffer | Uint8Array | Blob, but theFileParttype restrictsbytestostringonly. This forces a cast at line 59, which obscures the type mismatch and makes the actual capability of the function invisible to the type system.🐛 Proposed type fix
type FilePart = { kind: 'file'; fileId?: string; file?: { - bytes?: string; + bytes?: string | ArrayBuffer | Uint8Array | Blob; mimeType?: string; name?: string; }; metadata?: Record<string, unknown>; };Then remove the cast at line 59:
- const normalizedBytes = await normalizeFileBytes(file.bytes as string | ArrayBuffer | Uint8Array | Blob); + const normalizedBytes = await normalizeFileBytes(file.bytes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/stores/chat.ts` around lines 21 - 29, The FilePart type restricts file.bytes to string, but normalizeFileBytes accepts string | ArrayBuffer | Uint8Array | Blob; update the FilePart definition (the FilePart.file.bytes field) to union type string | ArrayBuffer | Uint8Array | Blob to reflect actual inputs, then remove the unnecessary cast when calling normalizeFileBytes() so the call uses the widened type directly; references: FilePart, FilePart.file.bytes, and normalizeFileBytes().bindu/server/endpoints/voice_endpoints.py-388-397 (1)
388-397:⚠️ Potential issue | 🟠 MajorDo not let preflight block a configured TTS fallback.
create_tts_service()can fall back when the primary provider is missing credentials or dependencies, but this preflight rejects the request before fallback can run. For example,tts_provider="elevenlabs"withoutVOICE__TTS_API_KEYandtts_fallback_provider="piper"still fails here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 388 - 397, The preflight currently fails when the configured primary tts_provider lacks credentials even if a valid fallback exists; update the validation in voice_endpoints.py to allow a configured fallback to satisfy requirements: instead of immediately returning errors for missing primary credentials (e.g., when tts_provider == "elevenlabs" and no app_settings.voice.tts_api_key), check app_settings.voice.tts_fallback_provider and validate that at least one provider (primary or fallback) is supported and has required credentials/region; reference tts_provider, tts_fallback_provider, app_settings.voice.*, and create_tts_service to mirror the runtime fallback logic so preflight only errors when neither primary nor fallback can be constructed.frontend/src/lib/services/voice-client.ts-310-437 (1)
310-437:⚠️ Potential issue | 🟠 MajorHarden audio setup cleanup and flush the final PCM buffer.
If
resume()/addModule()/node setup fails, the mic stream andAudioContextcan remain open. Also, the worklet never flushes its partial 4096-sample buffer onstopAudioStreaming(), so the end of a turn can be lost. Finally, verifyaudioContext.sampleRate; browsers may not honor16000.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/services/voice-client.ts` around lines 310 - 437, The audio setup can leak MediaStream/AudioContext on failures and never flush the worklet's partial buffer at stop; wrap the setup in try/catch/finally to always stop stream tracks and close audioContext if any step (navigator.mediaDevices.getUserMedia, audioContext.resume, audioContext.audioWorklet.addModule, node creation) fails, and ensure URL.revokeObjectURL runs (already in finally) but also perform cleanup there if audioContext or stream exist; modify the inline PCM16Processor (class PCM16Processor / registerProcessor 'pcm16-processor') to handle a port message like {cmd:'flush'} that calls flushBuffer(), and when tearing down (stopAudioStreaming / cleanupAudioStreaming / before closing audioContext) postMessage({cmd:'flush'}) to workletNode.port and await a short confirmation or give a brief timeout to ensure the partial buffer is sent; finally check actual audioContext.sampleRate after creation and, if it differs from desiredSampleRate (16000), mark that you must resample audio in the processor or perform client-side resampling before sending so sample-rate mismatches are handled.frontend/src/lib/utils/agentMessageHandler.ts-122-141 (1)
122-141:⚠️ Potential issue | 🟠 MajorAvoid byte-by-byte string concatenation for attachment base64 conversion.
For multi-MB
Blob/ArrayBufferattachments, this creates many intermediate strings and can freeze the UI. Build chunks and join them beforebtoa.⚡ Suggested conversion improvement
function base64FromBytes(bytes: Uint8Array): string { - let binary = ''; - for (let i = 0; i < bytes.length; i++) { - binary += String.fromCharCode(bytes[i] ?? 0); - } - return btoa(binary); + const chunkSize = 0x8000; + const chunks: string[] = []; + for (let i = 0; i < bytes.length; i += chunkSize) { + chunks.push(String.fromCharCode(...bytes.subarray(i, i + chunkSize))); + } + return btoa(chunks.join('')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/utils/agentMessageHandler.ts` around lines 122 - 141, The current base64 conversion in base64FromBytes and normalizeFileValue concatenates characters one-by-one which causes many intermediate strings and UI freezes for large Blobs/ArrayBuffers; fix it by converting the Uint8Array in chunks (e.g., ~32KB or 0x8000 bytes) using String.fromCharCode on each chunk, push each chunk string into an array, join the array once to produce the full binary string, then call btoa on the joined string; update base64FromBytes to implement chunked processing and keep normalizeFileValue's Blob/ArrayBuffer branches to delegate to the new chunked base64FromBytes implementation.bindu/server/endpoints/voice_endpoints.py-960-981 (1)
960-981:⚠️ Potential issue | 🟠 MajorExit on normal reader completion, not only exceptions.
asyncio.FIRST_EXCEPTIONdoes not return when the reader finishes normally from a disconnect/stopframe while the runner stays pending. That can keep the handler alive untilsession_timeoutand report a timeout instead of cleaning up immediately.🔁 Suggested wait mode fix
done, pending = await asyncio.wait( {runner_task, reader_task}, - return_when=asyncio.FIRST_EXCEPTION, + return_when=asyncio.FIRST_COMPLETED, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 960 - 981, The wait currently uses asyncio.FIRST_EXCEPTION which won't return when reader_task completes normally; change the asyncio.wait call in the handler that awaits {runner_task, reader_task} to use return_when=asyncio.FIRST_COMPLETED (keeping the surrounding timeout) so the function exits immediately when reader_task finishes (e.g., disconnect/stop frame); after the wait, inspect done for reader_task and runner_task, cancel the other if still pending, and raise any exceptions found (reader_task.exception() / runner_task.exception()) and still await pending via asyncio.gather(..., return_exceptions=True) to ensure clean shutdown.bindu/server/endpoints/voice_endpoints.py-449-459 (1)
449-459:⚠️ Potential issue | 🟠 MajorValidate JSON control frames are objects before calling
.get().A client can send valid JSON like
[],"x", ornull;payload.get(...)then raises and gets reported as a 1011 pipeline error instead of a controlled malformed-frame close.🛡️ Suggested validation
try: - payload = json.loads(text) + payload = json.loads(text) except json.JSONDecodeError: await _send_error_and_close( websocket, "Malformed JSON control frame", send_lock=send_lock, ) return + if not isinstance(payload, dict): + await _send_error_and_close( + websocket, + "Malformed JSON control frame", + send_lock=send_lock, + ) + return frame_type = payload.get("type")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 449 - 459, The code assumes payload is a dict after json.loads and calls payload.get(...), which raises for JSON values like [], "x", or null; fix by validating payload is a mapping before using .get: after json.loads(text) check isinstance(payload, dict) (or collections.abc.Mapping) and if not call _send_error_and_close(websocket, "Malformed JSON control frame", send_lock=send_lock) and return; then safely read frame_type = payload.get("type").frontend/src/lib/components/chat/ChatWindow.svelte-165-184 (1)
165-184:⚠️ Potential issue | 🟠 MajorForward task routing options when sending agent messages.
sendAgentMessagesupportscurrentTaskId,taskState, andreplyToTaskId, but this call only passesfileParts. That drops explicit replies and continuation ofinput-requiredtasks.🔁 Suggested routing fix
const contextId = agentContextId ?? undefined; + const latestTaskMetadata = [...messages] + .reverse() + .find((message) => message.taskMetadata)?.taskMetadata; let success = false; try { let streamed = ""; - for await (const update of sendAgentMessage(message, contextId, { fileParts })) { + for await (const update of sendAgentMessage(message, contextId, { + fileParts, + currentTaskId: latestTaskMetadata?.taskId, + taskState: latestTaskMetadata?.status, + replyToTaskId: replyToTaskId ?? undefined, + })) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatWindow.svelte` around lines 165 - 184, The submit function is dropping task-routing options when calling sendAgentMessage (only fileParts is forwarded), which loses explicit replies and continuation of input-required tasks; update the sendAgentMessage invocation inside submit to include currentTaskId, taskState, and replyToTaskId in the options object (e.g., sendAgentMessage(message, contextId, { fileParts, currentTaskId, taskState, replyToTaskId })), and ensure those symbols (currentTaskId, taskState, replyToTaskId) are read from the component props/state and their types propagated so the call compiles and preserves routing behavior.frontend/src/lib/components/chat/ChatWindow.svelte-442-453 (1)
442-453:⚠️ Potential issue | 🟠 MajorMake the active voice button stop the current session.
When
$voiceSessionIdis set,toggleVoiceSession()returns immediately, so the active “End voice call” button cannot end the call.🎙️ Suggested behavior fix
import { startVoiceSession, + stopVoiceSession, voiceSessionId, voiceError, } from "$lib/stores/voice"; @@ async function toggleVoiceSession() { if ($voiceSessionId) { + await stopVoiceSession(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/chat/ChatWindow.svelte` around lines 442 - 453, toggleVoiceSession currently returns when $voiceSessionId is set, preventing the active "End voice call" button from ending the call; change it so when $voiceSessionId is truthy it calls the corresponding stop function (e.g., stopVoiceSession()) in a try/catch, clears any $error, and returns only after the stop completes; otherwise, keep the existing logic to start a session by calling startVoiceSession(agentContextId ?? undefined) with error handling that sets $error on failure.frontend/src/lib/stores/voice.ts-103-173 (1)
103-173:⚠️ Potential issue | 🟠 MajorReset
isStartingwhen cancelling an in-flight start.
endVoiceSession()invalidatesstartTokenCounterwhilestartVoiceSession()is pending. The pending start then hits the stale-token return path, but itsfinallyskipsisStarting = falsebecausestartToken !== startTokenCounter. After that, future starts always throw"A voice session is already starting".🐛 Proposed fix
export async function endVoiceSession(): Promise<void> { startTokenCounter += 1; + isStarting = false; const active = client; client = null;Also applies to: 176-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/stores/voice.ts` around lines 103 - 173, The pending start can be cancelled by endVoiceSession (which increments startTokenCounter), causing early returns in startVoiceSession's stale-token checks that skip resetting isStarting; update startVoiceSession so whenever you take the stale-token return path (both after await localClient.startSession(...) and after await localClient.connect(...)) you first set isStarting = false before calling localClient.stopSession() and returning; ensure the same change is applied at the second stale-token check (the one around lines 176-178) so isStarting is always cleared when a start is aborted.bindu/extensions/voice/agent_bridge.py-34-39 (1)
34-39: 🛠️ Refactor suggestion | 🟠 MajorMove voice defaults into
app_settings.This file defines local config defaults for timeouts/history/fallback text and then falls back to them via
getattr. Those values should live in settings so runtime behavior is controlled from one configuration surface. As per coding guidelines, "bindu/**/*.py: Usebindu.settings.app_settingsfor all configuration - never hardcode values such as URLs, ports, timeouts, API keys, or feature flags" and "NEVER create local config constants - useapp_settingsfrombindu.settings."Also applies to: 83-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/agent_bridge.py` around lines 34 - 39, Local config constants MAX_HISTORY_TURNS and the DEFAULT_* timeout/text values must be moved into bindu.settings.app_settings and the module should stop hardcoding/falling back to locals; add new app_settings attributes (e.g., voice_max_history_turns, voice_first_token_timeout_seconds, voice_total_response_timeout_seconds, voice_cancellation_grace_seconds, voice_thinking_text, voice_timeout_fallback_text), update agent_bridge.py to import app_settings and replace any use of the local constants or getattr(..., 'SOME_CONST', <local>) with direct reads from those new app_settings attributes, and remove the now-unneeded local constant definitions (also apply the same replacement for the other constants referenced later in the file around the DEFAULT_* group).
🧹 Nitpick comments (4)
examples/pdf_research_agent/pdf_research_agent.py (1)
83-92: Address Ruff B904/RUF010 in the PDF read error paths.Minor cleanup to chain exception context and drop the redundant
str()conversion flagged by static analysis:🛠️ Proposed fix
- except ImportError: - raise DocumentReadError( - f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" - ) + except ImportError as exc: + raise DocumentReadError( + f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" + ) from exc except Exception as e: if isinstance(e, DocumentReadError): raise raise DocumentReadError( - f"Error reading PDF '{resolved_path}': {str(e)}" + f"Error reading PDF '{resolved_path}': {e}" ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 83 - 92, Change the ImportError handler to capture the original exception and chain it, and remove the redundant str() conversion in the generic exception handler: use "except ImportError as e:" and raise DocumentReadError(f\"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf\") from e, and in the other handler keep "except Exception as e:" but format the message using {e} instead of {str(e)} (keeping the existing "raise ... from e" to preserve exception context); reference symbols: DocumentReadError and resolved_path.tests/unit/utils/test_notifications.py (1)
244-252: Return a concrete resolved IP in_post_oncetests.
patch.object(service, "_resolve_and_validate_destination")returns aMagicMock, so_post_onceskips thetarget_urlrewrite guarded byisinstance(destination_ip, str). These tests don’t currently verify the new IP-pinning behavior.Use a concrete return value and assert the request target/Host header.
Example adjustment
- with patch.object(service, "_resolve_and_validate_destination"): + with patch.object( + service, + "_resolve_and_validate_destination", + return_value="93.184.216.34", + ): with patch( "bindu.utils.notifications.request.urlopen", return_value=context_manager, - ): + ) as urlopen: with pytest.raises( NotificationDeliveryError, match="Unexpected status code: 500" ) as exc_info: service._post_once("http://example.com/webhook", {}, b"{}") assert exc_info.value.status == 500 + req = urlopen.call_args.args[0] + assert req.full_url == "http://93.184.216.34/webhook" + assert req.get_header("Host") == "example.com"Also applies to: 267-275, 283-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/test_notifications.py` around lines 244 - 252, The tests patching service._resolve_and_validate_destination currently leave it as a MagicMock so _post_once never exercises the IP-pinning branch; update those tests (including the blocks around lines with similar patterns) to have _resolve_and_validate_destination return a concrete IP string (e.g., "1.2.3.4"), call service._post_once("http://example.com/webhook", ..., b"{}"), and assert that the outgoing request target/Host header was rewritten to the original host ("example.com") while the request was sent to the resolved IP (the returned "1.2.3.4") so the new IP-pinning behavior is verified (referencing service._post_once, _resolve_and_validate_destination, target_url and Host header assertions).bindu/extensions/voice/session_factory.py (1)
21-24: Use the absolute import for the Redis session manager.♻️ Suggested import change
- from .redis_session_manager import RedisVoiceSessionManager + from bindu.extensions.voice.redis_session_manager import RedisVoiceSessionManagerAs per coding guidelines, use absolute imports with format
from bindu.server.storage import StorageBackendinstead of relative imports across packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/session_factory.py` around lines 21 - 24, The relative import of RedisVoiceSessionManager in session_factory.py should be replaced with an absolute import per project guidelines; locate the try/except block that imports RedisVoiceSessionManager and change the import to the absolute path (e.g., import RedisVoiceSessionManager from bindu.extensions.voice.redis_session_manager or the canonical package path used in the project), keeping the same try/except behavior and symbol name so RedisVoiceSessionManager is still conditionally imported and referenced elsewhere in the module.bindu/server/endpoints/voice_endpoints.py (1)
168-175: Avoid per-audio-frameinfologs.These paths can run dozens of times per second per session, which can flood logs and add avoidable overhead. Downgrade to
debugor sample the logs.Also applies to: 499-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/endpoints/voice_endpoints.py` around lines 168 - 175, The receive method in the FilteredWebSocket (async def receive) is emitting an info-level log for every audio frame; change it to a debug-level log (or implement sampling/conditional logging using logger.isEnabledFor(logging.DEBUG)) to avoid flooding logs — update the logger.info call in receive to logger.debug and apply the same change to the similar logging at the other location referenced (lines ~499-504) so per-frame logs are downgraded or sampled consistently.
| self._conversation_history.append({"role": "user", "content": cleaned}) | ||
| self._trim_history() | ||
|
|
||
| response_text = await self._invoke_agent_streaming(emit_frames=emit_frames) |
There was a problem hiding this comment.
Pass the user utterance to manifest.run, not the history list.
manifest.run is wired from bindu/penguin/manifest.py, where the first positional argument is resolved as input_msg: str. Passing list(self._conversation_history) changes the agent input type and can break existing text agents or prompt formatting. Keep history in session_context if needed, but pass the current utterance as the input message.
🐛 Proposed fix
- response_text = await self._invoke_agent_streaming(emit_frames=emit_frames)
+ response_text = await self._invoke_agent_streaming(
+ cleaned,
+ emit_frames=emit_frames,
+ )
@@
- response_text = await self._invoke_agent_streaming(emit_frames=True)
+ response_text = await self._invoke_agent_streaming(
+ user_text,
+ emit_frames=True,
+ )
@@
- async def _invoke_agent_streaming(self, *, emit_frames: bool) -> str | None:
+ async def _invoke_agent_streaming(
+ self,
+ user_text: str,
+ *,
+ emit_frames: bool,
+ ) -> str | None:
@@
- raw = self._manifest_run(list(self._conversation_history))
+ raw = self._manifest_run(
+ user_text,
+ session_context={
+ "context_id": self._context_id,
+ "conversation_history": list(self._conversation_history),
+ },
+ )Also applies to: 222-222, 245-245, 287-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/extensions/voice/agent_bridge.py` at line 186, The call chain currently
passes list(self._conversation_history) into manifest.run which expects the
current utterance as its first positional arg (input_msg: str); change the
invocation so manifest.run receives the user's latest utterance (the variable
holding the current user message) instead of the history list, and keep the full
history inside session_context (or pass it as a named session/context param) so
prompt formatting stays intact; update the calls routed through
self._invoke_agent_streaming and any direct manifest.run calls (references:
manifest.run, self._invoke_agent_streaming, self._conversation_history,
session_context) at the other sites flagged (around the other call locations) to
follow the same pattern.
| def _resolve_and_check_ip(hostname: str, port: int) -> str: | ||
| """Resolve hostname to an IP address and verify it is not in a blocked range.""" | ||
| try: | ||
| resolved_ip = str(socket.getaddrinfo(hostname, None)[0][4][0]) | ||
| addr = ipaddress.ip_address(resolved_ip) | ||
| infos = socket.getaddrinfo(hostname, port, type=socket.SOCK_STREAM) | ||
| addrs = [ipaddress.ip_address(info[4][0]) for info in infos] | ||
| except (socket.gaierror, ValueError) as exc: | ||
| raise ValueError( | ||
| f"Push notification URL hostname could not be resolved: {exc}" | ||
| ) from exc | ||
|
|
||
| for blocked in _BLOCKED_NETWORKS: | ||
| if addr in blocked: | ||
| raise ValueError( | ||
| f"Push notification URL resolves to a blocked address range " | ||
| f"({addr} is in {blocked}). Internal addresses are not allowed." | ||
| ) | ||
| for addr in addrs: | ||
| for blocked in _BLOCKED_NETWORKS: | ||
| if addr in blocked: | ||
| raise ValueError( | ||
| f"Push notification URL resolves to a blocked address range " | ||
| f"({addr} is in {blocked}). Internal addresses are not allowed." | ||
| ) | ||
|
|
||
| return resolved_ip | ||
| return str(addrs[0]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrates that IPv4-mapped IPv6 loopback is not contained in the current IPv4 loopback CIDR check.
python - <<'PY'
import ipaddress
addr = ipaddress.ip_address("::ffff:127.0.0.1")
blocked = [
ipaddress.ip_network("127.0.0.0/8"),
ipaddress.ip_network("10.0.0.0/8"),
ipaddress.ip_network("172.16.0.0/12"),
ipaddress.ip_network("192.168.0.0/16"),
]
print("direct_match:", any(addr in network for network in blocked))
print("mapped:", addr.ipv4_mapped)
print("mapped_match:", any(addr.ipv4_mapped in network for network in blocked))
PYRepository: GetBindu/Bindu
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Read the relevant sections of bindu/utils/notifications.py
cat -n bindu/utils/notifications.py | sed -n '35,65p'Repository: GetBindu/Bindu
Length of output: 1425
🏁 Script executed:
#!/bin/bash
# Read the second location mentioned in the review
cat -n bindu/utils/notifications.py | sed -n '135,170p'Repository: GetBindu/Bindu
Length of output: 1803
Normalize IPv4-mapped IPv6 addresses before blocked-range checks.
ipaddress.ip_address("::ffff:127.0.0.1") is an IPv6 address and will not match the IPv4 loopback CIDR entries. Normalize addr.ipv4_mapped before checking blocked networks, otherwise IPv4-mapped loopback/private destinations can bypass the SSRF guard.
Possible fix
+def _normalize_ip(addr: ipaddress.IPv4Address | ipaddress.IPv6Address) -> ipaddress.IPv4Address | ipaddress.IPv6Address:
+ if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None:
+ return addr.ipv4_mapped
+ return addr
+
+
+def _is_blocked_ip(addr: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool:
+ normalized = _normalize_ip(addr)
+ return any(normalized in blocked for blocked in _BLOCKED_NETWORKS)
+
@@
- for addr in addrs:
- for blocked in _BLOCKED_NETWORKS:
- if addr in blocked:
- raise ValueError(
- f"Push notification URL resolves to a blocked address range "
- f"({addr} is in {blocked}). Internal addresses are not allowed."
- )
+ for addr in addrs:
+ if _is_blocked_ip(addr):
+ raise ValueError(
+ f"Push notification URL resolves to a blocked address range "
+ f"({addr}). Internal addresses are not allowed."
+ )
@@
- for blocked in _BLOCKED_NETWORKS:
- if addr in blocked:
- raise ValueError(
- f"Push notification URL resolves to a blocked address range "
- f"({addr} is in {blocked}). Internal addresses are not allowed."
- )
+ if _is_blocked_ip(addr):
+ raise ValueError(
+ f"Push notification URL resolves to a blocked address range "
+ f"({addr}). Internal addresses are not allowed."
+ )Applies to lines 40–58 and 143–167.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/utils/notifications.py` around lines 40 - 58, The code in
_resolve_and_check_ip (and the similar block at lines 143–167) does not
normalize IPv4-mapped IPv6 addresses, so addresses like ::ffff:127.0.0.1 bypass
CIDR checks; update the logic to, for each ipaddress.ip_address object (e.g.,
variable addr), replace it with addr.ipv4_mapped if addr.version == 6 and
addr.ipv4_mapped is not None before comparing against _BLOCKED_NETWORKS and
before returning the resolved IP string; ensure the returned value uses the
normalized IPv4 form when mapped and keep using the same exception
types/messages on failure.
| req = request.Request(target_url, data=payload, method="POST") | ||
| req.add_header("Host", host_header) | ||
| for key, value in headers.items(): | ||
| req.add_header(key, value) | ||
|
|
||
| try: | ||
| # URL scheme is validated in validate_config() to only allow http/https | ||
| with request.urlopen(req, timeout=self.timeout) as response: # nosec B310 | ||
| status = response.getcode() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies the default urllib opener includes redirect handling.
python - <<'PY'
from urllib import request
opener = request.build_opener()
print([type(handler).__name__ for handler in opener.handlers if "Redirect" in type(handler).__name__])
# Expected: includes HTTPRedirectHandler, meaning urlopen/openers follow redirects unless disabled.
PYRepository: GetBindu/Bindu
Length of output: 81
🏁 Script executed:
# First, let's locate and examine the notifications.py file
fd -t f notifications.py bindu/Repository: GetBindu/Bindu
Length of output: 86
🏁 Script executed:
# Once we have the file, read the relevant section and surrounding context
head -310 bindu/utils/notifications.py | tail -50Repository: GetBindu/Bindu
Length of output: 2264
🏁 Script executed:
# Find the validate_config method to understand existing protections
rg -n "validate_config" bindu/utils/notifications.py -B 2 -A 10Repository: GetBindu/Bindu
Length of output: 1718
🏁 Script executed:
# Search for webhook/URL handling and SSRF-related code
rg -n "urlopen|redirect|ssrf|SSRF" bindu/utils/notifications.py -i -B 2 -A 2Repository: GetBindu/Bindu
Length of output: 1622
🏁 Script executed:
# Check what triggers the webhook POST (understand the threat model)
rg -n "def.*webhook|send.*webhook" bindu/ -i -B 1 -A 5 --type pyRepository: GetBindu/Bindu
Length of output: 9751
🏁 Script executed:
# Check if there's existing redirect handling in the codebase
rg -n "NoRedirectHandler|HTTPRedirectHandler|redirect" bindu/utils/notifications.py -iRepository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Understand which code path is used (socket vs urlopen)
# Look at the full _post_with_retry method to see the logic flow
sed -n '200,350p' bindu/utils/notifications.pyRepository: GetBindu/Bindu
Length of output: 5932
Disable redirects for HTTP webhooks to prevent SSRF bypass.
urllib.request.urlopen follows HTTP redirects by default. The initial hostname is validated and replaced with a resolved IP address to prevent SSRF, but a 3xx redirect response bypasses this protection entirely—the redirect target is not validated against the blocked IP ranges. A public webhook can redirect the POST to an internal address (e.g., cloud metadata endpoints, private services).
This vulnerability only affects the HTTP code path (lines 267–318); the HTTPS path uses raw socket I/O and does not follow redirects.
Possible fix
+ class NoRedirectHandler(request.HTTPRedirectHandler):
+ def redirect_request(self, req, fp, code, msg, headers, newurl):
+ return None
+
+ opener = request.build_opener(NoRedirectHandler)
+
try:
# URL scheme is validated in validate_config() to only allow http/https
- with request.urlopen(req, timeout=self.timeout) as response: # nosec B310
+ with opener.open(req, timeout=self.timeout) as response:
status = response.getcode()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| req = request.Request(target_url, data=payload, method="POST") | |
| req.add_header("Host", host_header) | |
| for key, value in headers.items(): | |
| req.add_header(key, value) | |
| try: | |
| # URL scheme is validated in validate_config() to only allow http/https | |
| with request.urlopen(req, timeout=self.timeout) as response: # nosec B310 | |
| status = response.getcode() | |
| req = request.Request(target_url, data=payload, method="POST") | |
| req.add_header("Host", host_header) | |
| for key, value in headers.items(): | |
| req.add_header(key, value) | |
| class NoRedirectHandler(request.HTTPRedirectHandler): | |
| def redirect_request(self, req, fp, code, msg, headers, newurl): | |
| return None | |
| opener = request.build_opener(NoRedirectHandler) | |
| try: | |
| # URL scheme is validated in validate_config() to only allow http/https | |
| with opener.open(req, timeout=self.timeout) as response: # nosec B310 | |
| status = response.getcode() |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 296-296: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 303-303: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/utils/notifications.py` around lines 296 - 304, The webhook HTTP path
currently uses request.urlopen(req, timeout=self.timeout) which follows
redirects and can bypass the hostname/IP validation; fix by creating and using a
urllib opener that disables redirects (e.g., a custom HTTPRedirectHandler that
returns None or raises on redirect) and call opener.open(req,
timeout=self.timeout) instead of request.urlopen; keep using the existing req,
target_url, headers, and self.timeout variables and ensure you treat any 3xx
response as a failure/error so redirects cannot forward the POST to an
unvalidated host.
| async insertOne( | ||
| doc: Partial<T> | T | ||
| ): Promise<{ insertedId: ObjectId; acknowledged: boolean }> { | ||
| const source = doc as Record<string, unknown>; | ||
| let id: ObjectId; | ||
| if ("_id" in source && source._id) { | ||
| if (source._id instanceof ObjectId) { | ||
| id = source._id; | ||
| } else { | ||
| try { | ||
| id = new ObjectId(String(source._id)); | ||
| } catch { | ||
| id = new ObjectId(); | ||
| } | ||
| } | ||
| } else { | ||
| id = new ObjectId(); | ||
| } | ||
| const docWithId = { ...source, _id: id } as T; | ||
| this.data.set(id.toString(), docWithId); | ||
| return { insertedId: id, acknowledged: true }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for callers that rely on duplicate-key errors from insertOne.
rg -nP -C3 '\binsertOne\s*\(' --type=ts -g '!**/database.ts'
rg -nP -C2 '11000|E11000|DuplicateKey' --type=tsRepository: GetBindu/Bindu
Length of output: 20564
🏁 Script executed:
#!/bin/bash
# Examine the migration lock code that uses insertOne and checks acknowledged
cat -n frontend/src/lib/migrations/lock.ts | head -30
# Check the imports in database.ts to understand ObjectId sources
head -20 frontend/src/lib/server/database.ts | grep -n "import\|ObjectId"
# Search for any code that explicitly checks the acknowledged flag or handles duplicates
rg -n "acknowledged" --type=tsRepository: GetBindu/Bindu
Length of output: 3089
🏁 Script executed:
#!/bin/bash
# Search for index definitions or constraint setup
rg -n "semaphores" --type=ts | grep -i "index\|unique\|constraint"
# Check if there's any collection initialization code
rg -n "createIndex\|createCollection" --type=ts -A 2 -B 2
# Look for schema or type definitions that might hint at constraints
cat -n frontend/src/lib/types/Semaphore.ts 2>/dev/null || echo "File not found"Repository: GetBindu/Bindu
Length of output: 935
insertOne silently overwrites on duplicate _id — breaks the lock mechanism.
The insertOne method at line 113 silently overwrites an existing document with the same _id instead of throwing a duplicate-key error. This breaks the semaphore/lock mechanism:
frontend/src/lib/migrations/lock.ts(line 22–24) expects a unique-key violation exception when acquiring a lock on a duplicatekey- The semaphores collection has a unique index on the
keyfield (defined inmigrations.spec.tsline 16), but the mock database does not enforce it - If
acquireLock(key)is called twice with the same key, both calls succeed and return different lock IDs; the second overwrites the first without throwing
The lock mechanism will silently fail under concurrent access or repeated lock attempts on the same key. Implement constraint enforcement to throw a mock duplicate-key error when inserting documents that would violate a unique index.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/lib/server/database.ts` around lines 94 - 115, The mock
insertOne in insertOne should not silently overwrite existing entries: before
setting this.data, check whether the generated or provided _id (id.toString())
already exists and, if so, throw a duplicate-key error (simulate Mongo's error
with code 11000 and a message naming the conflicting _id); additionally enforce
any unique-indexed fields used by tests (e.g., the semaphores collection's "key"
field) by scanning this.data for an existing document with the same unique field
value and throwing the same 11000-style error when found; update insertOne (and
any helpers it uses) to perform these existence checks using
this.data.get()/iteration and only set this.data.set(...) when no conflict is
detected.
dc8a54d to
5bac2a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (1)
bindu/extensions/voice/agent_bridge.py (1)
255-260:⚠️ Potential issue | 🔴 Critical
manifest.runstill receives the full history list instead of the current user utterance.
_invoke_agent_streamingcallsself._manifest_run(list(self._conversation_history)), butmanifest.runinbindu/penguin/manifest.pyresolves the first positional argument asinput_msg: str. Passing a list will either break existing text-mode handlers or force them to defensively sniff the argument type. The current utterance should be passed as the input, with history provided via a session/context kwarg (e.g.,session_context={"context_id": self._context_id, "conversation_history": list(self._conversation_history)}). Also applies to the call site in_invoke_and_emitvia_handle_user_utterance(line 217) and the convenience path inprocess_transcription(line 156).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/agent_bridge.py` around lines 255 - 260, The call sites are passing the entire conversation list as the first positional arg to manifest.run; change them to pass the current user utterance as the first arg and provide history via a session/context kwarg: update _invoke_agent_streaming to call self._manifest_run(current_utterance, session_context={"context_id": self._context_id, "conversation_history": list(self._conversation_history)}) (use the variable holding the current user utterance), and make the same change for the other sites invoking self._manifest_run in _invoke_and_emit (via _handle_user_utterance) and in process_transcription so manifest.run’s first positional param (input_msg: str) receives a string and history is supplied via session_context.
🧹 Nitpick comments (4)
bindu/settings.py (1)
1126-1133: Silent clamp ofsession_token_ttlmasks misconfiguration.The intent to fail-safe at startup is reasonable, but the clamp happens without any observability, so an operator setting
VOICE__SESSION_TOKEN_TTL=3600alongsideVOICE__SESSION_TIMEOUT=300will never know the TTL was silently reduced to 300. Please log a warning so this is traceable in deployment logs.♻️ Proposed refactor
`@model_validator`(mode="after") def _validate_session_token_ttl(self) -> "VoiceSettings": # Enforce that tokens never outlive sessions. # If misconfigured via env (e.g. TTL > timeout), clamp TTL to timeout # to fail safe without preventing the server from starting. if self.session_token_ttl > self.session_timeout: + import warnings + warnings.warn( + f"VOICE__SESSION_TOKEN_TTL ({self.session_token_ttl}) exceeds " + f"VOICE__SESSION_TIMEOUT ({self.session_timeout}); clamping to session_timeout.", + stacklevel=2, + ) self.session_token_ttl = self.session_timeout return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/settings.py` around lines 1126 - 1133, The _validate_session_token_ttl model_validator on VoiceSettings silently clamps session_token_ttl to session_timeout when TTL > timeout; change it to emit a warning when this clamp occurs so operators can observe misconfiguration—inside _validate_session_token_ttl, detect the condition (self.session_token_ttl > self.session_timeout), log a warning that includes both the configured TTL and the effective clamped value and the field names (session_token_ttl, session_timeout, VoiceSettings), then set session_token_ttl = session_timeout and return self; use the project logging facility (or Python's logging.getLogger(__name__)) rather than printing.bindu/extensions/voice/service_factory.py (1)
42-64: Minor: constant-stringgetattrcalls flagged by Ruff B009.All four
getattr(module, "ClassName")sites (lines 45, 111, 148, 196) can be replaced with direct attribute access since the attribute name is a literal. Wrap intry/except AttributeError(already in the surroundingexcept) rather than usinggetattrfor "safety." Purely stylistic — feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/service_factory.py` around lines 42 - 64, Replace the literal getattr calls with direct attribute access and rely on the existing try/except to catch AttributeError: e.g., in the STT branch use deepgram_module.DeepgramSTTService instead of getattr(deepgram_module, "DeepgramSTTService") (keep the surrounding try/except ImportError/AttributeError), and make the same change for the three other literal getattr sites referenced in the review so their module attributes are accessed directly rather than via getattr.examples/voice-agent/main.py (1)
35-44: Minor: non-numeric env vars will crash module import with an unhelpful error.
int(os.environ.get("OPENROUTER_MEMORY_TURNS", "4") or "4")will raiseValueErrorif someone setsOPENROUTER_MEMORY_TURNS=four, and it's raised at import time. Since this is example code, it's low-risk, but wrapping with a try/except and falling back to the default (with a printed warning) would make the example more newcomer-friendly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/voice-agent/main.py` around lines 35 - 44, The module currently parses OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES using int(...) at import time which will raise ValueError for non-numeric env values; update the parsing for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES to safely coerce the environment value inside a try/except (or use a helper like safe_int) that catches ValueError/TypeError, falls back to the default (4 and 2 respectively), and emits a simple warning/print mentioning the variable name and the fallback so the import won’t crash on bad env input.bindu/extensions/voice/agent_bridge.py (1)
33-38: Move user-facing TTS fallback strings toVoiceSettingsfor i18n/config.
DEFAULT_THINKING_TEXTandDEFAULT_TIMEOUT_FALLBACK_TEXTare spoken to end users but hardcoded at module scope. Per the repo rule to route configuration throughapp_settings, and given the voice stack already supports locale selection viastt_language, these should be exposed asVoiceSettingsfields so operators can localize/customize them without patching code. The numericDEFAULT_*_SECONDSconstants are fine as pure fallbacks sincevoice_settingsoverrides them. As per coding guidelines: "Usebindu.settings.app_settingsfor all configuration — never hardcode values such as URLs, ports, timeouts, API keys, or feature flags" and "NEVER create local config constants - useapp_settingsfrombindu.settings".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/voice/agent_bridge.py` around lines 33 - 38, The two user-facing TTS strings DEFAULT_THINKING_TEXT and DEFAULT_TIMEOUT_FALLBACK_TEXT must be moved out of module-scope constants and into the VoiceSettings model so they can be configured/localized via app_settings/voice_settings; update any use sites in agent_bridge.py to read voice_settings.thinking_text and voice_settings.timeout_fallback_text (fall back to the current DEFAULT_* constants if voice_settings is None or fields are empty), and ensure VoiceSettings is loaded from bindu.settings.app_settings (respecting stt_language/locale already in voice_settings); keep numeric DEFAULT_*_SECONDS as-is but do not hardcode spoken strings in agent_bridge.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/extensions/voice/service_factory.py`:
- Around line 106-133: The current _create_tts_service_for_provider uses
getattr(piper_module, "PiperTTSSettings", None) and falls back to the deprecated
voice_id kwarg; change this to instantiate the nested settings class on the
service (PiperTTSService.Settings) instead of checking for a module-level
PiperTTSSettings so the code always calls
PiperTTSService(settings=PiperTTSService.Settings(voice=voice_id),
sample_rate=...) or switch to PiperHttpTTSService with its Settings/base_url if
you prefer HTTP mode; update the instantiation in
_create_tts_service_for_provider to remove the getattr branch and always use
PiperTTSService.Settings(voice=voice_id) (or use PiperHttpTTSService.Settings
with base_url) when creating the tts instance.
In `@bindu/server/endpoints/voice_endpoints.py`:
- Around line 318-343: Update the WebSocket subprotocol parsing and acceptance
so tokens are not echoed and label/token are separated: change
_extract_ws_session_token to parse websocket.headers["sec-websocket-protocol"]
as a comma-separated list, treat the first non-empty item as the protocol label
and the second as the candidate token (fallback to _extract_bearer_token for
Authorization), enforce that the label equals the fixed protocol name
"bindu.voice.v1" and reject otherwise, perform token validation using
hmac.compare_digest for constant-time comparison, and call
websocket.accept(subprotocol="bindu.voice.v1") (never the raw token) only after
successful comparison; also update _has_ws_subprotocols checks to expect the
fixed label.
- Around line 690-750: The websocket handshake can reject after a session was
created with session_manager.get_session, leaving the session stuck in
"connecting"; in voice_websocket, after obtaining session via
session_manager.get_session, ensure you call and await
session_manager.end_session(session_id) before closing the websocket on failure
branches (rate limit rejection from _rate_limit_allow_ip, missing session token
on receive, invalid/expired token checks, and any other early close paths where
session is truthy); also catch and ignore errors from end_session so cleanup is
best-effort and does not mask the original close.
- Around line 465-491: In the stop-frame handling block (where frame_type ==
"stop") avoid returning from inside the finally which masks exceptions from
await websocket.close(); instead, set control.stopped = True and send the
"ended" state via _send_json, then call await websocket.close() inside a
try/except Exception as e that logs the failure (e.g., logger.debug or
logger.exception) and after that try/except perform a single return; remove the
finally:return pattern so errors from websocket.close() are not swallowed (refer
to control.stopped, _send_json, and websocket.close()).
In `@examples/document-analyzer/document_analyzer.py`:
- Line 199: Replace the personal email in the example metadata: locate the
"author" entry (the string key "author" in document_analyzer.py) and change the
value from the personal address "vyomrohila@gmail.com" to a project-owned alias
or a neutral placeholder (e.g., "project@example.com" or
"maintainer@example.com") so the public example contains no personal contact
information.
- Around line 177-180: The current branch hides partial extraction errors when
some files succeeded (extracted_docs) because it only returns errors if no
extracted_docs; update the conditional around extracted_docs and errors so
partial failures are surfaced: either fail-fast by returning the joined errors
immediately when errors is non-empty, or (preferably) return a combined response
that includes the analysis result plus a visible warning line with
"\n".join(errors) when both extracted_docs and errors exist. Locate the block
using the variables extracted_docs and errors and change the logic to (a) check
errors first to return failure, or (b) when extracted_docs and errors are both
truthy, prepend/append a clear warning message containing the error list to the
successful result before returning.
- Around line 23-25: The extractor currently treats an empty string from
extract_text_from_docx as a valid document and appends it; change the collector
logic in document_analyzer.py so that when extract_text_from_docx (and
equivalent extraction calls around lines noted) returns an empty string or only
whitespace you treat it as a failure: log an error/warning with context (file
name or path and reason) via logger from get_logger, and skip appending that
document (or raise/return failure upstream) instead of appending "" — update all
extraction call sites (the extract_text_from_docx call and the similar blocks at
the other indicated regions) to perform this emptiness check before adding to
the documents list.
- Around line 183-194: The handler currently returns the full RunOutput object
from agent.run(), causing inconsistent return types versus error paths that
return strings; change the success path to return result.content (i.e., extract
the generated text from the RunOutput) so the handler always returns a
JSON-serializable string; locate the call to agent.run() that assigns to result
and replace the returned value with result.content to match the error responses.
In `@examples/pdf_research_agent/pdf_research_agent.py`:
- Around line 18-20: The ALLOWED_BASE_DIR constant and the path-containment
check reject absolute paths shown in the docs; make the sandbox
explicit/configurable by reading ALLOWED_BASE_DIR from an environment variable
(fallback to the current example dir) and update the example message strings to
show a path under that directory. Concretely: change the ALLOWED_BASE_DIR
constant to load os.environ.get("ALLOWED_BASE_DIR", "<current-default>"), keep
the containment check (the function that validates/normalizes incoming document
paths—e.g., the path validation helper used where incoming user content is
parsed), and update the example prompt texts (the printed/returned sample JSON
like "/path/to/paper.pdf") to format a sample path using ALLOWED_BASE_DIR so
examples always point inside the allowed sandbox.
- Around line 200-204: Replace the current broad except Exception as e return
that echoes raw exception text with a server-side logged error and a generic
client-facing message: inside the except block (the except Exception as e
handler that returns the dict with keys "success","data","error") call the
module or app logger (e.g., logger.error or app.logger.error) to record the full
exception details and stack trace, then return {"success": False, "data": None,
"error": "An internal error occurred while processing the request"} so no
internal paths or provider internals are exposed to clients.
- Around line 183-185: The code currently short-circuits based on document_text
starting with "[" or "Error" which is stale now that _read_content() raises
DocumentReadError; remove the conditional that checks
document_text.startswith("[") or document_text.startswith("Error") and let valid
document text beginning with those characters be processed normally, and instead
ensure any callers of _read_content() catch DocumentReadError (reference the
_read_content() function and the document_text variable) so actual read errors
are handled via exceptions rather than string matching.
- Around line 83-92: Change the ImportError handler to capture the original
exception and chain it when re-raising, e.g. use "except ImportError as exc" and
raise DocumentReadError(...) from exc; also update the generic exception raise
to use f-string conversion by replacing "{str(e)}" with "{e!s}" so the
DocumentReadError raise reads f"Error reading PDF '{resolved_path}': {e!s}".
Ensure these edits are applied in the same try/except block where
DocumentReadError and resolved_path are used.
- Around line 193-198: The return currently places the raw result from agent.run
into the response dict which can be non-serializable; update the post-run
handling for result (the variable assigned from
agent.run(input=enriched_messages)) to unwrap the actual serializable payload
before returning—check for attributes like "content" or "response" on result
(e.g., hasattr(result, "content") then use result.content, else hasattr(result,
"response") then use result.response) and fall back to result if neither exists,
then return {"success": True, "data": unwrapped, "error": None}; keep this logic
adjacent to the agent.run call so it replaces the current direct use of result.
In `@examples/voice-agent/.env.example`:
- Around line 3-19: The example .env shows VOICE__TTS_MODEL=piper-local which is
ineffective with VOICE__TTS_PROVIDER=piper because
bindu/extensions/voice/service_factory.py only uses config.tts_model for the
ElevenLabs branch and constructs PiperTTSService with voice_id/sample_rate;
update the example by either removing VOICE__TTS_MODEL or changing the provider
to ElevenLabs (e.g., set VOICE__TTS_PROVIDER=elevenlabs) so VOICE__TTS_MODEL is
meaningful, and optionally add a short comment explaining that Piper ignores
tts_model (referencing config.tts_model, PiperTTSService, and
VoiceAgentExtension for context).
---
Duplicate comments:
In `@bindu/extensions/voice/agent_bridge.py`:
- Around line 255-260: The call sites are passing the entire conversation list
as the first positional arg to manifest.run; change them to pass the current
user utterance as the first arg and provide history via a session/context kwarg:
update _invoke_agent_streaming to call self._manifest_run(current_utterance,
session_context={"context_id": self._context_id, "conversation_history":
list(self._conversation_history)}) (use the variable holding the current user
utterance), and make the same change for the other sites invoking
self._manifest_run in _invoke_and_emit (via _handle_user_utterance) and in
process_transcription so manifest.run’s first positional param (input_msg: str)
receives a string and history is supplied via session_context.
---
Nitpick comments:
In `@bindu/extensions/voice/agent_bridge.py`:
- Around line 33-38: The two user-facing TTS strings DEFAULT_THINKING_TEXT and
DEFAULT_TIMEOUT_FALLBACK_TEXT must be moved out of module-scope constants and
into the VoiceSettings model so they can be configured/localized via
app_settings/voice_settings; update any use sites in agent_bridge.py to read
voice_settings.thinking_text and voice_settings.timeout_fallback_text (fall back
to the current DEFAULT_* constants if voice_settings is None or fields are
empty), and ensure VoiceSettings is loaded from bindu.settings.app_settings
(respecting stt_language/locale already in voice_settings); keep numeric
DEFAULT_*_SECONDS as-is but do not hardcode spoken strings in agent_bridge.py.
In `@bindu/extensions/voice/service_factory.py`:
- Around line 42-64: Replace the literal getattr calls with direct attribute
access and rely on the existing try/except to catch AttributeError: e.g., in the
STT branch use deepgram_module.DeepgramSTTService instead of
getattr(deepgram_module, "DeepgramSTTService") (keep the surrounding try/except
ImportError/AttributeError), and make the same change for the three other
literal getattr sites referenced in the review so their module attributes are
accessed directly rather than via getattr.
In `@bindu/settings.py`:
- Around line 1126-1133: The _validate_session_token_ttl model_validator on
VoiceSettings silently clamps session_token_ttl to session_timeout when TTL >
timeout; change it to emit a warning when this clamp occurs so operators can
observe misconfiguration—inside _validate_session_token_ttl, detect the
condition (self.session_token_ttl > self.session_timeout), log a warning that
includes both the configured TTL and the effective clamped value and the field
names (session_token_ttl, session_timeout, VoiceSettings), then set
session_token_ttl = session_timeout and return self; use the project logging
facility (or Python's logging.getLogger(__name__)) rather than printing.
In `@examples/voice-agent/main.py`:
- Around line 35-44: The module currently parses OPENROUTER_MEMORY_TURNS and
VOICE_MAX_SENTENCES using int(...) at import time which will raise ValueError
for non-numeric env values; update the parsing for OPENROUTER_MEMORY_TURNS and
VOICE_MAX_SENTENCES to safely coerce the environment value inside a try/except
(or use a helper like safe_int) that catches ValueError/TypeError, falls back to
the default (4 and 2 respectively), and emits a simple warning/print mentioning
the variable name and the fallback so the import won’t crash on bad env input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08dd8046-417a-4149-88ba-28ae434957d1
📒 Files selected for processing (29)
SPLIT_PLAN.mdbindu/extensions/voice/agent_bridge.pybindu/extensions/voice/pipeline_builder.pybindu/extensions/voice/redis_session_manager.pybindu/extensions/voice/service_factory.pybindu/extensions/voice/session_factory.pybindu/server/applications.pybindu/server/endpoints/voice_endpoints.pybindu/settings.pybindu/utils/worker/messages.pydocs/VOICE.mddocs/VOICE_AGENT_PLAN.mdexamples/README.mdexamples/cerina_bindu/cbt/README.mdexamples/document-analyzer/document_analyzer.pyexamples/multilingual-collab-agent/main.pyexamples/multilingual-collab-agent/skills/collaborate/skill.yamlexamples/multilingual-collab-agent/skills/research/skill.yamlexamples/multilingual-collab-agent/skills/translate/skill.yamlexamples/pdf_research_agent/README.mdexamples/pdf_research_agent/pdf_research_agent.pyexamples/pdf_research_agent/skills/pdf-research-skill/skill.yamlexamples/typescript-langchain-agent/quiz-agent.tsexamples/typescript-langchain-quiz-agent/quiz-agent.tsexamples/voice-agent/.env.exampleexamples/voice-agent/README.mdexamples/voice-agent/main.pyexamples/voice-agent/skills/voice-brief-response-skill/skill.yamltests/unit/extensions/voice/test_agent_bridge.py
✅ Files skipped from review due to trivial changes (13)
- examples/README.md
- examples/pdf_research_agent/README.md
- examples/cerina_bindu/cbt/README.md
- examples/multilingual-collab-agent/skills/research/skill.yaml
- examples/multilingual-collab-agent/skills/collaborate/skill.yaml
- SPLIT_PLAN.md
- examples/voice-agent/skills/voice-brief-response-skill/skill.yaml
- bindu/server/applications.py
- bindu/extensions/voice/pipeline_builder.py
- docs/VOICE.md
- examples/typescript-langchain-agent/quiz-agent.ts
- tests/unit/extensions/voice/test_agent_bridge.py
- examples/voice-agent/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/typescript-langchain-quiz-agent/quiz-agent.ts
- examples/multilingual-collab-agent/skills/translate/skill.yaml
- examples/multilingual-collab-agent/main.py
- bindu/utils/worker/messages.py
- examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
- bindu/extensions/voice/redis_session_manager.py
| def _create_tts_service_for_provider(provider: str, config: VoiceAgentExtension) -> Any: | ||
| if provider == "piper": | ||
| voice_id = config.tts_voice_id | ||
| try: | ||
| piper_module = importlib.import_module("pipecat.services.piper.tts") | ||
| PiperTTSService = getattr(piper_module, "PiperTTSService") | ||
| PiperTTSSettings = getattr(piper_module, "PiperTTSSettings", None) | ||
| except (ImportError, AttributeError) as e: | ||
| raise ImportError( | ||
| "Piper TTS requires pipecat[piper]. " | ||
| "Install with: pip install 'bindu[voice]'" | ||
| ) from e | ||
|
|
||
| if PiperTTSSettings is not None: | ||
| tts = PiperTTSService( | ||
| settings=PiperTTSSettings( | ||
| voice=voice_id, | ||
| ), | ||
| sample_rate=config.sample_rate, | ||
| ) | ||
| else: | ||
| tts = PiperTTSService( | ||
| voice_id=voice_id, | ||
| sample_rate=config.sample_rate, | ||
| ) | ||
|
|
||
| logger.info(f"Created Piper TTS: voice={voice_id}") | ||
| return tts |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pipecat PiperTTSService vs PiperHttpTTSService 0.0.105 initialization settings
💡 Result:
PiperTTSService vs PiperHttpTTSService in pipecat 0.0.105: PiperTTSService: Local TTS synthesis. Runs Piper inference directly in-process, automatically downloads voice models. Initialization: PiperTTSService( voice_id: str | None = None, # Deprecated 0.0.105, use settings download_dir: Path | None = None, force_redownload: bool = False, use_cuda: bool = False, settings: PiperTTSSettings | None = None, # Preferred: PiperTTSService.Settings(voice="en_US-ryan-high") kwargs ) Example: tts = PiperTTSService(settings=PiperTTSService.Settings(voice="en_US-ryan-high")) PiperHttpTTSService: Connects to external Piper HTTP server for synthesis. Supports streaming, requires running server (e.g., python -m piper.http_server -m en_US-ryan-high). Initialization: PiperHttpTTSService( base_url: str, # e.g. "http://localhost:5000" aiohttp_session: ClientSession, voice_id: str | None = None, # Deprecated 0.0.105, use settings settings: PiperHttpTTSSettings | None = None, # Preferred: PiperHttpTTSService.Settings(voice="en_US-ryan-high") kwargs ) Example: async with aiohttp.ClientSession as session: tts = PiperHttpTTSService( base_url="http://localhost:5000", aiohttp_session=session, settings=PiperHttpTTSService.Settings(voice="en_US-ryan-high") ) Key differences: - Local vs remote (HTTP server) - PiperTTSService: auto-downloads models, CUDA option, no network - PiperHttpTTSService: needs separate server, aiohttp session required, base_url In 0.0.105: voice_id deprecated; use settings.voice=... . Settings runtime-updatable via TTSUpdateSettingsFrame. Both inherit TTSSettings (model, voice, language, extra).
Citations:
- 1: https://reference-server.pipecat.ai/en/stable/api/pipecat.services.piper.tts.html
- 2: https://reference-server.pipecat.ai/en/stable/_modules/pipecat/services/piper/tts.html
- 3: https://docs.pipecat.ai/server/services/tts/piper
- 4: https://reference-server.pipecat.ai/en/latest/api/pipecat.services.piper.tts.html
- 5: https://docs.pipecat.ai/api-reference/server/services/tts/piper
- 6: https://docs.pipecat.ai/pipecat/fundamentals/service-settings
Fix Piper settings instantiation: nested class reference, not module-level attribute.
In pipecat ≥0.0.105, the settings classes are nested (PiperTTSService.Settings, PiperHttpTTSService.Settings), not module-level PiperTTSSettings. The getattr(piper_module, "PiperTTSSettings", None) check will always return None, causing silent fallback to the deprecated voice_id= kwarg:
tts = PiperTTSService(
voice_id=voice_id, # deprecated; raises warning in 0.0.105, breaks in future releases
sample_rate=config.sample_rate,
)Either:
- Remove the conditional; use
PiperTTSService.Settings(voice=voice_id)directly, or - Switch to
PiperHttpTTSServicewith an explicitbase_urlconfig setting (recommended for distributed deployments).
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 111-111: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/extensions/voice/service_factory.py` around lines 106 - 133, The
current _create_tts_service_for_provider uses getattr(piper_module,
"PiperTTSSettings", None) and falls back to the deprecated voice_id kwarg;
change this to instantiate the nested settings class on the service
(PiperTTSService.Settings) instead of checking for a module-level
PiperTTSSettings so the code always calls
PiperTTSService(settings=PiperTTSService.Settings(voice=voice_id),
sample_rate=...) or switch to PiperHttpTTSService with its Settings/base_url if
you prefer HTTP mode; update the instantiation in
_create_tts_service_for_provider to remove the getattr branch and always use
PiperTTSService.Settings(voice=voice_id) (or use PiperHttpTTSService.Settings
with base_url) when creating the tts instance.
| def _extract_bearer_token(value: str | None) -> str | None: | ||
| if not value: | ||
| return None | ||
| parts = value.split() | ||
| if len(parts) == 2 and parts[0].lower() == "bearer": | ||
| token = parts[1].strip() | ||
| return token or None | ||
| return None | ||
|
|
||
|
|
||
| def _extract_ws_session_token(websocket: WebSocket) -> str | None: | ||
| """Extract session token from WS headers (subprotocol or Authorization).""" | ||
| subprotocols = websocket.headers.get("sec-websocket-protocol") | ||
| if subprotocols: | ||
| # Starlette exposes the raw comma-separated list. | ||
| for item in subprotocols.split(","): | ||
| token = item.strip() | ||
| if token: | ||
| return token | ||
|
|
||
| return _extract_bearer_token(websocket.headers.get("authorization")) | ||
|
|
||
|
|
||
| def _has_ws_subprotocols(websocket: WebSocket) -> bool: | ||
| return bool(websocket.headers.get("sec-websocket-protocol")) | ||
|
|
There was a problem hiding this comment.
WebSocket subprotocol auth flow is fragile and leaks the token as a negotiated protocol.
Two related problems in the handshake path:
-
_extract_ws_session_token(lines 330–336) treats every comma-separatedSec-WebSocket-Protocolvalue equally and returns the first non-empty token. If a client uses the common pattern of sending a protocol name + token (e.g.Sec-WebSocket-Protocol: bindu.voice.v1, <token>),providedwill be"bindu.voice.v1", auth will fail, and there is no way for the server to distinguish the protocol label from the credential. -
await websocket.accept(subprotocol=provided)(line 732) echoes whatever the client sent as the negotiated subprotocol. Because the client is forced to send the raw token as the subprotocol, that token is echoed back in theSec-WebSocket-Protocolresponse header (visible in browser devtools, intermediary logs, etc.) — and is also accepted before we validate it at line 742.
Please adopt a fixed protocol name (e.g. "bindu.voice.v1") and a separate token slot:
Sec-WebSocket-Protocol: bindu.voice.v1, <token>
Then:
- parse the subprotocol list, take the first entry as the protocol label and the second as the candidate token,
- call
accept(subprotocol="bindu.voice.v1")after a constant-time token comparison (hmac.compare_digest), - reject connections that don't present the expected label.
This also closes the timing side channel on provided != expected.
Also applies to: 724-747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/server/endpoints/voice_endpoints.py` around lines 318 - 343, Update the
WebSocket subprotocol parsing and acceptance so tokens are not echoed and
label/token are separated: change _extract_ws_session_token to parse
websocket.headers["sec-websocket-protocol"] as a comma-separated list, treat the
first non-empty item as the protocol label and the second as the candidate token
(fallback to _extract_bearer_token for Authorization), enforce that the label
equals the fixed protocol name "bindu.voice.v1" and reject otherwise, perform
token validation using hmac.compare_digest for constant-time comparison, and
call websocket.accept(subprotocol="bindu.voice.v1") (never the raw token) only
after successful comparison; also update _has_ws_subprotocols checks to expect
the fixed label.
| if frame_type == "stop": | ||
| control.stopped = True | ||
| await _send_json( | ||
| websocket, {"type": "state", "state": "ended"}, send_lock | ||
| ) | ||
| try: | ||
| await websocket.close() | ||
| finally: | ||
| return | ||
| if frame_type == "user_text": | ||
| user_text = payload.get("text") | ||
| if isinstance(user_text, str) and user_text.strip() and on_user_text: | ||
| try: | ||
| await on_user_text(user_text.strip()) | ||
| except Exception: | ||
| logger.exception("Failed to handle user_text control frame") | ||
| continue | ||
| if frame_type in {"start", "commit_turn"}: | ||
| # If VAD is disabled, the transport/STT may rely on explicit turn boundary | ||
| # control frames (e.g. commit_turn). Forward these to the transport. | ||
| if not vad_enabled: | ||
| await inbound_queue.put(message) | ||
| continue | ||
|
|
||
| # Unknown control frame: ignore to preserve forward-compat. | ||
| continue | ||
|
|
There was a problem hiding this comment.
return inside finally silences errors from websocket.close() (Ruff B012).
If await websocket.close() raises (e.g. connection already torn down), the finally: return swallows it and the stop handler exits silently. Prefer an explicit try/except Exception: logger.debug(...) and then return after the block so you don't mask real failures:
♻️ Proposed tweak
if frame_type == "stop":
control.stopped = True
await _send_json(
websocket, {"type": "state", "state": "ended"}, send_lock
)
try:
await websocket.close()
- finally:
- return
+ except Exception:
+ logger.debug("Error closing websocket after stop frame", exc_info=True)
+ return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if frame_type == "stop": | |
| control.stopped = True | |
| await _send_json( | |
| websocket, {"type": "state", "state": "ended"}, send_lock | |
| ) | |
| try: | |
| await websocket.close() | |
| finally: | |
| return | |
| if frame_type == "user_text": | |
| user_text = payload.get("text") | |
| if isinstance(user_text, str) and user_text.strip() and on_user_text: | |
| try: | |
| await on_user_text(user_text.strip()) | |
| except Exception: | |
| logger.exception("Failed to handle user_text control frame") | |
| continue | |
| if frame_type in {"start", "commit_turn"}: | |
| # If VAD is disabled, the transport/STT may rely on explicit turn boundary | |
| # control frames (e.g. commit_turn). Forward these to the transport. | |
| if not vad_enabled: | |
| await inbound_queue.put(message) | |
| continue | |
| # Unknown control frame: ignore to preserve forward-compat. | |
| continue | |
| if frame_type == "stop": | |
| control.stopped = True | |
| await _send_json( | |
| websocket, {"type": "state", "state": "ended"}, send_lock | |
| ) | |
| try: | |
| await websocket.close() | |
| except Exception: | |
| logger.debug("Error closing websocket after stop frame", exc_info=True) | |
| return | |
| if frame_type == "user_text": | |
| user_text = payload.get("text") | |
| if isinstance(user_text, str) and user_text.strip() and on_user_text: | |
| try: | |
| await on_user_text(user_text.strip()) | |
| except Exception: | |
| logger.exception("Failed to handle user_text control frame") | |
| continue | |
| if frame_type in {"start", "commit_turn"}: | |
| # If VAD is disabled, the transport/STT may rely on explicit turn boundary | |
| # control frames (e.g. commit_turn). Forward these to the transport. | |
| if not vad_enabled: | |
| await inbound_queue.put(message) | |
| continue | |
| # Unknown control frame: ignore to preserve forward-compat. | |
| continue |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 473-473: return inside finally blocks cause exceptions to be silenced
(B012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/server/endpoints/voice_endpoints.py` around lines 465 - 491, In the
stop-frame handling block (where frame_type == "stop") avoid returning from
inside the finally which masks exceptions from await websocket.close(); instead,
set control.stopped = True and send the "ended" state via _send_json, then call
await websocket.close() inside a try/except Exception as e that logs the failure
(e.g., logger.debug or logger.exception) and after that try/except perform a
single return; remove the finally:return pattern so errors from
websocket.close() are not swallowed (refer to control.stopped, _send_json, and
websocket.close()).
| async def voice_websocket(websocket: WebSocket) -> None: | ||
| """Bidirectional voice WebSocket handler using Pipecat pipeline.""" | ||
| app: BinduApplication = websocket.app # type: ignore[assignment] | ||
| session_id = websocket.path_params.get("session_id", "") | ||
|
|
||
| session_manager = getattr(app, "_voice_session_manager", None) | ||
| if session_manager is None: | ||
| await websocket.close(code=1008, reason="Voice extension is not enabled") | ||
| return | ||
| if not app_settings.voice.enabled: | ||
| await websocket.close(code=1008, reason="Voice is disabled") | ||
| return | ||
|
|
||
| preflight_error = _voice_preflight_error() | ||
| if preflight_error: | ||
| await websocket.close(code=1008, reason=preflight_error) | ||
| return | ||
|
|
||
| session = await session_manager.get_session(session_id) | ||
| if session is None: | ||
| await websocket.close(code=1008, reason="Invalid session ID") | ||
| return | ||
|
|
||
| # Per-IP rate limit on websocket connects (best-effort) | ||
| client_host = websocket.client.host if websocket.client else None | ||
| if client_host: | ||
| allowed = await _rate_limit_allow_ip( | ||
| client_host, | ||
| limit_per_minute=int(app_settings.voice.rate_limit_per_ip_per_minute), | ||
| ) | ||
| if not allowed: | ||
| await websocket.close(code=1008, reason="Rate limit exceeded") | ||
| return | ||
|
|
||
| if app_settings.voice.session_auth_required: | ||
| expected = getattr(session, "session_token", None) | ||
| expires_at = getattr(session, "session_token_expires_at", None) | ||
| provided = _extract_ws_session_token(websocket) | ||
|
|
||
| # If the client sent Sec-WebSocket-Protocol, the server should select one. | ||
| # We select the token itself as the negotiated subprotocol. | ||
| if provided and _has_ws_subprotocols(websocket): | ||
| await websocket.accept(subprotocol=provided) | ||
| else: | ||
| await websocket.accept() | ||
| if not provided: | ||
| try: | ||
| provided = (await websocket.receive_text()).strip() | ||
| except Exception: | ||
| await websocket.close(code=1008, reason="Missing session token") | ||
| return | ||
|
|
||
| if not expected or provided != expected: | ||
| await websocket.close(code=1008, reason="Invalid session token") | ||
| return | ||
| if isinstance(expires_at, (int, float)) and time.time() > float(expires_at): | ||
| await websocket.close(code=1008, reason="Session token expired") | ||
| return | ||
| else: | ||
| await websocket.accept() | ||
|
|
There was a problem hiding this comment.
Missed session-state update on rejected WebSocket handshake.
Along the failure paths (session is None, rate limit hit, bad/expired token, invalid session token) the session may already be in connecting state in the manager but is never moved to ended. Combined with the get_session call preceding both rate limiting and auth, a rejected WS leaves a live session record until the timeout reaper kicks in — which inflates get_active_count and can cause legitimate clients to get 429s on /voice/session/start. Consider awaiting session_manager.end_session(session_id) on the auth/rate-limit rejection branches to free the slot immediately.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 738-738: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/server/endpoints/voice_endpoints.py` around lines 690 - 750, The
websocket handshake can reject after a session was created with
session_manager.get_session, leaving the session stuck in "connecting"; in
voice_websocket, after obtaining session via session_manager.get_session, ensure
you call and await session_manager.end_session(session_id) before closing the
websocket on failure branches (rate limit rejection from _rate_limit_allow_ip,
missing session token on receive, invalid/expired token checks, and any other
early close paths where session is truthy); also catch and ignore errors from
end_session so cleanup is best-effort and does not mask the original close.
| from bindu.utils.logging import get_logger | ||
|
|
||
| logger = get_logger("examples.document_analyzer") |
There was a problem hiding this comment.
Don’t treat failed or empty extraction as a valid document.
extract_text_from_docx currently logs and returns "", and the collector appends that value. A corrupt DOCX or textless document can bypass the failure path and send empty document content to the LLM.
Suggested fix
-from bindu.utils.logging import get_logger
-
-logger = get_logger("examples.document_analyzer")
-
...
def extract_text_from_docx(file_bytes):
"""Extract text from docx bytes"""
- try:
- doc = Document(io.BytesIO(file_bytes))
- return "\n".join([p.text for p in doc.paragraphs])
- except Exception as e:
- logger.error(f"Error extracting DOCX text: {e}")
- return ""
+ doc = Document(io.BytesIO(file_bytes))
+ return "\n".join(p.text for p in doc.paragraphs)
...
doc_text = extract_document_text(file_bytes, mime_type)
+ if not doc_text.strip():
+ raise ValueError(
+ f"No extractable text found for file type: {mime_type}"
+ )
extracted_docs.append(doc_text)Also applies to: 76-83, 154-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/document-analyzer/document_analyzer.py` around lines 23 - 25, The
extractor currently treats an empty string from extract_text_from_docx as a
valid document and appends it; change the collector logic in
document_analyzer.py so that when extract_text_from_docx (and equivalent
extraction calls around lines noted) returns an empty string or only whitespace
you treat it as a failure: log an error/warning with context (file name or path
and reason) via logger from get_logger, and skip appending that document (or
raise/return failure upstream) instead of appending "" — update all extraction
call sites (the extract_text_from_docx call and the similar blocks at the other
indicated regions) to perform this emptiness check before adding to the
documents list.
| except ImportError: | ||
| return ( | ||
| f"[pypdf not installed — cannot read '{source.strip()}'. " | ||
| "Run: uv add pypdf]" | ||
| raise DocumentReadError( | ||
| f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" | ||
| ) | ||
| except Exception as e: | ||
| return f"Error reading PDF '{source.strip()}': {str(e)}" | ||
| if isinstance(e, DocumentReadError): | ||
| raise | ||
| raise DocumentReadError( | ||
| f"Error reading PDF '{resolved_path}': {str(e)}" | ||
| ) from e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
cat -n examples/pdf_research_agent/pdf_research_agent.py | sed -n '75,100p'Repository: GetBindu/Bindu
Length of output: 1264
🏁 Script executed:
# Let's also check the full context around lines 83-92
cat -n examples/pdf_research_agent/pdf_research_agent.py | sed -n '70,105p'Repository: GetBindu/Bindu
Length of output: 1775
🌐 Web query:
Ruff exception handling chaining rules ImportError B904 exception context
💡 Result:
Ruff's B904 rule, named "raise-without-from-inside-except", detects raise statements inside except or except* clauses that lack a 'from' clause. It enforces explicit exception chaining using 'raise ... from err' to set the cause attribute (for direct causation) or 'raise ... from None' to suppress chaining, distinguishing intentional raises from handling errors. Without 'from', Python implicitly chains via context, but explicit use improves debugging clarity in tracebacks (e.g., "The above exception was the direct cause" vs. "During handling"). This applies to ImportError or any exception. Example violation: try: ... except ImportError: raise ValueError("Failed to import") Suggested fix: try: ... except ImportError as exc: raise ValueError("Failed to import") from exc Or to suppress: raise ValueError("Failed to import") from None The rule originates from flake8-bugbear, added in Ruff v0.0.138, and supports except* (Python 3.11+). Recent docs (as of 2026 issues) clarify nuances between implicit/explicit chaining. No auto-fix available.
Citations:
- 1: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/
- 2: Misleading documentation for B904: raise-without-from-inside-except astral-sh/ruff#22541
- 3: https://docs.astral.sh/ruff/rules/
- 4: Improve error messages for except* (B025, B029, B030, B904) #14791 astral-sh/ruff#14815
- 5: Misleading descriptions for raise-from exception chaining rules (B904 and TRY200) astral-sh/ruff#8736
🌐 Web query:
Ruff f-string conversion flag {e!s} vs str(e) rule
💡 Result:
Ruff's rule RUF010 (explicit-f-string-type-conversion) flags the use of explicit str(e), repr(e), or ascii(e) calls inside f-strings, recommending f-string conversion flags like {e!s} instead. This is because conversion flags are more succinct and idiomatic. For str(e), replace with {e!s}, though often {e} alone suffices since f-strings auto-convert to str unless the object has a custom format. The rule provides an unsafe fix to automate replacement, added in Ruff v0.0.267.
Citations:
- 1: https://beta.ruff.rs/docs/rules/explicit-f-string-type-conversion/
- 2: https://docs.astral.sh/ruff/rules/explicit-f-string-type-conversion/
- 3: https://docs.astral.sh/ruff/rules/
- 4: https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fruff%2Frules%2Fexplicit_f_string_type_conversion.rs
Fix the Ruff exception-handling findings.
Line 83: ImportError must be chained with from to satisfy the B904 rule (raise-without-from-inside-except). Capture the exception with as exc and re-raise with from exc.
Line 91: Replace {str(e)} with {e!s} to satisfy the RUF010 rule (explicit-f-string-type-conversion), which recommends using f-string conversion flags instead of explicit function calls.
Suggested fix
- except ImportError:
+ except ImportError as exc:
raise DocumentReadError(
f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf"
- )
+ ) from exc
except Exception as e:
if isinstance(e, DocumentReadError):
raise
raise DocumentReadError(
- f"Error reading PDF '{resolved_path}': {str(e)}"
+ f"Error reading PDF '{resolved_path}': {e!s}"
) from e🧰 Tools
🪛 Ruff (0.15.10)
[warning] 84-86: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 91-91: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 83 - 92,
Change the ImportError handler to capture the original exception and chain it
when re-raising, e.g. use "except ImportError as exc" and raise
DocumentReadError(...) from exc; also update the generic exception raise to use
f-string conversion by replacing "{str(e)}" with "{e!s}" so the
DocumentReadError raise reads f"Error reading PDF '{resolved_path}': {e!s}".
Ensure these edits are applied in the same try/except block where
DocumentReadError and resolved_path are used.
| # Check if document processing failed | ||
| if document_text.startswith("[") or document_text.startswith("Error"): | ||
| return document_text |
There was a problem hiding this comment.
Remove the stale string-error short-circuit.
_read_content() now raises DocumentReadError; valid document text beginning with [ or Error will currently be returned unsummarized.
Suggested fix
- # Check if document processing failed
- if document_text.startswith("[") or document_text.startswith("Error"):
- return document_text
-
# Limit document size to prevent token overflow📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if document processing failed | |
| if document_text.startswith("[") or document_text.startswith("Error"): | |
| return document_text |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 183 - 185,
The code currently short-circuits based on document_text starting with "[" or
"Error" which is stale now that _read_content() raises DocumentReadError; remove
the conditional that checks document_text.startswith("[") or
document_text.startswith("Error") and let valid document text beginning with
those characters be processed normally, and instead ensure any callers of
_read_content() catch DocumentReadError (reference the _read_content() function
and the document_text variable) so actual read errors are handled via exceptions
rather than string matching.
| # Build a prompt that includes the full document text | ||
| prompt = f"Summarize the following document and highlight the key insights:\n\n{document_text}" | ||
| enriched_messages = [{"role": "user", "content": prompt}] | ||
|
|
||
| result = agent.run(input=enriched_messages) | ||
| return result | ||
| return {"success": True, "data": result, "error": None} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect existing agent.run(input=...) usage and response unwrapping patterns.
rg -n -C4 'agent\.run\(input=|hasattr\(result,\s*["'\'']content["'\'']|hasattr\(result,\s*["'\'']response["'\'']' --iglob '*.py'Repository: GetBindu/Bindu
Length of output: 11555
🏁 Script executed:
# Check if other examples return results wrapped in dicts with "data" field
rg -n '{"success"\s*:\s*True.*"data"' --iglob '*.py' -B2 -A2Repository: GetBindu/Bindu
Length of output: 162
🏁 Script executed:
# Get full context of pdf_research_agent.py to understand the return pattern
sed -n '180,210p' examples/pdf_research_agent/pdf_research_agent.pyRepository: GetBindu/Bindu
Length of output: 1296
🏁 Script executed:
# Check what agent.run() accepts - is message list vs string actually different?
rg -n 'def run\(' --iglob '*.py' -A10 | head -100Repository: GetBindu/Bindu
Length of output: 7893
🏁 Script executed:
# Check if any other examples wrap agent.run() results in dicts
rg -n 'return\s*{.*"success"' --iglob '*.py' -B5 -A1Repository: GetBindu/Bindu
Length of output: 171
🏁 Script executed:
# Look at the handler function signature and return type expectations
sed -n '165,210p' examples/pdf_research_agent/pdf_research_agent.pyRepository: GetBindu/Bindu
Length of output: 1823
🏁 Script executed:
# Check the full handler signature and what it's expected to return
sed -n '155,165p' examples/pdf_research_agent/pdf_research_agent.pyRepository: GetBindu/Bindu
Length of output: 408
🏁 Script executed:
# Verify if message list vs string input format has any documented difference
rg -n 'agent\.run.*input=' examples/ -B2 -A2 | head -60Repository: GetBindu/Bindu
Length of output: 3908
Unwrap agent result before wrapping in response dict.
The raw result object is being stored in data without extracting the serializable content. Other examples (premium-advisor, web-scraping-agent, weather-research) unwrap results using hasattr(result, "content") or hasattr(result, "response") before returning. This prevents JSON serialization failures.
Suggested fix
# Build a prompt that includes the full document text
prompt = f"Summarize the following document and highlight the key insights:\n\n{document_text}"
- enriched_messages = [{"role": "user", "content": prompt}]
- result = agent.run(input=enriched_messages)
- return {"success": True, "data": result, "error": None}
+ result = agent.run(input=prompt)
+ if hasattr(result, "content"):
+ data = result.content
+ elif hasattr(result, "response"):
+ data = result.response
+ else:
+ data = str(result)
+ return {"success": True, "data": data, "error": None}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build a prompt that includes the full document text | |
| prompt = f"Summarize the following document and highlight the key insights:\n\n{document_text}" | |
| enriched_messages = [{"role": "user", "content": prompt}] | |
| result = agent.run(input=enriched_messages) | |
| return result | |
| return {"success": True, "data": result, "error": None} | |
| # Build a prompt that includes the full document text | |
| prompt = f"Summarize the following document and highlight the key insights:\n\n{document_text}" | |
| result = agent.run(input=prompt) | |
| if hasattr(result, "content"): | |
| data = result.content | |
| elif hasattr(result, "response"): | |
| data = result.response | |
| else: | |
| data = str(result) | |
| return {"success": True, "data": data, "error": None} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 193 - 198,
The return currently places the raw result from agent.run into the response dict
which can be non-serializable; update the post-run handling for result (the
variable assigned from agent.run(input=enriched_messages)) to unwrap the actual
serializable payload before returning—check for attributes like "content" or
"response" on result (e.g., hasattr(result, "content") then use result.content,
else hasattr(result, "response") then use result.response) and fall back to
result if neither exists, then return {"success": True, "data": unwrapped,
"error": None}; keep this logic adjacent to the agent.run call so it replaces
the current direct use of result.
| except Exception as e: | ||
| return f"Error processing request: {str(e)}" | ||
| return { | ||
| "success": False, | ||
| "data": None, | ||
| "error": f"Error processing request: {e}", |
There was a problem hiding this comment.
Avoid echoing unexpected exception details to clients.
The broad catch is flagged by Ruff and returns raw exception text, which can expose local paths or provider internals. Prefer logging server-side and returning a generic error for unexpected failures.
Suggested fix
- except Exception as e:
+ except Exception:
return {
"success": False,
"data": None,
- "error": f"Error processing request: {e}",
+ "error": "Error processing request.",
}🧰 Tools
🪛 Ruff (0.15.10)
[warning] 200-200: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 200 - 204,
Replace the current broad except Exception as e return that echoes raw exception
text with a server-side logged error and a generic client-facing message: inside
the except block (the except Exception as e handler that returns the dict with
keys "success","data","error") call the module or app logger (e.g., logger.error
or app.logger.error) to record the full exception details and stack trace, then
return {"success": False, "data": None, "error": "An internal error occurred
while processing the request"} so no internal paths or provider internals are
exposed to clients.
| VOICE__TTS_PROVIDER=piper | ||
| VOICE__TTS_VOICE_ID=en_US-ryan-high | ||
|
|
||
| # Optional fallback TTS (recommended for resiliency) | ||
| VOICE__TTS_FALLBACK_PROVIDER=azure | ||
| VOICE__AZURE_TTS_API_KEY=your-azure-speech-key | ||
| VOICE__AZURE_TTS_REGION=eastus | ||
| VOICE__AZURE_TTS_VOICE=en-US-SaraNeural | ||
|
|
||
| # If you still use ElevenLabs as primary/fallback: | ||
| # VOICE__TTS_API_KEY=your-elevenlabs-api-key | ||
|
|
||
| # Optional tuning | ||
| VOICE__ENABLED=true | ||
| VOICE__STT_MODEL=nova-3 | ||
| VOICE__STT_LANGUAGE=en | ||
| VOICE__TTS_MODEL=piper-local |
There was a problem hiding this comment.
VOICE__TTS_MODEL=piper-local is a no-op with VOICE__TTS_PROVIDER=piper.
In bindu/extensions/voice/service_factory.py, config.tts_model is only consumed by the ElevenLabs branch (and stored on VoiceAgentExtension for metadata). The Piper branch constructs PiperTTSService using only voice_id/sample_rate, so piper-local is inert and likely to confuse users who think it selects a Piper model. Consider dropping the line, or swapping the example to ElevenLabs where the field is meaningful.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 8-8: [UnorderedKey] The VOICE__AZURE_TTS_API_KEY key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
[warning] 9-9: [UnorderedKey] The VOICE__AZURE_TTS_REGION key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The VOICE__AZURE_TTS_VOICE key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The VOICE__STT_LANGUAGE key should go before the VOICE__STT_MODEL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/voice-agent/.env.example` around lines 3 - 19, The example .env
shows VOICE__TTS_MODEL=piper-local which is ineffective with
VOICE__TTS_PROVIDER=piper because bindu/extensions/voice/service_factory.py only
uses config.tts_model for the ElevenLabs branch and constructs PiperTTSService
with voice_id/sample_rate; update the example by either removing
VOICE__TTS_MODEL or changing the provider to ElevenLabs (e.g., set
VOICE__TTS_PROVIDER=elevenlabs) so VOICE__TTS_MODEL is meaningful, and
optionally add a short comment explaining that Piper ignores tts_model
(referencing config.tts_model, PiperTTSService, and VoiceAgentExtension for
context).
# Conflicts: # examples/cerina_bindu/cbt/README.md # examples/multilingual-collab-agent/agent_config.json # examples/multilingual-collab-agent/main.py # examples/multilingual-collab-agent/skills/collaborate/skill.yaml # examples/multilingual-collab-agent/skills/research/skill.yaml # examples/multilingual-collab-agent/skills/translate/skill.yaml # examples/pdf_research_agent/README.md # examples/pdf_research_agent/pdf_research_agent.py # examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
Co-vengers
force-pushed
the
feat(voice-agent)/documentation
branch
from
5bac2a9 to
79523ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
examples/document-analyzer/document_analyzer.py (4)
199-199:⚠️ Potential issue | 🟡 MinorReplace the personal email in example metadata.
Use a neutral placeholder or project-owned alias for public example config.
Suggested fix
- "author": "vyomrohila@gmail.com", + "author": "example@example.com",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` at line 199, Update the example metadata entry where the "author" field is set to a personal email (the string "author": "vyomrohila@gmail.com") in examples/document-analyzer/document_analyzer.py and replace it with a neutral placeholder or project-owned alias (e.g., "author": "example@example.com" or "author": "project@example.com") so the public example does not include a personal email address.
175-180:⚠️ Potential issue | 🟠 MajorSurface partial extraction failures.
Line 177 only checks
errorswhen no documents were extracted, so mixed success/failure uploads silently hide failed files.Fail-fast option
prompt, extracted_docs, errors = _collect_prompt_and_documents(messages) + if errors: + return "Failed to process documents:\n" + "\n".join(errors) + if not extracted_docs: - if errors: - return "Failed to process documents:\n" + "\n".join(errors) return "No valid document found in the messages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` around lines 175 - 180, The current logic after calling _collect_prompt_and_documents(prompt, extracted_docs, errors) only surfaces errors when no documents were extracted, which hides partial failures; update the handling around the variables prompt, extracted_docs, and errors so that if extracted_docs is non-empty but errors is also non-empty you still surface those errors (e.g., append or return a message listing failed files or include errors in the response/log alongside successful results), ensuring partial extraction failures are reported rather than silently ignored.
183-194:⚠️ Potential issue | 🟠 MajorReturn the generated content, not the
RunOutputobject.
agent.run()returns aRunOutput; the response text is in.content, per the Agno run docs. Returning the object keeps the success path inconsistent with the string error paths.Suggested fix
result = agent.run( input=f""" User Prompt: {prompt} @@ """ ) - return result + return result.content or ""Agno Agent.run returns RunOutput content attribute official documentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` around lines 183 - 194, agent.run() returns a RunOutput object, not the string response; currently the code assigns that to result and returns it, causing inconsistency with string error paths—change the return to return the text payload by returning result.content (i.e., replace the final "return result" with "return result.content") and optionally guard if result.content is None by converting to an empty string or raising a clear error; locate the call to agent.run and the variable result to make this change.
76-83:⚠️ Potential issue | 🟠 MajorDon’t append failed or empty extraction results.
extract_text_from_docx()still turns failures into"", and Line 160 appends the result without validating content. A corrupt/textless file can still be analyzed as a valid empty document.Suggested fix
def extract_text_from_docx(file_bytes): """Extract text from docx bytes""" - try: - doc = Document(io.BytesIO(file_bytes)) - return "\n".join([p.text for p in doc.paragraphs]) - except Exception as e: - logger.error(f"Error extracting DOCX text: {e}") - return "" + doc = Document(io.BytesIO(file_bytes)) + return "\n".join(p.text for p in doc.paragraphs) @@ doc_text = extract_document_text(file_bytes, mime_type) + if not doc_text.strip(): + raise ValueError( + f"No extractable text found for file type: {mime_type}" + ) extracted_docs.append(doc_text)Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/document-analyzer/document_analyzer.py` around lines 76 - 83, extract_text_from_docx currently returns an empty string on error which allows callers to append empty/failed extractions as valid documents; change extract_text_from_docx to return None (or raise a specific exception) when extraction fails or yields no text, and update the caller that appends the result (the code that does documents.append(extract_text_from_docx(...)) or similar) to check the return value and only append when it's truthy/non-None and non-empty; ensure logging still records errors in extract_text_from_docx but do not propagate empty strings upstream.examples/pdf_research_agent/pdf_research_agent.py (5)
20-20:⚠️ Potential issue | 🟠 MajorMake the PDF sandbox match the advertised path usage.
ALLOWED_BASE_DIRis still fixed to the example directory, but both examples advertise/path/to/paper.pdf, which will be rejected unless that path is under this script directory. Make the sandbox configurable or update the examples to point inside the allowed directory.Suggested fix
-ALLOWED_BASE_DIR = Path(__file__).parent.resolve() +ALLOWED_BASE_DIR = Path( + os.getenv("PDF_RESEARCH_ALLOWED_DIR", Path(__file__).parent) +).expanduser().resolve()- print('🔧 Example: {"role": "user", "content": "/path/to/paper.pdf"}') + sample_pdf = ALLOWED_BASE_DIR / "paper.pdf" + print(f'🔧 Example: {{"role": "user", "content": "{sample_pdf}"}}')Also applies to: 38-38, 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` at line 20, The examples use "/path/to/paper.pdf" but ALLOWED_BASE_DIR is hardcoded to the script's example directory so those example inputs get rejected; update the code so the sandbox matches the advertised usage by either (A) making ALLOWED_BASE_DIR configurable (e.g., read from env var or a CLI arg used by the PDF sandbox/validator) and ensure the PDF access check uses that configurable ALLOWED_BASE_DIR, or (B) change the example inputs in the example calls to point to files under the existing ALLOWED_BASE_DIR; look for the ALLOWED_BASE_DIR variable and the PDF access/validation logic that enforces it and adjust those spots and all example occurrences of "/path/to/paper.pdf" (lines around the example usages) accordingly.
200-204:⚠️ Potential issue | 🟡 MinorAvoid echoing unexpected exception details to clients.
The broad handler still returns raw exception text, which can expose local paths or provider internals. Log the exception server-side and return a generic error.
Suggested fix
+import logging import os- except Exception as e: + except Exception: # noqa: BLE001 - boundary handler converts failures to structured responses + logging.exception("Unexpected error processing PDF research request") return { "success": False, "data": None, - "error": f"Error processing request: {e}", + "error": "Error processing request.", }Verify with:
#!/bin/bash # Description: Verify the Ruff BLE001 finding in the PDF research example. ruff check --select BLE001 examples/pdf_research_agent/pdf_research_agent.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 200 - 204, The except Exception as e block returns raw exception text to clients; instead, log the full exception server-side (e.g., use logger.exception or processLogger.error with the exception/stack) inside that except handler and change the returned payload from including f"{e}" to a generic message like "Internal server error" or "An unexpected error occurred" while preserving success=False and data=None; update the handler around the except Exception as e to perform secure logging and return the non-sensitive generic error string.
183-185:⚠️ Potential issue | 🟠 MajorRemove the stale string-error short-circuit.
_read_content()now raisesDocumentReadError, so this branch can incorrectly return valid text starting with[orErrorwithout summarizing it, and it bypasses the structured response format.Suggested fix
- # Check if document processing failed - if document_text.startswith("[") or document_text.startswith("Error"): - return document_text - # Limit document size to prevent token overflow🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 183 - 185, The branch that returns document_text when it startswith "[" or "Error" is stale because _read_content() now raises DocumentReadError; remove this short-circuit so valid document text that begins with those prefixes isn't prematurely returned and instead flows through the normal summarization/structured response path—locate the conditional that checks document_text.startswith("[") or document_text.startswith("Error") (around the call site of _read_content() and variable document_text) and delete it, relying on DocumentReadError exceptions for error handling and the existing structured-response logic to handle successful text.
83-92:⚠️ Potential issue | 🟡 MinorFix the Ruff exception-handling findings.
Line 83 should capture and chain the
ImportError; Line 91 should use an f-string conversion flag instead ofstr(e).Suggested fix
- except ImportError: + except ImportError as exc: raise DocumentReadError( f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" - ) + ) from exc except Exception as e: if isinstance(e, DocumentReadError): raise raise DocumentReadError( - f"Error reading PDF '{resolved_path}': {str(e)}" + f"Error reading PDF '{resolved_path}': {e!s}" ) from eVerify with:
#!/bin/bash # Description: Verify Ruff B904/RUF010 findings in the PDF research example. ruff check --select B904,RUF010 examples/pdf_research_agent/pdf_research_agent.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 83 - 92, Catch and chain the ImportError and use an f-string conversion flag for the generic exception: in the except ImportError block (around the PDF open code) change it to "except ImportError as e:" and re-raise DocumentReadError with the original ImportError chained via "from e"; in the generic except block keep the early "if isinstance(e, DocumentReadError): raise" check, and when re-raising use an f-string conversion flag (e.g. f"{e!s}") instead of str(e) and retain the "from e" chaining so both DocumentReadError and ImportError are properly chained and reported (referencing DocumentReadError and resolved_path).
193-198:⚠️ Potential issue | 🟠 MajorUnwrap the agent result before returning it.
resultis still returned raw indata; ifagent.run()returns a response object, the service response can fail JSON serialization.Suggested fix
# Build a prompt that includes the full document text prompt = f"Summarize the following document and highlight the key insights:\n\n{document_text}" - enriched_messages = [{"role": "user", "content": prompt}] - result = agent.run(input=enriched_messages) - return {"success": True, "data": result, "error": None} + result = agent.run(input=prompt) + if hasattr(result, "content"): + data = result.content + elif hasattr(result, "response"): + data = result.response + else: + data = str(result) + return {"success": True, "data": data, "error": None}Verify existing response-unwrapping patterns with:
#!/bin/bash # Description: Inspect agent.run result handling patterns without executing repository code. rg -n -C3 'agent\.run\(input=|hasattr\(result,\s*["'"'"'](content|response)["'"'"']' --iglob '*.py'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/pdf_research_agent/pdf_research_agent.py` around lines 193 - 198, The code returns the raw agent.run() output in result which may be a non-serializable object; unwrap and normalize it before returning by inspecting the agent.run() result (e.g., check for attributes like .content or .response or if it's already a dict/list) and set data to that primitive (string/dict/list) instead of the raw object; update the block that calls agent.run(input=enriched_messages) to extract the actual text/content (fallback to str(result) if necessary) and return that normalized value in the {"success": True, "data": ... , "error": None} response so JSON serialization will succeed.examples/voice-agent/.env.example (1)
19-19:⚠️ Potential issue | 🟡 MinorRemove the inert Piper TTS model setting.
With
VOICE__TTS_PROVIDER=piper, this value is misleading because Piper selection is driven by the voice id/sample rate, notVOICE__TTS_MODEL.Proposed fix
-VOICE__TTS_MODEL=piper-local +# VOICE__TTS_MODEL is only needed for providers that consume model ids.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/voice-agent/.env.example` at line 19, Remove the inert VOICE__TTS_MODEL setting from the .env.example file because Piper selection is determined by VOICE__TTS_PROVIDER (set to "piper") and by the voice id/sample rate, not VOICE__TTS_MODEL; locate and delete the VOICE__TTS_MODEL=piper-local line so the example does not mislead users about configuration.
🧹 Nitpick comments (1)
docs/VOICE_AGENT_PLAN.md (1)
26-26: Minor: add language identifiers to fenced code blocks.markdownlint (MD040) flags blocks at lines 26, 417, 444, 471, 579, 594, 661, 702, 883 as missing a language. For ASCII-art/diagram blocks, use ```text to satisfy the linter without changing rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/VOICE_AGENT_PLAN.md` at line 26, The markdown file has fenced code blocks missing language identifiers (markdownlint MD040). Edit VOICE_AGENT_PLAN.md and update the affected triple-backtick blocks (e.g., the blocks at lines noted in the review) to include an appropriate language tag — use ```text for ASCII-art/diagram blocks and the relevant language (```bash, ```json, ```js, etc.) for real code snippets so the linter stops flagging them; ensure you only add the language after the opening ``` and do not change the block contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/voice-agent/.env.example`:
- Around line 6-10: The example .env currently populates optional
fallback/provider variables (e.g., VOICE__TTS_FALLBACK_PROVIDER,
VOICE__AZURE_TTS_API_KEY, VOICE__AZURE_TTS_REGION, VOICE__AZURE_TTS_VOICE and
the OpenRouter placeholders referenced around lines 31-35) with non-empty
placeholders; change these to empty values or comment them out so copying .env
does not enable integrations with invalid credentials—leave provider unset
(empty) and clear API key/region/voice placeholders to force explicit opt-in by
users.
In `@examples/voice-agent/main.py`:
- Around line 233-243: The example currently hard-codes the "voice" dict in
main.py so changes to the documented environment knobs (VOICE__TTS_PROVIDER,
VOICE__TTS_VOICE_ID, VOICE__VAD_ENABLED, VOICE__SAMPLE_RATE,
VOICE__STT_PROVIDER, VOICE__STT_MODEL, VOICE__STT_LANGUAGE, etc.) have no
effect; replace the static "voice" dict creation with values read from the
environment (e.g., via os.getenv or the project's existing config loader) and
fall back to the current literals as defaults so the keys used elsewhere (the
"voice" dict referenced by the voice setup code) honor the VOICE__* env vars
documented in .env.example.
- Around line 37-44: The environment parsing for OPENROUTER_MEMORY_TURNS and
VOICE_MAX_SENTENCES currently calls int(...) at import time and will crash if
the env value is non-numeric; change these to robustly parse with a safe
fallback: read the raw string via os.environ.get(...), attempt to convert to int
inside a try/except (or use str.isdigit check), and on failure use the intended
default (4 for OPENROUTER_MEMORY_TURNS, 2 for VOICE_MAX_SENTENCES) while also
enforcing the min bounds via max(...). Update the constants
OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES in main.py so invalid values
(e.g., "abc") do not raise and instead fall back to the defaults.
- Around line 182-198: The stream exception handler can fall back and produce a
second answer even after partial output has been yielded; add a flag (e.g.,
yielded_any or had_partial) initialized false before the async-for loop and set
it true whenever you yield clamped/partial output (where built,
_append_stream_piece and _clamp_sentences are used with VOICE_MAX_SENTENCES),
then in the except Exception block avoid running the demo fallback (or
re-raising/returning) if yielded_any is true so you don't emit a second,
unrelated response for the same user turn.
In `@pyproject.toml`:
- Around line 63-64: The pyproject currently lists "agno>=2.5.2" and
"openai>=2.17.0" as mandatory dependencies; change them to optional extras
instead (e.g., under an [project.optional-dependencies] group like "agents" or
"observability") so they are not installed by default, and ensure any mentions
in bindu/observability/openinference.py remain dynamically imported only when
the extra is present; update pyproject.toml to remove those two packages from
the main dependencies and add them to the chosen extras group (named "agents" or
"observability") so installs without the extra do not pull agno/openai.
---
Duplicate comments:
In `@examples/document-analyzer/document_analyzer.py`:
- Line 199: Update the example metadata entry where the "author" field is set to
a personal email (the string "author": "vyomrohila@gmail.com") in
examples/document-analyzer/document_analyzer.py and replace it with a neutral
placeholder or project-owned alias (e.g., "author": "example@example.com" or
"author": "project@example.com") so the public example does not include a
personal email address.
- Around line 175-180: The current logic after calling
_collect_prompt_and_documents(prompt, extracted_docs, errors) only surfaces
errors when no documents were extracted, which hides partial failures; update
the handling around the variables prompt, extracted_docs, and errors so that if
extracted_docs is non-empty but errors is also non-empty you still surface those
errors (e.g., append or return a message listing failed files or include errors
in the response/log alongside successful results), ensuring partial extraction
failures are reported rather than silently ignored.
- Around line 183-194: agent.run() returns a RunOutput object, not the string
response; currently the code assigns that to result and returns it, causing
inconsistency with string error paths—change the return to return the text
payload by returning result.content (i.e., replace the final "return result"
with "return result.content") and optionally guard if result.content is None by
converting to an empty string or raising a clear error; locate the call to
agent.run and the variable result to make this change.
- Around line 76-83: extract_text_from_docx currently returns an empty string on
error which allows callers to append empty/failed extractions as valid
documents; change extract_text_from_docx to return None (or raise a specific
exception) when extraction fails or yields no text, and update the caller that
appends the result (the code that does
documents.append(extract_text_from_docx(...)) or similar) to check the return
value and only append when it's truthy/non-None and non-empty; ensure logging
still records errors in extract_text_from_docx but do not propagate empty
strings upstream.
In `@examples/pdf_research_agent/pdf_research_agent.py`:
- Line 20: The examples use "/path/to/paper.pdf" but ALLOWED_BASE_DIR is
hardcoded to the script's example directory so those example inputs get
rejected; update the code so the sandbox matches the advertised usage by either
(A) making ALLOWED_BASE_DIR configurable (e.g., read from env var or a CLI arg
used by the PDF sandbox/validator) and ensure the PDF access check uses that
configurable ALLOWED_BASE_DIR, or (B) change the example inputs in the example
calls to point to files under the existing ALLOWED_BASE_DIR; look for the
ALLOWED_BASE_DIR variable and the PDF access/validation logic that enforces it
and adjust those spots and all example occurrences of "/path/to/paper.pdf"
(lines around the example usages) accordingly.
- Around line 200-204: The except Exception as e block returns raw exception
text to clients; instead, log the full exception server-side (e.g., use
logger.exception or processLogger.error with the exception/stack) inside that
except handler and change the returned payload from including f"{e}" to a
generic message like "Internal server error" or "An unexpected error occurred"
while preserving success=False and data=None; update the handler around the
except Exception as e to perform secure logging and return the non-sensitive
generic error string.
- Around line 183-185: The branch that returns document_text when it startswith
"[" or "Error" is stale because _read_content() now raises DocumentReadError;
remove this short-circuit so valid document text that begins with those prefixes
isn't prematurely returned and instead flows through the normal
summarization/structured response path—locate the conditional that checks
document_text.startswith("[") or document_text.startswith("Error") (around the
call site of _read_content() and variable document_text) and delete it, relying
on DocumentReadError exceptions for error handling and the existing
structured-response logic to handle successful text.
- Around line 83-92: Catch and chain the ImportError and use an f-string
conversion flag for the generic exception: in the except ImportError block
(around the PDF open code) change it to "except ImportError as e:" and re-raise
DocumentReadError with the original ImportError chained via "from e"; in the
generic except block keep the early "if isinstance(e, DocumentReadError): raise"
check, and when re-raising use an f-string conversion flag (e.g. f"{e!s}")
instead of str(e) and retain the "from e" chaining so both DocumentReadError and
ImportError are properly chained and reported (referencing DocumentReadError and
resolved_path).
- Around line 193-198: The code returns the raw agent.run() output in result
which may be a non-serializable object; unwrap and normalize it before returning
by inspecting the agent.run() result (e.g., check for attributes like .content
or .response or if it's already a dict/list) and set data to that primitive
(string/dict/list) instead of the raw object; update the block that calls
agent.run(input=enriched_messages) to extract the actual text/content (fallback
to str(result) if necessary) and return that normalized value in the {"success":
True, "data": ... , "error": None} response so JSON serialization will succeed.
In `@examples/voice-agent/.env.example`:
- Line 19: Remove the inert VOICE__TTS_MODEL setting from the .env.example file
because Piper selection is determined by VOICE__TTS_PROVIDER (set to "piper")
and by the voice id/sample rate, not VOICE__TTS_MODEL; locate and delete the
VOICE__TTS_MODEL=piper-local line so the example does not mislead users about
configuration.
---
Nitpick comments:
In `@docs/VOICE_AGENT_PLAN.md`:
- Line 26: The markdown file has fenced code blocks missing language identifiers
(markdownlint MD040). Edit VOICE_AGENT_PLAN.md and update the affected
triple-backtick blocks (e.g., the blocks at lines noted in the review) to
include an appropriate language tag — use ```text for ASCII-art/diagram blocks
and the relevant language (```bash, ```json, ```js, etc.) for real code snippets
so the linter stops flagging them; ensure you only add the language after the
opening ``` and do not change the block contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4572beca-8b53-4137-8ba4-32292c38a459
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
SPLIT_PLAN.mddocs/VOICE.mddocs/VOICE_AGENT_PLAN.mdexamples/README.mdexamples/cerina_bindu/cbt/README.mdexamples/document-analyzer/document_analyzer.pyexamples/multilingual-collab-agent/main.pyexamples/multilingual-collab-agent/skills/collaborate/skill.yamlexamples/multilingual-collab-agent/skills/research/skill.yamlexamples/multilingual-collab-agent/skills/translate/skill.yamlexamples/pdf_research_agent/README.mdexamples/pdf_research_agent/pdf_research_agent.pyexamples/pdf_research_agent/skills/pdf-research-skill/skill.yamlexamples/typescript-langchain-agent/quiz-agent.tsexamples/typescript-langchain-quiz-agent/quiz-agent.tsexamples/voice-agent/.env.exampleexamples/voice-agent/README.mdexamples/voice-agent/main.pyexamples/voice-agent/skills/voice-brief-response-skill/skill.yamlpyproject.toml
✅ Files skipped from review due to trivial changes (10)
- examples/README.md
- examples/multilingual-collab-agent/skills/research/skill.yaml
- examples/pdf_research_agent/README.md
- examples/cerina_bindu/cbt/README.md
- examples/multilingual-collab-agent/skills/collaborate/skill.yaml
- SPLIT_PLAN.md
- examples/voice-agent/README.md
- docs/VOICE.md
- examples/typescript-langchain-agent/quiz-agent.ts
- examples/voice-agent/skills/voice-brief-response-skill/skill.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/multilingual-collab-agent/skills/translate/skill.yaml
- examples/multilingual-collab-agent/main.py
- examples/typescript-langchain-quiz-agent/quiz-agent.ts
- examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
| # Optional fallback TTS (recommended for resiliency) | ||
| VOICE__TTS_FALLBACK_PROVIDER=azure | ||
| VOICE__AZURE_TTS_API_KEY=your-azure-speech-key | ||
| VOICE__AZURE_TTS_REGION=eastus | ||
| VOICE__AZURE_TTS_VOICE=en-US-SaraNeural |
There was a problem hiding this comment.
Keep optional provider credentials disabled by default.
Copying this file to .env leaves non-empty placeholder credentials for Azure fallback and OpenRouter. That can activate integrations with invalid credentials instead of cleanly using Piper/demo fallback.
Proposed fix
# Optional fallback TTS (recommended for resiliency)
-VOICE__TTS_FALLBACK_PROVIDER=azure
-VOICE__AZURE_TTS_API_KEY=your-azure-speech-key
-VOICE__AZURE_TTS_REGION=eastus
-VOICE__AZURE_TTS_VOICE=en-US-SaraNeural
+# VOICE__TTS_FALLBACK_PROVIDER=azure
+# VOICE__AZURE_TTS_API_KEY=your-azure-speech-key
+# VOICE__AZURE_TTS_REGION=eastus
+# VOICE__AZURE_TTS_VOICE=en-US-SaraNeural
@@
# Optional LLM backend for smarter responses
-OPENROUTER_API_KEY=your-openrouter-api-key
+# OPENROUTER_API_KEY=your-openrouter-api-key
OPENROUTER_MODEL=openai/gpt-oss-120b:free
OPENROUTER_MEMORY_TURNS=4
VOICE_MAX_SENTENCES=2Also applies to: 31-35
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 8-8: [UnorderedKey] The VOICE__AZURE_TTS_API_KEY key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
[warning] 9-9: [UnorderedKey] The VOICE__AZURE_TTS_REGION key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The VOICE__AZURE_TTS_VOICE key should go before the VOICE__TTS_FALLBACK_PROVIDER key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/voice-agent/.env.example` around lines 6 - 10, The example .env
currently populates optional fallback/provider variables (e.g.,
VOICE__TTS_FALLBACK_PROVIDER, VOICE__AZURE_TTS_API_KEY, VOICE__AZURE_TTS_REGION,
VOICE__AZURE_TTS_VOICE and the OpenRouter placeholders referenced around lines
31-35) with non-empty placeholders; change these to empty values or comment them
out so copying .env does not enable integrations with invalid credentials—leave
provider unset (empty) and clear API key/region/voice placeholders to force
explicit opt-in by users.
| OPENROUTER_MEMORY_TURNS = max( | ||
| 0, | ||
| int(os.environ.get("OPENROUTER_MEMORY_TURNS", "4") or "4"), | ||
| ) | ||
| VOICE_MAX_SENTENCES = max( | ||
| 1, | ||
| int(os.environ.get("VOICE_MAX_SENTENCES", "2") or "2"), | ||
| ) |
There was a problem hiding this comment.
Guard numeric env parsing so a typo does not crash the example.
Line 39 and Line 43 call int(...) at import time, so OPENROUTER_MEMORY_TURNS=abc or VOICE_MAX_SENTENCES=abc prevents the example from starting.
Proposed fix
+def _read_int_env(name: str, default: int, minimum: int) -> int:
+ raw = os.environ.get(name, "").strip()
+ if not raw:
+ return default
+ try:
+ value = int(raw)
+ except ValueError:
+ print(f"Invalid {name}={raw!r}; using default {default}")
+ return default
+ return max(minimum, value)
+
+
OPENROUTER_API_KEY = os.environ.get("OPENROUTER_API_KEY", "").strip()
OPENROUTER_MODEL = os.environ.get("OPENROUTER_MODEL", "openai/gpt-4o-mini").strip()
-OPENROUTER_MEMORY_TURNS = max(
- 0,
- int(os.environ.get("OPENROUTER_MEMORY_TURNS", "4") or "4"),
-)
-VOICE_MAX_SENTENCES = max(
- 1,
- int(os.environ.get("VOICE_MAX_SENTENCES", "2") or "2"),
-)
+OPENROUTER_MEMORY_TURNS = _read_int_env("OPENROUTER_MEMORY_TURNS", 4, 0)
+VOICE_MAX_SENTENCES = _read_int_env("VOICE_MAX_SENTENCES", 2, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/voice-agent/main.py` around lines 37 - 44, The environment parsing
for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES currently calls int(...) at
import time and will crash if the env value is non-numeric; change these to
robustly parse with a safe fallback: read the raw string via
os.environ.get(...), attempt to convert to int inside a try/except (or use
str.isdigit check), and on failure use the intended default (4 for
OPENROUTER_MEMORY_TURNS, 2 for VOICE_MAX_SENTENCES) while also enforcing the min
bounds via max(...). Update the constants OPENROUTER_MEMORY_TURNS and
VOICE_MAX_SENTENCES in main.py so invalid values (e.g., "abc") do not raise and
instead fall back to the defaults.
| built = "" | ||
| async for chunk in stream: | ||
| delta = chunk.choices[0].delta.content if chunk.choices else None | ||
| if not delta: | ||
| continue | ||
| built = _append_stream_piece(built, delta) | ||
| clamped, done = _clamp_sentences(built, VOICE_MAX_SENTENCES) | ||
| if clamped: | ||
| yield clamped | ||
| if done: | ||
| break | ||
|
|
||
| if not built.strip(): | ||
| yield "I couldn't generate a response this time. Please try again." | ||
| return | ||
| except Exception as exc: | ||
| print(f"OpenRouter error: {exc}") |
There was a problem hiding this comment.
Do not fall back after already yielding partial LLM output.
If the OpenRouter stream raises after Line 190 has yielded text, execution falls through to the demo fallback and produces a second, unrelated answer for the same user turn.
Proposed fix
+ emitted_any = False
built = ""
async for chunk in stream:
delta = chunk.choices[0].delta.content if chunk.choices else None
if not delta:
continue
built = _append_stream_piece(built, delta)
clamped, done = _clamp_sentences(built, VOICE_MAX_SENTENCES)
if clamped:
yield clamped
+ emitted_any = True
if done:
break
if not built.strip():
yield "I couldn't generate a response this time. Please try again."
return
except Exception as exc:
print(f"OpenRouter error: {exc}")
+ if "emitted_any" in locals() and emitted_any:
+ return🧰 Tools
🪛 Ruff (0.15.10)
[warning] 197-197: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/voice-agent/main.py` around lines 182 - 198, The stream exception
handler can fall back and produce a second answer even after partial output has
been yielded; add a flag (e.g., yielded_any or had_partial) initialized false
before the async-for loop and set it true whenever you yield clamped/partial
output (where built, _append_stream_piece and _clamp_sentences are used with
VOICE_MAX_SENTENCES), then in the except Exception block avoid running the demo
fallback (or re-raising/returning) if yielded_any is true so you don't emit a
second, unrelated response for the same user turn.
| "voice": { | ||
| "stt_provider": "deepgram", | ||
| "stt_model": "nova-3", | ||
| "stt_language": "en", | ||
| "tts_provider": "piper", | ||
| "tts_voice_id": "en_US-ryan-high", | ||
| "tts_model": "piper-local", | ||
| "sample_rate": 16000, | ||
| "allow_interruptions": True, | ||
| "vad_enabled": False, | ||
| }, |
There was a problem hiding this comment.
Wire the voice config to the documented .env knobs.
The example hard-codes provider/model/audio settings here, while .env.example documents VOICE__TTS_PROVIDER, VOICE__TTS_VOICE_ID, VOICE__VAD_ENABLED, VOICE__SAMPLE_RATE, etc. If bindufy consumes this explicit config, edits to those env vars will not affect the example.
Proposed fix
"voice": {
- "stt_provider": "deepgram",
- "stt_model": "nova-3",
- "stt_language": "en",
- "tts_provider": "piper",
- "tts_voice_id": "en_US-ryan-high",
- "tts_model": "piper-local",
- "sample_rate": 16000,
- "allow_interruptions": True,
- "vad_enabled": False,
+ "stt_provider": os.environ.get("VOICE__STT_PROVIDER", "deepgram").strip() or "deepgram",
+ "stt_model": os.environ.get("VOICE__STT_MODEL", "nova-3").strip() or "nova-3",
+ "stt_language": os.environ.get("VOICE__STT_LANGUAGE", "en").strip() or "en",
+ "tts_provider": os.environ.get("VOICE__TTS_PROVIDER", "piper").strip() or "piper",
+ "tts_voice_id": os.environ.get("VOICE__TTS_VOICE_ID", "en_US-ryan-high").strip() or "en_US-ryan-high",
+ "tts_model": os.environ.get("VOICE__TTS_MODEL", "").strip() or None,
+ "sample_rate": _read_int_env("VOICE__SAMPLE_RATE", 16000, 1),
+ "allow_interruptions": os.environ.get("VOICE__ALLOW_INTERRUPTIONS", "true").lower() == "true",
+ "vad_enabled": os.environ.get("VOICE__VAD_ENABLED", "false").lower() == "true",
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/voice-agent/main.py` around lines 233 - 243, The example currently
hard-codes the "voice" dict in main.py so changes to the documented environment
knobs (VOICE__TTS_PROVIDER, VOICE__TTS_VOICE_ID, VOICE__VAD_ENABLED,
VOICE__SAMPLE_RATE, VOICE__STT_PROVIDER, VOICE__STT_MODEL, VOICE__STT_LANGUAGE,
etc.) have no effect; replace the static "voice" dict creation with values read
from the environment (e.g., via os.getenv or the project's existing config
loader) and fall back to the current literals as defaults so the keys used
elsewhere (the "voice" dict referenced by the voice setup code) honor the
VOICE__* env vars documented in .env.example.
| "agno>=2.5.2", | ||
| "openai>=2.17.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether agno/openai are imported unconditionally at package import time
rg -nP '^\s*(from|import)\s+(agno|openai)\b' --type=py -g '!tests/**' -g '!examples/**'Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the full dependency structure in pyproject.toml around those lines
cat -n pyproject.toml | head -100Repository: GetBindu/Bindu
Length of output: 3362
🏁 Script executed:
# Search for conditional imports or try/except patterns with agno/openai
rg -nA5 -B5 'try:|except' --type=py -g '!tests/**' -g '!examples/**' | rg -A5 -B5 'agno|openai'Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the agents extra definition in pyproject.toml
rg -nA20 '\[project.optional-dependencies\]|\[project.extras\]|agents' pyproject.tomlRepository: GetBindu/Bindu
Length of output: 757
🏁 Script executed:
# Search for any references to agno or openai in Python files
rg 'agno|openai' --type=py -g '!tests/**' -g '!examples/**' -nRepository: GetBindu/Bindu
Length of output: 828
🏁 Script executed:
# Also check if they appear in __init__.py or entry points
fd '(__init__|setup|__main__).py' --type=py | head -20Repository: GetBindu/Bindu
Length of output: 227
🏁 Script executed:
# Check if agno/openai are imported in settings.py and openinference.py
rg -n 'import' bindu/settings.py | head -30Repository: GetBindu/Bindu
Length of output: 269
🏁 Script executed:
# Check openinference.py for imports
cat -n bindu/observability/openinference.py | head -80Repository: GetBindu/Bindu
Length of output: 3997
🏁 Script executed:
# Check the main package __init__.py
cat -n bindu/__init__.pyRepository: GetBindu/Bindu
Length of output: 475
Move agno and openai back to optional dependencies.
agno>=2.5.2 and openai>=2.17.0 are now mandatory runtime dependencies (lines 63–64), forcing every pip install bindu installation to pull both packages regardless of use case. However, they are only referenced in bindu/observability/openinference.py as optional instrumentation providers in a dynamically-detected frameworks list—they are never imported unconditionally at package init. This adds unnecessary bloat and licensing surface area for users who only need the core protocol framework without observability/LLM support. Keep them optional (under the agents extra or a dedicated observability extra) unless they're actually imported at import time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 63 - 64, The pyproject currently lists
"agno>=2.5.2" and "openai>=2.17.0" as mandatory dependencies; change them to
optional extras instead (e.g., under an [project.optional-dependencies] group
like "agents" or "observability") so they are not installed by default, and
ensure any mentions in bindu/observability/openinference.py remain dynamically
imported only when the extra is present; update pyproject.toml to remove those
two packages from the main dependencies and add them to the chosen extras group
(named "agents" or "observability") so installs without the extra do not pull
agno/openai.
2 participants
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Verification
Environment
Steps to Test
Expected Behavior
Actual Behavior
Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
Compatibility / Migration
Yes/No) YesYes/No) No (documentation-only)Yes/No) NoFailure Recovery (if this breaks)
docs/and README files only.Risks and Mitigations
None
Checklist
uv run pytest)uv run pre-commit run --all-files)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores