feat(minimax): add native text-to-speech support - #621
Conversation
📝 WalkthroughWalkthroughMiniMax now implements ChangesMiniMax audio support
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 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 `@internal/providers/minimax/audio_test.go`:
- Around line 141-157: Add a test near TestCreateSpeech_ReturnsNativeStatusError
named TestCreateSpeech_ReturnsUpstreamHTTPStatusError that makes the mock server
respond with a non-2xx status such as http.StatusUnauthorized and a JSON error
payload, then call provider.CreateSpeech and assert it returns a non-nil error
containing the upstream status message. Ensure the test exercises the
core.ParseProviderError path for HTTP-level failures.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78ced0da-7807-43f0-9467-7e8151d3624b
📒 Files selected for processing (2)
internal/providers/minimax/audio.gointernal/providers/minimax/audio_test.go
| func TestCreateSpeech_ReturnsNativeStatusError(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"data":null,"base_resp":{"status_code":1004,"status_msg":"invalid voice"}}`)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| provider := NewWithHTTPClient("key", server.URL, server.Client(), llmclient.Hooks{}) | ||
| _, err := provider.CreateSpeech(context.Background(), &core.AudioSpeechRequest{ | ||
| Model: "speech-2.8-hd", | ||
| Input: "hello", | ||
| Voice: "voice-id", | ||
| }) | ||
| if err == nil || !strings.Contains(err.Error(), "invalid voice") { | ||
| t.Fatalf("CreateSpeech() error = %v, want native status message", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the upstream non-2xx HTTP status path.
The existing tests cover a native business error returned with HTTP 200 (TestCreateSpeech_ReturnsNativeStatusError) and malformed hex audio. No test exercises the branch in audio.go that calls core.ParseProviderError when the upstream HTTP status itself is outside 200-299 (audio.go lines 111-113). Add a test where the mock server returns a non-2xx status code (for example 401 or 500) to confirm the error is parsed and surfaced correctly.
As per path instructions, **/*_test.go: "Add or update table-driven tests for behavior changes, covering request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping."
✅ Proposed additional test
func TestCreateSpeech_ReturnsUpstreamHTTPStatusError(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusUnauthorized)
_, _ = w.Write([]byte(`{"base_resp":{"status_code":1004,"status_msg":"invalid api key"}}`))
}))
defer server.Close()
provider := NewWithHTTPClient("key", server.URL, server.Client(), llmclient.Hooks{})
_, err := provider.CreateSpeech(context.Background(), &core.AudioSpeechRequest{
Model: "speech-2.8-hd",
Input: "hello",
Voice: "voice-id",
})
if err == nil {
t.Fatal("CreateSpeech() error = nil, want non-nil for non-2xx upstream status")
}
}🤖 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 `@internal/providers/minimax/audio_test.go` around lines 141 - 157, Add a test
near TestCreateSpeech_ReturnsNativeStatusError named
TestCreateSpeech_ReturnsUpstreamHTTPStatusError that makes the mock server
respond with a non-2xx status such as http.StatusUnauthorized and a JSON error
payload, then call provider.CreateSpeech and assert it returns a non-nil error
containing the upstream status message. Ensure the test exercises the
core.ParseProviderError path for HTTP-level failures.
Source: Path instructions
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Confidence Score: 3/5Not safe to merge until T-Rex findings are addressed. Production-path checks reproduced a local rejection for compatible requests containing Instructions and confirmed that large chunked provider responses are accepted without an application-level limit. T-Rex reproduced 2 failing behaviors at runtime in internal/providers/minimax/audio.go; the change needs fixes before it is safe to merge. Files Needing Attention: internal/providers/minimax/audio.go
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if strings.TrimSpace(req.Instructions) != "" { | ||
| return nil, core.NewInvalidRequestError("minimax speech does not support instructions", nil) | ||
| } |
There was a problem hiding this comment.
An otherwise-valid OpenAI-compatible speech request with a non-empty Instructions value is rejected locally and never reaches MiniMax. MiniMax does not consume this field, so it should be ignored during native request translation rather than causing callers that share speech requests across providers to fail.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Focused MiniMax instructions reproduction source
- A Go harness starts a real local HTTP server, invokes the production MiniMax provider, and records whether the upstream endpoint was called, providing the focused execution probe.
MiniMax speech control request without instructions
- The control run invokes the production provider without Instructions and records a successful simulated MiniMax call, establishing the otherwise-valid baseline.
MiniMax speech request with instructions rejected locally
- The non-empty Instructions run records the local unsupported-instructions error and zero upstream calls, proving MiniMax was not called.
Existing MiniMax instructions validation test
- The repository's focused existing test passes while asserting that Instructions produces the unsupported-instructions validation error, corroborating the observed behavior.
| } | ||
| defer func() { _ = upstream.Body.Close() }() | ||
|
|
||
| responseBody, err := io.ReadAll(upstream.Body) |
There was a problem hiding this comment.
io.ReadAll(upstream.Body) buffers the complete MiniMax response before JSON parsing and hexadecimal audio decoding. A chunked response has no application-level cap, so a large or malicious upstream payload causes several large allocations and can exhaust process memory. Limit the encoded response size before reading it and return an error once the limit is exceeded.
Artifacts
Authored chunked oversized-response harness source
- This harness sends a MiniMax-shaped chunked response without Content-Length through production CreateSpeech and asserts that its entire hex audio payload is accepted and decoded, proving the test exercises the unbounded read path.
1 MiB decoded-audio chunked response execution log
- The production-path harness accepted a 2,097,214-byte chunked JSON response and returned 1,048,576 decoded audio bytes with exit code 0, proving the response is buffered rather than rejected by an explicit cap.
16 MiB decoded-audio chunked response execution log
- The production-path harness accepted a 33,554,494-byte chunked JSON response and returned 16,777,216 decoded audio bytes with exit code 0, proving substantially oversized provider output is accepted without a bound.
Existing native MiniMax speech path execution log
- The existing native endpoint and hex-decode test passed against the same provider package with exit code 0, confirming the focused validation ran in the real MiniMax speech flow.
|
@octo-patch Looks promising! Check out the comments made by AI reviewers. |
|
Merging this one because it's definitely an improvement. Also I'll add a few missing pieces here in the follow-up PR. Thank you for your contribution! Feel free to be a GoModel's sponsor if you like the software! |
3 participants
Reason: Add native MiniMax text-to-speech support through the existing audio route.
Summary
core.AudioProviderfor the synchronous/t2a_v2endpoint.Checks
go test ./internal/providers/minimaxgo vet ./internal/providers/minimaxgolangci-lint run ./internal/providers/minimax/...go test ./internal/...git diff --check origin/main...HEADorigin/main...HEADSummary by CodeRabbit
New Features
Limitations
Tests