Audit fixes: /ws/call auth, offline default, STT turn assembly, provider API 500, CI lint gate#84
Open
jaylfc wants to merge 14 commits into
Open
Audit fixes: /ws/call auth, offline default, STT turn assembly, provider API 500, CI lint gate#84jaylfc wants to merge 14 commits into
jaylfc wants to merge 14 commits into
Conversation
…plan Analysis-only deliverable. Documents verified bugs (unauthenticated /ws/call, /api/providers KeyError 500, default-backend split brain, interim-transcript duplication), severity-rated audit across security/correctness/architecture/ testing/deps/docs, a 4-theme improvement strategy, and a milestone task plan with quick wins. No code changes. https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
Add the missing 'backend' field to the agent entries in PROVIDER_DEPS, make get_provider_status() defensive with .get(), and add endpoint + registry invariant tests so the provider browser can't silently break again. Fixes #68 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
…ke server.py - agents/__init__.py: single DEFAULT_AGENT_PROVIDER='ollama' constant, imported by server.py so the displayed provider and the selected backend can't drift; 'auto' (or empty, written by the settings UI) keeps auto-detect behaviour; ollama default model aligned to qwen3:8b - add reset_agent_backend() so tests/settings changes can invalidate the singleton - installers: every wizard choice now writes an explicit AGENT_PROVIDER (preserved on re-run; falls back to ollama) - test_offline_defaults now asserts the backend the factory actually builds on a scrubbed env, not the cosmetic constant Fixes #67 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
…nce timeout - /ws/call now requires a single-use stream token: each <Connect><Stream> embeds a <Parameter> minted at TwiML time (TTL 120s, bound to the CallSid, constant-time compare); connections whose start event doesn't redeem one are dropped. Closes the PIN-gate bypass on the public port. - Turn assembly extracted into _accumulate_stt_message(): only finalized Deepgram segments are buffered (interims repeated the same words into the LLM input), and the turn now ends on speech_final endpointing rather than per-segment is_final (kept as fallback when the attribute is absent). Buffer shared with the Deepgram callback thread is now lock-guarded. - ws.receive timeout no longer kills the session: silence/hold gaps continue the loop; only stop events or a closed connection end it. Fixes #66, #69, #71 Fixes #25 (turn boundary now speech_final-driven) https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
escHtml now escapes all five dangerous characters (it previously left quotes intact via the textContent/innerHTML round trip), and the delete button uses data-sid + addEventListener instead of interpolating the sid into an inline JS string. Fixes #73 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
ensure_models() now only checks importability and falls back to cloud voice with a hint; installation moves to install.sh (which already pre-installs the MLX packages) or the new 'python local_voice.py --install'. Running pip at boot blocked startup for up to 300s and caused a macOS fork-safety crash (Network framework atfork handlers in pip's forked build children). Fixes #51 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
rumps/pywebview are macOS-only and audioop-lts only exists for 3.13+, so 'pip install -r requirements.txt' now works on Linux (matching the README's claim) and CI stops hand-maintaining a parallel dep list. Fixes #75 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
- pyproject.toml: ruff config (default E4/E7/E9 + F rules, plus E722 bare-except; AppKit bulk imports in native_settings.py exempted from F401) - CI gains a 'ruff check .' step before tests - Fix the 60 existing findings: unused imports, bare excepts (now Exception/OSError), f-strings without placeholders, mid-file 'import time as _time' (top-level time was already imported), dead AVSpeech stub code in NativeTTS, unused call_sid in voicemail_complete Fixes #76 https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughImplements one-time per-call WebSocket stream tokens and thread-safe STT turn assembly; aligns AGENT_PROVIDER defaults across agents, server, and installers; adds provider registry backend fields; enforces Ruff in CI with platform-marked requirements; and includes tests, docs (AUDIT.md), and multiple small code-quality fixes. ChangesAgent Provider Configuration & Defaults
WebSocket Stream Token Authentication
Provider Registry Backend Field & Status Fix
Dependency Management & Linting
Code Quality & Security Improvements
Technical Audit & Contributing Workflow
Sequence Diagram(s) sequenceDiagram
participant TwiML as Twilio (Call)
participant Server
participant Client as Browser/Agent WS
participant Deepgram
participant LLM
TwiML->>Server: generate TwiML <Connect><Stream token=one_time>
Server->>TwiML: return TwiML with token
Client->>Server: WebSocket connect -> sends start with customParameters token
Server->>Server: _redeem_stream_token(token, call_sid)
Server-->>Client: accept or reject WS (based on token)
Client->>Deepgram: media forwarded
Deepgram->>Server: on_message (interim/final)
Server->>Server: _accumulate_stt_message(msg, transcript_buf) (lock)
Server->>LLM: when turn finalized -> send transcript
LLM->>Server: response
Server->>Client: TTS audio frames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AUDIT.md`:
- Around line 7-10: Update the EXECUTIVE SUMMARY in AUDIT.md to clarify this is
a pre-fix baseline by either prefixing the section title "Executive Summary"
with "Before this PR" or splitting the report into "Before this PR" and "After
this PR" sections; modify the header and the opening paragraph under the
"Executive Summary" block so it explicitly states the findings reflect the state
before the current PR, and if splitting, create a new "After this PR" subsection
summarizing fixes introduced by the PR (refer to the existing "Executive
Summary" text to edit or duplicate).
- Around line 128-193: Markdown heading lines (e.g., "### Theme 1 — Close the
gap..." and subsequent "### Theme 2/3/4" and "### Milestone 0 — Safety net") are
missing the required blank line above them, which triggers MD022; fix by
inserting a single blank line before each top-level "###" heading in this block
so every heading is surrounded by blank lines; search for the headings "###
Theme 1", "### Theme 2", "### Theme 3", "### Theme 4", "### Milestone 0" and
ensure there is one empty line immediately before each heading to satisfy the
linter.
In `@install-linux.sh`:
- Around line 316-323: The Ollama branches (the case handling that sets
AGENT_PROVIDER="ollama" — see the block that sets LLM_KEY, LLM_BASE_URL,
LLM_PROVIDER and AGENT_PROVIDER) must set a local LLM_MODEL default that matches
the new Ollama/qwen3 default; update both the choice 4 block (where LLM_MODEL is
defaulted to "llama3") and the choice 7 block (where it currently writes
"gpt-4o-mini") so that when AGENT_PROVIDER="ollama" you prompt for LLM_MODEL but
default it to "qwen3" (i.e., change the LLM_MODEL="${LLM_MODEL:-llama3}" and the
other default to use "qwen3" for Ollama paths) ensuring any fresh install points
to a provisioned local Ollama model.
In `@install.sh`:
- Around line 190-196: When AGENT_PROVIDER is set to "ollama" the script must
not default LLM_MODEL to the legacy "llama3"; change the logic around the
LLM_MODEL prompt/assignment so that when AGENT_PROVIDER="ollama" the default
LLM_MODEL uses the previously pulled OLLAMA_MODEL (or leaves it empty) instead
of "llama3". Update the block setting LLM_KEY, LLM_BASE_URL, LLM_PROVIDER,
AGENT_PROVIDER and the LLM_MODEL assignment so
agents._resolve_openai_compat("ollama") receives the correct model (reference
variables LLM_MODEL, AGENT_PROVIDER and OLLAMA_MODEL).
In `@server.py`:
- Line 80: The module currently freezes AGENT_PROVIDER at import time; instead,
stop using the module-level AGENT_PROVIDER constant and read the provider
dynamically via env() wherever used (e.g., in health()/settings handlers and any
code that queries provider), and when api_update_settings() changes
AGENT_PROVIDER or related LLM keys invoke agents.reset_agent_backend() to clear
the cached backend; alternatively, if you don't support live changes, have
api_update_settings() mark the settings as restart-required so the UI/health
reflect that.
- Around line 257-273: The functions _issue_stream_token and
_redeem_stream_token access and mutate the shared stream_tokens dict without
synchronization, causing possible RuntimeError or lost tokens under concurrency;
fix by introducing a threading.Lock (e.g., stream_tokens_lock) and acquire it
around all accesses and mutations of stream_tokens (including the cleanup loop
in _issue_stream_token, the assignment stream_tokens[token]=..., and the pop()
in _redeem_stream_token) to ensure atomic iteration and modifications and
prevent race conditions.
In `@tests/test_call_flow.py`:
- Around line 73-79: The test test_outgoing_stream_carries_token assumes exactly
one token in server.stream_tokens before unpacking; add an explicit assertion
like assert len(server.stream_tokens) == 1 (or assert server.stream_tokens and
len(...) == 1 for clearer failure messaging) immediately before the unpacking
line to produce a clear test assertion if 0 or multiple tokens are present,
referencing server.stream_tokens and the existing unpacking (rec,) to locate
where to insert it.
In `@tests/test_offline_defaults.py`:
- Around line 32-33: The test test_display_constant_matches_factory_default
currently reads server.AGENT_PROVIDER from an already-imported module so it
reflects ambient process state; modify the test to scrub
os.environ["AGENT_PROVIDER"] (or delete it) and then re-import or reload the
server module so server.AGENT_PROVIDER is evaluated against a clean environment,
then assert server.AGENT_PROVIDER == agents.DEFAULT_AGENT_PROVIDER == "ollama";
reference the server module, the AGENT_PROVIDER symbol, and
agents.DEFAULT_AGENT_PROVIDER when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efd16e0a-2bc7-48ad-8895-633bd037f1a1
📒 Files selected for processing (19)
.github/workflows/ci.ymlAUDIT.mdREADME.mdagents/__init__.pydashboard.htmlinstall-linux.shinstall.shlocal_voice.pymenubar.pynative_settings.pyprovider_registry.pypyproject.tomlrequirements.txtserver.pytests/test_all_settings_editable.pytests/test_call_flow.pytests/test_offline_defaults.pytests/test_provider_registry.pytests/test_settings_overrides.py
- stream_tokens guarded by a lock (webhook threads issue, WS threads redeem) - AGENT_PROVIDER no longer frozen at import for status reporting: /health and /api/settings read it live, and updating any agent-affecting setting resets the cached backend singleton and health cache (with tests) - installers: Ollama wizard branches now default LLM_MODEL to the provisioned model (macOS: $OLLAMA_MODEL RAM tier; Linux: qwen3:8b) instead of legacy llama3/gpt-4o-mini; read -r - AUDIT.md labelled as pre-fix baseline; MD022 heading spacing - test_offline_defaults: fresh-process subprocess check instead of ambient-state constant assertion; test_call_flow asserts token count before unpacking https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== server.py ==="
cat server.py
echo ""
echo "=== agents/__init__.py ==="
cat agents/__init__.py
echo ""
echo "=== dashboard.html ==="
cat dashboard.html
echo ""
echo "=== provider_registry.py ==="
cat provider_registry.py
echo ""
echo "=== requirements.txt ==="
cat requirements.txt
echo ""
echo "=== pyproject.toml ==="
cat pyproject.toml
echo ""
echo "=== local_voice.py ==="
cat local_voice.py
echo ""
echo "=== tests/test_call_flow.py ==="
cat tests/test_call_flow.py
echo ""
echo "=== tests/test_offline_defaults.py ==="
cat tests/test_offline_defaults.py
echo ""
echo "=== tests/test_provider_registry.py ==="
cat tests/test_provider_registry.py
echo ""
echo "=== .github/workflows/ci.yml ==="
cat .github/workflows/ci.yml
echo ""
echo "=== README.md ==="
cat README.md
echo ""
echo "=== menubar.py ==="
cat menubar.py
echo ""
echo "=== native_settings.py ==="
cat native_settings.py
echo ""
echo "=== install.sh ==="
cat install.sh
echo ""
echo "=== install-linux.sh ==="
cat install-linux.shLength of output: 242810 🏁 Script executed: #!/bin/bash
echo "=== conftest.py ==="
cat tests/conftest.py 2>/dev/null || echo "NOT FOUND"
echo ""
echo "=== tests/ directory listing ==="
ls -la tests/
echo ""
echo "=== Check for AGENT_PROVIDER in case 5 of install.sh ==="
grep -n "AGENT_PROVIDER\|openai-compat\|LLM_CHOICE" install.sh | head -30
echo ""
echo "=== Check media handling in handle_ws before auth ==="
grep -n "event.*media\|dg_conn.send\|_redeem\|stream_sid\|call_sid" server.py | head -40
echo ""
echo "=== Check test_all_settings_editable ==="
cat tests/test_all_settings_editable.pyLength of output: 8504
Summary
🔴 Pre-auth audio leaks to Deepgram in
|
CodeRabbit caught that media frames were forwarded to Deepgram before the start event's token validation, so a direct connection to the public WS could burn STT quota without authenticating. The handler now sets an authenticated flag only after successful token redemption and gates send_media on it; stream_sid/call_sid are likewise only assigned post-auth. Also: register /ws/call via call instead of decorator (flask-sock's route decorator returns None, which unbound handle_ws and made it untestable), add WS-level regression tests driving handle_ws with a fake socket + fake Deepgram client (media-before-start, media-after- invalid-start, media-after-valid-start), and reword the NativeTTS docstring per review. https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by nex-n2-pro:free · 1,176,849 tokens |
…rmed payloads Kilo review findings on PR #84: - check_auth/require_dashboard_auth now read the dashboard token live via _dashboard_token() (env first, import-time fallback), and changing or clearing DASHBOARD_TOKEN through /api/settings revokes all existing dashboard sessions — making the schema hint's promise true. Old token stops working and the new one works without a restart; covered by tests. - _accumulate_stt_message guards against Deepgram payloads with no alternatives so an edge-case message can't raise IndexError on the callback thread and silently kill transcript processing for the call. Partially addresses #72 (DASHBOARD_TOKEN portion). https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
Owner
Author
|
Kilo findings triaged: Fixed in the latest commit — the two in-diff items: Already tracked as issues — the five "other observations" in unchanged code: import-time Generated by Claude Code |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if not expected: | ||
| return True # No token set = no auth (first run) | ||
| return hmac.compare_digest(token, DASHBOARD_TOKEN) | ||
| return hmac.compare_digest(token, expected) |
There was a problem hiding this comment.
WARNING: check_auth() can raise TypeError on non-string tokens
request.json may contain {"token": null} or another non-string value, which makes hmac.compare_digest(None, expected) raise TypeError and break the login flow. Guard the input before comparing.
Suggested change
| return hmac.compare_digest(token, expected) | |
| return isinstance(token, (str, bytes)) and hmac.compare_digest(token, expected) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -1150,6 +1233,13 @@ def api_update_settings(): | |||
| else: | |||
| update_setting(key, str(value)) | |||
There was a problem hiding this comment.
WARNING: JSON null values are stringified as "None"
api_update_settings() accepts arbitrary JSON values and calls str(value) for every setting. If a client sends DASHBOARD_TOKEN: null, the token becomes the literal string "None" instead of being cleared or rejected, which can leave the dashboard with an unintended password.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First batch of fixes from the technical audit (included as
AUDIT.md): the critical security gap, the worst correctness bugs, and the CI/dependency quick wins.Security
/ws/call(Unauthenticated /ws/call WebSocket bypasses the PIN gate (anyone can talk to the agent and burn STT/LLM/TTS quota) #66): every<Connect><Stream>TwiML now embeds a single-use stream token as a<Parameter>(TTL 120s, bound to the CallSid, constant-time compare). Connections whosestartevent doesn't redeem a valid token are dropped — closes the PIN-gate bypass on the public webhook port.escHtmlnow escapes quotes; the delete button usesdata-sid+addEventListenerinstead of an inlineonclickJS string.Correctness
DEFAULT_AGENT_PROVIDER = "ollama"constant shared by the factory and server; installers write an explicitAGENT_PROVIDERper wizard choice;auto/empty keeps auto-detect. A fresh install now selects local Ollama instead of silently landing on NoOpAgent while/healthclaimed "ollama".GET /api/providersno longer 500s (GET /api/providers returns 500 on every request — KeyError: 'backend' #68): agent registry entries gained the missingbackendkey;get_provider_status()is defensive.speech_final(endpointing), withis_finalkept solely as a fallback when the attribute is absent. The Deepgram-callback-thread/WS-loop shared buffer is now lock-guarded. Logic extracted into_accumulate_stt_message()for unit testing.ws.receivetimeout continues the loop instead of ending the session.ensure_models()only checks importability; install moves toinstall.shor the newpython local_voice.py --install. This also removes the macOS fork-safety crash vector (Network-framework atfork handlers in pip's forked build children) confirmed in the owner's crash report.CI / dependencies / docs
requirements.txt(requirements.txt has no platform markers (contradicts README); CI installs a hand-maintained dep list that will drift #75):rumps/pywebvieware Darwin-only,audioop-ltsis 3.13+-only;pip install -r requirements.txtnow works on Linux and CI installs from it instead of a hand-maintained list.Tests
56 passing (was 30). New coverage:
/voice/*TwiML branches (greeting/PIN/lockout/voicemail), stream-token issue/redeem/single-use/TTL, STT turn assembly (interim vs final vs speech_final),/api/providersendpoints, and a fresh-install backend-selection test that asserts the factory's actual choice on a scrubbed env (addresses the core of #77).Fixes #25, fixes #51, fixes #66, fixes #67, fixes #68, fixes #69, fixes #71, fixes #73, fixes #75, fixes #76, fixes #82.
https://claude.ai/code/session_01R2pGk31EzMvwumYCQ8TFNo
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores