fix: MCP/REST generations incorrectly triggering autoplay#637
Conversation
Agent-initiated generations (via MCP tool or POST /speak) were stored in the database with source="manual" because generate_speech() ignored the caller's origin. The frontend SSE handler checks gen.source to skip autoplay for agent sources, but it received "manual" and played anyway. Pass source="mcp" / source="rest" through GenerationRequest so the DB row carries the correct origin and the frontend's AGENT_SOURCES guard works reliably. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a GenerationSource Literal and optional GenerationRequest.source; converts many Pydantic List[...] annotations to built-in list[...]. MCP and REST entry points tag requests with "mcp" or "rest"; generate_speech preserves caller-supplied source and conditionally applies "personality_speak"; history.create_generation accepts GenerationSource. ChangesRequest Source Attribution
sequenceDiagram
participant MCP
participant REST
participant Generator as generate_speech
participant History as history.create_generation
participant Models as backend.models
MCP->>Generator: request(source="mcp")
REST->>Generator: request(source="rest")
Generator->>Generator: resolve source (data.source? -> resolved_source)
Generator->>History: create_generation(..., source=resolved_source)
History->>Models: store GenerationResponse with source/versions
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (1)
backend/models.py (1)
103-105: ⚡ Quick winConsider adding a validation pattern for the
sourcefield.For consistency with other fields in this model (e.g.,
engine,language,model_size), consider restrictingsourceto known values to prevent typos and tighten the API contract.✨ Suggested validation pattern
source: Optional[str] = Field( - None, description="Origin of the request (e.g. 'mcp', 'rest'). Internal use — not exposed to public API docs." + None, + pattern="^(manual|mcp|rest|personality_speak|import)$", + description="Origin of the request (e.g. 'mcp', 'rest'). Internal use — not exposed to public API docs." )🤖 Prompt for 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. In `@backend/models.py` around lines 103 - 105, The source Field (source: Optional[str] = Field(...)) should be constrained to known values to prevent typos; replace the free-form string with a typed constraint (e.g., a Source enum or typing.Literal of allowed values like 'mcp' and 'rest') and update the Field declaration on the model to use that type (or add a regex/pattern argument to Field) so validation occurs automatically when constructing instances of the model.
🤖 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.
Nitpick comments:
In `@backend/models.py`:
- Around line 103-105: The source Field (source: Optional[str] = Field(...))
should be constrained to known values to prevent typos; replace the free-form
string with a typed constraint (e.g., a Source enum or typing.Literal of allowed
values like 'mcp' and 'rest') and update the Field declaration on the model to
use that type (or add a regex/pattern argument to Field) so validation occurs
automatically when constructing instances of the model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1844f41-e371-46d6-aff2-957fbca9ea33
📒 Files selected for processing (4)
backend/mcp_server/tools.pybackend/models.pybackend/routes/generations.pybackend/routes/speak.py
Replaces free-form Optional[str] with a Literal type so Pydantic rejects unknown source values at validation time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
depopry
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/models.py (1)
6-6: ⚡ Quick winPrefer modern
listovertyping.Listfor Python 3.9+.The
typing.Listimport is deprecated in favor of the built-inlisttype for type hints (PEP 585). Since this codebase likely targets Python 3.9 or later, you can uselist[...]directly.♻️ Proposed fix
-from typing import Literal, Optional, List +from typing import Literal, OptionalThen replace
List[...]withlist[...]throughout the file (lines 41, 102, etc.).🤖 Prompt for 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. In `@backend/models.py` at line 6, Replace the deprecated typing.List import with the built-in generic list by removing List from the import line and updating all uses of List[...] in this module (e.g., type annotations referenced around the symbols on lines noted such as the annotations near the class/variable usages at lines ~41 and ~102) to use list[...]; ensure the top import becomes "from typing import Literal, Optional" and change every List[...] occurrence to list[...] to adopt PEP 585 style.
🤖 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.
Nitpick comments:
In `@backend/models.py`:
- Line 6: Replace the deprecated typing.List import with the built-in generic
list by removing List from the import line and updating all uses of List[...] in
this module (e.g., type annotations referenced around the symbols on lines noted
such as the annotations near the class/variable usages at lines ~41 and ~102) to
use list[...]; ensure the top import becomes "from typing import Literal,
Optional" and change every List[...] occurrence to list[...] to adopt PEP 585
style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 743b9c63-ba9e-45da-bc60-2c818f1aa898
📒 Files selected for processing (2)
backend/models.pybackend/services/history.py
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/models.py (1)
126-126: ⚡ Quick winUse
GenerationSourcetype for consistency and validation.The
sourcefield inGenerationResponseuses plainstr, whileGenerationRequest.source(line 105) uses theGenerationSourceLiteral type. For consistency, type safety, and to enable Pydantic validation of source values, consider usingGenerationSourcehere as well.♻️ Proposed fix for type consistency
- source: str = "manual" + source: GenerationSource = "manual"🤖 Prompt for 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. In `@backend/models.py` at line 126, Change the type of the source field on the GenerationResponse model from plain str to the existing GenerationSource Literal so Pydantic will validate and ensure consistency with GenerationRequest; update the source declaration in the GenerationResponse class (symbol: GenerationResponse.source) to use GenerationSource and run tests/type checks to confirm no other code expects arbitrary strings.
🤖 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.
Nitpick comments:
In `@backend/models.py`:
- Line 126: Change the type of the source field on the GenerationResponse model
from plain str to the existing GenerationSource Literal so Pydantic will
validate and ensure consistency with GenerationRequest; update the source
declaration in the GenerationResponse class (symbol: GenerationResponse.source)
to use GenerationSource and run tests/type checks to confirm no other code
expects arbitrary strings.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/models.py`:
- Around line 105-107: The public model GenerationRequest exposes the internal
provenance field source (of type GenerationSource) allowing clients to spoof
origin; remove the `source: Optional[GenerationSource]` Field from the
GenerationRequest model, stop accepting it on public endpoints (/generate,
/generate/stream), and instead propagate source server-side (e.g., set a handler
parameter or use a separate internal model like InternalGenerationRequest or a
request context value) so only server code assigns GenerationRequest.source
internally when creating downstream objects that rely on source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The source field was exposed on the public API model, allowing clients to spoof request provenance. Now source is only set server-side via a function parameter on generate_speech(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Fixes #635.
POST /speak) were stored withsource="manual"in the DB becausegenerate_speech()ignored the caller's origingen.sourceto skip autoplay for agent sources ("mcp","rest"), but always received"manual"— so autoplay fired even when disabledsource="mcp"/source="rest"throughGenerationRequestso the DB row carries the correct origin and the frontend'sAGENT_SOURCESguard works reliablyTest plan
voicebox.speak) with autoplay disabled — should NOT auto-play in main windowPOST /speakwith autoplay disabled — should NOT auto-play in main window"mcp", not downgrade to"personality_speak"🤖 Generated with Claude Code
Summary by CodeRabbit