-
Notifications
You must be signed in to change notification settings - Fork 25
fix: ensure user agent is consistent across http/websockets #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRenames the private header builder to a public get_headers(additional_headers=None); updates call sites to use it; adds a User-Agent header ("fish-audio/python/legacy") to HTTP clients and WebSocket setup; updates tests/mocks to use get_headers; increases WebSocket integration test retries and delays. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR ensures consistent User-Agent headers across both HTTP and WebSocket connections in the Fish Audio SDK.
- Renamed
_get_headerstoget_headers(public API) in the core client wrapper to enable reuse for WebSocket connections - Updated TTS WebSocket connections to use the centralized header builder instead of manually constructing headers
- Added User-Agent headers to the legacy SDK's HTTP clients for consistency
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/fishaudio/core/client_wrapper.py |
Changed _get_headers to public get_headers method and updated docstring to mention user agent |
src/fishaudio/resources/tts.py |
Updated WebSocket connections to use get_headers() instead of manually building authorization headers |
src/fish_audio_sdk/websocket.py |
Added User-Agent header to WebSocket session client initialization (both sync and async) |
src/fish_audio_sdk/io.py |
Added User-Agent header to HTTP client initialization (both sync and async) |
tests/unit/test_core.py |
Updated tests to call the renamed public get_headers method |
tests/unit/test_tts_realtime.py |
Added get_headers mock implementation to test fixtures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/fish_audio_sdk/websocket.py (1)
27-30: User-Agent and auth defaults on legacy WebSocket sessionsInitializing the sync and async HTTP clients with Authorization plus
"fish-audio/python/legacy"makes legacy WebSocket calls consistent with the rest of the legacy SDK. To avoid future drift withRemoteCallinio.py, consider extracting this legacy User-Agent string into a shared constant reused in both places.Also applies to: 103-106
src/fish_audio_sdk/io.py (1)
37-40: Legacy IO clients now emit a consistent User-AgentAdding Authorization and
"fish-audio/python/legacy"at async and sync client initialization aligns HTTP calls with the legacy WebSocket sessions. As withwebsocket.py, you may want to centralize this UA value in a single constant to keep the legacy identifier consistent if it ever changes.Also applies to: 47-50
tests/unit/test_core.py (1)
112-123: Tests correctly cover the newget_headersAPIThese tests validate that both wrappers expose
get_headers()with the expected Authorization and a User-Agent header, and that additional headers are merged in for the sync wrapper. If you want perfect symmetry, you could add a smallAsyncClientWrapper.get_headerstest with additional headers too, but what’s here is already sufficient for this change.Also applies to: 140-144
src/fishaudio/core/client_wrapper.py (1)
63-73: Centralized header builder and merge logic look sound
get_headersnow standardizes the Authorization andfish-audio/python/{__version__}User-Agent, and_prepare_request_kwargssensibly layersRequestOptions.additional_headersand any explicitheaderskwarg on top. To reduce duplication, you could delegate the options merge intoget_headersitself, e.g.headers = self.get_headers(request_options.additional_headers if request_options else None), before applying any per-callkwargs["headers"].Also applies to: 80-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/fish_audio_sdk/io.py(1 hunks)src/fish_audio_sdk/websocket.py(2 hunks)src/fishaudio/core/client_wrapper.py(4 hunks)src/fishaudio/resources/tts.py(2 hunks)tests/unit/test_core.py(2 hunks)tests/unit/test_tts_realtime.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/test_tts_realtime.py (2)
src/fishaudio/core/client_wrapper.py (1)
get_headers(63-73)tests/unit/conftest.py (1)
mock_api_key(9-11)
src/fishaudio/resources/tts.py (1)
src/fishaudio/core/client_wrapper.py (1)
get_headers(63-73)
tests/unit/test_core.py (1)
src/fishaudio/core/client_wrapper.py (2)
get_headers(63-73)ClientWrapper(97-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Integration Tests
- GitHub Check: Agent
- GitHub Check: Integration Tests
🔇 Additional comments (3)
tests/unit/test_tts_realtime.py (1)
19-24: Mockedget_headerscorrectly mirrors client wrapper behaviorThe fixtures now return Authorization, a test User-Agent, and merged additional headers, which matches how
ClientWrapper.get_headersis used in the TTS WebSocket paths and keeps these tests aligned with the public API.Also applies to: 35-40
src/fishaudio/resources/tts.py (1)
332-333: WebSocket TTS now uses centralized header constructionUsing
self._client.get_headers({"model": model})for both sync and asyncstream_websocketensures the model header is combined with the standard Authorization and User-Agent headers from the client wrapper, keeping WebSocket behavior consistent with the rest of the HTTP stack and the updated tests.Also applies to: 630-631
src/fishaudio/core/client_wrapper.py (1)
116-116: Client initialization now includes default auth and User-AgentPassing
headers=self.get_headers()when constructing the sync and async HTTP clients ensures that even direct uses of.clientstart from the same Authorization and User-Agent defaults as higher-level wrapper methods and the WebSocket TTS entry points.Also applies to: 188-188
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/test_tts_websocket_integration.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Integration Tests
- GitHub Check: Integration Tests
4184b01 to
9412f24
Compare
Summary by CodeRabbit
Updates
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.