test: add comprehensive unit tests for MimoHandler provider#210
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSanitizes streamed tool-call IDs in MimoHandler and adds tests for createMessage streaming scenarios (multiple tool calls, stream interruption, ID sanitization, system prompt) plus expanded completePrompt edge-case and error handling tests. ChangesMimo Provider Handler Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 856-860: The test currently only asserts that toolChunks[0].id
exists and is a string; update the assertion to validate the sanitized output
itself by checking that the id has had forbidden characters removed (or equals
the expected sanitized value). Locate the array variable toolChunks in the test
in mimo.spec.ts and replace/extend the typeof/id existence checks with
assertions such as: verify toolChunks[0].id does not contain disallowed
characters (e.g., spaces, quotes, slashes) or assert equality against the known
sanitized form for the input used in this test so the sanitization contract is
actually verified.
🪄 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 Plus
Run ID: e470bb67-ed17-4308-a1a9-89578f98ff5e
📒 Files selected for processing (1)
src/api/providers/__tests__/mimo.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
811da32 to
37f5b0a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/api/providers/__tests__/mimo.spec.ts (1)
856-860:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the sanitized ID value, not just string presence.
This test can pass even if ID sanitization is broken, because it only checks existence/type. Assert the expected sanitized output (or forbidden-char removal) to validate the contract.
Proposed assertion update
const toolChunks = chunks.filter((c) => c.type === "tool_call_partial") expect(toolChunks.length).toBeGreaterThan(0) - expect(toolChunks[0].id).toBeDefined() - expect(typeof toolChunks[0].id).toBe("string") + expect(toolChunks[0].id).toBe(sanitizeOpenAiCallId("call_with-special.chars@123")) + expect(toolChunks[0].id).not.toMatch(/[^a-zA-Z0-9_-]/)🤖 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 `@src/api/providers/__tests__/mimo.spec.ts` around lines 856 - 860, The test currently only checks existence/type of toolChunks[0].id which won't catch sanitization regressions; update the assertions to validate the sanitized ID value itself by asserting toolChunks[0].id equals the expected sanitized string (or at minimum matches the allowed-character pattern and contains no forbidden characters). Locate the test variables chunks and toolChunks and replace/extend the two checks on toolChunks[0].id with an assertion that the ID either equals the known sanitized output for the input used in the test or matches a regex like allowed characters (e.g., alphanumerics, dash/underscore) and does not include forbidden chars.
🤖 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.
Duplicate comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 856-860: The test currently only checks existence/type of
toolChunks[0].id which won't catch sanitization regressions; update the
assertions to validate the sanitized ID value itself by asserting
toolChunks[0].id equals the expected sanitized string (or at minimum matches the
allowed-character pattern and contains no forbidden characters). Locate the test
variables chunks and toolChunks and replace/extend the two checks on
toolChunks[0].id with an assertion that the ID either equals the known sanitized
output for the input used in the test or matches a regex like allowed characters
(e.g., alphanumerics, dash/underscore) and does not include forbidden chars.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c35683b9-8b33-4554-8501-724a5b0e9aad
📥 Commits
Reviewing files that changed from the base of the PR and between 811da32 and 37f5b0aaea7cfbc57c9e774d951511f2be8c26e2.
📒 Files selected for processing (1)
src/api/providers/__tests__/mimo.spec.ts
|
Adding context on why this PR now includes a small This was not driven by a MiMo-specific public requirement I found for streamed tool-call IDs. The reason is that the repo already treats OpenAI-compatible tool IDs as needing normalization through MiMo was already opting into that normalization on the request/history side via When I tightened the new test to assert the normalized ID, it failed immediately. That showed the provider was internally inconsistent: sanitized for round-trip message conversion, unsanitized for streaming emission. So the code change here is just making the streaming path match the normalization policy MiMo was already using elsewhere in this provider, rather than introducing a brand-new rule. |
Add 45 unit tests covering the MimoHandler provider: - Constructor: model selection, default model fallback, base URL config - getModel(): model info for v2.5-pro, v2.5, unknown models - completePrompt(): happy path, multi-turn, JSON mode, model override - createMessage() with Anthropic format and custom baseUrl - Edge cases: empty choices, null content, network/rate limit errors - Streaming: multi-tool calls, parallel tools, tool call IDs, interruption - Sanitization: model ID, tool call IDs, prompt caching - convertToR1Format: empty arrays, thinking blocks, nested structures All 45 tests passing.
3d254d5 to
c8b87c6
Compare
Rebased onto main. The completePrompt coverage from the original PR is now provided by the merged Zoo-Code-Org#210, so this focuses on its net-new contribution: 4 advanced streaming resilience tests (multi-chunk text concatenation, interleaved reasoning + tool calls, missing usage in final chunk, and zero-valued cache tokens). 49 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add comprehensive unit tests for MimoHandler provider
Summary
Adds 45 unit tests for
src/api/providers/MimoHandler.ts, covering the complete provider API surface. This was identified as a high-impact contribution opportunity — the provider had 0 tests despite being a production-critical component.Test Coverage
getModel()getModelId()autofallback,openai/prefix handling)completePrompt()— happy pathcompletePrompt()— parameterscompletePrompt()— error handlingcompletePrompt()— edge casesstreamResponse)createMessage()— Anthropic formatcreateMessage()— edge casesconvertToR1Format()finishReason()Why This Matters
Test Approach
@ai-sdk/openai,@ai-sdk-internal/fake-llm, and@roo-code/typesOpenAiNativeHandler.test.tsand other provider test filesChecklist
vitest run src/api/providers/__tests__/mimo.spec.ts)Summary by CodeRabbit
Bug Fixes
Tests