Openrouter штеупкфешщт

[T0ha]

No description provided.

[claude]

Pull Request Review: OpenRouter Integration

Overview

This PR adds OpenRouter as an alternative AI provider to the existing Gemini implementation. The implementation follows a clean behavior-based architecture pattern and includes comprehensive documentation.

Strengths

Code Quality

• Clean abstraction: Good use of the AIClient behavior to make providers pluggable
• Consistent naming: Successfully refactored ask_gemini to ask_llm throughout the codebase
• Good error handling: Proper handling of different response formats and HTTP status codes
• Logging: Appropriate use of Logger for warnings and errors
• Documentation: Excellent README additions with clear setup instructions

Architecture

• The provider switching mechanism is well-designed and follows Elixir conventions
• Finch pool naming change (Gemini to LLM) is more generic and appropriate

Critical Issues

1. Crash on API errors (lib/bodhi/open_router.ex:54)

When handle_finch_response returns error response body, Jason.decode!() will crash if the body is not valid JSON. Should use Jason.decode/1 and handle the error case.

2. Documentation/Code mismatch

• README says default model is anthropic/claude-3.5-sonnet (line 26)
• Actual code uses deepseek/deepseek-r1-0528:free (line 9)
• This inconsistency will confuse users

3. Config default changed (config/config.exs:84)

The default was changed from Bodhi.Gemini to Bodhi.OpenRouter. This is a BREAKING CHANGE for existing deployments. Should either revert to Bodhi.Gemini as default or document this breaking change in PR description.

4. Pattern matching bug (lib/bodhi/open_router.ex:33-36)

The first pattern only matches when chat_id == user_id. If they are different, it falls through to the second pattern, incorrectly marking the message as assistant.

5. Inconsistent error handling (lib/bodhi/open_router.ex:75-87)

The function returns {:ok, content} or {:error, reason}, but request_openrouter/2 at line 54 uses Jason.decode!() which will raise instead of returning an error tuple. Error handling path is broken.

Security Concerns

6. Hardcoded API token in build (lib/bodhi/open_router.ex:45)

Application.get_env is evaluated at compile time in some contexts. Should use runtime evaluation with a helper function.

7. Missing API key validation

Neither module checks if the API key is present before making requests. This will cause confusing errors at runtime.

Test Coverage

8. Mock not used (test/test_helper.exs:6)

The OpenRouterMock is defined but never configured in config/test.exs. Tests still use GeminiMock.

9. No integration tests

No tests were added for the Bodhi.OpenRouter module itself. Should add unit tests for message preparation, error handling, response parsing, and different status codes.

Minor Issues

10. Typo in PR title: should be integration
11. Magic strings: https://lamabot.io should be a config value
12. Missing nil text handling in messages
13. Undocumented behavior: README does not mention that changing the model requires code changes

Recommendations

High Priority (Fix before merge)

1. Fix the Jason.decode!/1 crash issue
2. Align README documentation with actual default model
3. Fix the build_message/1 pattern matching bug
4. Revert default config to Bodhi.Gemini or document breaking change
5. Add error handling for nil text and missing API keys

Medium Priority

6. Add unit tests for Bodhi.OpenRouter
7. Configure and use OpenRouterMock in tests
8. Move API token retrieval to runtime
9. Add validation for API keys at startup

Low Priority

10. Make model configurable via application config
11. Move hardcoded URLs to config
12. Add typespecs for private functions

Summary

This is a solid foundation for multi-provider AI support with good architectural choices. However, there are several bugs that will cause runtime crashes and one breaking change that needs attention. With the critical fixes, this will be a valuable addition to the codebase.

[claude]

Code Review: OpenRouter Integration

Summary

This PR adds OpenRouter as an alternative AI provider alongside Google Gemini, with proper refactoring to support multiple AI backends. The implementation is solid overall, with good documentation and consistent patterns.

✅ Strengths

1. Well-structured abstraction: The existing AIClient behaviour makes adding new providers clean and maintainable
2. Consistent naming refactor: Renaming ask_gemini → ask_llm properly reflects the provider-agnostic design
3. Good documentation: The README additions are comprehensive and user-friendly
4. Proper error handling: OpenRouter module handles multiple error cases (API errors, unexpected formats)
5. Configuration flexibility: Runtime config properly supports both providers

🐛 Issues Found

Critical

1. Test configuration mismatch (config/test.exs:40)

• Tests still use Bodhi.GeminiMock while production config uses Bodhi.OpenRouter
• This creates a discrepancy where tests don't validate the actual default provider
• Recommendation: Update to use a generic mock or test both providers

High Priority

2. Missing error handling in non-200 responses (lib/bodhi/open_router.ex:70-73)

• Non-200 responses may not be valid JSON, causing crashes in Jason.decode!/1
• Gemini has the same issue (lib/bodhi/gemini.ex:65-68)
• Recommendation: Return {:error, _} tuple for non-200 responses instead of passing body to Jason.decode!

3. Security: API token interpolation (config/runtime.exs:16)

• String interpolation of nil creates "nil" instead of actual nil
• Should be: openrouter_token: System.get_env("OPENROUTER_API_KEY")
• Same issue exists for gemini_token (line 15)

4. Potential crash on missing prompt (lib/bodhi/open_router.ex:23)

• Uses bang function that raises on failure
• No error handling if database is empty
• Recommendation: Add proper error handling or document that prompt must exist

[claude]

Medium Priority Issues (continued)

5. No test coverage for OpenRouter module

• Bodhi.OpenRouterMock is defined but never used
• No tests verify OpenRouter integration works correctly
• Recommendation: Add tests similar to existing Gemini tests, or integration tests for both providers

6. Inconsistent module atom naming (lib/bodhi/application.ex:23)

• Changed from Gemini → LLM but kept as bare atom (not module)
• Consider using Bodhi.LLM or documenting why it's a bare atom
• Not a bug, but could be clearer

7. Unused mock definition (test/test_helper.exs:6)

• Mox.defmock(Bodhi.OpenRouterMock, ...) defined but never referenced in tests
• Either use it or remove it to avoid confusion

💡 Suggestions

Code Quality

1. Consider extracting common HTTP logic: Both Gemini and OpenRouter have similar patterns for HTTP requests and error handling. A shared module could reduce duplication.

2. Environment variable validation: Consider validating that required API keys are present at startup, failing fast if misconfigured.

3. Make model configurable via runtime config: Currently hardcoded as @default_model, but could be runtime configurable like API keys

Documentation

1. API key security note: Consider adding a warning in README about keeping .envrc out of version control

2. Model pricing info: The free tier is great, but worth mentioning in docs that some models have costs

🔒 Security Review

• ✅ No obvious injection vulnerabilities
• ✅ API keys properly externalized to environment variables
• ⚠️ Token interpolation issue mentioned above
• ✅ HTTPS used for all API calls
• ✅ No sensitive data logged (tokens not in logs)

📊 Performance Considerations

• ✅ Finch connection pooling configured appropriately (size: 10)
• ✅ Reuses existing HTTP client infrastructure
• ℹ️ Consider timeout configuration for slow API responses (currently uses Finch defaults)

🎯 Overall Assessment

Recommendation: Approve with requested changes

This is a well-executed feature addition that follows existing patterns and improves the codebase's flexibility. The main concerns are:

1. Fix the error handling for non-200 HTTP responses (prevents crashes)
2. Fix the environment variable interpolation (security/correctness)
3. Update test configuration to match production default
4. Add basic test coverage for OpenRouter

The PR demonstrates good software engineering practices with proper abstraction, documentation, and incremental improvement. Once the critical issues are addressed, this will be a solid addition to the codebase.