Conversation
📝 WalkthroughWalkthroughA real-time voice conversation feature was added: voice extension, STT/TTS service factories, AgentBridge pipecat processor, session managers (in-memory + Redis), server REST/WebSocket endpoints, frontend client/UI/stores, examples, tests, dependency tooling, and related utility updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Browser Frontend
participant Backend as Voice Endpoints
participant SessionMgr as Session Manager
participant Bridge as Agent Bridge
participant Agent as Agent Handler
participant STT as STT Provider
participant TTS as TTS Provider
User->>Frontend: Start voice session
Frontend->>Backend: POST /voice/session/start
Backend->>SessionMgr: create_session(context_id)
SessionMgr-->>Backend: session_id, ws_url
Backend-->>Frontend: 201 {session_id, ws_url}
Frontend->>Backend: WebSocket connect /ws/voice/{session_id}
Backend->>Backend: authenticate & build_voice_pipeline()
Frontend->>Backend: send binary audio frames
Backend->>STT: stream audio
STT-->>Backend: TranscriptionFrame(text)
Backend->>Bridge: process_transcription(text)
Bridge->>Agent: manifest_run(messages)
Agent-->>Bridge: streamed/text response
Bridge-->>Backend: TextFrame(s)
Backend->>TTS: synthesize(text)
TTS-->>Backend: audio chunks
Backend-->>Frontend: send audio binary frames + transcript frames
Frontend->>User: play audio
sequenceDiagram
participant Client
participant Factory
participant Env
participant Pipecat
Client->>Factory: create_tts_service(config)
Factory->>Env: read provider & api keys
alt missing creds
Factory-->>Client: raise ValueError
else provider supported
Factory->>Pipecat: dynamic import provider module
Pipecat-->>Factory: TTSService class
Factory-->>Client: instantiated service
else failure and fallback configured
Factory->>Factory: try fallback provider
alt fallback succeeds
Factory-->>Client: fallback service
else
Factory-->>Client: raise RuntimeError
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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: 1
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 (4)
bindu/server/handlers/message_handlers.py (1)
119-122:⚠️ Potential issue | 🟠 MajorMove metadata sanitization before
submit_task()to prevent persisting sensitive_payment_context.At Line 122,
messageis persisted to storage before the cleanup at Lines 141-158. For PostgresStorage, the message is serialized to JSON during insertion, creating a database copy that retains_payment_contextand malformed metadata. For MemoryStorage, the message object is stored by reference, but relying on subsequent mutations to clean persisted data is fragile and unsafe.Metadata validation and
_payment_contextremoval must occur beforesubmit_task(...)to ensure no storage backend retains sensitive information.🔧 Suggested change
message = request_params["message"] context_id = self.context_id_parser(message.get("context_id")) + message_metadata = message.get("metadata") + if message_metadata is None: + message_metadata = {} + message["metadata"] = message_metadata + elif not isinstance(message_metadata, dict): + logger.warning( + "Invalid metadata type received in message", + extra={"type": type(message_metadata).__name__}, + ) + message["metadata"] = {} + message_metadata = message["metadata"] + + payment_context = message_metadata.pop("_payment_context", None) + task: Task = await self.storage.submit_task(context_id, message) @@ - message_metadata = message.get("metadata") - ... - payment_context = message_metadata.pop("_payment_context", None) if payment_context is not None: scheduler_params["payment_context"] = payment_context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/handlers/message_handlers.py` around lines 119 - 122, Move the metadata sanitization and removal of sensitive fields so it runs before persisting the task: validate and clean message["metadata"] (and remove message["_payment_context"]) prior to calling self.storage.submit_task; update the code around context_id = self.context_id_parser(message.get("context_id")) and the subsequent submit_task call to use the sanitized message object so that Task storage (submit_task) never receives or serializes sensitive/malformed data.bindu/penguin/config_validator.py (1)
72-94:⚠️ Potential issue | 🟠 MajorNormalize all validation failures to
ConfigError.
validate_and_process()now introducesConfigError, but later validation paths still raise plainValueError(_validate_field_types, auth validation,debug_levelchecks, etc.). Callers that switched to catchingConfigErrorwill still miss a subset of invalid configs.Proposed fix
`@classmethod` def validate_and_process(cls, config: Dict[str, Any]) -> Dict[str, Any]: """Validate and process agent configuration.""" if not isinstance(config, dict): raise ConfigError( "ConfigError: configuration must be a dictionary passed to bindufy(config=...)." ) - # Validate required fields first (fail-fast) - cls._validate_required_fields(config) - - # Start with defaults - processed_config = cls.DEFAULTS.copy() - - # Update with provided config - processed_config.update(config) - - # Process complex fields - processed_config = cls._process_complex_fields(processed_config) - - # Validate field types - cls._validate_field_types(processed_config) - - return processed_config + try: + cls._validate_required_fields(config) + + processed_config = cls.DEFAULTS.copy() + processed_config.update(config) + processed_config = cls._process_complex_fields(processed_config) + cls._validate_field_types(processed_config) + return processed_config + except ValueError as exc: + if isinstance(exc, ConfigError): + raise + raise ConfigError(f"ConfigError: {exc}") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/penguin/config_validator.py` around lines 72 - 94, validate_and_process introduces ConfigError but other validators still raise ValueError (e.g., _validate_field_types, auth validation code paths, debug_level checks), causing inconsistent error types; update all validation functions and code paths referenced by validate_and_process—specifically _validate_required_fields, _validate_field_types, _process_complex_fields and any inline auth/debug checks—to raise ConfigError instead of ValueError (or catch existing ValueError/TypeError within validate_and_process and re-raise as ConfigError), ensuring every validation failure surface is normalized to ConfigError.examples/document-analyzer/document_analyzer.py (1)
133-165:⚠️ Potential issue | 🟠 MajorThe advertised chat-format path never produces any extracted documents.
When messages come from the runtime worker, uploaded files have already been flattened into
msg["content"]. This branch only appends that content toprompt_parts, sohandler()falls into"No valid document found..."for the manifest-worker path it claims to support.🤖 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 133 - 165, The chat-format branch only appends msg["content"] to prompt_parts and never extracts files, so handler() never fills extracted_docs for runtime/manifest-worker messages; update the content handling in document_analyzer.py so that when content is a non-empty string you first attempt to parse it as JSON (or detect embedded file structures) and if it contains file(s) or parts, run the same extraction logic as the compatibility path: decode base64 bytes, call extract_document_text(...) and append results to extracted_docs, adding any exceptions to errors; otherwise, keep the original behavior of appending the raw string to prompt_parts. Ensure you reference msg, content, prompt_parts, extracted_docs, errors, extract_document_text, and handler() so the new branch mirrors the existing parts/file handling.bindu/server/applications.py (1)
412-447:⚠️ Potential issue | 🟠 MajorTear down the voice session manager on startup failures too.
Once
start_cleanup_loop()succeeds, any later exception beforeyieldskips the shutdown block entirely. That leaves the background cleanup loop running and the session manager unclosed if TaskManager startup fails. Wrap this lifecycle intry/finally(orAsyncExitStack) so partial startup still cleans it up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/applications.py` around lines 412 - 447, The voice session manager is started (create_session_manager + start_cleanup_loop) but not guaranteed to be closed if an exception occurs before the yield, leaving the background loop running; wrap the lifecycle in a try/finally (or use an AsyncExitStack) so that after app._voice_session_manager is created and start_cleanup_loop returns you proceed with startup inside a try block and always run a finally block that calls close_session_manager(app._voice_session_manager) (and logs) if app._voice_session_manager is set; also ensure you don’t double-stop it (check for truthiness) and retain existing TaskManager startup/teardown inside the try so partial startup cleans up the voice session manager on failures.
🟠 Major comments (27)
bindu/extensions/did/did_agent_extension.py-243-255 (1)
243-255:⚠️ Potential issue | 🟠 MajorUse Windows APIs to resolve the current process identity instead of
USERNAMEenvironment variable.Line 243 retrieves the trustee from
os.getenv("USERNAME"), but this environment variable is not reliably set in Windows services, scheduled tasks, or domain/local account scenarios. This can cause the ACL hardening to fail with a RuntimeError or, if the variable is set to a different user context, silently grant access to the wrong principal. Use Windows APIs (via ctypes or pywin32) to resolve the actual token identity and SID of the current process, then apply the ACL grant to that authenticated principal instead.🤖 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 - 255, The code currently uses os.getenv("USERNAME") to build the icacls grant (variable username and cmd), which is unreliable; replace that with a call that resolves the actual current process identity/SID (e.g., implement a helper like get_current_process_sid() using Windows APIs via pywin32 or ctypes to call OpenProcessToken/GetTokenInformation/LookupAccountSid or equivalent), and use the returned SID or account name when constructing the icacls command (instead of username) to grant access to the real process principal; ensure get_current_process_sid() raises a clear error on failure and that the calling code (where username and cmd are set) catches/propagates that error.bindu/extensions/did/did_agent_extension.py-213-218 (1)
213-218:⚠️ Potential issue | 🟠 MajorWrite private key to a temporary file and harden ACL before placing on disk.
On Windows, the code writes the private key directly to disk at line 215 with inherited directory ACLs, only attempting to harden the ACL afterward at line 217. If
_harden_windows_key_file_acl()fails, the private key persists on disk with overly broad permissions. The POSIX branch correctly sets permissions atomically during file creation; the Windows branch should use a similar atomic pattern. Write both key files to temporary locations first, harden their ACLs, and only then move them to their final paths. Clean up temporary files if ACL hardening fails.Suggested approach
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) + private_tmp = self.private_key_path.with_suffix(".tmp") + public_tmp = self.public_key_path.with_suffix(".tmp") + try: + private_tmp.write_bytes(private_pem) + self._harden_windows_key_file_acl(private_tmp) + private_tmp.replace(self.private_key_path) + + public_tmp.write_bytes(public_pem) + self._harden_windows_key_file_acl(public_tmp) + public_tmp.replace(self.public_key_path) + except Exception: + private_tmp.unlink(missing_ok=True) + public_tmp.unlink(missing_ok=True) + raise🤖 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, On Windows, avoid writing private/public keys directly to final paths; instead write private_pem and public_pem to unique temporary files (e.g., sibling temp names), call _harden_windows_key_file_acl() on each temp file and verify success, then atomically replace/move the temp files to self.private_key_path and self.public_key_path (preserving existing atomicity semantics), and if hardening fails remove the temp files and raise/log the error—update the block that currently writes to self.private_key_path/self.public_key_path and calls _harden_windows_key_file_acl() afterwards to perform temp-write→harden→atomic-move with cleanup on failure..github/workflows/ci.yml-43-43 (1)
43-43:⚠️ Potential issue | 🟠 MajorUnit test job now collects non-unit suites.
uv run pytest tests/broadens collection beyond unit scope and can pull in integration tests that are neithere2enorslow, which undermines the “fast, hermetic” unit job contract.💡 Suggested fix
- uv run pytest tests/ \ + uv run pytest tests/unit/ \ --cov=bindu \ --cov-report=term-missing \ --cov-report=xml:coverage.xml \ --cov-fail-under=60 \ -vBased 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 runs the broad command "uv run pytest tests/" which collects non-unit suites; change it to target only unit tests by running pytest against the unit directory (e.g., replace "uv run pytest tests/" with a command that runs "uv run pytest tests/unit/" or an equivalent pattern that restricts discovery to tests/unit/**) and ensure async tests under tests/unit/ use pytest.mark.asyncio as needed for proper execution.bindu/server/scheduler/memory_scheduler.py-31-33 (1)
31-33:⚠️ Potential issue | 🟠 MajorMove queue size and send timeout to
app_settings(no hardcoded scheduler config).
_TASK_QUEUE_BUFFER_SIZE = 100andfail_after(5.0)introduce local hardcoded config inbindu/**. Please source both values frombindu.settings.app_settingsinstead.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: 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 hardcoded scheduler constants with values from app_settings: import app_settings from bindu.settings, remove the local _TASK_QUEUE_BUFFER_SIZE constant and instead use app_settings.SCHEDULER_TASK_QUEUE_SIZE where the queue is constructed, and replace the literal fail_after(5.0) calls with fail_after(app_settings.SCHEDULER_SEND_TIMEOUT) (also update the other occurrence around the code paths referenced near lines 80-81); ensure the new app_settings keys exist (SCHEDULER_TASK_QUEUE_SIZE and SCHEDULER_SEND_TIMEOUT) or add them to app_settings so no local hardcoded values remain in memory_scheduler.py.frontend/src/lib/components/chat/ChatMessage.svelte-70-83 (1)
70-83:⚠️ Potential issue | 🟠 MajorExclude the current wrapper from the interactive-element guard.
closest("[role='button']")now matches the assistant container itself at Line 271, so every click exits early. That breaks tap toggling and also prevents image clicks from opening the lightbox.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 interactiveAncestor = target?.closest( + "button, a, input, textarea, select, [role='button'], [data-no-toggle]" + ); + if (interactiveAncestor && interactiveAncestor !== e.currentTarget) { return; } handleContentClick(e); if (!(e.target instanceof HTMLImageElement)) { 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 - 83, The interactive-element guard in handleMessageClick is too broad because target.closest(...) can match the message wrapper itself; fix by capturing the message container (const container = e.currentTarget as HTMLElement) and change the guard to ignore matches that are the container itself (i.e., const clickInteractive = target && target.closest("button, a, input, textarea, select, [role='button'], [data-no-toggle]") && target.closest(...) !== container) or equivalent so that only ancestor interactive elements other than the wrapper short-circuit; keep the existing calls to handleContentClick and the image check (e.g., isTapped toggle) unchanged.examples/voice-agent/skills/voice-brief-response-skill/skill.yaml-1-47 (1)
1-47:⚠️ Potential issue | 🟠 MajorFix manifest schema keys to match loader expectations.
This skill uses
skill_idinstead ofid, andinput_structure/output_formatinstead ofinput_modes/output_modes. The loader (bindu/utils/skills/loader.py) and all skill endpoints expect the standard keys. Change:
skill_id→idinput_structure→input_modesoutput_format→output_modesWithout these changes, the skill will not be recognized by the loader, and skill lookups will fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/voice-agent/skills/voice-brief-response-skill/skill.yaml` around lines 1 - 47, Update the skill manifest keys to match the loader's expected schema: rename the top-level key "skill_id" to "id", rename "input_structure" to "input_modes", and rename "output_format" to "output_modes", preserving their existing values and structure; ensure the manifest still contains the same "name", "version", "description", "capabilities", "tags", "documentation", and "assessment" entries so the loader in bindu/utils/skills/loader.py can correctly recognize the skill.bindu/settings.py-1025-1133 (1)
1025-1133:⚠️ Potential issue | 🟠 MajorAdd bounds to the new voice env fields.
These values come straight from environment variables, but the model currently accepts invalid numbers like
VOICE__AUDIO_CHANNELS=0,VOICE__SESSION_TIMEOUT=0, or negative retry/backoff settings. That lets startup succeed and pushes the failure deeper into the voice pipeline.Representative fix
- sample_rate: int = 16000 - audio_channels: int = 1 + sample_rate: int = Field(default=16000, gt=0) + audio_channels: int = Field(default=1, ge=1) - chunk_throttle_ms: int = 800 + chunk_throttle_ms: int = Field(default=800, ge=0) - session_timeout: int = 300 # seconds (5 min) - max_concurrent_sessions: int = 10 + session_timeout: int = Field(default=300, ge=1) # seconds (5 min) + max_concurrent_sessions: int = Field(default=10, ge=1) - session_token_ttl: int = 300 # seconds; must be <= session_timeout - rate_limit_per_ip_per_minute: int = 120 + session_token_ttl: int = Field(default=300, ge=1) # seconds; must be <= session_timeout + rate_limit_per_ip_per_minute: int = Field(default=120, ge=0)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/settings.py` around lines 1025 - 1133, VoiceSettings accepts invalid numeric env values (e.g., audio_channels=0, session_timeout=0, negative retry/backoff) because fields lack bounds; add validation by declaring Field constraints on the affected attributes (e.g., audio_channels, sample_rate, audio_sample_width_bytes, chunk_overlap_fraction, chunk_throttle_ms, vad_threshold, agent_timeout_secs, utterance_timeout_secs, retry_attempts, retry_backoff_start_ms, retry_backoff_factor, retry_backoff_max_ms, cancellation_grace_secs, conversation_history_limit, session_timeout, max_concurrent_sessions, redis_session_ttl, session_token_ttl, rate_limit_per_ip_per_minute) using ge/gt/le as appropriate and/or add a model_validator (or validators) in the VoiceSettings class (extend _validate_session_token_ttl or add a new `@model_validator`) to clamp or raise on out-of-range values so env-provided numbers are validated at startup.examples/document-analyzer/document_analyzer.py-76-83 (1)
76-83:⚠️ Potential issue | 🟠 MajorPropagate DOCX parse failures instead of returning
"".Returning an empty string here makes
_collect_prompt_and_documents()count the file as successfully extracted, sohandler()can call the model with blank document content instead of surfacing the upload failure.🤖 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 parsing errors and returns an empty string, which makes _collect_prompt_and_documents treat failed DOCX uploads as successful; change extract_text_from_docx to propagate failures instead of returning "" by either removing the broad try/except or re-raising the caught exception after logging (i.e., keep logger.error but raise the exception), so callers like _collect_prompt_and_documents and handler will receive the error and can surface the upload failure rather than calling the model with blank content.bindu/extensions/voice/voice_agent_extension.py-31-61 (1)
31-61:⚠️ Potential issue | 🟠 MajorResolve default voice values from
app_settings.voice, not hardcoded literals.A plain
VoiceAgentExtension()still hardcodesdeepgram/elevenlabs/16000, so changingapp_settings.voicehas no effect unless every field is passed manually. The constructor should resolve defaults fromvoice_settings or app_settings.voicefirst, then validate the resolved sample rate.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; 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/voice_agent_extension.py` around lines 31 - 61, The constructor for VoiceAgentExtension currently hardcodes defaults instead of using app_settings.voice; change __init__ to first load defaults = app_settings.voice (or assign self.voice_settings = voice_settings or app_settings.voice), then resolve each config field (stt_provider, stt_model, stt_language, tts_provider, tts_voice_id, tts_model, sample_rate, allow_interruptions, vad_enabled, description) by using the provided argument when not None/empty else defaults.<br/>After resolving and assigning self.* attributes, call validate_sample_rate(self.sample_rate); keep the validate_sample_rate import and ensure no hardcoded literals remain so all defaults come from voice_settings/app_settings.voice and passed args override them.bindu/utils/worker/messages.py-21-21 (1)
21-21: 🛠️ Refactor suggestion | 🟠 MajorMove the upload-size limit into
app_settings.This adds another hardcoded 10MB ceiling inside
bindu, so the worker-side validator can drift from the upload endpoint and frontend limits. Read the limit frombindu.settings.app_settingsinstead of a module constant.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; 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/utils/worker/messages.py` at line 21, Replace the hardcoded MAX_FILE_SIZE constant in bindu/utils/worker/messages.py with a value read from bindu.settings.app_settings: import app_settings from bindu.settings and use the app_settings file-upload setting (e.g., app_settings.FILE_UPLOAD_MAX_SIZE or the exact upload limit constant used by the upload endpoint); if that attribute may be missing, fall back to the current 10 * 1024 * 1024 default. Update any code that referenced MAX_FILE_SIZE to read from app_settings (or the local fallback) so the worker validator uses the same centralized config as the endpoint/frontend.frontend/src/lib/server/database.ts-99-106 (1)
99-106:⚠️ Potential issue | 🟠 MajorNormalize caller-supplied
_idbefore returning it asObjectId.
insertOne()preserves any truthy_idbut returns{ insertedId: ObjectId }. The cast on line 103 is compile-time only and does nothing at runtime. If a caller inserts{ _id: "abc" }, the returnedinsertedIdis a string, not anObjectId. This breaks the type contract and diverges from MongoDB, where a string_idis preserved as-is. Callers relying onObjectIdmethods would crash. This issue also propagates toinsertMany()(line 113), which collects unvalidatedinsertedIdvalues.🤖 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 99 - 106, insertOne() currently casts a caller-supplied _id to ObjectId at compile time but may return a non-ObjectId at runtime; change the logic to normalize the id value: check if source._id exists — if it's an ObjectId use it, if it's another type (e.g. string) preserve it as-is, and if missing generate a new ObjectId; assign that actual id to docWithId (use the runtime id variable) and return insertedId equal to that actual id. Apply the same fix in insertMany(): for each doc determine the runtime id the same way, store docs with that id in this.data, and collect/return the actual insertedId values (not a forced ObjectId cast).frontend/src/lib/server/database.ts-496-512 (1)
496-512:⚠️ Potential issue | 🟠 MajorEmit download events once per stream lifecycle, not once per listener registration.
Both
on()andonce()independently schedule event emissions viasetTimeoutevery time a listener is attached. If multiple listeners are registered before the timeout fires, this mock will emit duplicate chunks/events to each listener, which does not match Node.js readable-stream 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 496 - 512, The mock currently schedules emissions inside both on() and once(), causing duplicate emissions when multiple listeners register; update the downloadStream logic so emissions for each event ("error","data","end") are scheduled at most once per stream lifecycle—e.g., add per-event flags (scheduled/emitted) in the closure and only call setTimeout(() => emit(...)) when the flag for that event is false, setting it to true thereafter; apply the same guard for both on() and once() and ensure the once() wrapped listener still removes itself before invoking the callback.bindu/utils/capabilities.py-60-75 (1)
60-75:⚠️ Potential issue | 🟠 MajorUse manifest extension access instead of scanning
capabilities.extensions.On Line 71, the new voice lookup reads from
manifest.capabilities. The project rule is to use manifest auto-loaded extensions directly, not capability-list scanning.🔧 Proposed fix
def get_voice_extension_from_capabilities(manifest: Any) -> Optional[Any]: @@ from bindu.extensions.voice import VoiceAgentExtension - for ext in manifest.capabilities.get("extensions", []): - if isinstance(ext, VoiceAgentExtension): - return ext - - return None + voice_ext = getattr(manifest, "voice", None) + return voice_ext if isinstance(voice_ext, VoiceAgentExtension) else NoneAs per coding guidelines "Extensions are auto-loaded from manifest - access via
app.manifest.{extension_name}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/capabilities.py` around lines 60 - 75, The function get_voice_extension_from_capabilities should stop scanning manifest.capabilities and instead access the auto-loaded extension directly from the manifest; replace the for-loop that inspects manifest.capabilities.get("extensions", []) with a direct attribute lookup like getattr(manifest, "voice", None) (or manifest.voice) and then verify it is a VoiceAgentExtension (referencing VoiceAgentExtension) before returning it; keep the function signature and return None if the attribute is missing or not an instance of VoiceAgentExtension.bindu/penguin/bindufy.py-498-499 (1)
498-499:⚠️ Potential issue | 🟠 MajorVoice config is silently ignored for empty dicts.
if voice_config and isinstance(voice_config, dict)skips{}entirely, so default-driven voice initialization never runs and misconfiguration can be silently hidden.💡 Suggested 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 try: voice_extension = VoiceAgentExtension(**voice_config)🤖 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, The current guard "if voice_config and isinstance(voice_config, dict):" skips empty dicts so default-driven voice initialization is never run; change the condition to allow empty dicts by checking for None instead, e.g. use "if voice_config is not None and isinstance(voice_config, dict):" (or simply "if isinstance(voice_config, dict):") around the import/initialization of VoiceAgentExtension in bindufy.py so that {} triggers the default voice setup rather than being silently ignored.frontend/src/lib/server/models.ts-16-22 (1)
16-22:⚠️ Potential issue | 🟠 MajorThese metadata fields never get hydrated.
buildModels()only consumesfetchBinduAgent(), and neither the fallback config nor the fetched-agent mapping copieswebsiteUrl,modelUrl,datasetName,datasetUrl,logoUrl,promptExamples, orproviders. In this module they will therefore always stayundefined, so the new frontend surface never gets real data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/models.ts` around lines 16 - 22, buildModels() currently builds models only from fetchBinduAgent() but never copies metadata fields so websiteUrl, modelUrl, datasetName, datasetUrl, logoUrl, promptExamples, and providers remain undefined; update the model construction logic in buildModels() (and in the fallback config mapping path) to copy these fields from the fetched agent object (returned by fetchBinduAgent()) or from the fallback config into the resulting model entries, ensuring the same property names (websiteUrl, modelUrl, datasetName, datasetUrl, logoUrl, promptExamples, providers) are assigned when present and preserved when missing.bindu/server/applications.py-112-115 (1)
112-115: 🛠️ Refactor suggestion | 🟠 MajorRead the voice extension from
self.manifestinstead of re-deriving it.This introduces a second extension-resolution path in the app initializer, which can drift from the manifest-loaded extension instance the rest of the server is supposed to use.
Based on learnings: Extensions are auto-loaded from manifest - access via
app.manifest.{extension_name}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/applications.py` around lines 112 - 115, The code re-derives the voice extension using get_voice_extension_from_capabilities(manifest) instead of using the already-loaded manifest instance; replace that call and assignment so the initializer reads the voice extension directly from the manifest (e.g., use self.manifest.voice or app.manifest.voice consistent with how x402_ext is obtained), remove get_voice_extension_from_capabilities usage, and ensure any subsequent references use the manifest-held extension (voice_ext) to avoid a second resolution path.examples/voice-agent/.env.example-6-10 (1)
6-10:⚠️ Potential issue | 🟠 MajorDon't enable the Azure fallback by default.
Line 7 turns an “optional” fallback into extra required setup: anyone copying this file verbatim now has to provide valid Azure creds on Lines 8-10 or remember to delete the block before the example starts cleanly. For a runnable example, optional providers should stay opt-in.
Suggested change
-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🤖 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, Remove the default Azure fallback by making VOICE__TTS_FALLBACK_PROVIDER opt-in (e.g., set VOICE__TTS_FALLBACK_PROVIDER= or comment it out) and either comment out or remove the accompanying VOICE__AZURE_TTS_API_KEY, VOICE__AZURE_TTS_REGION, and VOICE__AZURE_TTS_VOICE lines so users copying the example aren't forced to supply Azure credentials; keep the Azure vars present as commented examples or in an “optional” block with a short inline note that they are optional.frontend/src/lib/components/voice/LiveTranscript.svelte-11-23 (1)
11-23:⚠️ Potential issue | 🟠 MajorAnnounce transcript updates to assistive tech.
This panel updates live, but it is not exposed as a live region, so screen-reader users won't hear new utterances as they arrive. Add
role="log"plusaria-live="polite"/aria-relevant="additions text"on the container.Suggested change
-<div class="flex max-h-72 flex-col gap-2 overflow-y-auto rounded-lg border border-gray-200 bg-white/80 p-3 text-sm dark:border-gray-700 dark:bg-gray-900/80"> +<div + class="flex max-h-72 flex-col gap-2 overflow-y-auto rounded-lg border border-gray-200 bg-white/80 p-3 text-sm dark:border-gray-700 dark:bg-gray-900/80" + role="log" + aria-live="polite" + aria-relevant="additions text" +>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/voice/LiveTranscript.svelte` around lines 11 - 23, The live transcript container in LiveTranscript.svelte isn't exposed as a live region; update the outermost div (the one rendering the transcript and using {`#if` items.length}) to include accessibility attributes role="log", aria-live="polite", and aria-relevant="additions text" so screen readers announce new items (keep these attributes on the container that renders the {`#each` items as item} list).tests/conftest_stubs.py-49-50 (1)
49-50:⚠️ Potential issue | 🟠 MajorFix OpenTelemetry stub to preserve active span context.
The test stub's
get_current_span()returns a fresh_Span()instance each time anduse_span(span)ignores the supplied span parameter. Worker code relies on this contract to restore trace context after task serialization: it reconstructs a remote parent span and callsuse_span(span)to activate it, then expects subsequentget_current_span()calls to return the activated span. With these stubs, the activated span is never preserved, so tests cannot exercise remote parent propagation semantics and may incorrectly pass/fail.Affected locations in conftest_stubs.py
- Lines 49-50, 73-74:
get_span_context()methods have correct structure but won't work without context preservation- Lines 76-77:
get_current_span()always returns fresh instance- Line 106:
use_span = lambda span: _SpanCtx()discards the span parameter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest_stubs.py` around lines 49 - 50, The stubs must preserve an active span so remote parent propagation works: change get_current_span() to return a stored active span (not a fresh _Span), have use_span(span) set that stored active span to the supplied span and return a context manager that restores the previous active span on exit, and ensure get_span_context() returns the supplied span's _context; update references to _Span and _SpanCtx to use this stored active span so activating a span via use_span(span) makes subsequent get_current_span() return that same span until the context exits.bindu/extensions/voice/pipeline_builder.py-64-71 (1)
64-71:⚠️ Potential issue | 🟠 MajorUse the same sample-rate source for VAD and the rest of the pipeline.
VAD is initialized from
app_settings.voice.sample_rate, while the TTS services in this same pipeline are built fromvoice_ext.sample_rate. If an agent overrides the sample rate, VAD can drift out of sync with the actual audio path.Suggested fix
- vad_analyzer = SileroVADAnalyzer( - sample_rate=app_settings.voice.sample_rate, - ) + vad_analyzer = SileroVADAnalyzer( + sample_rate=voice_ext.sample_rate, + )🤖 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, VAD is being initialized with app_settings.voice.sample_rate while the pipeline's TTS/services use voice_ext.sample_rate, which can desync when an agent overrides sample rate; update the VAD initialization in pipeline_builder.py so SileroVADAnalyzer (and thus vad_component / VADProcessor) uses the same sample rate source as the rest of the pipeline (voice_ext.sample_rate) instead of app_settings.voice.sample_rate—locate the SileroVADAnalyzer instantiation and replace the sample_rate argument to reference voice_ext.sample_rate.frontend/src/lib/utils/agentMessageHandler.ts-65-72 (1)
65-72:⚠️ Potential issue | 🟠 MajorEncode file bytes to base64 strings before JSON serialization.
The code accepts
Blob,ArrayBuffer, andUint8ArrayinFilePart.file.bytes(lines 65-72) but passes them directly into the JSON-RPC request body, which is serialized withJSON.stringify(requestBody)(line 309). This mangles binary data:ArrayBufferandBlobbecome{},Uint8Arraybecomes an index-keyed object like{"0": 1, "1": 2, ...}. The file upload path is broken unless callers manually pre-encode bytes as strings.Encode all binary input to base64 strings before including them in the message parts:
Suggested fix
interface FilePart { kind: 'file'; file: { - bytes: string | ArrayBuffer | Uint8Array | Blob; + bytes: string; mimeType?: string; name?: string; }; } + +async function encodeFileBytes( + value: string | ArrayBuffer | Uint8Array | Blob +): Promise<string> { + if (typeof value === 'string') { + return value; + } + + const bytes = + value instanceof Blob + ? new Uint8Array(await value.arrayBuffer()) + : value instanceof ArrayBuffer + ? new Uint8Array(value) + : value; + + let binary = ''; + for (const byte of bytes) { + binary += String.fromCharCode(byte); + } + return btoa(binary); +} @@ parts.push({ kind: 'file', file: { - bytes: value, + bytes: await encodeFileBytes(value), mimeType: mime, name, }, });🤖 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 65 - 72, The FilePart type allows Blob/ArrayBuffer/Uint8Array in FilePart.file.bytes but those are passed raw into requestBody and JSON.stringify(requestBody), which mangles binaries; update the code that builds the requestBody (the code that populates the message parts before calling JSON.stringify(requestBody)) to detect when a part is a FilePart and convert FilePart.file.bytes to a base64 string (normalize: Blob -> ArrayBuffer -> Uint8Array -> base64; ArrayBuffer/Uint8Array -> base64) and replace the bytes value with that string so JSON serialization is safe, keeping FilePart.file.mimeType/name as-is and annotate or document that bytes is now a base64 string. Ensure conversions are performed synchronously or via async helper used where the request body is assembled so JSON.stringify(requestBody) receives only primitives/strings.scripts/test_voice_providers.sh-160-164 (1)
160-164:⚠️ Potential issue | 🟠 MajorAdd timeouts to the provider HTTP checks.
Every
curlhere can hang indefinitely on DNS/TLS/provider stalls because none of them set--connect-timeoutor--max-time. That turns a smoke test into a stuck process instead of a fast failure.Suggested fix
+CURL_OPTS=(--connect-timeout 5 --max-time 30) + -DG_HTTP=$(curl -sS -o "$DG_BODY" -w "%{http_code}" \ +DG_HTTP=$(curl -sS "${CURL_OPTS[@]}" -o "$DG_BODY" -w "%{http_code}" \ "https://api.deepgram.com/v1/listen?model=nova-3&language=en" \ -H "Authorization: Token ${VOICE__STT_API_KEY}" \ -H "Content-Type: audio/wav" \ --data-binary @"${TEST_WAV_PATH}" || true)Apply the same
CURL_OPTSarray to the ElevenLabs, Azure,/health, and/voice/session/startrequests as well.Also applies to: 189-193, 206-211, 233-235, 314-327
🤖 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 160 - 164, The curl calls (e.g., the DG_HTTP assignment using curl) lack timeouts and can hang; define a shared CURL_OPTS array including sensible --connect-timeout and --max-time values and apply it to every provider check curl invocation (Deepgram DG_HTTP, ElevenLabs checks, Azure checks, /health, and /voice/session/start requests referenced in the script) by appending "${CURL_OPTS[@]}" to each curl command so all checks fail fast on DNS/TLS/provider stalls.examples/pdf_research_agent/pdf_research_agent.py-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorThis allowlist blocks the documented
/path/to/paper.pdfworkflow.The example only accepts PDFs under
examples/pdf_research_agent, so almost any real file path a user sends will be rejected even though the usage text advertises arbitrary paths. For a runnable example, either make the base directory configurable or remove the restriction and document the local-only trust boundary.Also applies to: 53-68
🤖 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 38, The allowlist currently defined by ALLOWED_BASE_DIR prevents using arbitrary user-supplied paths like /path/to/paper.pdf; update the code so the base directory is configurable or the restriction is removed: make ALLOWED_BASE_DIR read from an environment variable or CLI flag (or allow it to be None/empty to disable the restriction), and adjust the path-validation logic (the block referenced around lines 53-68 that checks incoming PDF paths against ALLOWED_BASE_DIR) to accept absolute paths when the config disables the trust boundary; also update the example text to document that local-only trust must be enforced if the restriction is removed or show how to set the env/flag to restrict to examples/pdf_research_agent for safer defaults.bindu/extensions/voice/session_manager.py-217-230 (1)
217-230:⚠️ Potential issue | 🟠 MajorReject invalid states at runtime in
update_state().
Literal["connecting", "active", "ending", "ended"]is only a type hint. A bad caller can still pass any string here, and this method will store it without validation, which corrupts the session state machine.Suggested fix
async def update_state( self, session_id: str, state: Literal["connecting", "active", "ending", "ended"], ) -> None: """Update the state of a session.""" + allowed_states = {"connecting", "active", "ending", "ended"} + if state not in allowed_states: + raise ValueError( + f"state must be one of {sorted(allowed_states)}; got {state!r}" + ) async with self._lock: if state == "ended": session = self._sessions.pop(session_id, None) else: session = self._sessions.get(session_id)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/extensions/voice/session_manager.py` around lines 217 - 230, The update_state method currently trusts the type hint and may accept arbitrary strings; modify update_state to validate the incoming state against the allowed set {"connecting","active","ending","ended"} (use the literal strings) while holding self._lock, and if the value is invalid raise a ValueError (do not mutate session.state or pop/put sessions on invalid input); update references around update_state, session.state, and self._sessions to ensure validation happens before any session lookup/mutation and keep existing behavior for valid states.frontend/src/lib/components/voice/VoiceCallPanel.svelte-74-80 (1)
74-80:⚠️ Potential issue | 🟠 MajorDon't rewind the playback queue after scheduling the current chunk.
Resetting
playbackNextTimeaftersource.start(startAt)means the next chunk can be queued atctx.currentTimewhile this chunk is still scheduled in the future, so buffered agent audio can overlap once backlog exceeds ~2s.Suggested fix
- const startAt = Math.max(ctx.currentTime, playbackNextTime); - source.start(startAt); - playbackNextTime = startAt + audioBuffer.duration; - - // Keep scheduling stable if there's a long pause between chunks. - if (playbackNextTime - ctx.currentTime > 2) { - playbackNextTime = ctx.currentTime; - } + // If the queue drifted too far ahead, reset before scheduling this chunk. + if (playbackNextTime - ctx.currentTime > 2) { + playbackNextTime = ctx.currentTime; + } + + const startAt = Math.max(ctx.currentTime, playbackNextTime); + source.start(startAt); + playbackNextTime = startAt + audioBuffer.duration;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/voice/VoiceCallPanel.svelte` around lines 74 - 80, The code resets playbackNextTime back to ctx.currentTime after scheduling the current chunk, which allows future chunks to be scheduled in the past and overlap; update the branch that currently sets playbackNextTime = ctx.currentTime so it does not rewind after source.start(startAt) — instead only clamp playbackNextTime when it would cause unbounded drift (e.g., cap it to startAt or floor to ctx.currentTime only when playbackNextTime is far in the past before computing startAt), ensuring playbackNextTime is always advanced by audioBuffer.duration after scheduling (use playbackNextTime = startAt + audioBuffer.duration) and remove the unconditional rewind in the if-block that compares (playbackNextTime - ctx.currentTime > 2).bindu/extensions/voice/session_manager.py-150-189 (1)
150-189:⚠️ Potential issue | 🟠 MajorValidate the session token fields before storing them.
create_session()validatescontext_idbut trustssession_tokenandsession_token_expires_at. Empty strings, booleans, or non-numeric expiries can be stored here even thoughVoiceSession.from_dict()rejects them, so the manager can persist invalid auth metadata.Suggested fix
if not isinstance(context_id, str) or not context_id.strip(): raise ValueError("context_id must be a non-empty string") + if session_token is not None and ( + not isinstance(session_token, str) or not session_token.strip() + ): + raise ValueError("session_token must be a non-empty string when provided") + if session_token_expires_at is not None and ( + not isinstance(session_token_expires_at, (int, float)) + or isinstance(session_token_expires_at, bool) + ): + raise ValueError( + "session_token_expires_at must be a numeric timestamp when provided" + )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/extensions/voice/session_manager.py` around lines 150 - 189, create_session currently validates context_id but accepts invalid session_token and session_token_expires_at values; update create_session to validate those inputs before constructing VoiceSession: ensure session_token is either None or a non-empty string (use isinstance(session_token, str) and session_token.strip() and explicitly reject booleans), and ensure session_token_expires_at is either None or a numeric type (isinstance(..., (int, float)) and not isinstance(..., bool)) and optionally positive; if validation fails raise ValueError with a clear message and only then create the VoiceSession (references: create_session, VoiceSession, session_token, session_token_expires_at, self._lock).frontend/src/lib/components/voice/VoiceCallPanel.svelte-146-165 (1)
146-165:⚠️ Potential issue | 🟠 MajorTear down playback resources on component destroy as well.
This cleanup only runs after
$voiceStatereachesidle/ended/error. The panel itself is mounted off the session id, so if the session id is cleared first the component can unmount without closing theAudioContextor cancellingspeechSynthesis.Suggested fix
+ import { onDestroy } from "svelte"; import LiveTranscript from "$lib/components/voice/LiveTranscript.svelte";+ function teardownPlayback(): void { + playbackNextTime = 0; + if (playbackContext) { + void playbackContext.close(); + playbackContext = null; + } + if (fallbackSpeechTimer) { + clearTimeout(fallbackSpeechTimer); + fallbackSpeechTimer = null; + } + if ("speechSynthesis" in window) { + window.speechSynthesis.cancel(); + } + lastFallbackSpoken = ""; + } + $effect(() => { if (!["idle", "ended", "error"].includes($voiceState)) { return; } - - playbackNextTime = 0; - if (playbackContext) { - void playbackContext.close(); - playbackContext = null; - } - - if (fallbackSpeechTimer) { - clearTimeout(fallbackSpeechTimer); - fallbackSpeechTimer = null; - } - if ("speechSynthesis" in window) { - window.speechSynthesis.cancel(); - } - lastFallbackSpoken = ""; + teardownPlayback(); }); + + onDestroy(() => { + teardownPlayback(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/components/voice/VoiceCallPanel.svelte` around lines 146 - 165, The cleanup currently done inside the $effect (checking $voiceState and closing playbackContext, clearing fallbackSpeechTimer and window.speechSynthesis, and resetting lastFallbackSpoken) must also run when the component is destroyed; add an onDestroy handler that performs the same teardown: if (playbackContext) close it and null it, clear fallbackSpeechTimer and null it, call window.speechSynthesis.cancel() if present, reset playbackNextTime and lastFallbackSpoken. Reuse the same logic/variable names (playbackContext, fallbackSpeechTimer, playbackNextTime, lastFallbackSpoken, window.speechSynthesis) to ensure resources are released on unmount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d3a1aa5-c59b-4c1a-b189-438a3561e040
⛔ 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 (119)
.github/workflows/ci.yml.gitignore.pre-commit-config.yamlbindu/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.pyexamples/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
Co-vengers
force-pushed
the
feat(voice-agent)/example-agent
branch
from
763d0e1 to
5eab8a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major 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 (6)
frontend/src/lib/components/chat/ChatMessage.svelte (1)
262-280:⚠️ Potential issue | 🟠 Major
role="button"on a container with interactive descendants is an a11y anti-pattern, and the keydown handler bypasses the interactive-element guard.Two concerns with this wrapper:
Nested interactive descendants under
role="button". This<div>renders the retry button,CopyToClipBoardBtn,Alternatives, model/provider<a>tags,TaskInfo, and any links/images produced byMarkdownRenderer. WAI‑ARIA requires that elements withrole="button"not contain interactive descendants; assistive technologies may flatten the subtree and hide those controls from users, and the button itself becomes a very large hit target whose accessible name is the concatenated message content. Consider droppingrole="button"/tabindex="0"and keeping the "tap to reveal actions" affordance via a dedicated, smaller control (or a non-interactive container with a discrete toggle button inside), instead of promoting the entire message to a button.Keyboard path diverges from the click path.
handleMessageClickskips the toggle whene.target.closest("button, a, input, textarea, select, [role='button'], [data-no-toggle]")matches, but the inlineonkeydownunconditionally togglesisTappedon Enter/Space andpreventDefaults. When focus is on a nested button/link and the user presses Enter or Space, the event bubbles here and will (a) toggleisTappedas a side effect and (b) for Space,preventDefault()runs even though the child control was the intended target. At minimum, mirror the sameclosest(...)guard (and likely requireevent.target === event.currentTarget) before toggling / preventDefault'ing.♻️ Minimal guard alignment for the keydown handler (does not address the nested-interactive-role concern)
- role="button" - tabindex="0" - onclick={handleMessageClick} - onkeydown={(event) => { - if (event.key === "Enter" || event.key === " ") { - event.preventDefault(); - isTapped = !isTapped; - } - }} + role="button" + tabindex="0" + onclick={handleMessageClick} + onkeydown={(event) => { + if (event.key !== "Enter" && event.key !== " ") return; + if (event.target !== event.currentTarget) return; + const el = event.target as HTMLElement | null; + if ( + el?.closest( + "button, a, input, textarea, select, [role='button'], [data-no-toggle]" + ) + ) { + return; + } + 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 262 - 280, The wrapper div currently has role="button" and tabindex="0" (the container that renders Retry, CopyToClipBoardBtn, Alternatives, model/provider links, TaskInfo and MarkdownRenderer) which violates WAI‑ARIA because it contains interactive descendants; remove role="button" and tabindex from that container and implement a dedicated smaller toggle control (or a non-interactive container with an internal button) to reveal actions, and update the keydown logic used for toggling (the inline onkeydown that currently toggles isTapped) to mirror the same guard as handleMessageClick (use event.target.closest("button, a, input, textarea, select, [role='button'], [data-no-toggle]") and require event.target === event.currentTarget before preventing default and toggling isTapped) so keyboard activation doesn't override or block child controls.frontend/src/lib/server/files/downloadFile.ts (1)
21-21:⚠️ Potential issue | 🟠 MajorDo not normalize missing MIME metadata to an empty string.
String(mime ?? "")turns missing metadata into"", so callers usingmime ?? "application/octet-stream"no longer get the fallback and may emit an emptyContent-Typeheader.🐛 Proposed fix
- const mime = file.metadata?.mime; + const metadataMime = file.metadata?.mime; + const mime = + typeof metadataMime === "string" && metadataMime.trim() + ? metadataMime.trim() + : "application/octet-stream"; const name = file.filename; @@ - return { type: "base64", name, value: buffer.toString("base64"), mime: String(mime ?? "") }; + return { type: "base64", name, value: buffer.toString("base64"), mime };Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/server/files/downloadFile.ts` at line 21, The code currently normalizes missing MIME metadata to an empty string via String(mime ?? ""), which breaks callers that rely on mime ?? "application/octet-stream"; instead leave the MIME value as-is (i.e., use the raw file.metadata?.mime) so that null/undefined propagates and caller-side nullish coalescing works; update occurrences where String(mime ?? "") appears (including the variable mime and the similar spot around line 60) to stop coercing to "" and use the original value or undefined.bindu/extensions/did/did_agent_extension.py (1)
214-229:⚠️ Potential issue | 🟠 MajorCreate Windows key files with hardened ACLs before writing private key material.
On Windows,
write_bytes()creates the file with inherited/default ACLs before_harden_windows_key_file_acl()runs, creating a confidentiality window where the private key may be readable. The POSIX implementation (lines 230–244) correctly usesos.open(mode=0o600)to set permissions atomically before writing. Apply the same pattern on Windows: create an empty file with hardened ACLs first, then write the private key material.🤖 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 214 - 229, The Windows branch currently writes key bytes with self.private_key_path.write_bytes(...) and then calls self._harden_windows_key_file_acl(...), which leaves a race where the file is created with inherited ACLs; change this to first create empty files for self.private_key_path and self.public_key_path (without writing key material), call self._harden_windows_key_file_acl(...) on each to set secure ACLs, and only after successful hardening write the private_pem and public_pem to those paths (preserving the existing rollback/unlink logic and re-raising on failure); ensure you still handle exceptions from _harden_windows_key_file_acl by removing created files (using unlink(missing_ok=True)) as currently implemented.frontend/src/lib/server/database.ts (1)
455-466:⚠️ Potential issue | 🟠 MajorEmit upload
finishafterend(), not when the listener is registered.
once("finish")currently fires on registration, beforeend()stores the file. Code awaiting upload completion can proceed before the data exists.🐛 Proposed fix
const id = new ObjectId(); const chunks: Buffer[] = []; + const finishListeners = new Set<(err?: Error) => void>(); + let finished = false; + + const emitFinish = () => { + if (finished) return; + finished = true; + setTimeout(() => { + for (const cb of finishListeners) cb(); + }, 0); + }; const stream = { id, write: (chunk: Buffer) => { chunks.push(chunk); }, end: () => { const data = Buffer.concat(chunks); this.files.set(id.toString(), { _id: id.toString(), data, filename, metadata: _options?.metadata }); + emitFinish(); }, on: (_event: string, cb: (err?: Error) => void) => { - if (_event === "finish") setTimeout(() => cb(), 0); + if (_event === "finish") { + finishListeners.add(cb); + if (finished) setTimeout(() => cb(), 0); + } return stream; }, once: (_event: string, cb: (err?: Error) => void) => { - if (_event === "finish") setTimeout(() => cb(), 0); + if (_event === "finish") { + const wrapped = (err?: Error) => { + finishListeners.delete(wrapped); + cb(err); + }; + finishListeners.add(wrapped); + if (finished) setTimeout(() => wrapped(), 0); + } return stream; },🤖 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 455 - 466, The finish event is being invoked immediately on registration in on()/once() instead of after end() stores the file, causing race conditions; change the logic so end() sets the file via this.files.set(...) then triggers any queued finish listeners; implement a boolean flag (e.g., finished) and a small array of finish callbacks (e.g., finishListeners) referenced by on(…) and once(…) to push callbacks rather than calling them immediately, and in end() after this.files.set(...) iterate the finishListeners and schedule each callback (setTimeout(cb, 0)) and mark finished = true so future on/once can call the callback immediately (also scheduled) if already finished; update references in end, on, once, id, stream accordingly.tests/integration/grpc/test_grpc_e2e.py (1)
269-299:⚠️ Potential issue | 🟠 MajorMove agent registration into a fixture to remove test-order coupling.
test_agent_card_availableandtest_send_message_and_get_responserely ontest_register_agenthaving already run. This will be flaky with xdist, reordered tests, or targeted test selection; make registration a module/class fixture that these tests depend on explicitly.🧪 Suggested fixture shape
+@pytest.fixture(scope="module") +def registered_agent(grpc_server: Any, mock_agent: MockAgentHandler) -> dict[str, str]: + channel = grpc.insecure_channel(f"{LOOPBACK_HOST}:{GRPC_PORT}") + try: + stub = agent_handler_pb2_grpc.BinduServiceStub(channel) + config = { + "author": "e2e-test@bindu.com", + "name": "e2e-test-agent", + "description": "E2E test agent", + "deployment": { + "url": f"http://{LOOPBACK_HOST}:{HTTP_PORT}", + "expose": True, + }, + } + response = stub.RegisterAgent( + agent_handler_pb2.RegisterAgentRequest( + config_json=json.dumps(config), + skills=[], + grpc_callback_address=f"{LOOPBACK_HOST}:{CALLBACK_PORT}", + ) + ) + assert response.success is True + _wait_for_http_ready(f"http://{LOOPBACK_HOST}:{HTTP_PORT}", timeout_s=30.0) + return {"agent_id": response.agent_id, "agent_url": response.agent_url} + finally: + channel.close()Also applies to: 333-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/grpc/test_grpc_e2e.py` around lines 269 - 299, Tests are order-dependent because test_register_agent performs agent setup used by test_agent_card_available and test_send_message_and_get_response; move the registration logic into a reusable fixture (module or class scope) that creates the agent, returns its agent_id/URL/DID (or the whole response), and calls _wait_for_http_ready so dependent tests can use it reliably; update tests to accept the new fixture instead of calling test_register_agent and reference symbols: test_register_agent (source logic), _wait_for_http_ready, and the dependent tests test_agent_card_available and test_send_message_and_get_response to consume the fixture.bindu/server/applications.py (1)
416-451:⚠️ Potential issue | 🟠 MajorProtect core cleanup from voice lifecycle failures.
create_session_manager()andclose_session_manager()can raise; today that can skip already-initialized storage/scheduler cleanup or prevent storage cleanup during shutdown. Wrap the lifespan body with failure-safe cleanup, or at minimum isolate voice shutdown intry/finallyso core resources always close.🛡️ Minimal shutdown hardening
# Stop voice session manager if app._voice_session_manager: from bindu.extensions.voice.session_factory import close_session_manager - await close_session_manager(app._voice_session_manager) - logger.info("🛑 Voice session manager stopped") + try: + await close_session_manager(app._voice_session_manager) + logger.info("🛑 Voice session manager stopped") + except Exception as exc: + logger.error(f"Error stopping voice session manager: {exc}") + finally: + app._voice_session_manager = None # Cleanup storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/server/applications.py` around lines 416 - 451, The voice session manager lifecycle can raise and short-circuit core cleanup; ensure failures in create_session_manager(), start_cleanup_loop(), or close_session_manager() do not prevent TaskManager shutdown and payment/session cleanup by isolating voice setup/teardown in safe blocks: call create_session_manager() and start_cleanup_loop() inside a try/except that logs but does not propagate, ensure the main lifespan yield and TaskManager async-with are wrapped in a finally so storage/scheduler cleanup and payment stop_cleanup_task() always run, and run close_session_manager() inside its own try/finally (or try/except) so exceptions during voice shutdown are caught and logged without aborting remaining cleanup. Reference symbols: create_session_manager, start_cleanup_loop, TaskManager, app.task_manager, stop_cleanup_task, close_session_manager, app._voice_session_manager.
🟡 Minor comments (9)
examples/multilingual-collab-agent/main.py-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorSet
_mem0_enabledafter Mem0 initializes successfully.
handler()checks this flag, butbuild_agent()never sets it toTrue, so the “Mem0 persistent memory enabled” message can never print.Proposed fix
tools = [DuckDuckGoTools()] try: tools.append(Mem0Tools(api_key=mem0_api_key)) + _mem0_enabled = True except Exception as e: + _mem0_enabled = False print(f"⚠️ Mem0 tools unavailable: {e}. Continuing without memory.")Also applies to: 57-60, 158-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multilingual-collab-agent/main.py` at line 42, The flag _mem0_enabled is never set after Mem0 is initialized, so update build_agent() to set the module-level _mem0_enabled = True immediately after successfully creating/configuring the Mem0 persistent memory instance (use the global _mem0_enabled declaration inside build_agent() before assignment); ensure any exception paths leave it False. Also mirror this change where Mem0 is initialized elsewhere (the other Mem0 init blocks referenced around lines 57-60 and 158-159) so handler() can reliably detect and print "Mem0 persistent memory enabled".frontend/.env.example-149-149 (1)
149-149:⚠️ Potential issue | 🟡 MinorNormalize inline assignment formatting to satisfy dotenv linting.
Static analysis flagged spaces around
=on these lines. Cleaning them avoids lint noise and parser ambiguity across dotenv implementations.Proposed cleanup
-WEBHOOK_URL_REPORT_ASSISTANT= # provide slack webhook url to get notified for reports/feature requests +WEBHOOK_URL_REPORT_ASSISTANT=# provide slack webhook url to get notified for reports/feature requests -ADMIN_API_SECRET= # secret to admin API calls, like computing usage stats or exporting parquet data +ADMIN_API_SECRET=# secret to admin API calls, like computing usage stats or exporting parquet dataAlso applies to: 162-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` at line 149, Normalize dotenv assignments by removing spaces around the '=' for the affected variables (e.g., WEBHOOK_URL_REPORT_ASSISTANT) in frontend/.env.example so lines follow the form KEY=VALUE (or KEY= if empty). Locate the WEBHOOK_URL_REPORT_ASSISTANT line and any other similar entries flagged (notably the one mentioned at 162) and remove the whitespace on both sides of '=' to satisfy dotenv linting and parsers.examples/document-analyzer/document_analyzer.py-76-84 (1)
76-84:⚠️ Potential issue | 🟡 MinorMinor: narrow the caught exception (Ruff BLE001).
Static analysis flags the bare
Exception. For DOCX parsing viapython-docx, a narrower(PackageNotFoundError, KeyError, ValueError, OSError)keeps unrelated bugs from being swallowed while still handling malformed-file cases the way you intend.🛡️ Suggested narrower catch
+from docx.opc.exceptions import PackageNotFoundError @@ 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: + except (PackageNotFoundError, KeyError, ValueError, OSError) as e: logger.error(f"Error extracting DOCX text: {e}") return ""🤖 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 - 84, The extract_text_from_docx function currently catches a bare Exception; replace that with a narrower exception tuple to avoid swallowing unrelated errors: catch (PackageNotFoundError, KeyError, ValueError, OSError) as e instead of Exception, and add the necessary import for PackageNotFoundError (from docx.opc.exceptions import PackageNotFoundError) so malformed or missing DOCX files are handled but other errors still surface; update the except block in extract_text_from_docx and keep the existing logger.error and return "" behavior.frontend/src/lib/utils/agentMessageHandler.ts-265-309 (1)
265-309:⚠️ Potential issue | 🟡 MinorClient-side validation is stricter than the protocol — may silently drop valid attachments.
The protocol (
FileWithBytes) marksmimeTypeandnameasNotRequired, but this block drops any file part where either is empty/missing and only logs aconsole.warn. When a browser can't infer a MIME type,File.typeis the empty string — those uploads will be silently dropped with no user-visible feedback. Consider either:
- Only requiring
bytes/value and letting the server accept missingmimeType/name, falling back to'application/octet-stream'and a generated name on the client; or- Surfacing the drop to the user (via
$erroror a toast) instead of swallowing it.♻️ Suggested softer validation
- if (!hasValue || !mime || !name) { - console.warn('[agentMessageHandler] Dropping invalid file part', { - hasValue, - mime, - name, - }); - continue; - } + if (!hasValue) { + console.warn('[agentMessageHandler] Dropping file part with empty value', { mime, name }); + continue; + } + const safeMime = mime || 'application/octet-stream'; + const safeName = name || 'attachment'; @@ - mimeType: mime, - name, + mimeType: safeMime, + name: safeName,🤖 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 265 - 309, The current loop over fileParts in agentMessageHandler rejects parts when mime or name are empty, silently dropping browser files with empty File.type; change the validation to only require a non-empty value (keep hasValue check), then default missing mime/name before pushing: set mime = mime || 'application/octet-stream' and set name = name || `file-${Date.now()}-${i}` (or derive from mime if desired) after awaiting normalizeFileValue; update the push to use these defaults (references: fileParts loop, normalizeFileValue, parts.push) so the client forwards bytes even when mime/name are omitted.frontend/src/lib/server/database.ts-94-114 (1)
94-114:⚠️ Potential issue | 🟡 MinorConsider restricting
_idinput validation to ObjectId only or document the string-to-ObjectId coercion behavior.This in-memory mock converts string
_idvalues toObjectId, and generates a newObjectIdfor invalid strings. Since the method signature declaresinsertedId: ObjectId, this behavior appears intentional; however, it deviates from MongoDB's support for arbitrary_idtypes (strings, UUIDs, etc.). If this mock is intended to closely match MongoDB's_idhandling for broader compatibility, preserve caller-supplied_idvalues instead of coercing them.Current usage in the codebase does not provide explicit
_idvalues during insertion, so this does not cause active failures today.Optional: Support arbitrary `_id` types
async insertOne( doc: Partial<T> | T - ): Promise<{ insertedId: ObjectId; acknowledged: boolean }> { + ): Promise<{ insertedId: unknown; 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 hasProvidedId = + Object.prototype.hasOwnProperty.call(source, "_id") && + source._id !== undefined && + source._id !== null; + const id = hasProvidedId ? source._id : new ObjectId(); const docWithId = { ...source, _id: id } as T; - this.data.set(id.toString(), docWithId); + this.data.set(String(id), docWithId); return { insertedId: id, acknowledged: true }; }Also update
insertMany's return type if changing_idsemantics.🤖 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 - 114, The mock insertOne currently coerces non-ObjectId _id values to an ObjectId; instead preserve caller-supplied _id types (or explicitly restrict to ObjectId and document that behavior). To preserve arbitrary _id types, change insertOne so when ("_id" in source && source._id) you do not convert strings to ObjectId — use the provided value as-is for docWithId and when storing in this.data use a stable string key (e.g., String(source._id)) so lookups still work; update the Promise return type from { insertedId: ObjectId; ... } to reflect the actual id type (e.g., { insertedId: T extends { _id: infer I } ? I : unknown; acknowledged: boolean }) and make the same semantic/type changes in insertMany and any callers; alternatively, if you choose to restrict to ObjectId, enforce and validate that source._id is an ObjectId and throw or document the enforcement in insertOne/insertMany and function comments.bindu/server/scheduler/memory_scheduler.py-31-33 (1)
31-33:⚠️ Potential issue | 🟡 MinorMove the queue size and send timeout into
app_settings.Line 33 adds a local config constant, and Line 80 hardcodes an operational timeout. These should be settings-backed so deployments can tune backpressure without code changes.
Suggested direction
+from bindu.settings import app_settings from bindu.utils.logging import get_logger from bindu.utils.retry import retry_scheduler_operation @@ -# Bounded buffer prevents unbounded memory growth while allowing the API -# handler to enqueue a task before the worker loop is ready to receive. -_TASK_QUEUE_BUFFER_SIZE = 100 - @@ self._write_stream, self._read_stream = anyio.create_memory_object_stream[ TaskOperation - ](_TASK_QUEUE_BUFFER_SIZE) + ](app_settings.scheduler.task_queue_buffer_size) @@ - with anyio.fail_after(5.0): + with anyio.fail_after(app_settings.scheduler.task_queue_send_timeout_seconds): await self._write_stream.send(task_op)Add the corresponding fields to
SchedulerSettings.As per coding guidelines,
bindu/**/*.py: Usebindu.settings.app_settingsfor all configuration and never create local config constants.Also applies to: 44-48, 66-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 hardcoded _TASK_QUEUE_BUFFER_SIZE and the send timeout literal in memory_scheduler.py with settings pulled from app_settings.SchedulerSettings: add fields like task_queue_buffer_size and task_send_timeout to SchedulerSettings in bindu.settings.app_settings, then in memory_scheduler.py read app_settings.SchedulerSettings().task_queue_buffer_size when creating the queue and use .task_send_timeout where the send/put timeout is used (e.g., in the enqueue/send call inside enqueue_task or the worker loop). Ensure no local config constants remain and default values match the previous behavior.examples/pdf_research_agent/pdf_research_agent.py-183-185 (1)
183-185:⚠️ Potential issue | 🟡 MinorRemove the stale error-string branch.
_read_content()now raisesDocumentReadError, so this branch can incorrectly return valid raw text starting with[orErrorinstead of summarizing it.Proposed 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 stale check that treats any document_text starting with "[" or "Error" as a processing failure should be removed: delete the conditional that returns document_text based on document_text.startswith("[") or document_text.startswith("Error") so that _read_content() exceptions (DocumentReadError) are allowed to surface or be handled by the caller; keep using the existing document_text variable and error class (DocumentReadError) so valid content that begins with those strings is not incorrectly treated as an error.examples/voice-agent/.env.example-6-10 (1)
6-10:⚠️ Potential issue | 🟡 MinorDo not enable Azure fallback with placeholder credentials by default.
Since these placeholder values are non-empty, validation will treat Azure as configured and may attempt real fallback calls with fake credentials. Prefer disabling fallback by default and leaving Azure lines commented for users who opt in.
🛠️ Proposed example-safe default
# 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=none +# VOICE__AZURE_TTS_API_KEY=your-azure-speech-key +# VOICE__AZURE_TTS_REGION=eastus +# VOICE__AZURE_TTS_VOICE=en-US-SaraNeural🤖 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 .env example currently enables Azure TTS fallback by default using placeholder values (VOICE__TTS_FALLBACK_PROVIDER and VOICE__AZURE_TTS_*), which can be mistaken for real credentials; change the defaults to disable fallback (set VOICE__TTS_FALLBACK_PROVIDER= or comment it out) and comment out or remove the VOICE__AZURE_TTS_API_KEY, VOICE__AZURE_TTS_REGION, and VOICE__AZURE_TTS_VOICE lines so users must explicitly opt in and supply real Azure credentials before the fallback is considered configured.bindu/server/endpoints/voice_endpoints.py-472-497 (1)
472-497:⚠️ Potential issue | 🟡 Minor
returninsidefinallysilences exceptions fromwebsocket.close().Ruff
B012: ifwebsocket.close()raises an unexpected exception (something other than a routine disconnect), thereturninfinallyswallows it, making the failure invisible to the caller and logs. The existingtry/except Exception: passpattern used elsewhere in this file (e.g., lines 413-415, 1001-1002) is the more explicit idiom here.🔧 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 + except Exception: + logger.exception("Failed to close WebSocket after stop frame") + 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 472 - 497, The current try/finally in the control frame handling uses "await websocket.close()" followed by "return" in finally which will swallow any exceptions raised by websocket.close(); change this to explicitly catch and ignore expected exceptions (or log unexpected ones) by replacing the try/finally with a try/except Exception: pass (or logger.exception) around await websocket.close() and then return after the close so that exceptions are not silently suppressed; focus edits around the control frame branch handling where websocket.close() is called (the block that sets control.stopped = True and sends the "ended" state).
🧹 Nitpick comments (11)
frontend/src/lib/jobs/refresh-conversation-stats.ts (1)
257-259: Prefer MongoDB'sDocumenttype overRecord<string, unknown>[]cast.The
aggregate()method acceptsDocument[](frommongodb). Casting toRecord<string, unknown>[]works but bypasses the driver's intended typing and can hide real shape mismatches if the pipeline is later edited. Consider typingpipelineexplicitly asDocument[]at declaration (line 60) so the cast at the call site is unnecessary.♻️ Proposed refactor
-import type ObjectId from "bson-objectid"; +import type ObjectId from "bson-objectid"; +import type { Document } from "mongodb"; @@ - const pipeline = [ + const pipeline: Document[] = [ @@ - await collections.conversations - .aggregate(pipeline as Record<string, unknown>[], { allowDiskUse: true }) - .next(); + await collections.conversations + .aggregate(pipeline, { allowDiskUse: true }) + .next();Also worth noting: this file change appears unrelated to the PR's stated scope (voice-agent example). Consider whether it belongs in a separate PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/jobs/refresh-conversation-stats.ts` around lines 257 - 259, The pipeline is being cast as Record<string, unknown>[] when passed to collections.conversations.aggregate; instead import and use MongoDB's Document type and declare the pipeline variable (the one created around line 60 named pipeline) as Document[] so you can remove the cast at the aggregate call site, e.g., update the pipeline declaration to Document[] and ensure you import Document from 'mongodb' and then call collections.conversations.aggregate(pipeline, { allowDiskUse: true }).frontend/src/lib/components/chat/ChatMessage.svelte (1)
296-300: Stalesvelte-ignoredirective.
onclick={handleContentClick}was removed from this wrapper, soa11y_click_events_have_key_eventsanda11y_no_static_element_interactionsshould no longer trigger here (oncopydoesn't require them). Consider removing the ignore comment so future real violations aren't silently suppressed.🧹 Proposed cleanup
- <!-- svelte-ignore a11y_click_events_have_key_events a11y_no_static_element_interactions --> <div bind:this={contentEl} oncopy={handleCopy} >🤖 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 296 - 300, The stale svelte-ignore comment above the div that binds contentEl and uses oncopy={handleCopy} should be removed: delete the comment line "// svelte-ignore a11y_click_events_have_key_events a11y_no_static_element_interactions" so accessibility warnings are not unnecessarily suppressed for this wrapper; locate the div with bind:this={contentEl} and oncopy={handleCopy} in ChatMessage.svelte (the onclick={handleContentClick} was removed) and remove the svelte-ignore directive only for that element.bindu/extensions/did/did_agent_extension.py (1)
256-264: Narrow the exception handling around user lookup.Ruff’s BLE001 warning is valid here: these fallbacks should catch the expected lookup failures rather than swallowing unrelated exceptions.
♻️ Proposed narrowing
if not username: try: username = os.getlogin().strip() - except Exception: + except OSError: username = "" if not username: try: username = (getpass.getuser() or "").strip() - except Exception: + except (OSError, ImportError, KeyError): username = ""🤖 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 256 - 264, The current broad excepts around os.getlogin() and getpass.getuser() swallow unrelated errors; narrow them to expected lookup failures by catching OSError when calling os.getlogin() and catching the specific failures getpass.getuser() can raise (e.g., KeyError, ImportError, OSError) when populating username; update the try/except blocks around os.getlogin() and getpass.getuser() accordingly so only those specific exception types set username = "" while allowing other exceptions to propagate.bindu/utils/notifications.py (2)
284-299: Minor: redundantisinstanceguard and IPv6-with-port edge case.
destination_ipis always a non-emptystrat this point (either passed in by the caller or returned from_resolve_and_validate_destination), soisinstance(destination_ip, str) and destination_ipis dead-defensive and makes the branch look conditionally optional. Also worth double-checking: for an IPv6 destination on a non-default port,netlocbecomes[::1]:8080viadestination_hostbracketing — good — but the rebuilttarget_urlmust still parse back correctly throughurllib; a quick unit test covering an IPv6 HTTP target would be worthwhile.♻️ Suggested simplification
- target_url = url - if isinstance(destination_ip, str) and destination_ip: - destination_host = ( - f"[{destination_ip}]" if ":" in destination_ip else destination_ip - ) - netloc = ( - f"{destination_host}:{parsed.port}" - if parsed.port is not None - else destination_host - ) - target_url = parsed._replace(netloc=netloc).geturl() + destination_host = ( + f"[{destination_ip}]" if ":" in destination_ip else destination_ip + ) + netloc = ( + f"{destination_host}:{parsed.port}" if parsed.port is not None else destination_host + ) + target_url = parsed._replace(netloc=netloc).geturl()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/utils/notifications.py` around lines 284 - 299, Remove the redundant isinstance check around destination_ip and simplify the branch to "if destination_ip:"; inside that branch keep building destination_host with bracketed IPv6 when ":" in destination_ip, construct netloc using parsed.port as currently done and replace parsed.netloc to form target_url (symbols: destination_ip, destination_host, parsed, netloc, target_url, request.Request); additionally add a unit test that posts to an IPv6 HTTP URL with a non-default port (e.g., "[::1]:8080") to verify urllib parsing round-trips correctly and the Host header is set as expected (symbols to locate: req.add_header and headers iteration).
40-58: Pinning only the first resolved IP can cause avoidable connection failures.
_resolve_and_check_ipvalidates every address returned bygetaddrinfo, but returnsstr(addrs[0])and callers use that single IP for the actual connect. If the first record is, e.g., AAAA on an IPv4-only host (or temporarily unreachable), delivery fails even though a valid A record exists. Consider returning the full validated list (or the first reachable family) and iterating at connect time.🤖 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 function _resolve_and_check_ip currently validates all addresses but returns only the first one, causing failures if that address is unusable; change _resolve_and_check_ip to return the full validated list of IP strings (e.g., List[str]) instead of a single string and update any callers that use its return value (where connections are made) to iterate over the returned addresses and attempt connecting in order until one succeeds (or handle selection by address family), ensuring you still keep the existing validation against _BLOCKED_NETWORKS inside _resolve_and_check_ip.frontend/src/lib/components/chat/ChatInput.svelte (1)
171-199: Inconsistent error UX between file-read failure and submit failure.File-read failures at line 177 surface via the
$errorstore (nice, themed), but submit failures at line 195 use a nativealert(). For consistency — and to respect the app's existing error presentation — route both through$error. Minor typing nit: annotatefilePartsso it doesn't rely on inference (let fileParts: Array<{ name: string; mime: string; value: string }> = [];).♻️ Suggested unification
- try { - if (typeof onsubmit === "function") { - const submitted = await onsubmit(value, fileParts); - if (submitted) { - // Clear files and input after successful submit - files = []; - value = ""; - } - } - } catch (err) { - console.error("Error submitting message:", err); - alert( - `Error submitting message: ${err instanceof Error ? err.message : String(err)}` - ); - } + try { + if (typeof onsubmit === "function") { + const submitted = await onsubmit(value, fileParts); + if (submitted) { + files = []; + value = ""; + } + } + } catch (err) { + console.error("Error submitting message:", err); + $error = `Error submitting message: ${err instanceof Error ? err.message : String(err)}`; + }🤖 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 171 - 199, The UX is inconsistent: file-read errors set the themed $error store but submit failures use alert(); also fileParts lacks an explicit type. Replace the alert() usage inside the onsubmit catch block by assigning the same $error store (e.g., $error = `Error submitting message: ${...}`) so both error paths use the app's error UI, and annotate fileParts as let fileParts: Array<{ name: string; mime: string; value: string }> = []; while preserving the existing resets of fileInputEl, files and value and keeping the getFileParts and onsubmit calls intact.examples/document-analyzer/document_analyzer.py (1)
120-164: Double-accumulation risk if a message carries bothcontentandparts.The helper appends to
prompt_partsfrom bothmsg["content"](runtime chat shape) and text items inmsg["parts"](A2A shape) on the same iteration. If any message ever contains both — e.g. a runtime path that also forwards original parts — the user prompt will appear twice in the LLM input. In today's flows this is unlikely, but a cheap guard avoids a surprising future bug.♻️ Suggested guard
- # Runtime path: manifest worker passes chat-format messages. - content = msg.get("content") - if isinstance(content, str) and content.strip(): - prompt_parts.append(content) - - # Compatibility path: raw A2A messages with parts. - parts = msg.get("parts") or [] + # Prefer runtime chat-format content; fall back to A2A parts. + content = msg.get("content") + parts = msg.get("parts") or [] + if isinstance(content, str) and content.strip(): + prompt_parts.append(content) + # If content is present, skip text parts to avoid double-count, + # but still process file parts. + parts = [p for p in parts if p.get("kind") != "text"]Also a small typing nit:
handler(messages: list[dict])on line 167 could belist[dict[str, Any]]to match the helper signature.🤖 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 120 - 164, In _collect_prompt_and_documents: avoid double-accumulating the same message by treating a message as either the runtime chat shape (use msg["content"]) or the A2A shape (use msg["parts"]) but not both; implement a guard such that if content (non-empty string) is present you skip processing parts for that message (so prompt_parts is only appended once), and preserve existing file extraction/error handling for parts when content is absent; also update the handler signature that currently accepts messages: list[dict] to use list[dict[str, Any]] to match _collect_prompt_and_documents.frontend/src/lib/utils/agentMessageHandler.ts (1)
122-141: base64FromBytes performance issue—affects both files; signature migration complete.The signature migration to options object is already complete; all callers in
conversation/[id]/+page.svelteandChatWindow.sveltecorrectly passabortSignal,currentTaskId,taskState, andreplyToTaskIdwithin the options object. No breaking signature issues found.However,
base64FromBytesis duplicated and uses a naive O(n²) approach in bothagentMessageHandler.ts(lines 122–128) andchat.ts(lines 34–40). For multi-MB attachments, per-byteString.fromCharCodewith+=concatenation measurably degrades performance. Consider chunking the conversion or usingFileReader.readAsDataURLfor Blob inputs to avoid the extra round trip.🤖 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 base64FromBytes implementation in base64FromBytes and its duplicate in chat.ts is O(n²) due to per-byte string concatenation; replace it with a chunked conversion (build an array of String.fromCharCode(...) results over fixed-size slices, join the array, then call btoa) to keep memory/time linear, update normalizeFileValue to use FileReader.readAsDataURL for Blob inputs to avoid manual arrayBuffer work, and remove the duplicate implementation in chat.ts so both modules import the single optimized base64FromBytes/normalizeFileValue utilities (refer to functions base64FromBytes and normalizeFileValue to locate and update the code, and the duplicate in chat.ts to remove).frontend/src/lib/server/database.ts (1)
153-159: Avoid typing unsupported pipeline updates as accepted.This compiles callers that pass update pipelines, then fails only at runtime. Either implement pipeline updates or keep the public type as
Record<string, unknown>.♻️ Proposed type alignment
async updateMany( filter: Record<string, unknown>, - update: Record<string, unknown> | Array<Record<string, unknown>> + update: Record<string, unknown> ): Promise<{ matchedCount: number; modifiedCount: number }> { - if (Array.isArray(update)) { - throw new Error("Pipeline-style updates are unsupported in InMemoryCollection.updateMany"); - } let count = 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 153 - 159, The public signature of InMemoryCollection.updateMany currently accepts update: Record<string, unknown> | Array<Record<string, unknown>> but only supports object-style updates, leading to runtime errors; change the method signature on updateMany to accept only update: Record<string, unknown> (remove the Array union) and remove or simplify the runtime Array.isArray check, or alternatively implement pipeline-style handling inside InMemoryCollection.updateMany if you need to keep the union; update any callers that pass an array to send object-style updates instead.bindu/settings.py (1)
6-13: Use the project logger for the new warning.The new
VoiceSettingsvalidator logs through stdliblogging; use the existing Bindu logger utility so this warning follows the app’s logging configuration.Proposed fix
-import logging - from pydantic import BaseModel, Field, HttpUrl, computed_field, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict from pydantic import AliasChoices from typing import Literal +from bindu.utils.logging import get_logger -logger = logging.getLogger(__name__) +logger = get_logger("bindu.settings")As per coding guidelines,
bindu/**/*.py: Useloggerfrombindu.utils.logginginstead of direct logging.Also applies to: 1161-1167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/settings.py` around lines 6 - 13, Replace direct stdlib logging usage in the VoiceSettings validator with the project logger: import the module-level logger from bindu.utils.logging (instead of calling logging.getLogger or using logging.*) and use that logger instance inside the validator function(s) (referencing VoiceSettings and its model_validator/computed_field methods). Update any other occurrences noted (lines around 1161-1167) to use the same bindu.utils.logging logger so all warning/info/error calls follow the app's logging configuration.bindu/extensions/voice/service_factory.py (1)
43-50: Use direct attribute access for required Pipecat service classes.Ruff flags these constant-name
getattrcalls on mandatory imports; direct access preserves the sameAttributeErrorhandling and avoids lint failures. Keepgetattr(..., None)for optional settings classes.♻️ Proposed lint cleanup
- DeepgramSTTService = getattr(deepgram_module, "DeepgramSTTService") + DeepgramSTTService = deepgram_module.DeepgramSTTService @@ - PiperTTSService = getattr(piper_module, "PiperTTSService") + PiperTTSService = piper_module.PiperTTSService @@ - ElevenLabsTTSService = getattr(elevenlabs_module, "ElevenLabsTTSService") + ElevenLabsTTSService = elevenlabs_module.ElevenLabsTTSService @@ - AzureTTSService = getattr(azure_module, "AzureTTSService") + AzureTTSService = azure_module.AzureTTSServiceAlso applies to: 108–116, 143–155, 193–201
🤖 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 43 - 50, Replace the getattr(...) calls used to obtain required Pipecat service classes with direct attribute access to avoid ruff lint warnings while preserving AttributeError behavior: for example, after importing pipecat.services.deepgram.stt, replace DeepgramSTTService = getattr(deepgram_module, "DeepgramSTTService") with DeepgramSTTService = deepgram_module.DeepgramSTTService; do the same for the other required service classes referenced in the file (the blocks around lines 108–116, 143–155, 193–201). Keep using getattr(..., None) for optional settings classes where absence is acceptable. Ensure you raise or propagate the same ImportError/AttributeError as currently done when the required attributes are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07364b89-8f49-483b-acea-edcfbfba2604
⛔ 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 (109)
.gitignore.pre-commit-config.yamlbindu/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/grpc/client.pybindu/grpc/server.pybindu/grpc/service.pybindu/penguin/bindufy.pybindu/penguin/config_validator.pybindu/server/applications.pybindu/server/endpoints/utils.pybindu/server/endpoints/voice_endpoints.pybindu/server/metrics.pybindu/server/scheduler/memory_scheduler.pybindu/server/workers/base.pybindu/settings.pybindu/utils/__init__.pybindu/utils/capabilities.pybindu/utils/logging.pybindu/utils/notifications.pybindu/utils/retry.pybindu/utils/worker/messages.pybindu/utils/worker/parts.pyexamples/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.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/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/agent-api.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/paymentHandler.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
✅ Files skipped from review due to trivial changes (33)
- examples/README.md
- frontend/src/routes/+page.svelte
- tests/unit/extensions/voice/init.py
- tests/unit/extensions/init.py
- frontend/.gitignore
- frontend/src/routes/settings/(nav)/authentication/+page.svelte
- frontend/src/lib/buildPrompt.ts
- bindu/utils/logging.py
- bindu/utils/worker/parts.py
- .gitignore
- bindu/extensions/init.py
- examples/multilingual-collab-agent/skills/collaborate/skill.yaml
- bindu/utils/init.py
- frontend/src/lib/utils/tree/buildSubtree.spec.ts
- bindu/extensions/voice/init.py
- bindu/server/metrics.py
- examples/cerina_bindu/cbt/README.md
- frontend/src/lib/types/ConvSidebar.ts
- tests/unit/utils/worker/test_messages.py
- .pre-commit-config.yaml
- frontend/src/lib/types/Session.ts
- frontend/src/lib/constants/mime.ts
- frontend/src/lib/components/voice/VoiceCallButton.svelte
- tests/unit/extensions/voice/test_voice_extension.py
- examples/voice-agent/README.md
- frontend/src/lib/migrations/routines/02-update-assistants-models.ts
- examples/voice-agent/skills/voice-brief-response-skill/skill.yaml
- frontend/src/lib/migrations/lock.ts
- bindu/extensions/voice/voice_agent_extension.py
- frontend/src/lib/components/voice/LiveTranscript.svelte
- tests/unit/extensions/voice/test_service_factory.py
- frontend/src/lib/utils/tree/addSibling.spec.ts
- tests/unit/extensions/voice/test_voice_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (35)
- examples/pdf_research_agent/README.md
- examples/multilingual-collab-agent/skills/research/skill.yaml
- frontend/src/lib/utils/tree/convertLegacyConversation.spec.ts
- bindu/penguin/config_validator.py
- pytest.ini
- tests/unit/test_minimax_example.py
- bindu/server/endpoints/utils.py
- frontend/src/routes/conversation/[id]/+page.svelte
- frontend/src/lib/types/Model.ts
- frontend/src/routes/settings/(nav)/negotiation/+page.svelte
- tests/unit/penguin/test_config_validator.py
- frontend/src/lib/utils/tree/addChildren.spec.ts
- bindu/utils/capabilities.py
- frontend/src/lib/server/models.ts
- tests/conftest_stubs.py
- tests/unit/server/scheduler/test_memory_scheduler.py
- bindu/server/workers/base.py
- examples/multilingual-collab-agent/skills/translate/skill.yaml
- tests/unit/utils/test_notifications.py
- frontend/src/lib/stores/chat.ts
- bindu/utils/retry.py
- frontend/src/routes/api/v2/conversations/[id]/message/[messageId]/+server.ts
- frontend/src/routes/conversation/[id]/+server.ts
- bindu/penguin/bindufy.py
- examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
- frontend/src/lib/server/config.ts
- tests/unit/extensions/voice/test_voice_websocket_integration.py
- bindu/extensions/voice/audio_config.py
- frontend/src/lib/server/files/uploadFile.ts
- frontend/src/routes/+layout.svelte
- bindu/extensions/voice/pipeline_builder.py
- bindu/extensions/voice/session_manager.py
- bindu/utils/worker/messages.py
- frontend/src/lib/services/voice-client.ts
- frontend/src/lib/components/chat/ChatWindow.svelte
| username = (os.getenv("USERNAME") or "").strip() | ||
| if not username: | ||
| try: | ||
| username = os.getlogin().strip() | ||
| except Exception: | ||
| username = "" | ||
| if not username: | ||
| try: | ||
| username = (getpass.getuser() or "").strip() | ||
| except Exception: | ||
| username = "" | ||
| if not username: | ||
| raise RuntimeError( | ||
| "Unable to determine Windows username for key ACL hardening" | ||
| ) | ||
|
|
||
| # Remove inherited ACLs and grant full control to current user only. | ||
| cmd = [ | ||
| "icacls", | ||
| str(path), | ||
| "/inheritance:r", | ||
| "/grant:r", | ||
| f"{username}:F", | ||
| "/remove:g", | ||
| "Users", | ||
| "Authenticated Users", | ||
| "Everyone", | ||
| ] | ||
| result = subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context of this file
head -n 300 bindu/extensions/did/did_agent_extension.py | tail -n 100Repository: GetBindu/Bindu
Length of output: 3689
🏁 Script executed:
# Find the full method/function containing this code
sed -n '220,290p' bindu/extensions/did/did_agent_extension.pyRepository: GetBindu/Bindu
Length of output: 2684
🏁 Script executed:
# Check if there are any imports or configuration related to Windows ACL handling
grep -n "import\|from\|icacls\|USERNAME" bindu/extensions/did/did_agent_extension.py | head -20Repository: GetBindu/Bindu
Length of output: 935
🌐 Web query:
Windows icacls unqualified username domain principal security best practices
💡 Result:
Using unqualified usernames (e.g., "username" instead of "DOMAIN\username" or "username@domain.com") with the Windows icacls command for domain principals works on domain-joined machines because the system resolves the name using the local computer's primary domain context via the security accounts manager (SAM) and Active Directory lookup. Official Microsoft documentation states that icacls accepts "friendly name form" for SIDs, which includes trustee names like domain users and groups. Examples in the docs use "User1" without domain prefix, implying local resolution succeeds for domain accounts when unambiguous. However, security best practices recommend always fully qualifying domain principals in icacls commands (e.g., DOMAIN\username or username@domain.com UPN) for the following reasons: - Ambiguity avoidance: Unqualified names match local accounts first if a local user with the same name exists, potentially granting permissions to the wrong principal. Domain lookup occurs only if local resolution fails. - Multi-domain forests or trusts: Unqualified names rely on the machine's primary domain; explicit qualification ensures correct domain targeting. - Scripting reliability: Scripts run on different machines or in varied contexts (e.g., workgroups, different domains) may fail or misresolve without qualification. - Auditability and clarity: Fully qualified names make ACLs self-documenting, reducing errors during review or migration. - Orphaned SID prevention: Explicit names help avoid unresolved SIDs if accounts are renamed or deleted. Examples from community sources (ServerFault, Reddit) show domain groups used as "DOMAIN\groupname", reinforcing explicit specification. To verify resolution before applying: Use wmic useraccount where name='username' get domain,name or nltest /user:username /query to check what principal an unqualified name resolves to. For highest security, prefer group principals over individual users, use deny ACEs judiciously, maintain canonical ACL order (as icacls preserves), and minimize explicit ACEs by leveraging inheritance.
Citations:
- 1: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls
- 2: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/icacls
- 3: https://technet.microsoft.com/library/cc753525%28v=ws.11%29
- 4: https://serverfault.com/questions/904743/powershell-icacls-permissions-with-domain-group
- 5: https://docs.microsoft.com/en-us/windows/security/identity-protection/access-control/security-principals
- 6: https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-principals
- 7: https://helgeklein.com/blog/permissions-a-primer-or-dacl-sacl-owner-sid-and-ace-explained
🌐 Web query:
icacls current user SID vs username security
💡 Result:
In Windows icacls command, both the current user's SID (e.g., prefixed with * for numeric form like *S-1-5-21-domain-rid) and username can be used to grant, deny, or remove permissions. There is no functional security difference between them: icacls resolves usernames to their corresponding SIDs internally, and permissions are always stored and enforced using SIDs in the NTFS ACL. Key points: - Use username (friendly name) for readability: icacls file.txt /grant Username:(R) - Use SID for reliability when name resolution might fail (e.g., remote contexts, deleted accounts): icacls file.txt /grant *S-1-1-0:(R) (example Everyone SID) - If username cannot be resolved (e.g., domain unavailable), icacls shows "No mapping between account names and security IDs was done." Using SID bypasses this. - When displaying ACLs (icacls file.txt), unresolved SIDs (orphaned from deleted users) show as raw SID strings instead of names, but permissions remain active for that SID. - Security risk with usernames: If a user is renamed or deleted, permissions persist via SID but may appear orphaned. SID usage prevents resolution errors during application. - Best practice: Prefer groups over individual users. Use usernames for local management; SIDs for scripts or cross-domain scenarios. Examples from official docs: icacls test1 /grant User1:(d,wdac) icacls TestFile /grant *S-1-1-0:(d,wdac)
Citations:
- 1: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/icacls
- 2: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls
- 3: https://technet.microsoft.com/library/cc753525%28v=ws.11%29
- 4: https://learn.microsoft.com/en-us/answers/questions/5827549/icacls-command-line-refuses-to-remove-acls
- 5: https://stackoverflow.com/questions/66626020/cant-remove-acl-entry-that-refers-to-orphaned-sid
- 6: http://www.permissionsreporter.com/support/orphaned-sids
🌐 Web query:
Windows ACL hardening cryptographic keys best practices
💡 Result:
Key takeaway: lock down the filesystem/registry ACLs that protect private key material (not just the application), use Windows CNG key isolation/appropriate key storage APIs, and follow least privilege + auditing for cryptographic key operations. 1) Know what Windows ACLs enforce (and what to audit) - Windows ACLs consist of ACEs inside a security descriptor. A DACL (discretionary ACL) controls allowed/denied access; if there’s no DACL, Windows grants full access to everyone, and if a DACL exists with no ACEs, attempts are denied. A SACL controls which access attempts get audited to the security event log. - Don’t treat key protection as only “DACL correctness”; ensure your SACLs audit the operations you care about (success/failure) for key objects. 2) Protect the private key storage locations that CNG/CryptoAPI use - CNG stores private keys in specific directories; applications access private keys through CNG’s key storage model, where the “key isolation” architecture helps ensure long-lived keys are isolated and all private-key access goes through the storage router and is audited by CNG. - For CryptoAPI (legacy) vs CNG, the private-key storage directory patterns differ. If you are hardening ACLs, target the correct directories for your key type/provider. 3) Hardening guidance specific to certificate private keys (MachineKeys) - Certificate pair keys (computer and users) are stored under the MachineKeys folder (under All Users Profile\Application Data\Microsoft\Crypto\RSA). If defaults were not tightened, you can end up with overly permissive access. - Microsoft documents the default permissions for MachineKeys on Windows Server 2003: Administrators (Full Control) and Everyone (Special) “This folder only”, with Everyone having limited rights (e.g., list/read and create/append/write extended attributes/attributes as documented). For hardening, treat this as a baseline to review against your environment and intended isolation model. 4) Apply least privilege at the level of cryptographic operations, not just object access - Windows supports cryptographic key-specific access rules (CryptoKeyAccessRule) where permissions map to cryptographic key operations. Use these to avoid granting broad “full control” when only a subset (e.g., read permissions vs change permissions vs delete) is required. - When defining ACL entries for key objects, ensure you are granting only the minimal CryptoKeyRights required by the service/app identity. 5) Avoid key material exposure via “user writeable” paths and permission mixing - A common failure mode is co-locating trusted executables/configs with user-writable directories. In Windows permission models, attackers can often tamper with untrusted-writable content to subvert a trust boundary. Use separate directories for trusted vs untrusted content and ACL them so only administrators can write to trusted material. 6) Use audited, centralized key management where possible - Where feasible, use purpose-built key management/HSM/KMS patterns rather than relying solely on local ACLs. Even in Windows-centric deployments, the goal is to reduce who can access plaintext key material and to make key lifecycle controls (rotation, revocation, auditing) consistent. 7) Operational controls: rotation, monitoring, and secure provisioning - Security recommendations for key management emphasize least privilege access, hardware security modules (HSMs), logging/monitoring, and regular key rotation. Concrete checklist (Windows-focused) A. Identify which key technology/provider your workload uses - CNG (KSPs, key isolation model) vs legacy CryptoAPI. This determines the private-key storage locations and how access is mediated. B. Enumerate private key directories and their ACLs - For certificate private keys: review MachineKeys folder ACLs (and child key files) permissions. - For CNG/legacy keys: review the documented CNG/CryptoAPI private key directories for the user/service contexts involved. C. Ensure ACLs grant only required cryptographic operations - Use CryptoKeyRights/CryptoKeyAccessRule concepts so services don’t get more than needed (avoid “full control” unless required). D. Configure SACL auditing for key access attempts - Add auditing for successful and/or failed access to key objects so compromise attempts are visible. E. Re-check inheritance and avoid breaking protection boundaries - In NTFS ACLs, inheritance changes what descendants do. Ensure that inheritance isn’t unintentionally widening access to key files under key directories. F. Validate using effective permissions + event logs - Verify that only the intended identities can read/use/modify/delete key material. - Confirm that auditing generates events for key access attempts. If you tell me what kind of “cryptographic keys” you mean (e.g., AD CS private keys, IIS TLS cert private keys, DPAPI master keys, CNG KSP keys for a service, VPN/IKE keys), plus Windows Server version(s) and whether you use domain services, I can tailor the ACL targets and a minimal-permissions approach.
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/secauthz/access-control-lists
- 2: https://learn.microsoft.com/en-us/windows/win32/seccng/key-storage-and-retrieval
- 3: https://learn.microsoft.com/en-us/troubleshoot/windows-server/windows-security/default-permissions-machinekeys-folders
- 4: https://msdn.microsoft.com/library/system.security.accesscontrol.cryptokeyaccessrule
- 5: https://technet.microsoft.com/library/system.security.accesscontrol.cryptokeyrights
- 6: https://learn.microsoft.com/en-us/archive/msdn-magazine/2008/november/access-control-understanding-windows-file-and-registry-permissions
- 7: https://wafaicloud.com/blog/best-practices-for-secure-key-management-in-windows-server/
Use fully qualified account name or SID for icacls principal to avoid domain resolution ambiguity.
USERNAME environment variable and os.getlogin()/getpass.getuser() return unqualified names that can resolve to the wrong principal on domain-joined systems: local accounts take precedence if a matching local user exists, potentially granting permissions to an unintended principal. Use fully qualified format (DOMAIN\username or UPN) or stable SID instead. Additionally, the hardcoded group names "Users", "Authenticated Users", "Everyone" are localized and may not resolve correctly in non-English environments; prefer SID equivalents (e.g., *S-1-5-32-545 for Users, *S-1-1-0 for Everyone).
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 258-258: Do not catch blind exception: Exception
(BLE001)
[warning] 263-263: Do not catch blind exception: Exception
(BLE001)
[error] 282-282: subprocess call: check for execution of untrusted input
(S603)
🤖 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 254 - 282, The
current Windows ACL call in did_agent_extension.py uses unqualified username
(USERNAME/os.getlogin()/getpass.getuser()) and literal group names in the icacls
command (cmd list) which can resolve incorrectly on domain-joined or non-English
systems; change the username resolution to produce a fully qualified account
(DOMAIN\username or UPN) or resolve the account to its well-known SID, and
replace the hardcoded group principals ("Users","Authenticated
Users","Everyone") with their SID equivalents (e.g. S-1-5-32-545, S-1-5-11,
S-1-1-0) when building the cmd list passed to subprocess.run; use Windows APIs
(LookupAccountName/ConvertStringSidToSid or pywin32/win32security) to map
account names to SIDs or to obtain the domain-qualified name and ensure
f"{username}:F" uses the qualified form or SID form.
| cmd = [ | ||
| "icacls", | ||
| str(path), | ||
| "/inheritance:r", | ||
| "/grant:r", | ||
| f"{username}:F", | ||
| "/remove:g", | ||
| "Users", | ||
| "Authenticated Users", | ||
| "Everyone", | ||
| ] | ||
| result = subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and examine the target lines
if [ -f "bindu/extensions/did/did_agent_extension.py" ]; then
echo "=== File exists, examining lines 271-282 ==="
sed -n '265,290p' bindu/extensions/did/did_agent_extension.py | cat -n
else
echo "File not found at bindu/extensions/did/did_agent_extension.py"
fiRepository: GetBindu/Bindu
Length of output: 1177
🏁 Script executed:
#!/bin/bash
# Check the overall structure of the did_agent_extension.py file
echo "=== File size and context ==="
wc -l bindu/extensions/did/did_agent_extension.py
echo ""
echo "=== Imports and subprocess usage ==="
head -50 bindu/extensions/did/did_agent_extension.py | cat -nRepository: GetBindu/Bindu
Length of output: 2401
🏁 Script executed:
#!/bin/bash
# Search for app_settings usage and did configuration
echo "=== Checking for app_settings and did config ==="
rg "app_settings" bindu/extensions/did/ -A 2 -B 2
echo ""
echo "=== Searching for did configuration in settings ==="
fd -e py . bindu/settings | head -20Repository: GetBindu/Bindu
Length of output: 7983
🏁 Script executed:
#!/bin/bash
# Check the bindu.settings structure
echo "=== Checking bindu/settings files ==="
ls -la bindu/settings/ 2>/dev/null || echo "No bindu/settings directory found"
echo ""
# Search for DID-related settings
rg "icacls|acl_command" bindu/ --type pyRepository: GetBindu/Bindu
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Find the settings module
echo "=== Finding settings module ==="
find bindu -name "settings.py" -o -name "settings" -type d | head -20
echo ""
echo "=== Looking for settings configuration ==="
rg "class.*Settings|icacls|acl_command" bindu/ --type py -B 2 -A 2Repository: GetBindu/Bindu
Length of output: 4772
🏁 Script executed:
#!/bin/bash
# Check app_settings initialization
echo "=== Checking app_settings structure ==="
rg "app_settings\s*=" bindu/ --type py -B 3 -A 3 | head -60
echo ""
# Look for DID configuration specifically
echo "=== Searching for DID settings class ==="
rg "class.*DID|settings\.did" bindu/ --type pyRepository: GetBindu/Bindu
Length of output: 3898
🏁 Script executed:
#!/bin/bash
# Find config/settings files
echo "=== Finding all settings-related files ==="
find bindu -type f -name "*.py" | xargs grep -l "app_settings" | head -10
echo ""
# Check the main settings file
find bindu -type f -name "settings.py" -exec cat {} \;Repository: GetBindu/Bindu
Length of output: 44232
🏁 Script executed:
#!/bin/bash
# Double-check if icacls_path or acl_command_timeout exist anywhere in the codebase
echo "=== Searching for icacls_path or acl_command_timeout ==="
rg "icacls_path|acl_command_timeout" bindu/ --type py
echo ""
echo "=== Full DIDSettings class ==="
sed -n '/^class DIDSettings/,/^class [A-Z]/p' bindu/settings.py | head -80Repository: GetBindu/Bindu
Length of output: 1617
🏁 Script executed:
#!/bin/bash
# Check if the file uses Windows-specific code and if there are other subprocess calls
echo "=== Checking for Windows-specific code in did_agent_extension.py ==="
rg "platform|windows|icacls" bindu/extensions/did/did_agent_extension.py -B 3 -A 3
echo ""
echo "=== Looking for other subprocess usage ==="
rg "subprocess\." bindu/extensions/did/did_agent_extension.pyRepository: GetBindu/Bindu
Length of output: 1436
Add timeout and configurable icacls path to ACL hardening subprocess call.
subprocess.run() currently hardcodes "icacls" (allowing PATH resolution) and lacks a timeout, risking indefinite hangs during key generation on Windows. Per coding guidelines, hardcoded configuration values must be externalized to app_settings.
To fix this:
- Add
icacls_pathandacl_command_timeout_secondsfields toDIDSettingsinbindu/settings.py - Update the subprocess call to use these settings with an explicit timeout
🛡️ Suggested shape
cmd = [
- "icacls",
+ app_settings.did.icacls_path,
str(path),
"/inheritance:r",
"/grant:r",
f"{username}:F",
@@
- result = subprocess.run(cmd, capture_output=True, text=True, check=False)
+ result = subprocess.run(
+ cmd,
+ capture_output=True,
+ text=True,
+ check=False,
+ timeout=app_settings.did.acl_command_timeout_seconds,
+ )And in bindu/settings.py (DIDSettings class), add:
# Windows ACL hardening
icacls_path: str = "icacls" # Override with full path (e.g., "C:\\Windows\\System32\\icacls.exe") if needed
acl_command_timeout_seconds: int = 10🧰 Tools
🪛 Ruff (0.15.10)
[error] 282-282: subprocess call: check for execution of untrusted input
(S603)
🤖 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 271 - 282, Add two
new settings to DIDSettings (icacls_path: str and acl_command_timeout_seconds:
int) in bindu/settings.py and update the subprocess invocation in
did_agent_extension.py to use those settings instead of the hardcoded "icacls"
and to pass a timeout value; specifically replace the literal "icacls" in the
cmd list with settings.icacls_path (or equivalent) and add the timeout=
settings.acl_command_timeout_seconds argument to the subprocess.run call (keep
capture_output=True, text=True, check=False and preserve existing cmd elements
like path and username).
| try: | ||
| async with asyncio.timeout(self._total_response_timeout_seconds): | ||
| if emit_frames: | ||
| try: | ||
| 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( | ||
| {first_task}, timeout=timeout_seconds | ||
| ) | ||
| if not done: | ||
| # TTS filler so the agent doesn't feel "dead air". | ||
| await self.push_frame( | ||
| TextFrame(text=DEFAULT_THINKING_TEXT) | ||
| ) | ||
| first = await first_task | ||
| except StopAsyncIteration: | ||
| return None | ||
|
|
||
| ( | ||
| streamed_text, | ||
| last_emitted, | ||
| started_speaking, | ||
| ) = await _consume_chunk( | ||
| first, streamed_text, last_emitted, started_speaking | ||
| ) | ||
|
|
||
| async for chunk_text in chunks: | ||
| ( | ||
| streamed_text, | ||
| last_emitted, | ||
| started_speaking, | ||
| ) = await _consume_chunk( | ||
| chunk_text, streamed_text, last_emitted, started_speaking | ||
| ) | ||
| except TimeoutError: | ||
| 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 None |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does asyncio.timeout cancel tasks created with asyncio.create_task inside its async-with block when the timeout fires in Python 3.12?
💡 Result:
No, asyncio.timeout does not automatically cancel tasks created with asyncio.create_task inside its async-with block when the timeout fires. It only cancels the enclosing/current task (the one running the async-with block).
Citations:
- 1: https://github.com/python/cpython/blob/main/Lib/asyncio/timeouts.py
- 2: https://docs.python.org/3.12/library/asyncio-task.html
- 3: https://docs.python.org/release/3.12.0/library/asyncio-task.html
- 4: https://superfastpython.com/asyncio-timeout_at/
- 5: https://superfastpython.com/asyncio-timeout/
first_task can leak resources when the total-response timeout fires.
When the outer asyncio.timeout(self._total_response_timeout_seconds) context fires during await first_task at line 340, Python 3.12's asyncio.timeout only cancels the current await point—it does not automatically cancel the first_task task itself. The task continues running in the background, still holding the chunks async-generator and the underlying manifest_run iterator, allowing LLM/provider calls to continue after the timeout fallback has been emitted to the user.
Additionally, the chunks async-generator is never explicitly aclose()d on the timeout path, preventing cleanup of upstream resources until garbage collection.
🔧 Suggested fix
chunks = self._iter_text_chunks(raw)
+ first_task: asyncio.Task[Any] | None = None
try:
async with asyncio.timeout(self._total_response_timeout_seconds):
if emit_frames:
try:
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(
{first_task}, timeout=timeout_seconds
)
if not done:
# TTS filler so the agent doesn't feel "dead air".
await self.push_frame(
TextFrame(text=DEFAULT_THINKING_TEXT)
)
first = await first_task
except StopAsyncIteration:
return None
...
except TimeoutError:
+ if first_task is not None and not first_task.done():
+ first_task.cancel()
+ try:
+ await first_task
+ except (asyncio.CancelledError, Exception):
+ pass
+ try:
+ await chunks.aclose()
+ except Exception:
+ logger.debug("Failed to close chunk generator on timeout", exc_info=True)
if emit_frames:
...🤖 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 323 - 369, The timeout
path can leave first_task and the chunks async-generator running; ensure you
cancel and await first_task and aclose() the chunks when the asyncio.timeout
expires. Specifically, in the block around asyncio.timeout where first_task =
asyncio.create_task(anext(chunks)) and chunks is iterated, on TimeoutError (or
in the except/cleanup path) check if first_task exists and is not done, call
first_task.cancel(), await it inside a try/except (suppress CancelledError and
any CancelledError/Exception), and if chunks supports aclose(), call await
chunks.aclose() (also wrapped in try/except) to release upstream resources
before returning the fallback or None; apply this cleanup both when emit_frames
is true and false so the manifest_run and provider calls are not left running.
| """Voice session REST + WebSocket endpoints. | ||
|
|
||
| Provides: | ||
| POST /voice/session/start → Start a new voice session | ||
| DELETE /voice/session/{session_id} → End a voice session | ||
| GET /voice/session/{session_id}/status → Get session status | ||
| WS /ws/voice/{session_id} → Bidirectional audio stream | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import asyncio | ||
| import importlib | ||
| import secrets | ||
| import time | ||
| import uuid | ||
| from dataclasses import dataclass | ||
| from typing import TYPE_CHECKING, Any | ||
| from uuid import uuid4 | ||
|
|
||
| from fastapi import HTTPException | ||
| from starlette.requests import Request | ||
| from starlette.responses import JSONResponse, Response | ||
| from starlette.websockets import WebSocket, WebSocketDisconnect, WebSocketState | ||
|
|
||
| from bindu.settings import app_settings | ||
| from bindu.utils.logging import get_logger | ||
| from bindu.server.endpoints.utils import handle_endpoint_errors | ||
|
|
||
| if TYPE_CHECKING: | ||
| from bindu.server.applications import BinduApplication | ||
|
|
||
| logger = get_logger("bindu.server.endpoints.voice") | ||
|
|
||
| _BACKGROUND_TASKS: set[asyncio.Task[None]] = set() | ||
| _VOICE_RATE_LIMIT_LOCK = asyncio.Lock() | ||
| _VOICE_RATE_LIMIT_IP_BUCKET: dict[str, list[float]] = {} | ||
| _VOICE_RATE_LIMIT_REDIS_LOCK = asyncio.Lock() | ||
| _VOICE_RATE_LIMIT_REDIS_CLIENT: Any | None = None | ||
|
|
||
| try: | ||
| import redis.asyncio as _redis_async # type: ignore[import-not-found] | ||
|
|
||
| _REDIS_AVAILABLE = True | ||
| except Exception: # pragma: no cover | ||
| _redis_async = None # type: ignore[assignment] | ||
| _REDIS_AVAILABLE = False | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
File exceeds the project's ~500-line module size guideline — consider extracting submodules.
This module is ~1055 lines and mixes several concerns: rate-limiting (lines 36-144), serializers/wrapper classes (146-264), small helpers (267-399), error handling (402-415), control reader (418-538), REST endpoints (546-703), and the large WebSocket handler (711-1055). Splitting at natural seams would significantly improve readability and testability, e.g.:
voice/rate_limit.py—_RATE_LIMIT_LUA,_get_rate_limit_redis_client,_rate_limit_allow_ipvoice/ws_io.py—_FilteredWebSocket,_RawAudioFrameSerializer,_send_json,_send_error_and_close,_voice_control_reader,_VoiceControlStatevoice/ws_auth.py—_extract_bearer_token,_extract_ws_session_token,_has_ws_subprotocols,_voice_preflight_error,_classify_voice_pipeline_errorvoice/rest.py—voice_session_start/voice_session_end/voice_session_statusvoice/ws.py—voice_websocket
As per coding guidelines: "Keep Python files under ~500 lines of code - extract modules when larger".
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 46-46: 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 1 - 50, This file is
too large and mixes distinct concerns; refactor by extracting related code into
separate modules: move rate-limiting constants and functions (_RATE_LIMIT_LUA,
_get_rate_limit_redis_client, _rate_limit_allow_ip, _VOICE_RATE_LIMIT_LOCK,
_VOICE_RATE_LIMIT_IP_BUCKET, _VOICE_RATE_LIMIT_REDIS_LOCK,
_VOICE_RATE_LIMIT_REDIS_CLIENT and any redis-related helpers) into a new
voice/rate_limit.py; move WebSocket I/O helpers and serializers
(_FilteredWebSocket, _RawAudioFrameSerializer, _send_json,
_send_error_and_close, _voice_control_reader, _VoiceControlState and related
helper functions) into voice/ws_io.py; move auth and preflight helpers
(_extract_bearer_token, _extract_ws_session_token, _has_ws_subprotocols,
_voice_preflight_error, _classify_voice_pipeline_error) into voice/ws_auth.py;
move REST handlers (voice_session_start, voice_session_end, voice_session_status
and their serializers) into voice/rest.py; and keep the top-level
voice_websocket handler in voice/ws.py. After moving, update imports in each new
module and the original file to import the relocated symbols, ensure
type-checking guards (TYPE_CHECKING) still work, and run tests to fix any import
cycles. This keeps each file under ~500 lines and preserves the original public
symbols (function/class names) so callers remain unchanged.
| if scheme == "https": | ||
| path = parsed.path or "/" | ||
| if parsed.query: | ||
| path = f"{path}?{parsed.query}" | ||
|
|
||
| context = ssl.create_default_context() | ||
| request_headers = {"Host": host_header, "Connection": "close", **headers} | ||
| request_headers["Content-Length"] = str(len(payload)) | ||
| try: | ||
| request_lines = [f"POST {path} HTTP/1.1\r\n"] | ||
| request_lines.extend( | ||
| f"{key}: {value}\r\n" for key, value in request_headers.items() | ||
| ) | ||
| request_lines.append("\r\n") | ||
| request_bytes = "".join(request_lines).encode("latin-1") + payload | ||
|
|
||
| with socket.create_connection( | ||
| (destination_ip, port), timeout=self.timeout | ||
| ) as sock: | ||
| with context.wrap_socket( | ||
| sock, server_hostname=hostname | ||
| ) as tls_socket: | ||
| tls_socket.sendall(request_bytes) | ||
| response = http.client.HTTPResponse(tls_socket) | ||
| response.begin() | ||
| status = response.status | ||
| body = response.read() or b"" | ||
| if 200 <= status < 300: | ||
| return status | ||
| message = body.decode("utf-8", errors="ignore").strip() | ||
| raise NotificationDeliveryError( | ||
| status, message or f"HTTP error {status}" | ||
| ) | ||
| except (OSError, http.client.HTTPException, ssl.SSLError) as exc: | ||
| raise NotificationDeliveryError( | ||
| None, f"Connection error: {exc}" | ||
| ) from exc |
There was a problem hiding this comment.
CRLF header-injection risk in the manually constructed HTTPS request.
The HTTPS path builds the raw request line/headers with string concatenation and sendall, bypassing http.client's validation of header values. Header values here include Authorization: Bearer {token} (and any callers-supplied entries via _build_headers), where token comes from PushNotificationConfig (attacker-controllable data). If a value contains \r\n, additional headers or a request body can be smuggled into the upstream request — a defense that http.client / urllib.request normally provide for you but that is lost here.
Consider validating header keys/values (reject \r, \n, non-printable) before formatting, or better, reuse http.client.HTTPSConnection with a pre-connected, hostname-pinned socket so the stdlib performs header validation:
🛡️ Sketch: delegate framing/validation to http.client
- try:
- request_lines = [f"POST {path} HTTP/1.1\r\n"]
- request_lines.extend(
- f"{key}: {value}\r\n" for key, value in request_headers.items()
- )
- request_lines.append("\r\n")
- request_bytes = "".join(request_lines).encode("latin-1") + payload
-
- with socket.create_connection(
- (destination_ip, port), timeout=self.timeout
- ) as sock:
- with context.wrap_socket(
- sock, server_hostname=hostname
- ) as tls_socket:
- tls_socket.sendall(request_bytes)
- response = http.client.HTTPResponse(tls_socket)
- response.begin()
- status = response.status
- body = response.read() or b""
+ try:
+ sock = socket.create_connection(
+ (destination_ip, port), timeout=self.timeout
+ )
+ conn = http.client.HTTPSConnection(
+ hostname, port, timeout=self.timeout, context=context
+ )
+ conn.sock = context.wrap_socket(sock, server_hostname=hostname)
+ try:
+ conn.request("POST", path, body=payload, headers=request_headers)
+ response = conn.getresponse()
+ status = response.status
+ body = response.read() or b""
+ finally:
+ conn.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/utils/notifications.py` around lines 246 - 282, The manual construction
of the HTTPS request in NotificationsSender (the block that builds
request_lines, encodes them and calls tls_socket.sendall) allows CRLF header
injection from unvalidated header values (e.g., Authorization token); replace
this manual framing by using http.client.HTTPSConnection so the stdlib validates
and frames headers for you (create an HTTPSConnection, call connect() or supply
a pre-wrapped socket via socket.create_connection + context.wrap_socket and pass
it into the connection, then use conn.request("POST", path, body=payload,
headers=request_headers) and conn.getresponse()), or if you must keep manual
send, strictly validate header keys/values in _build_headers / before formatting
(reject any '\r' or '\n' and non-printable characters, and ensure Content-Length
is correct) and then read response using http.client.HTTPResponse as before;
update error handling to raise NotificationDeliveryError with the response
status/body as currently done.
| if (startToken !== startTokenCounter) { | ||
| await localClient.stopSession().catch(() => undefined); | ||
| return; | ||
| } | ||
| voiceSessionId.set(session.session_id); | ||
| voiceContextId.set(session.context_id); | ||
|
|
||
| client = localClient; | ||
| await localClient.connect(session.ws_url, session.session_id, session.session_token); | ||
| if (startToken !== startTokenCounter) { | ||
| await localClient.stopSession().catch(() => undefined); | ||
| return; | ||
| } | ||
| } catch (err) { | ||
| // On failure, clear the partially-initialized client | ||
| if (client === localClient) { | ||
| client = null; | ||
| } | ||
| const errorMessage = err instanceof Error ? err.message : String(err); | ||
| voiceError.set(errorMessage); | ||
| voiceState.set("error"); | ||
| await localClient.stopSession().catch(() => undefined); | ||
| voiceSessionId.set(null); | ||
| voiceContextId.set(null); | ||
| throw err; | ||
| } finally { | ||
| if (startToken === startTokenCounter) { | ||
| isStarting = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear isStarting on stale-start cancellation.
If endVoiceSession() increments startTokenCounter while startup is pending, Lines 144-155 return through finally with a stale token, so Line 171 never runs. After that, every future start throws “A voice session is already starting”.
Proposed fix
} finally {
- if (startToken === startTokenCounter) {
- isStarting = false;
- }
+ isStarting = false;
}
}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 144 - 173, The startup
early-return checks use startToken !== startTokenCounter to cancel a stale
startup but they return without clearing isStarting, leaving isStarting stuck
true and blocking future starts; update both early-return sites (the checks that
compare startToken and startTokenCounter around the connect/initialization
logic) to set isStarting = false before calling
localClient.stopSession().catch(() => undefined) and returning, and likewise
ensure any other identical stale-token return paths (the later check referenced)
do the same so isStarting is always cleared when a stale-start cancels.
| fileStream.on("data", (chunk) => { | ||
| if (chunk instanceof Uint8Array) { | ||
| chunks.push(chunk); | ||
| return; | ||
| } | ||
| reject(new Error("Unexpected chunk type from avatar download stream")); | ||
| }); | ||
| fileStream.on("error", (err) => | ||
| reject(err ?? new Error("Avatar download failed")) | ||
| ); |
There was a problem hiding this comment.
Handle avatar stream rejections so the export response cannot hang.
The new reject(...) paths bubble into Promise.all(promises), but that promise has no rejection handler and the response stream is returned immediately. If an avatar stream errors or emits an unexpected chunk, zipfile.end() is skipped and the client can wait indefinitely.
🐛 One way to fail the request before returning the ZIP stream
- Promise.all(promises).then(async () => {
+ try {
+ await Promise.all(promises);
logger.info(
{
userId: locals.user?._id,
...stats,
},
"Exported user data"
);
zipfile.end();
if (locals.user?._id) {
await collections.messageEvents.insertOne({
userId: locals.user?._id,
type: "export",
createdAt: new Date(),
expiresAt: new Date(Date.now() + 1000 * 60 * 60), // 1 hour
});
}
- });
+ } catch (err) {
+ logger.error({ err, userId: locals.user?._id }, "Failed to export user data");
+ error(500, "Data export failed");
+ }Also applies to: 178-195
🤖 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 - 138, The
avatar download stream can call reject(...) which will leave zipfile.end()
uncalled and hang the response; update the logic around Promise.all(promises)
and the fileStream handlers so any rejection (from the "Unexpected chunk type"
branch or fileStream "error") triggers cleanup: ensure zipfile.end() is called
and the export request is failed before returning the ZIP stream. Concretely,
add a rejection handler for Promise.all(promises) (or use try/catch/finally) to
call zipfile.end() and propagate the error, and in the fileStream "data" and
"error" handlers ensure they signal the same failure path (instead of only
calling reject locally) so both the 129-138 block and the similar 178-195 block
cleanly end the zip and abort the response on errors.
| grpc = [ | ||
| "grpcio>=1.62.0", | ||
| "grpcio-tools>=1.62.0", | ||
| "protobuf>=4.25.0", | ||
| "grpcio>=1.67.0,<1.78.0", | ||
| "grpcio-tools>=1.67.0,<1.78.0", | ||
| "protobuf>=5.29.6,<6.0.0", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unconditional grpc imports in tests and whether dev deps include grpc.
rg -n -C3 '^import grpc|from bindu\.grpc\.generated|from bindu\.grpc\.server' --type=py
python - <<'PY'
from pathlib import Path
text = Path("pyproject.toml").read_text()
dev_block = text.split("[dependency-groups]", 1)[1].split("[tool.coverage.run]", 1)[0]
print(dev_block)
print("dev_has_grpc =", any(name in dev_block for name in ("grpcio", "bindu[grpc]", "protobuf")))
PYRepository: GetBindu/Bindu
Length of output: 5411
Guard gRPC test modules with importorskip or add gRPC to dev dependencies.
dev no longer includes gRPC dependencies, but test modules have unconditional imports of grpc and generated stubs:
tests/integration/grpc/test_grpc_e2e.pyimportsgrpc,agent_handler_pb2,agent_handler_pb2_grpc, andstart_grpc_serverat module leveltests/unit/grpc/test_service.pyandtests/unit/grpc/test_client.pyimport frombindu.grpc.generated- The generated file
bindu/grpc/generated/agent_handler_pb2_grpc.pyunconditionally importsgrpc
This will cause test collection to fail when gRPC is not installed. Either add bindu[grpc] to the dev dependency group or guard test modules with pytest.importorskip("grpc").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 87 - 91, Test collection fails when gRPC is not
installed because tests import grpc and generated stubs at module scope (see
tests/integration/grpc/test_grpc_e2e.py, tests/unit/grpc/test_service.py,
tests/unit/grpc/test_client.py and the generated
bindu/grpc/generated/agent_handler_pb2_grpc.py which unconditionally imports
grpc); fix by either adding the gRPC extra to dev dependencies (add bindu[grpc]
or grpc entries to the dev group in pyproject.toml) or guard the tests so they
skip when grpc is absent (add pytest.importorskip("grpc") at the top of each
test module that imports grpc or generated stubs); choose one approach and apply
consistently to all referenced test modules so pytest collection no longer
fails.
| # Voice agent support (use: pip install bindu[voice]) | ||
| voice = [ | ||
| "pipecat-ai[deepgram,elevenlabs,silero]~=0.0.105", | ||
| "websockets>=14.0", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which extras pipecat-ai 0.0.105 exposes on PyPI.
python - <<'PY'
import json
import urllib.request
with urllib.request.urlopen("https://pypi.org/pypi/pipecat-ai/0.0.105/json", timeout=10) as response:
data = json.load(response)
print(sorted(data["info"].get("provides_extra") or []))
PYRepository: GetBindu/Bindu
Length of output: 2639
🏁 Script executed:
# Find voice-related example files
fd -t f -e py -e yaml -e json . | grep -i voice | head -20Repository: GetBindu/Bindu
Length of output: 933
🏁 Script executed:
# Check the current pyproject.toml around the voice extra definition
cat -n pyproject.toml | sed -n '85,105p'Repository: GetBindu/Bindu
Length of output: 725
🏁 Script executed:
# Search for voice example or voice-related code
rg -l "voice" --type py | head -20Repository: GetBindu/Bindu
Length of output: 880
🏁 Script executed:
# Search for pipecat imports to understand how it's used
rg "pipecat" --type py -A 3 | head -50Repository: GetBindu/Bindu
Length of output: 4385
🏁 Script executed:
# Check the voice example main.py
cat -n examples/voice-agent/main.py | head -100Repository: GetBindu/Bindu
Length of output: 3842
🏁 Script executed:
# Search for Piper and Azure TTS references
rg -i "piper|azure" --type py | grep -i "tts\|voice\|agent" | head -30Repository: GetBindu/Bindu
Length of output: 3081
🏁 Script executed:
# Examine service_factory.py which likely configures TTS
cat -n bindu/extensions/voice/service_factory.pyRepository: GetBindu/Bindu
Length of output: 9561
🏁 Script executed:
# Check if silero is actually used in the voice code
rg -i "silero" --type py bindu/extensions/voice/Repository: GetBindu/Bindu
Length of output: 248
🏁 Script executed:
# Look for any pipecat version documentation or extras reference
rg -i "pipecat.*extra|0\.0\.105" --type py --type toml --type md | head -20Repository: GetBindu/Bindu
Length of output: 197
🏁 Script executed:
# Search for STT provider mentions to understand silero's role
rg "stt_provider|STT" bindu/extensions/voice/service_factory.py -A 2Repository: GetBindu/Bindu
Length of output: 1475
🌐 Web query:
pipecat-ai 0.0.105 PyPI extras available piper azure
💡 Result:
For pipecat-ai version 0.0.105 on PyPI, both 'piper' and 'azure' extras are available. The PyPI project page explicitly lists 'piper' and 'azure' among the available extras, including: aic, anthropic, assemblyai, asyncai, aws, aws-nova-sonic, azure, camb, cartesia, cerebras, daily, deepgram, deepseek, elevenlabs, fal, fireworks, fish, gladia, google, gradium, grok, groq, gstreamer, heygen, hume, inworld, koala, kokoro, krisp, langchain, lemonslice, livekit, lmnt, local, local-smart-turn, mcp, mem0, mistral, mlx-whisper, moondream, neuphonic, noisereduce, nvidia, openai, rnnoise, openpipe, openrouter, perplexity, piper, qwen, remote-smart-turn, resembleai, rime, riva, runner, sagemaker, sambanova, sarvam, sentry, silero, simli, soniox, soundfile, speechmatics, strands, tavus, together, tracing, ultravox, webrtc, websocket, websockets-base, whisper. Piper provides local text-to-speech synthesis using Piper voice models (core framework supports it; no additional deps shown in pyproject.toml snippets, likely minimal or uses base deps). Azure provides integration with Azure Cognitive Services Speech SDK (depends on azure-cognitiveservices-speech). Install with: pip install "pipecat-ai[piper,azure]".
Citations:
- 1: https://pypi.org/project/pipecat-ai/0.0.105/
- 2: https://github.com/pipecat-ai/pipecat/blob/b67af19d/pyproject.toml
- 3: https://github.com/pipecat-ai/pipecat/blob/main/pyproject.toml
- 4: https://github.com/pipecat-ai/pipecat-flows/blob/main/pyproject.toml
- 5: https://reference-server.pipecat.ai/en/stable/api/pipecat.services.piper.tts.html
- 6: https://docs.pipecat.ai/api-reference/server/services/tts/piper
Add missing TTS providers to the voice extra.
The voice example defaults to Piper TTS and supports Azure TTS as a fallback, but bindu[voice] currently only includes Deepgram, ElevenLabs, and Silero extras. Without Piper and Azure, uv sync --extra voice will fail when the code attempts to import and use these services.
🔧 Add piper and azure extras
voice = [
- "pipecat-ai[deepgram,elevenlabs,silero]~=0.0.105",
+ "pipecat-ai[azure,deepgram,elevenlabs,piper,silero]~=0.0.105",
"websockets>=14.0",
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 93 - 97, The voice extra currently lists
pipecat-ai with only [deepgram,elevenlabs,silero], which omits the Piper and
Azure TTS providers used by the voice agent; update the voice extra definition
in pyproject.toml (the voice = [...] block listing
"pipecat-ai[deepgram,elevenlabs,silero]~=0.0.105") to include the piper and
azure extras (add piper and azure to the bracketed extras for pipecat-ai) so
installing bindu[voice] brings in those TTS providers as well.
|
|
||
| echo "" | ||
| echo "Running TTS provider checks..." | ||
| check_tts_provider "$TTS_PROVIDER" "Primary TTS" | ||
| if [[ "$(to_lower "$TTS_FALLBACK_PROVIDER")" != "none" && -n "${TTS_FALLBACK_PROVIDER}" ]]; then | ||
| check_tts_provider "$TTS_FALLBACK_PROVIDER" "Fallback TTS" | ||
| fi | ||
|
|
||
| # --- Optional Bindu integration test --- | ||
| if [[ "$BINDU_CHECK" == true ]]; then | ||
| echo "" | ||
| echo "Running Bindu endpoint checks at ${BINDU_BASE_URL} ..." | ||
|
|
||
| HEALTH_BODY="$TMP_DIR/health.json" | ||
| HEALTH_HTTP=$(curl -sS -o "$HEALTH_BODY" -w "%{http_code}" "${BINDU_BASE_URL}/health" || true) | ||
|
|
||
| if [[ "$HEALTH_HTTP" == "200" ]]; then | ||
| pass "Bindu health endpoint reachable" | ||
| else | ||
| fail "Bindu health endpoint failed (HTTP $HEALTH_HTTP)" | ||
| head -c 300 "$HEALTH_BODY"; echo | ||
| fi | ||
|
|
||
| SESSION_BODY="$TMP_DIR/session.json" | ||
| SESSION_HTTP=$(curl -sS -o "$SESSION_BODY" -w "%{http_code}" \ | ||
| -X POST "${BINDU_BASE_URL}/voice/session/start" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{}' || true) | ||
|
|
||
| if [[ "$SESSION_HTTP" == "201" ]]; then | ||
| if grep -q '"ws_url"' "$SESSION_BODY"; then | ||
| pass "Bindu voice session start works (HTTP 201, ws_url returned)" | ||
| else | ||
| warn "Voice session created but ws_url field missing" | ||
| head -c 400 "$SESSION_BODY"; echo | ||
| fi | ||
| else | ||
| fail "Bindu voice session start failed (HTTP $SESSION_HTTP)" | ||
| head -c 400 "$SESSION_BODY"; echo | ||
| fi | ||
| fi | ||
|
|
||
| save_artifacts |
There was a problem hiding this comment.
set -e + TTS check failures cause early exit, skipping fallback check and artifact saving.
check_tts_provider propagates the failing return code (e.g., 1) from the sub-checks (check_elevenlabs_tts, check_azure_tts, check_piper_tts). Because this script runs under set -euo pipefail, a non-zero return from the top-level call on line 303 will terminate the script, so the fallback TTS check (line 305), the Bindu endpoint checks (lines 309-340), and save_artifacts (line 342) never run when the primary TTS fails — which is exactly when you'd most want the diagnostic artifacts and fallback validation.
🔧 Suggested fix
echo ""
echo "Running TTS provider checks..."
-check_tts_provider "$TTS_PROVIDER" "Primary TTS"
+check_tts_provider "$TTS_PROVIDER" "Primary TTS" || true
if [[ "$(to_lower "$TTS_FALLBACK_PROVIDER")" != "none" && -n "${TTS_FALLBACK_PROVIDER}" ]]; then
- check_tts_provider "$TTS_FALLBACK_PROVIDER" "Fallback TTS"
+ check_tts_provider "$TTS_FALLBACK_PROVIDER" "Fallback TTS" || true
fiConsider similarly guarding the Deepgram block (already uses || true on curl) and tracking an overall exit code to report a non-zero status at the end if any check failed, so CI still surfaces failures but all diagnostics are produced.
🤖 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 300 - 342, The script exits
early under set -euo pipefail because check_tts_provider (which calls
check_elevenlabs_tts/check_azure_tts/check_piper_tts) returns non-zero and
aborts the whole run, preventing fallback checks, Bindu tests and
save_artifacts; fix by preventing immediate exit on those checks (e.g., append
|| true or run them in a subshell) and instead capture their return codes into a
cumulative variable like overall_exit (initialize overall_exit=0, set
overall_exit=1 if any check returns non-zero), ensure the BINDU_CHECK block
still runs (keep its curl || true behavior) and always call save_artifacts, then
exit with the cumulative overall_exit at the very end so CI fails but
diagnostics are always produced.
5eab8a8 to
6477cf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
examples/typescript-langchain-agent/quiz-agent.ts (1)
65-65:⚠️ Potential issue | 🟠 MajorAwait
bindufy()and handle startup failures.This top-level call is still fire-and-forget; if registration rejects, the example can fail as an unhandled promise rejection instead of a clear startup error.
Verify the SDK contract and current call site:
#!/bin/bash # Description: Verify whether bindufy is async and whether this example observes its returned promise. rg -nP --type=ts -C3 'export\s+async\s+function\s+bindufy\s*\(' rg -nP --type=ts -C3 '^\s*bindufy\s*\(' examples/typescript-langchain-agent/quiz-agent.tsProposed fix
-bindufy( - { +async function main(): Promise<void> { + await bindufy( + { author: "raahul@getbindu.com", name: "quiz-generator-agent", description: "Educational assessment expert for MCQ generation", @@ - }, + }, - /** - * Handler - */ - async (messages: ChatMessage[]) => { + /** + * Handler + */ + async (messages: ChatMessage[]) => { @@ - } -); + } + ); +} + +void main().catch((error: unknown) => { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/typescript-langchain-agent/quiz-agent.ts` at line 65, The call to bindufy is currently fire-and-forget and can cause unhandled promise rejections; modify the top-level invocation of bindufy() to await its returned promise and handle errors by wrapping the await in a try/catch (log the error via the existing logger or console.error and exit/stop startup with a non-zero code). Locate the bindufy() call in quiz-agent.ts, change it to an awaited call inside an async main/bootstrap function or top-level await, and ensure the catch block reports the error and terminates startup cleanly (e.g., process.exit(1) or equivalent).
🧹 Nitpick comments (5)
examples/pdf_research_agent/pdf_research_agent.py (1)
53-53: Make the PDF extension check case-insensitive.
"paper.PDF".endswith(".pdf")isFalse, so uppercase-extension paths silently fall through to the raw-text branch at Line 93 and get sent to the LLM verbatim.Proposed fix
- if source_text.endswith(".pdf"): + if source_text.lower().endswith(".pdf"):🤖 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 53, The PDF extension check using source_text.endswith(".pdf") is case-sensitive; change it to a case-insensitive check by normalizing the filename (e.g., use source_text.lower().endswith(".pdf")) where the current conditional sits (the if using source_text.endswith(".pdf")), so paths like "paper.PDF" are correctly detected as PDFs and handled by the PDF branch rather than falling through to the raw-text logic later in the file.examples/voice-agent/main.py (1)
37-44: Parse errors onOPENROUTER_MEMORY_TURNS/VOICE_MAX_SENTENCEScrash import.
int(os.environ.get(..., "N") or "N")raisesValueErrorat module load if the env var is set to a non-numeric value, taking down the example with a stack trace rather than falling back to the default. Consider a small guarded helper.🔧 Proposed fix
-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"), -) +def _env_int(name: str, default: int, *, floor: int = 0) -> int: + raw = os.environ.get(name, "").strip() + try: + return max(floor, int(raw)) if raw else max(floor, default) + except ValueError: + print(f"Warning: {name}={raw!r} is not an integer; using default {default}") + return max(floor, default) + +OPENROUTER_MEMORY_TURNS = _env_int("OPENROUTER_MEMORY_TURNS", 4, floor=0) +VOICE_MAX_SENTENCES = _env_int("VOICE_MAX_SENTENCES", 2, floor=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 current module-level parsing for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES uses int(os.environ.get(... ) or "...") which raises ValueError on non-numeric env values and crashes import; add a small safe parser helper (e.g., parse_int_env or safe_int) that takes the env name, a default int, and optional min value, tries to convert the env value with try/except, and returns the default on ValueError or None/empty; then replace the direct int(...) calls for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES with calls to that helper, enforcing the min bounds (0 for OPENROUTER_MEMORY_TURNS and 1 for VOICE_MAX_SENTENCES).bindu/server/endpoints/voice_endpoints.py (1)
914-945: Timeout vs task cancellation: confirm intent aroundrunner_task.
async with asyncio.timeout(...)wraps onlyawait runner_task. When the timeout fires, Python cancels the enclosing coroutine (this handler) — notrunner_taskitself — then re-raises asTimeoutErrorat L918. Thefinallyblock does explicitly cancelrunner_task(L938-945), so the net behavior is correct, but this pattern is a frequent source of leaks. A short comment here documenting the intent (timeout → finally cancels runner) will help future maintainers, and mirrors the same pattern discussed inagent_bridge.py.🤖 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 914 - 945, The code relies on asyncio.timeout around awaiting runner_task and then cancelling runner_task in the finally block; add an explicit comment near the asyncio.timeout(...) and the finally block (around runner_task.cancel()/await runner_task) stating the intended control flow: the timeout cancels this handler, not runner_task, and the finally block is responsible for cancelling and awaiting runner_task to avoid leaks (mirror the explanation in agent_bridge.py); reference runner_task, asyncio.timeout, the TimeoutError handler, and the finally cancellation/await logic so future maintainers understand the pattern.bindu/settings.py (1)
1126-1133: Log a warning when clampingsession_token_ttlso misconfigs are visible.The validator silently rewrites
session_token_ttltosession_timeoutwhen misconfigured. Without a log line, operators have no signal that their configured TTL was overridden, which can confuse debugging of WebSocket auth expiry. Prefer a warning so the clamp remains fail-safe but discoverable.🔧 Proposed fix
`@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 TTL to 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 validator _validate_session_token_ttl in VoiceSettings currently clamps session_token_ttl to session_timeout silently; modify it to log a warning when this clamp occurs. Inside the method (decorated with `@model_validator`(mode="after")), detect when self.session_token_ttl > self.session_timeout, call the project logger (or module logger) to emit a warning that includes both the configured session_token_ttl and the applied session_timeout, then set self.session_token_ttl = self.session_timeout and return self. Ensure the log message is descriptive enough to identify VoiceSettings and the fields involved.bindu/extensions/voice/service_factory.py (1)
42-60: Ruff B009 ongetattrwith constant is a false positive here.For
DeepgramSTTService,PiperTTSService,ElevenLabsTTSService,AzureTTSService(no default), direct attribute access would achieve the same outcome since they are wrapped intry/except (ImportError, AttributeError). For the*Settingslookups (withNonedefault) thegetattrform is deliberately correct. Safe to either silence B009 or swap tomodule.ClassNamefor the no-default cases — no functional impact either way.🤖 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 - 60, Ruff B009 flags the use of getattr with a constant name for DeepgramSTTService as a false positive; change the dynamic getattr call to direct attribute access on the imported module (e.g., use deepgram_module.DeepgramSTTService instead of getattr(deepgram_module, "DeepgramSTTService")) in the get_stt creation branch (the block referencing DeepgramSTTService and importlib.import_module("pipecat.services.deepgram.stt")), or alternatively add a linter ignore for B009 at that line — ensure the try/except (ImportError, AttributeError) remains to preserve the original error handling.
🤖 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/agent_bridge.py`:
- Line 38: Replace the Unicode curly apostrophe in DEFAULT_TIMEOUT_FALLBACK_TEXT
with a plain ASCII single quote (') so the string uses only ASCII characters;
update the constant DEFAULT_TIMEOUT_FALLBACK_TEXT in agent_bridge.py to use
"Sorry - I'm having trouble responding right now." (or keep the original wording
but with the ASCII apostrophe) to satisfy the RUF001 lint rule and avoid
encoding/test issues.
In `@bindu/extensions/voice/service_factory.py`:
- Around line 177-220: The code allows azure_voice to fall back to
config.tts_voice_id (an ElevenLabs id) which is invalid for Azure; when provider
== "azure" detect if app_settings.voice.azure_tts_voice is empty or equals
config.tts_voice_id and in that case avoid passing that invalid id into
AzureTTSService — either raise a ValueError requiring a valid azure voice or
prefer Azure's default by leaving the voice unset so AzureTTSSettings/
AzureTTSService uses its built-in default; update the construction logic around
azure_voice and the AzureTTSSettings/AzureTTSService instantiation to implement
this fallback/validation (referencing azure_voice, config.tts_voice_id,
AzureTTSSettings, AzureTTSService).
In `@bindu/server/endpoints/voice_endpoints.py`:
- Around line 465-473: The current finally block in the stop-handling branch
(where frame_type == "stop", control.stopped is set and _send_json is awaited)
contains a bare return that will swallow exceptions from websocket.close();
change this so websocket.close() errors are not silenced — either move the
return out of the finally block (call await websocket.close() then return) or
explicitly catch exceptions from await websocket.close(), log or re-raise them,
then return; ensure you reference the same symbols (frame_type, control.stopped,
_send_json, websocket.close) so the fix is applied to the stop path.
In `@examples/document-analyzer/document_analyzer.py`:
- Around line 76-83: The extract_text_from_docx function should not silently
return empty string on failure; change it to return None or raise a clear
exception (preserve the caught exception) and include that error detail in the
log so callers can detect failures (refer to extract_text_from_docx). Update the
calling code that currently appends return values around Line ~160 (where
documents are collected) to skip None/empty results and add an entry to an
errors list with file identity and the extraction error; also update the
multi-file result logic (the block around Lines ~177-193 that currently ignores
errors when at least one document succeeded) to fail the overall operation or
return partial results plus the errors list instead of silently dropping failed
files so all extraction failures are reported back to the user.
In `@examples/pdf_research_agent/pdf_research_agent.py`:
- Around line 83-86: The except ImportError block currently re-raises a
DocumentReadError without chaining the original ImportError, losing the
traceback; change the raise in that except block to chain the caught exception
(use "raise DocumentReadError(... ) from err") so the original ImportError is
preserved—update the except ImportError handler that references resolved_path
and DocumentReadError accordingly.
- Around line 183-185: The guard that checks document_text.startswith("[") or
document_text.startswith("Error") and returns document_text is dead and returns
a raw str that violates the structured-response contract; remove that
conditional and its return from the function that calls _read_content()
(reference symbols: document_text and _read_content()), relying on the existing
DocumentReadError handling (caught earlier) so the function always returns the
{success, data, error} dict shape.
In `@examples/typescript-langchain-agent/quiz-agent.ts`:
- Around line 88-119: The handler is returning raw JSON strings for input errors
which bypasses the SDK's structured HandlerResponse; update the validation
branches (the initial empty check for messages and the "Invalid or empty user
input." branch around variables messages, last, userInput) to return proper
HandlerResponse objects instead of JSON.stringify—specifically return an object
with state: "input-required" and a prompt message (and keep error metadata if
needed) so the SDK can validate/display prompts; leave the successful path
(langchainMessages, llm.invoke, and the normal response handling) unchanged, but
ensure the catch block also returns a HandlerResponse error object rather than a
raw JSON string.
In `@examples/voice-agent/main.py`:
- Around line 99-110: The system prompt string hardcodes "at most 2 short
sentences" which can mismatch the configurable VOICE_MAX_SENTENCES and the
runtime clamping in _clamp_sentences; update the system message construction
where the list with the system role is returned to interpolate the
VOICE_MAX_SENTENCES constant (or its runtime value) into the sentence limit text
so the prompt and _clamp_sentences remain consistent (reference
VOICE_MAX_SENTENCES and _clamp_sentences and the block that returns the system
role with "You are a concise, helpful voice assistant..." to locate the change).
---
Duplicate comments:
In `@examples/typescript-langchain-agent/quiz-agent.ts`:
- Line 65: The call to bindufy is currently fire-and-forget and can cause
unhandled promise rejections; modify the top-level invocation of bindufy() to
await its returned promise and handle errors by wrapping the await in a
try/catch (log the error via the existing logger or console.error and exit/stop
startup with a non-zero code). Locate the bindufy() call in quiz-agent.ts,
change it to an awaited call inside an async main/bootstrap function or
top-level await, and ensure the catch block reports the error and terminates
startup cleanly (e.g., process.exit(1) or equivalent).
---
Nitpick comments:
In `@bindu/extensions/voice/service_factory.py`:
- Around line 42-60: Ruff B009 flags the use of getattr with a constant name for
DeepgramSTTService as a false positive; change the dynamic getattr call to
direct attribute access on the imported module (e.g., use
deepgram_module.DeepgramSTTService instead of getattr(deepgram_module,
"DeepgramSTTService")) in the get_stt creation branch (the block referencing
DeepgramSTTService and
importlib.import_module("pipecat.services.deepgram.stt")), or alternatively add
a linter ignore for B009 at that line — ensure the try/except (ImportError,
AttributeError) remains to preserve the original error handling.
In `@bindu/server/endpoints/voice_endpoints.py`:
- Around line 914-945: The code relies on asyncio.timeout around awaiting
runner_task and then cancelling runner_task in the finally block; add an
explicit comment near the asyncio.timeout(...) and the finally block (around
runner_task.cancel()/await runner_task) stating the intended control flow: the
timeout cancels this handler, not runner_task, and the finally block is
responsible for cancelling and awaiting runner_task to avoid leaks (mirror the
explanation in agent_bridge.py); reference runner_task, asyncio.timeout, the
TimeoutError handler, and the finally cancellation/await logic so future
maintainers understand the pattern.
In `@bindu/settings.py`:
- Around line 1126-1133: The validator _validate_session_token_ttl in
VoiceSettings currently clamps session_token_ttl to session_timeout silently;
modify it to log a warning when this clamp occurs. Inside the method (decorated
with `@model_validator`(mode="after")), detect when self.session_token_ttl >
self.session_timeout, call the project logger (or module logger) to emit a
warning that includes both the configured session_token_ttl and the applied
session_timeout, then set self.session_token_ttl = self.session_timeout and
return self. Ensure the log message is descriptive enough to identify
VoiceSettings and the fields involved.
In `@examples/pdf_research_agent/pdf_research_agent.py`:
- Line 53: The PDF extension check using source_text.endswith(".pdf") is
case-sensitive; change it to a case-insensitive check by normalizing the
filename (e.g., use source_text.lower().endswith(".pdf")) where the current
conditional sits (the if using source_text.endswith(".pdf")), so paths like
"paper.PDF" are correctly detected as PDFs and handled by the PDF branch rather
than falling through to the raw-text logic later in the file.
In `@examples/voice-agent/main.py`:
- Around line 37-44: The current module-level parsing for
OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES uses int(os.environ.get(... ) or
"...") which raises ValueError on non-numeric env values and crashes import; add
a small safe parser helper (e.g., parse_int_env or safe_int) that takes the env
name, a default int, and optional min value, tries to convert the env value with
try/except, and returns the default on ValueError or None/empty; then replace
the direct int(...) calls for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES
with calls to that helper, enforcing the min bounds (0 for
OPENROUTER_MEMORY_TURNS and 1 for VOICE_MAX_SENTENCES).
🪄 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: 79273c43-a116-4d06-8e6f-eeeaf80c2edc
📒 Files selected for processing (26)
bindu/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.pyexamples/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 (10)
- examples/README.md
- examples/multilingual-collab-agent/skills/research/skill.yaml
- examples/pdf_research_agent/README.md
- examples/multilingual-collab-agent/skills/collaborate/skill.yaml
- examples/cerina_bindu/cbt/README.md
- examples/voice-agent/README.md
- examples/multilingual-collab-agent/skills/translate/skill.yaml
- bindu/extensions/voice/pipeline_builder.py
- examples/voice-agent/skills/voice-brief-response-skill/skill.yaml
- tests/unit/extensions/voice/test_agent_bridge.py
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/typescript-langchain-quiz-agent/quiz-agent.ts
- examples/multilingual-collab-agent/main.py
- bindu/utils/worker/messages.py
- examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
- bindu/server/applications.py
| DEFAULT_TOTAL_RESPONSE_TIMEOUT_SECONDS = 30.0 | ||
| DEFAULT_CANCELLATION_GRACE_SECONDS = 0.5 | ||
| DEFAULT_THINKING_TEXT = "One moment." | ||
| DEFAULT_TIMEOUT_FALLBACK_TEXT = "Sorry — I’m having trouble responding right now." |
There was a problem hiding this comment.
Replace curly apostrophe with a plain ASCII one.
DEFAULT_TIMEOUT_FALLBACK_TEXT uses U+2019 (’) which Ruff flags via RUF001. Since this string is synthesized to TTS and also surfaced in logs/tests, an ASCII apostrophe avoids encoding surprises and keeps string equality predictable.
🔧 Proposed fix
-DEFAULT_TIMEOUT_FALLBACK_TEXT = "Sorry — I’m having trouble responding right now."
+DEFAULT_TIMEOUT_FALLBACK_TEXT = "Sorry — I'm having trouble responding right now."📝 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.
| DEFAULT_TIMEOUT_FALLBACK_TEXT = "Sorry — I’m having trouble responding right now." | |
| DEFAULT_TIMEOUT_FALLBACK_TEXT = "Sorry — I'm having trouble responding right now." |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 38-38: String contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF001)
🤖 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 38, Replace the Unicode curly
apostrophe in DEFAULT_TIMEOUT_FALLBACK_TEXT with a plain ASCII single quote (')
so the string uses only ASCII characters; update the constant
DEFAULT_TIMEOUT_FALLBACK_TEXT in agent_bridge.py to use "Sorry - I'm having
trouble responding right now." (or keep the original wording but with the ASCII
apostrophe) to satisfy the RUF001 lint rule and avoid encoding/test issues.
| if provider == "azure": | ||
| azure_api_key = app_settings.voice.azure_tts_api_key | ||
| azure_region = app_settings.voice.azure_tts_region | ||
| azure_voice = app_settings.voice.azure_tts_voice or config.tts_voice_id | ||
| if not azure_api_key: | ||
| logger.warning( | ||
| "Azure TTS configuration incomplete: missing API key", | ||
| setting="VOICE__AZURE_TTS_API_KEY", | ||
| ) | ||
| raise ValueError("Azure TTS configuration incomplete") | ||
| if not azure_region: | ||
| logger.warning( | ||
| "Azure TTS configuration incomplete: missing region", | ||
| setting="VOICE__AZURE_TTS_REGION", | ||
| ) | ||
| raise ValueError("Azure TTS configuration incomplete") | ||
|
|
||
| try: | ||
| azure_module = importlib.import_module("pipecat.services.azure.tts") | ||
| AzureTTSService = getattr(azure_module, "AzureTTSService") | ||
| AzureTTSSettings = getattr(azure_module, "AzureTTSSettings", None) | ||
| except (ImportError, AttributeError) as e: | ||
| raise ImportError( | ||
| "Azure TTS requires pipecat[azure]. " | ||
| "Install with: pip install 'bindu[voice]'" | ||
| ) from e | ||
|
|
||
| if AzureTTSSettings is not None: | ||
| tts = AzureTTSService( | ||
| api_key=azure_api_key, | ||
| region=azure_region, | ||
| settings=AzureTTSSettings(voice=azure_voice), | ||
| sample_rate=config.sample_rate, | ||
| ) | ||
| else: | ||
| tts = AzureTTSService( | ||
| api_key=azure_api_key, | ||
| region=azure_region, | ||
| voice=azure_voice, | ||
| sample_rate=config.sample_rate, | ||
| ) | ||
|
|
||
| logger.info(f"Created Azure TTS: voice={azure_voice}, region={azure_region}") | ||
| return tts |
There was a problem hiding this comment.
Azure voice fallback can produce a provider-rejected voice name.
If VOICE__AZURE_TTS_VOICE is explicitly set to an empty string, this falls back to config.tts_voice_id, which is typically an ElevenLabs voice identifier (e.g. "21m00Tcm4TlvDq8ikWAM") — not a valid Azure neural voice name like "en-US-SaraNeural". When provider == "azure" (primary or fallback), this is a silent misconfiguration that will surface only at synthesis time. Either require a non-empty azure_tts_voice during preflight, or keep the VoiceSettings default ("en-US-SaraNeural") as the last-resort fallback rather than the ElevenLabs voice id.
🔧 Proposed fix
- azure_voice = app_settings.voice.azure_tts_voice or config.tts_voice_id
+ azure_voice = app_settings.voice.azure_tts_voice
+ if not azure_voice:
+ logger.warning(
+ "Azure TTS configuration incomplete: missing voice",
+ setting="VOICE__AZURE_TTS_VOICE",
+ )
+ raise ValueError("Azure TTS configuration incomplete")🧰 Tools
🪛 Ruff (0.15.10)
[warning] 196-196: 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 177 - 220, The code
allows azure_voice to fall back to config.tts_voice_id (an ElevenLabs id) which
is invalid for Azure; when provider == "azure" detect if
app_settings.voice.azure_tts_voice is empty or equals config.tts_voice_id and in
that case avoid passing that invalid id into AzureTTSService — either raise a
ValueError requiring a valid azure voice or prefer Azure's default by leaving
the voice unset so AzureTTSSettings/ AzureTTSService uses its built-in default;
update the construction logic around azure_voice and the
AzureTTSSettings/AzureTTSService instantiation to implement this
fallback/validation (referencing azure_voice, config.tts_voice_id,
AzureTTSSettings, AzureTTSService).
| if frame_type == "stop": | ||
| control.stopped = True | ||
| await _send_json( | ||
| websocket, {"type": "state", "state": "ended"}, send_lock | ||
| ) | ||
| try: | ||
| await websocket.close() | ||
| finally: | ||
| return |
There was a problem hiding this comment.
return inside finally silences websocket.close() errors (Ruff B012).
If websocket.close() raises, the bare return in finally swallows it. Either move the return out of the finally block or catch-and-log explicitly, so close failures during the stop path are observable.
🔧 Proposed fix
if frame_type == "stop":
control.stopped = True
await _send_json(
websocket, {"type": "state", "state": "ended"}, send_lock
)
- try:
- await websocket.close()
- finally:
- return
+ try:
+ await websocket.close()
+ except Exception:
+ logger.debug("Failed to close websocket on stop", exc_info=True)
+ return🧰 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 - 473, The
current finally block in the stop-handling branch (where frame_type == "stop",
control.stopped is set and _send_json is awaited) contains a bare return that
will swallow exceptions from websocket.close(); change this so websocket.close()
errors are not silenced — either move the return out of the finally block (call
await websocket.close() then return) or explicitly catch exceptions from await
websocket.close(), log or re-raise them, then return; ensure you reference the
same symbols (frame_type, control.stopped, _send_json, websocket.close) so the
fix is applied to the stop path.
| def extract_text_from_docx(file_bytes): | ||
| """Extract text from docx bytes""" | ||
| doc = Document(io.BytesIO(file_bytes)) | ||
| return "\n".join([p.text for p in doc.paragraphs]) | ||
| 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 "" |
There was a problem hiding this comment.
Avoid silently analyzing incomplete or empty document content.
Line 82 returns "" for DOCX extraction failures, and Line 160 appends that empty text as a valid document. Also, Lines 177-193 ignore errors whenever at least one document succeeded, so multi-file analysis can silently omit failed uploads.
🐛 Proposed fix to preserve extraction failures and reject empty document text
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.warning(f"Invalid DOCX file: {e}")
+ raise ValueError(f"Invalid DOCX file: {e}") from e
+
+ return "\n".join(p.text for p in doc.paragraphs)
@@
- doc_text = extract_document_text(file_bytes, mime_type)
+ doc_text = extract_document_text(file_bytes, mime_type).strip()
+ if not doc_text:
+ raise ValueError("No extractable text found in document")
extracted_docs.append(doc_text)
except Exception as e:
errors.append(str(e))
@@
if not extracted_docs:
if errors:
return "Failed to process documents:\n" + "\n".join(errors)
return "No valid document found in the messages."
+
+ if errors:
+ return "Failed to process some documents:\n" + "\n".join(errors)
combined_document = "\n\n".join(extracted_docs)Also applies to: 159-193
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 81-81: 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/document-analyzer/document_analyzer.py` around lines 76 - 83, The
extract_text_from_docx function should not silently return empty string on
failure; change it to return None or raise a clear exception (preserve the
caught exception) and include that error detail in the log so callers can detect
failures (refer to extract_text_from_docx). Update the calling code that
currently appends return values around Line ~160 (where documents are collected)
to skip None/empty results and add an entry to an errors list with file identity
and the extraction error; also update the multi-file result logic (the block
around Lines ~177-193 that currently ignores errors when at least one document
succeeded) to fail the overall operation or return partial results plus the
errors list instead of silently dropping failed files so all extraction failures
are reported back to the user.
| 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" | ||
| ) |
There was a problem hiding this comment.
Chain the re-raised exception (Ruff B904).
Preserve the original traceback by using raise ... from err (or from None if intentionally hidden).
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🧰 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)
🤖 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
except ImportError block currently re-raises a DocumentReadError without
chaining the original ImportError, losing the traceback; change the raise in
that except block to chain the caught exception (use "raise
DocumentReadError(... ) from err") so the original ImportError is
preserved—update the except ImportError handler that references resolved_path
and DocumentReadError accordingly.
| # Check if document processing failed | ||
| if document_text.startswith("[") or document_text.startswith("Error"): | ||
| return document_text |
There was a problem hiding this comment.
Dead code returns a raw string and violates the structured-response contract.
Since _read_content() now raises DocumentReadError on failure (caught at Lines 180-181) instead of returning "[..." / "Error..." strings, this branch is unreachable. Worse, if it ever were reached it would return a bare str instead of the new {success, data, error} dict, breaking any consumer relying on the documented shape. Remove it.
Proposed fix
- try:
- document_text = _read_content(user_input)
- except DocumentReadError as exc:
- return {"success": False, "data": None, "error": str(exc)}
-
- # Check if document processing failed
- if document_text.startswith("[") or document_text.startswith("Error"):
- return document_text
+ try:
+ document_text = _read_content(user_input)
+ except DocumentReadError as exc:
+ return {"success": False, "data": None, "error": str(exc)}📝 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 | |
| try: | |
| document_text = _read_content(user_input) | |
| except DocumentReadError as exc: | |
| return {"success": False, "data": None, "error": str(exc)} |
🤖 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 guard that checks document_text.startswith("[") or
document_text.startswith("Error") and returns document_text is dead and returns
a raw str that violates the structured-response contract; remove that
conditional and its return from the function that calls _read_content()
(reference symbols: document_text and _read_content()), relying on the existing
DocumentReadError handling (caught earlier) so the function always returns the
{success, data, error} dict shape.
| if (!messages || messages.length === 0) { | ||
| return JSON.stringify({ error: true, message: "No input provided." }); | ||
| } | ||
|
|
||
| // Extract latest user input with validation | ||
| const last = messages[messages.length - 1]; | ||
| let userInput = ""; | ||
| if (last && typeof last.content === "string" && last.content.trim().length > 0) { | ||
| userInput = last.content; | ||
| } else { | ||
| // Optionally: return error or fallback | ||
| return JSON.stringify({ error: true, message: "Invalid or empty user input." }); | ||
| } | ||
|
|
||
| // Construct LangChain messages | ||
| const langchainMessages = [ | ||
| { role: "system", content: SYSTEM_PROMPT }, | ||
| { role: "user", content: userInput }, | ||
| ]; | ||
|
|
||
| // Call LLM | ||
| const response = await llm.invoke(langchainMessages); | ||
|
|
||
| // Return response | ||
| return typeof response.content === "string" | ||
| ? response.content | ||
| : JSON.stringify(response.content); | ||
| } catch (error: any) { | ||
| return JSON.stringify({ | ||
| error: true, | ||
| message: error?.message ?? String(error), | ||
| }); |
There was a problem hiding this comment.
Return HandlerResponse objects for validation errors.
JSON.stringify({ error: true, ... }) is rendered as plain text and bypasses the SDK’s structured response fields. Use state: "input-required"/prompt for recoverable input errors, per sdks/typescript/src/types.ts.
Proposed fix
if (!messages || messages.length === 0) {
- return JSON.stringify({ error: true, message: "No input provided." });
+ return {
+ state: "input-required",
+ prompt: "Please provide source text to generate a quiz.",
+ content: "No input provided.",
+ };
}
@@
if (last && typeof last.content === "string" && last.content.trim().length > 0) {
userInput = last.content;
} else {
- // Optionally: return error or fallback
- return JSON.stringify({ error: true, message: "Invalid or empty user input." });
+ return {
+ state: "input-required",
+ prompt: "Please provide non-empty source text to generate a quiz.",
+ content: "Invalid or empty user input.",
+ };
}
@@
- } catch (error: any) {
- return JSON.stringify({
- error: true,
- message: error?.message ?? String(error),
- });
+ } catch (error: unknown) {
+ return {
+ content:
+ error instanceof Error
+ ? `Failed to generate quiz: ${error.message}`
+ : "Failed to generate quiz.",
+ };
}📝 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 (!messages || messages.length === 0) { | |
| return JSON.stringify({ error: true, message: "No input provided." }); | |
| } | |
| // Extract latest user input with validation | |
| const last = messages[messages.length - 1]; | |
| let userInput = ""; | |
| if (last && typeof last.content === "string" && last.content.trim().length > 0) { | |
| userInput = last.content; | |
| } else { | |
| // Optionally: return error or fallback | |
| return JSON.stringify({ error: true, message: "Invalid or empty user input." }); | |
| } | |
| // Construct LangChain messages | |
| const langchainMessages = [ | |
| { role: "system", content: SYSTEM_PROMPT }, | |
| { role: "user", content: userInput }, | |
| ]; | |
| // Call LLM | |
| const response = await llm.invoke(langchainMessages); | |
| // Return response | |
| return typeof response.content === "string" | |
| ? response.content | |
| : JSON.stringify(response.content); | |
| } catch (error: any) { | |
| return JSON.stringify({ | |
| error: true, | |
| message: error?.message ?? String(error), | |
| }); | |
| if (!messages || messages.length === 0) { | |
| return { | |
| state: "input-required", | |
| prompt: "Please provide source text to generate a quiz.", | |
| content: "No input provided.", | |
| }; | |
| } | |
| // Extract latest user input with validation | |
| const last = messages[messages.length - 1]; | |
| let userInput = ""; | |
| if (last && typeof last.content === "string" && last.content.trim().length > 0) { | |
| userInput = last.content; | |
| } else { | |
| return { | |
| state: "input-required", | |
| prompt: "Please provide non-empty source text to generate a quiz.", | |
| content: "Invalid or empty user input.", | |
| }; | |
| } | |
| // Construct LangChain messages | |
| const langchainMessages = [ | |
| { role: "system", content: SYSTEM_PROMPT }, | |
| { role: "user", content: userInput }, | |
| ]; | |
| // Call LLM | |
| const response = await llm.invoke(langchainMessages); | |
| // Return response | |
| return typeof response.content === "string" | |
| ? response.content | |
| : JSON.stringify(response.content); | |
| } catch (error: unknown) { | |
| return { | |
| content: | |
| error instanceof Error | |
| ? `Failed to generate quiz: ${error.message}` | |
| : "Failed to generate quiz.", | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/typescript-langchain-agent/quiz-agent.ts` around lines 88 - 119, The
handler is returning raw JSON strings for input errors which bypasses the SDK's
structured HandlerResponse; update the validation branches (the initial empty
check for messages and the "Invalid or empty user input." branch around
variables messages, last, userInput) to return proper HandlerResponse objects
instead of JSON.stringify—specifically return an object with state:
"input-required" and a prompt message (and keep error metadata if needed) so the
SDK can validate/display prompts; leave the successful path (langchainMessages,
llm.invoke, and the normal response handling) unchanged, but ensure the catch
block also returns a HandlerResponse error object rather than a raw JSON string.
| return [ | ||
| { | ||
| "role": "system", | ||
| "content": ( | ||
| "You are a concise, helpful voice assistant. " | ||
| "Answer in at most 2 short sentences unless the user asks for detail. " | ||
| "Be precise, avoid repetition, and avoid markdown/bullets. " | ||
| "Do not repeat prior phrasing in the same response." | ||
| ), | ||
| }, | ||
| *history, | ||
| ] |
There was a problem hiding this comment.
System prompt hardcodes "2 sentences" while VOICE_MAX_SENTENCES is configurable.
If an operator tunes VOICE_MAX_SENTENCES (e.g. to 4), the model is still instructed to answer in "at most 2 short sentences" — so the prompt and _clamp_sentences enforcement disagree. Interpolate the configured value so they stay in sync.
🔧 Proposed fix
"role": "system",
"content": (
"You are a concise, helpful voice assistant. "
- "Answer in at most 2 short sentences unless the user asks for detail. "
+ f"Answer in at most {VOICE_MAX_SENTENCES} short sentence(s) unless the user asks for detail. "
"Be precise, avoid repetition, and avoid markdown/bullets. "
"Do not repeat prior phrasing in the same response."
),🤖 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 99 - 110, The system prompt string
hardcodes "at most 2 short sentences" which can mismatch the configurable
VOICE_MAX_SENTENCES and the runtime clamping in _clamp_sentences; update the
system message construction where the list with the system role is returned to
interpolate the VOICE_MAX_SENTENCES constant (or its runtime value) into the
sentence limit text so the prompt and _clamp_sentences remain consistent
(reference VOICE_MAX_SENTENCES and _clamp_sentences and the block that returns
the system role with "You are a concise, helpful voice assistant..." to locate
the change).
# 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)/example-agent
branch
from
6477cf4 to
db3a274
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
examples/voice-agent/main.py (1)
99-105:⚠️ Potential issue | 🟡 MinorPrompt sentence limit is still hardcoded and can drift from runtime config.
At Line 104, the prompt still says “at most 2 short sentences” while enforcement uses
VOICE_MAX_SENTENCES(Line 188). Please interpolate the configured value so instruction and runtime behavior stay aligned.Suggested fix
{ "role": "system", "content": ( "You are a concise, helpful voice assistant. " - "Answer in at most 2 short sentences unless the user asks for detail. " + f"Answer in at most {VOICE_MAX_SENTENCES} short sentence(s) unless the user asks for detail. " "Be precise, avoid repetition, and avoid markdown/bullets. " "Do not repeat prior phrasing in the same response." ), },🤖 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 99 - 105, The system prompt string hardcodes "at most 2 short sentences" which can drift from runtime config; change the prompt construction in main.py to interpolate the VOICE_MAX_SENTENCES constant (used elsewhere) instead of the literal "2" so the instruction text matches enforcement; locate the prompt object that includes "You are a concise, helpful voice assistant..." and replace the hardcoded number with a formatted value (e.g., using f-string or .format) referencing VOICE_MAX_SENTENCES to keep behavior and wording in sync.pyproject.toml (1)
95-95:⚠️ Potential issue | 🟠 MajorAdd the Azure Pipecat extra wherever voice dependencies are declared.
The voice dependency set now includes Piper but still omits
azure. If Azure TTS/provider selection is supported,bindu[voice]and the dev environment will miss Pipecat’s Azure dependency chain. PyPI listsazureas a supportedpipecat-aiextra.🔧 Add Azure to both declarations
voice = [ - "pipecat-ai[deepgram,elevenlabs,piper,silero]~=0.0.105", + "pipecat-ai[azure,deepgram,elevenlabs,piper,silero]~=0.0.105", "websockets>=14.0", ] dev = [ "bindu", - "pipecat-ai[deepgram,elevenlabs,piper,silero]~=0.0.105", + "pipecat-ai[azure,deepgram,elevenlabs,piper,silero]~=0.0.105",🔎 Verify Azure support and Pipecat extra availability
#!/bin/bash # Description: Confirm Azure voice references in this repo and verify pipecat-ai 0.0.105 exposes the azure extra. # Expected: If Azure references are present and PyPI reports azure=true, include azure in the dependency declarations. rg -n -C3 -i '(AzureTTSService|tts_provider.*azure|azure.*tts|AZURE_)' \ --glob '!**/.venv/**' python - <<'PY' import json import urllib.request with urllib.request.urlopen("https://pypi.org/pypi/pipecat-ai/0.0.105/json", timeout=10) as response: data = json.load(response) extras = set(data["info"].get("provides_extra") or []) print("azure extra available:", "azure" in extras) print("matching extras:", sorted(extra for extra in extras if extra in {"azure", "deepgram", "elevenlabs", "piper", "silero"})) PYAlso applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 95, The pipecat-ai dependency string "pipecat-ai[deepgram,elevenlabs,piper,silero]~=0.0.105" is missing the azure extra; update both occurrences of that dependency to include azure (e.g., add ",azure" inside the brackets) so the binder/dev env and bindu[voice] consumers get Pipecat’s Azure dependency chain, and verify both declarations that currently match that exact string are changed.examples/pdf_research_agent/pdf_research_agent.py (3)
83-86:⚠️ Potential issue | 🟡 MinorChain the re-raised import failure.
Ruff B904 is valid here: preserve the missing-
pypdftraceback by raising from the caughtImportError.Proposed fix
- except ImportError: + except ImportError as exc: raise DocumentReadError( f"pypdf not installed and cannot read '{resolved_path}'. Run: uv add pypdf" - ) + ) from exc🤖 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 except ImportError block that raises DocumentReadError for inability to read resolved_path should preserve the original ImportError traceback by chaining the exception; modify the except to capture the ImportError (e.g., except ImportError as e) and re-raise DocumentReadError(...) from that caught exception so the original missing-pypdf trace is retained.
183-185:⚠️ Potential issue | 🟠 MajorRemove the raw-string return path.
_read_content()now raisesDocumentReadErrorfor read failures, already handled above. If this branch ever runs, it returns a barestrinstead of the documented{success, data, error}shape.Proposed 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, Remove the raw-string return path that returns document_text; instead either raise DocumentReadError(document_text) or return the standardized result shape {"success": False, "data": None, "error": document_text} so callers always receive the documented structure; update the branch in _read_content (the if checking document_text.startswith("[") or document_text.startswith("Error")) to perform one of those two actions (prefer raising DocumentReadError to match existing failure handling) and remove the bare string return.
18-20:⚠️ Potential issue | 🟠 MajorKeep the allowed PDF root configurable.
The docs still show
"/path/to/paper.pdf", but Line 38 restricts PDF reads toexamples/pdf_research_agent/, so the documented usage is rejected for most users. Keep the containment check, but allow the root to be configured via env or constructor/default config.Proposed fix
-ALLOWED_BASE_DIR = Path(__file__).parent.resolve() +ALLOWED_BASE_DIR = Path( + os.getenv("PDF_RESEARCH_ALLOWED_BASE_DIR", Path(__file__).parent) +).expanduser().resolve()Usage ----- export OPENROUTER_API_KEY="your_api_key_here" # pragma: allowlist secret + export PDF_RESEARCH_ALLOWED_BASE_DIR="/path/to/documents" python pdf_research_agent.pyAlso applies to: 38-68
🤖 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 18 - 20, The README/example hardcodes PDF containment to "examples/pdf_research_agent/" which rejects user paths; make the allowed PDF root configurable by replacing the hardcoded path with a configurable value (e.g., read from an environment variable PDF_ROOT or accept a constructor/default config parameter) used by the containment check (the code that currently references "examples/pdf_research_agent/"); keep the containment check logic but default to the existing examples/pdf_research_agent/ when no config is provided, and update the example usage message to reference the configured root (or a placeholder like "<PDF_ROOT>/paper.pdf") so user-supplied paths are validated against the configurable root.
🧹 Nitpick comments (1)
examples/voice-agent/.env.example (1)
16-16: Clarify whetherVOICE__ENABLEDis truly optional.This variable appears in the "Optional tuning" section, but it's unclear whether setting it to
falsewould disable the voice feature entirely or if it's required to betruefor the example to work. Consider moving it to the required section if the voice agent example requires it, or add a comment explaining the default behavior.🤖 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 16, The VOICE__ENABLED env var is ambiguous; clarify whether the voice feature requires it set true or if false simply disables optional tuning. Update the .env example so VOICE__ENABLED is either moved to the required section when the voice agent depends on it, or keep it in "Optional tuning" but add a short comment next to VOICE__ENABLED explaining the default behavior (e.g., "true enables voice agent; set to false to disable voice features") and how the application reacts when it's unset. Ensure the change references the VOICE__ENABLED variable so reviewers can verify behavior and placement.
🤖 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/pdf_research_agent/pdf_research_agent.py`:
- Around line 197-198: The Agno response returned by
agent.run(enriched_messages) is not JSON-serializable; convert or normalize it
before returning. Replace returning the raw result variable with a
JSON-serializable object (for example call result.to_dict() and extract the
relevant field like ["content"] or return the full dict), i.e., transform the
value from agent.run(enriched_messages) into a plain dict/primitive and then
return {"success": True, "data": <converted_result>, "error": None} so the
response can be serialized.
In `@examples/voice-agent/.env.example`:
- Around line 31-35: Rename the environment variable VOICE_MAX_SENTENCES to use
the double-underscore voice prefix for consistency with other voice settings:
change VOICE_MAX_SENTENCES to VOICE__MAX_SENTENCES in the .env example so it
matches the VOICE__* naming convention used elsewhere.
- Line 19: Remove the extraneous VOICE__TTS_MODEL=piper-local entry from the
example or move it into a provider-specific block with a comment clarifying it
only applies when VOICE__TTS_PROVIDER is elevenlabs or azure; specifically
update the examples/voice-agent .env example so VOICE__TTS_MODEL is not present
for the piper-focused configuration (when VOICE__TTS_PROVIDER=piper) or is
relocated and annotated to indicate it is only used by non-piper providers.
In `@examples/voice-agent/main.py`:
- Around line 197-199: The current broad "except Exception as exc" in the
streaming path should be replaced with explicit handlers for expected errors
(e.g., network/client errors like requests.exceptions.RequestException or
library-specific errors such as OpenRouter/OpenAI client exceptions and
websocket/asyncio.CancelledError) and use structured logging; update the block
around the "except Exception as exc" line to add separate except clauses for
those specific exception types, log error details via the existing logger
(including stack/metadata), handle/cleanup or re-raise CancelledError and other
fatal exceptions, and keep a final very narrow fallback except Exception that
logs the error with context before re-raising to avoid silently swallowing
unexpected failures.
- Around line 37-44: The module currently calls int(...) directly for
OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES which will raise ValueError on
malformed env values; change to guarded parsing: read the raw env string via
os.environ.get(...), try to int() it inside a try/except (or use a small helper
like parse_int_with_default), and on exception or None/empty input fall back to
the intended defaults (4 for OPENROUTER_MEMORY_TURNS, 2 for
VOICE_MAX_SENTENCES), then apply the existing max(...) wrapper so the final
values remain non-negative and >=1 as originally intended; update the code paths
that reference OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES accordingly.
In `@pyproject.toml`:
- Around line 63-64: Remove "agno>=2.5.2" and "openai>=2.17.0" from the
[project].dependencies list and add them under an optional extras group named
"agents" (i.e. create or update [project.optional-dependencies] with agents =
["agno>=2.5.2", "openai>=2.17.0"]); this keeps the packages available via pip
install .[agents] but out of the core install. Ensure you reference the exact
package strings "agno" and "openai" when moving them so versions are preserved.
---
Duplicate comments:
In `@examples/pdf_research_agent/pdf_research_agent.py`:
- Around line 83-86: The except ImportError block that raises DocumentReadError
for inability to read resolved_path should preserve the original ImportError
traceback by chaining the exception; modify the except to capture the
ImportError (e.g., except ImportError as e) and re-raise DocumentReadError(...)
from that caught exception so the original missing-pypdf trace is retained.
- Around line 183-185: Remove the raw-string return path that returns
document_text; instead either raise DocumentReadError(document_text) or return
the standardized result shape {"success": False, "data": None, "error":
document_text} so callers always receive the documented structure; update the
branch in _read_content (the if checking document_text.startswith("[") or
document_text.startswith("Error")) to perform one of those two actions (prefer
raising DocumentReadError to match existing failure handling) and remove the
bare string return.
- Around line 18-20: The README/example hardcodes PDF containment to
"examples/pdf_research_agent/" which rejects user paths; make the allowed PDF
root configurable by replacing the hardcoded path with a configurable value
(e.g., read from an environment variable PDF_ROOT or accept a
constructor/default config parameter) used by the containment check (the code
that currently references "examples/pdf_research_agent/"); keep the containment
check logic but default to the existing examples/pdf_research_agent/ when no
config is provided, and update the example usage message to reference the
configured root (or a placeholder like "<PDF_ROOT>/paper.pdf") so user-supplied
paths are validated against the configurable root.
In `@examples/voice-agent/main.py`:
- Around line 99-105: The system prompt string hardcodes "at most 2 short
sentences" which can drift from runtime config; change the prompt construction
in main.py to interpolate the VOICE_MAX_SENTENCES constant (used elsewhere)
instead of the literal "2" so the instruction text matches enforcement; locate
the prompt object that includes "You are a concise, helpful voice assistant..."
and replace the hardcoded number with a formatted value (e.g., using f-string or
.format) referencing VOICE_MAX_SENTENCES to keep behavior and wording in sync.
In `@pyproject.toml`:
- Line 95: The pipecat-ai dependency string
"pipecat-ai[deepgram,elevenlabs,piper,silero]~=0.0.105" is missing the azure
extra; update both occurrences of that dependency to include azure (e.g., add
",azure" inside the brackets) so the binder/dev env and bindu[voice] consumers
get Pipecat’s Azure dependency chain, and verify both declarations that
currently match that exact string are changed.
---
Nitpick comments:
In `@examples/voice-agent/.env.example`:
- Line 16: The VOICE__ENABLED env var is ambiguous; clarify whether the voice
feature requires it set true or if false simply disables optional tuning. Update
the .env example so VOICE__ENABLED is either moved to the required section when
the voice agent depends on it, or keep it in "Optional tuning" but add a short
comment next to VOICE__ENABLED explaining the default behavior (e.g., "true
enables voice agent; set to false to disable voice features") and how the
application reacts when it's unset. Ensure the change references the
VOICE__ENABLED variable so reviewers can verify behavior and placement.
🪄 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: e4ca31ec-d5c6-4467-a443-df890148ed0b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
examples/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 (7)
- examples/README.md
- examples/pdf_research_agent/README.md
- examples/multilingual-collab-agent/skills/research/skill.yaml
- examples/cerina_bindu/cbt/README.md
- examples/multilingual-collab-agent/skills/collaborate/skill.yaml
- examples/voice-agent/README.md
- examples/voice-agent/skills/voice-brief-response-skill/skill.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/typescript-langchain-quiz-agent/quiz-agent.ts
- examples/multilingual-collab-agent/main.py
- examples/multilingual-collab-agent/skills/translate/skill.yaml
- examples/pdf_research_agent/skills/pdf-research-skill/skill.yaml
- examples/typescript-langchain-agent/quiz-agent.ts
| 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 local usage patterns and dependency pins for Agno Agent.run return handling.
set -euo pipefail
echo "Agno dependency declarations:"
rg -n -C2 '\bagno\b' pyproject.toml uv.lock requirements*.txt 2>/dev/null || true
echo
echo "Agent.run call sites with surrounding return handling:"
rg -n -C4 '\.run\s*\(\s*input=' --type=pyRepository: GetBindu/Bindu
Length of output: 11428
🏁 Script executed:
# Check what agno.Agent returns and how it's being used
# Look at actual imports and type hints for the response
fd -t py -x grep -l "Agent\|agent.run" {} \; | head -5 | while read f; do
echo "=== $f ==="
head -50 "$f" | grep -E "^import|^from|class|def"
doneRepository: GetBindu/Bindu
Length of output: 227
🏁 Script executed:
# Check if there's any serialization handling or JSON encoding in the codebase
rg -n "to_dict|json\.dumps|JSONEncoder|to_json" --type=py | head -20Repository: GetBindu/Bindu
Length of output: 1977
🏁 Script executed:
# Look for any actual error handling or serialization issues in pdf_research_agent
cat -n examples/pdf_research_agent/pdf_research_agent.py | sed -n '190,210p'Repository: GetBindu/Bindu
Length of output: 1046
🏁 Script executed:
# Check what the actual response structure looks like from agno package
# Look for any response class definitions or type hints
rg -n "class.*Response|AgentResponse|def run.*->" --type=py | grep -i agno | head -10Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look at how bindufy.py actually handles the returned result from handler
cat -n bindu/penguin/bindufy.py | sed -n '680,720p'Repository: GetBindu/Bindu
Length of output: 1591
🏁 Script executed:
# Check how the handler result is processed in bindufy
rg -n -A 10 "def handler" bindu/penguin/bindufy.py | head -40Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for any serialization or response handling in the pdf agent or test files
rg -n "json\." examples/pdf_research_agent/ 2>/dev/null || echo "No json usage found"Repository: GetBindu/Bindu
Length of output: 77
🏁 Script executed:
# Check if there are tests for pdf_research_agent that would show serialization
find . -name "*pdf*" -type f | grep -i testRepository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look at the actual message handler to see how responses are converted for HTTP
rg -n "_to_jsonable|MessageHandlers" bindu/penguin/bindufy.py | head -10Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Find where handler results are processed and converted to HTTP responses
rg -n "handler_callable\|_to_jsonable" bindu/penguin/bindufy.py | head -20Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the server/handlers to see how results are serialized
cat -n bindu/server/handlers/message_handlers.py | head -100Repository: GetBindu/Bindu
Length of output: 4009
🏁 Script executed:
# Look for _to_jsonable implementation
rg -n -A 15 "def _to_jsonable" bindu/server/handlers/message_handlers.pyRepository: GetBindu/Bindu
Length of output: 791
🏁 Script executed:
# Check if agno Response objects have __dict__ or are pydantic models
rg -n "from agno|import agno" examples/ bindu/ | head -10Repository: GetBindu/Bindu
Length of output: 965
🏁 Script executed:
# Look for any Response or result handling patterns in actual example output
rg -n -B 5 -A 5 'return result' examples/pdf_research_agent/ examples/weather_research/ examples/web-scraping-agent/ | head -50Repository: GetBindu/Bindu
Length of output: 1311
🏁 Script executed:
# Check how the handler result is actually used in the HTTP response
rg -n "json\.dumps\|JSONEncoder\|response\|return.*handler" bindu/penguin/bindufy.py | head -30Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look at the actual request/response flow in _bindufy_core
cat -n bindu/penguin/bindufy.py | sed -n '400,500p'Repository: GetBindu/Bindu
Length of output: 4670
🏁 Script executed:
# Check if there's any actual HTTP serialization that would fail with a raw agno object
rg -n "\.json\(\)|json\.dumps.*result" bindu/server/ | head -20Repository: GetBindu/Bindu
Length of output: 416
🏁 Script executed:
# Let's look at how the handler is called and result is processed
rg -n -B 3 -A 10 "handler_callable" bindu/penguin/bindufy.py | head -50Repository: GetBindu/Bindu
Length of output: 2213
🏁 Script executed:
# Look for where handler is invoked and result processing
rg -n "handler_callable\(" bindu/server/ | head -20Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Search for task execution and result handling
rg -n -B 5 -A 10 "await handler_callable\|handler_callable\(" bindu/ --type py | head -80Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check if there's a serialization step or if the result is directly converted to JSON
rg -n "\.json\(\)|json.dumps" bindu/server/endpoints/ | head -20Repository: GetBindu/Bindu
Length of output: 636
🏁 Script executed:
# Look at the RPC request handler to see how handler results are returned
cat -n bindu/server/rpc/rpc_handler.py | head -100Repository: GetBindu/Bindu
Length of output: 121
🏁 Script executed:
# Find RPC related files
find bindu/server -name "*rpc*" -o -name "*task*" | grep -E "\.py$"Repository: GetBindu/Bindu
Length of output: 125
🏁 Script executed:
# Look for where tasks are executed and results processed
rg -n "execute_task\|run_task\|handler" bindu/server/scheduler/ | head -30Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the scheduler implementation for how handler results are serialized
cat -n bindu/server/scheduler/scheduler.py | head -150Repository: GetBindu/Bindu
Length of output: 125
🏁 Script executed:
# Look for message execution or handler invocation
rg -n -B 5 -A 15 "def.*execute\|def.*handle.*message" bindu/server/ | head -100Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look at task_handlers.py for how results are processed
cat -n bindu/server/handlers/task_handlers.py | head -200Repository: GetBindu/Bindu
Length of output: 5984
🏁 Script executed:
# Check task_manager.py for execution logic
cat -n bindu/server/task_manager.py | head -200Repository: GetBindu/Bindu
Length of output: 8792
🏁 Script executed:
# Search for where handler is actually called in task processing
rg -n "handler\|execute\|\.run\(" bindu/server/handlers/task_handlers.py | head -30Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for serialization in task result handling
rg -n -B 3 -A 10 "result.*json\|json.*result\|to_dict\|to_json" bindu/server/ | head -60Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for the Worker implementation that executes the handler
find bindu/server -name "*worker*" -type fRepository: GetBindu/Bindu
Length of output: 97
🏁 Script executed:
# Check for Runner implementation
find bindu/server -name "*runner*" -type fRepository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for manifest worker which likely executes the handler
cat -n bindu/server/workers/manifest_worker.py | head -250Repository: GetBindu/Bindu
Length of output: 12101
🏁 Script executed:
# Search for where handler result is stored or processed
rg -n -B 5 -A 10 "result\[.*\]\|\.result\s*=" bindu/server/workers/ | head -80Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for ResultProcessor implementation
find bindu/server -name "*result*" -o -name "*response*" | grep -E "\.py$"Repository: GetBindu/Bindu
Length of output: 156
🏁 Script executed:
# Check the helpers module
cat -n bindu/server/workers/helpers.py | head -300Repository: GetBindu/Bindu
Length of output: 121
🏁 Script executed:
# Look for how normalized results are stored
rg -n -B 3 -A 10 "normalize_result\|ResultProcessor" bindu/server/workers/manifest_worker.pyRepository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the result_processor
cat -n bindu/server/workers/helpers/result_processor.pyRepository: GetBindu/Bindu
Length of output: 6254
🏁 Script executed:
# Check the response_detector
cat -n bindu/server/workers/helpers/response_detector.pyRepository: GetBindu/Bindu
Length of output: 5069
🏁 Script executed:
# Check if agno response objects are actually JSON-serializable by looking at test cases
rg -n "json\|serialize\|JSONEncoder" tests/ | grep -i agno | head -20Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look at what the actual agno.Agent.run returns by checking how it's used in tests
rg -n -B 5 -A 10 "agent\.run\|\.to_dict\|\.content" tests/ | head -100Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check if there are any actual JSON serialization errors reported in issues or tests
rg -n "TypeError.*serializ\|not JSON serializable\|Object.*is not JSON" . 2>/dev/null | head -20Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Verify the normalize_result is being called on the returned dict from pdf_research_agent
# by understanding the flow: handler returns dict -> manifest stores it -> needs serialization
rg -n -B 3 -A 10 "normalize_result\|artifact\|store.*result" bindu/server/workers/manifest_worker.py | head -60Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Look for where the task result (stored artifact) is serialized to JSON for HTTP response
rg -n -B 5 -A 10 "json\.dumps\|JSONEncoder\|to_json" bindu/server/workers/ bindu/server/handlers/ | head -100Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the _handle_terminal_state method which stores the results
cat -n bindu/server/workers/manifest_worker.py | sed -n '330,420p'Repository: GetBindu/Bindu
Length of output: 4041
🏁 Script executed:
# Look at how artifacts are created and stored
rg -n -B 3 -A 10 "Artifact\|artifact" bindu/server/workers/manifest_worker.py | head -80Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Find the _handle_terminal_state method
rg -n -A 80 "_handle_terminal_state" bindu/server/workers/manifest_worker.py | head -120Repository: GetBindu/Bindu
Length of output: 5084
🏁 Script executed:
# Look for ArtifactBuilder usage
rg -n -B 3 -A 10 "ArtifactBuilder" bindu/server/workers/manifest_worker.pyRepository: GetBindu/Bindu
Length of output: 1323
🏁 Script executed:
# Check how artifacts with content are stored
rg -n "artifact\[.*\]\|content.*artifact" bindu/server/ | head -30Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check ArtifactBuilder.from_result to see if it serializes to JSON
cat -n bindu/utils/worker/artifact_builder.py | head -150Repository: GetBindu/Bindu
Length of output: 128
🏁 Script executed:
# Look for JSON serialization in the storage layer
rg -n "json\.dumps\|JSONEncoder\|serialize" bindu/server/storage/ | head -30Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check if there are test examples showing serialization of agno response
find . -name "*.py" -type f -exec grep -l "agno.*json\|json.*agno" {} \; | head -5Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Find ArtifactBuilder
find bindu -name "*.py" -type f -exec grep -l "class ArtifactBuilder" {} \;Repository: GetBindu/Bindu
Length of output: 89
🏁 Script executed:
# Look at the actual terminal state handling in manifest_worker
cat -n bindu/server/workers/manifest_worker.py | sed -n '424,550p'Repository: GetBindu/Bindu
Length of output: 6383
🏁 Script executed:
# Check what happens after normalize_result is called
rg -n -B 2 -A 15 "normalized.*=.*normalize_result\|results.*=.*ResultProcessor" bindu/server/workers/manifest_worker.pyRepository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
# Check the artifacts.py file to see how results are converted to artifacts (JSON serializable)
cat -n bindu/utils/worker/artifacts.py | head -200Repository: GetBindu/Bindu
Length of output: 3184
🏁 Script executed:
# Look for from_result method in artifacts
rg -n -A 30 "def from_result" bindu/utils/worker/artifacts.pyRepository: GetBindu/Bindu
Length of output: 1223
🏁 Script executed:
# Check if there's any serialization logic in the storage layer when saving artifacts
rg -n "artifact\|serialize" bindu/server/storage/ -A 3 | head -80Repository: GetBindu/Bindu
Length of output: 40
Ensure agno response object is JSON-serializable before returning.
agent.run(...) returns an Agno response object that won't serialize cleanly when wrapped in the returned dict. The bindufy.py example (line 687) shows the pattern: result.to_dict()["content"]. Extract the content or normalize to a dict before returning.
Proposed fix
result = agent.run(input=enriched_messages)
- return {"success": True, "data": result, "error": None}
+ data = (
+ result.to_dict().get("content")
+ if hasattr(result, "to_dict")
+ else getattr(result, "content", 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.
| result = agent.run(input=enriched_messages) | |
| return result | |
| return {"success": True, "data": result, "error": None} | |
| result = agent.run(input=enriched_messages) | |
| data = ( | |
| result.to_dict().get("content") | |
| if hasattr(result, "to_dict") | |
| else getattr(result, "content", 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 197 - 198,
The Agno response returned by agent.run(enriched_messages) is not
JSON-serializable; convert or normalize it before returning. Replace returning
the raw result variable with a JSON-serializable object (for example call
result.to_dict() and extract the relevant field like ["content"] or return the
full dict), i.e., transform the value from agent.run(enriched_messages) into a
plain dict/primitive and then return {"success": True, "data":
<converted_result>, "error": None} so the response can be serialized.
| VOICE__ENABLED=true | ||
| VOICE__STT_MODEL=nova-3 | ||
| VOICE__STT_LANGUAGE=en | ||
| VOICE__TTS_MODEL=piper-local |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check usage of VOICE__TTS_MODEL and VOICE__TTS_PROVIDER to determine if both are needed
# Search for how TTS_MODEL and TTS_PROVIDER are used
rg -n -C3 --type=py 'TTS_MODEL|TTS_PROVIDER' -g '!examples/**'Repository: GetBindu/Bindu
Length of output: 1373
🏁 Script executed:
#!/bin/bash
# Search for TTS_MODEL references across all files (not just Python)
echo "=== Searching for TTS_MODEL in all file types ==="
rg -n 'TTS_MODEL' -g '!examples/**'
echo ""
echo "=== Checking settings/config schema files ==="
rg -n 'tts_model' -g '!examples/**'
echo ""
echo "=== Looking for VoiceSettings or voice config definitions ==="
rg -n 'class.*Voice|VoiceSettings|voice.*config' --type=py -A 5 -g '!examples/**' | head -100Repository: GetBindu/Bindu
Length of output: 8828
🏁 Script executed:
#!/bin/bash
# Read the .env.example file to confirm lines 3 and 19
echo "=== Contents of examples/voice-agent/.env.example ==="
cat -n examples/voice-agent/.env.example
echo ""
echo "=== How piper TTS provider uses tts_model in service_factory ==="
rg -n -B5 -A10 'piper' bindu/extensions/voice/service_factory.py | head -80Repository: GetBindu/Bindu
Length of output: 2421
🏁 Script executed:
#!/bin/bash
# Check the full service_factory code to see all provider branches and tts_model usage
echo "=== Full _create_tts_service_for_provider function ==="
rg -n -B2 -A50 'def _create_tts_service_for_provider' bindu/extensions/voice/service_factory.pyRepository: GetBindu/Bindu
Length of output: 2313
🏁 Script executed:
#!/bin/bash
# Verify that tts_model is used by ElevenLabs but NOT by piper
echo "=== ElevenLabs provider implementation ==="
rg -n -A30 'if provider == "elevenlabs"' bindu/extensions/voice/service_factory.py | head -50
echo ""
echo "=== Check VoiceAgentExtension to see if tts_model defaults are provider-specific ==="
cat -n bindu/extensions/voice/voice_agent_extension.py | head -60Repository: GetBindu/Bindu
Length of output: 3950
🏁 Script executed:
#!/bin/bash
# Check Azure provider implementation to complete the picture
echo "=== Azure provider implementation ==="
rg -n -A30 'if provider == "azure"' bindu/extensions/voice/service_factory.pyRepository: GetBindu/Bindu
Length of output: 1582
🏁 Script executed:
#!/bin/bash
# Final check: confirm Azure provider implementation for tts_model usage
echo "=== Complete Azure provider implementation ==="
rg -n -A40 'if provider == "azure"' bindu/extensions/voice/service_factory.py | tail -20Repository: GetBindu/Bindu
Length of output: 899
Remove VOICE__TTS_MODEL=piper-local or document it's only used by non-piper providers.
Line 19's VOICE__TTS_MODEL=piper-local is unused when VOICE__TTS_PROVIDER=piper (line 3). The Piper TTS implementation does not accept or use a model parameter—it only configures voice ID and sample rate. This setting is only relevant for ElevenLabs and should not appear in a piper-focused example, or should be moved to a provider-specific section with a comment explaining it applies only to VOICE__TTS_PROVIDER=elevenlabs and VOICE__TTS_PROVIDER=azure.
🤖 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 extraneous
VOICE__TTS_MODEL=piper-local entry from the example or move it into a
provider-specific block with a comment clarifying it only applies when
VOICE__TTS_PROVIDER is elevenlabs or azure; specifically update the
examples/voice-agent .env example so VOICE__TTS_MODEL is not present for the
piper-focused configuration (when VOICE__TTS_PROVIDER=piper) or is relocated and
annotated to indicate it is only used by non-piper providers.
| # Optional LLM backend for smarter responses | ||
| OPENROUTER_API_KEY=your-openrouter-api-key | ||
| OPENROUTER_MODEL=openai/gpt-oss-120b:free | ||
| OPENROUTER_MEMORY_TURNS=4 | ||
| VOICE_MAX_SENTENCES=2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if OPENROUTER variables are used in non-voice contexts
# Search for OPENROUTER usage patterns
rg -n -C2 --type=py 'OPENROUTER_API_KEY|OPENROUTER_MODEL' -g '!examples/**'
# Search for VOICE_MAX_SENTENCES vs VOICE__MAX_SENTENCES
rg -n --type=py 'VOICE_MAX_SENTENCES|VOICE__MAX_SENTENCES'Repository: GetBindu/Bindu
Length of output: 2410
🏁 Script executed:
cat -n examples/voice-agent/.env.example | head -40Repository: GetBindu/Bindu
Length of output: 1298
🏁 Script executed:
rg -n 'OPENROUTER_MODEL|OPENROUTER_MEMORY_TURNS' bindu/Repository: GetBindu/Bindu
Length of output: 40
🏁 Script executed:
rg -n 'OPENROUTER_MODEL|OPENROUTER_MEMORY_TURNS' examples/voice-agent/Repository: GetBindu/Bindu
Length of output: 876
Fix naming inconsistency: VOICE_MAX_SENTENCES should use double underscore.
Line 35 uses VOICE_MAX_SENTENCES with a single underscore, while all other voice configuration variables (lines 1-29) consistently use VOICE__ with double underscores. Change to VOICE__MAX_SENTENCES for consistency.
The OPENROUTER_* variables (lines 32-34) intentionally use a different prefix because they configure the LLM backend provider, not the voice system itself.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 34-34: [UnorderedKey] The OPENROUTER_MEMORY_TURNS key should go before the OPENROUTER_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 31 - 35, Rename the
environment variable VOICE_MAX_SENTENCES to use the double-underscore voice
prefix for consistency with other voice settings: change VOICE_MAX_SENTENCES to
VOICE__MAX_SENTENCES in the .env example so it matches the VOICE__* naming
convention used elsewhere.
| 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.
Module import can fail on invalid numeric env values.
At Lines 37-44, direct int(...) parsing will raise ValueError if env vars are malformed, preventing the example from starting. Consider guarded parsing with sane fallbacks.
Suggested fix
+def _env_int(name: str, default: int) -> int:
+ raw = (os.environ.get(name, str(default)) or str(default)).strip()
+ try:
+ return int(raw)
+ except ValueError:
+ return default
+
OPENROUTER_MEMORY_TURNS = max(
0,
- int(os.environ.get("OPENROUTER_MEMORY_TURNS", "4") or "4"),
+ _env_int("OPENROUTER_MEMORY_TURNS", 4),
)
VOICE_MAX_SENTENCES = max(
1,
- int(os.environ.get("VOICE_MAX_SENTENCES", "2") or "2"),
+ _env_int("VOICE_MAX_SENTENCES", 2),
)🤖 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 module currently
calls int(...) directly for OPENROUTER_MEMORY_TURNS and VOICE_MAX_SENTENCES
which will raise ValueError on malformed env values; change to guarded parsing:
read the raw env string via os.environ.get(...), try to int() it inside a
try/except (or use a small helper like parse_int_with_default), and on exception
or None/empty input fall back to the intended defaults (4 for
OPENROUTER_MEMORY_TURNS, 2 for VOICE_MAX_SENTENCES), then apply the existing
max(...) wrapper so the final values remain non-negative and >=1 as originally
intended; update the code paths that reference OPENROUTER_MEMORY_TURNS and
VOICE_MAX_SENTENCES accordingly.
| except Exception as exc: | ||
| print(f"OpenRouter error: {exc}") | ||
|
|
There was a problem hiding this comment.
Avoid blind exception catch in streaming path.
At Line 197, except Exception is too broad and can hide unexpected failures. Catch expected client/network errors explicitly, then optionally keep a final narrow fallback with structured logging.
🧰 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 197 - 199, The current broad
"except Exception as exc" in the streaming path should be replaced with explicit
handlers for expected errors (e.g., network/client errors like
requests.exceptions.RequestException or library-specific errors such as
OpenRouter/OpenAI client exceptions and websocket/asyncio.CancelledError) and
use structured logging; update the block around the "except Exception as exc"
line to add separate except clauses for those specific exception types, log
error details via the existing logger (including stack/metadata), handle/cleanup
or re-raise CancelledError and other fatal exceptions, and keep a final very
narrow fallback except Exception that logs the error with context before
re-raising to avoid silently swallowing unexpected failures.
| "agno>=2.5.2", | ||
| "openai>=2.17.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether agno/openai are imported by non-example, non-test Python code.
# Expected: If this prints no production imports, keep these dependencies optional.
rg -n -C3 '^\s*(import|from)\s+(agno|openai)\b' --type=py \
--glob '!examples/**' \
--glob '!tests/**'Repository: GetBindu/Bindu
Length of output: 40
Move agno and openai to an optional agents extra.
These packages are only imported by examples/tests, not by production code. Including them in [project].dependencies makes them mandatory for every bindu install, unnecessarily broadening the default footprint. Move them to the agents optional extra to preserve a smaller core install.
Suggested refactor
dependencies = [
# Security
"detect-secrets==1.5.0",
"python-dotenv>=1.1.0",
- "agno>=2.5.2",
- "openai>=2.17.0",
]
agents = [
+ "agno>=2.5.2",
+ "openai>=2.17.0",
"ag2[openai]>=0.11.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 63 - 64, Remove "agno>=2.5.2" and
"openai>=2.17.0" from the [project].dependencies list and add them under an
optional extras group named "agents" (i.e. create or update
[project.optional-dependencies] with agents = ["agno>=2.5.2",
"openai>=2.17.0"]); this keeps the packages available via pip install .[agents]
but out of the core install. Ensure you reference the exact package strings
"agno" and "openai" when moving them so versions are preserved.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
examples/voice-agent/(env example, runnable main, skill YAML) and updated/added supporting example content underexamples/.Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
examples/voice-agent/(server + instructions +.env.example).Security Impact (required)
Yes/No) NoYes/No) NoYes/No) Yes (example code path only)Yes/No) NoYes/No) NoYes, explain risk + mitigation:.env.example; no secrets are committed; examples are not executed in production paths.Verification
Environment
Steps to Test
uv sync --dev --extra agents --extra voiceexamples/voice-agent/.env.example→examples/voice-agent/.envand fill required env varsuv run examples/voice-agent/main.pyExpected Behavior
GET /healthandGET /.well-known/agent.jsonsucceed;POST /voice/session/startreturns a WS URL; voice WS connects.Actual Behavior
Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
Compatibility / Migration
Yes/No) YesYes/No) No (example-only)Yes/No) NoFailure Recovery (if this breaks)
examples/tree only.Risks and Mitigations
None
Checklist
uv run pytest)uv run pre-commit run --all-files)Summary by CodeRabbit
New Features
Documentation
Configuration
Tests
Chores