feat(amp): add model mapping support for routing unavailable models to alternatives#390
Conversation
…o alternatives - Add AmpModelMapping config to route models like 'claude-opus-4.5' to 'claude-sonnet-4' - Add ModelMapper interface and DefaultModelMapper implementation with hot-reload support - Enhance FallbackHandler to apply model mappings before falling back to ampcode.com - Add structured logging for routing decisions (local provider, mapping, amp credits) - Update config.example.yaml with amp-model-mappings documentation
- Add model mapping feature to README.md Amp CLI section - Add detailed Model Mapping Configuration section to amp-cli-integration.md - Update architecture diagram to show model mapping flow - Update Model Fallback Behavior to include mapping step - Add Table of Contents entry for model mapping
- Store ampModule in Server struct to access it during config updates - Call ampModule.OnConfigUpdated() in UpdateClients() for hot reload - Watch config directory instead of file to handle atomic saves (vim, VSCode, etc.) - Improve config file event detection with basename matching - Add diagnostic logging for config reload tracing
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello @NguyenSiTrung, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature to the Amp CLI integration: model mapping. It allows users to configure rules that automatically redirect requests for specific AI models (that might not be locally available) to alternative models that are. This mechanism helps users leverage their existing local provider subscriptions, reducing reliance on Amp credits and enhancing the proxy's adaptability to varying model availabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust model mapping feature for the Amp CLI integration, allowing requests for unavailable models to be routed to local alternatives. The implementation is thread-safe, supports hot-reloading of configurations, and includes comprehensive unit tests. Key improvements include structured logging for routing decisions and a fix for atomic config file saves, enhancing the overall reliability and maintainability of the system. The documentation has also been thoroughly updated to reflect these new capabilities.
| } else { | ||
| log.Warnf("amp model mapper not initialized, skipping model mapping update") | ||
| } |
There was a problem hiding this comment.
The log.Warnf here suggests that m.modelMapper might be nil when OnConfigUpdated is called. While m.modelMapper is initialized in Register which uses sync.Once, it's important to ensure that Register is always called successfully before OnConfigUpdated can be invoked. If there's an edge case where OnConfigUpdated is triggered before Register completes or if Register fails, this warning would be relevant. Consider if there's a scenario where this could happen in normal operation or during module initialization, and if so, how to prevent it or handle it more gracefully.
| providers = util.GetProviderName(mappedModel) | ||
|
|
||
| // Continue to handler with remapped model | ||
| goto handleRequest |
There was a problem hiding this comment.
The use of goto handleRequest here, while functional, can sometimes make the control flow harder to follow and debug. In Go, goto is often discouraged for general control flow, although it has specific use cases like breaking out of nested loops or jumping to common error handling. For improved readability and maintainability, consider refactoring this logic to avoid goto, perhaps by encapsulating the common request handling in a separate function or restructuring the if/else blocks.
|
@ben-vargas Could you please review this PR when you have a moment? |
PR Review: feat(amp): add model mapping supportGreat work on this feature! The model mapping implementation is solid, well-tested, and the documentation is thorough. I have two minor suggestions to consider: 1. Consider refactoring
|
|
Overall I would generally guide users to use the Ref on |
@ben-vargas Please I will merge it to the dev branch after it is approved. If you feel this PR is unnecessary, please let me know and I can close it(maybe you can close it yourself, I think you have enough permission). |
|
@luispater - It looks like I have access to close it if needed. I'm going to dig into the Amp CLI minified source a bit more and decide whether this is something that makes sense to incorporate into CLIProxyAPI's Amp support or defer to client configuration... will update again later today. |
ben-vargas
left a comment
There was a problem hiding this comment.
Approved
Addressing Review Comments and Prior Concerns
Re: goto suggestion
The goto handleRequest pattern in fallback_handlers.go is a valid design choice here. This could be refactored into a helper function in a future cleanup PR if desired, but it's not a blocker.
Re: log level suggestion for uninitialized mapper warning
After tracing the initialization order, this warning cannot fire in production:
NewServer()callsRegister()synchronously, which initializesmodelMapperviaregisterOnce.Do()OnConfigUpdated()is only called via watcher callbacks after the server is fully created- The warning only appears in unit tests that test
OnConfigUpdated()in isolation without callingRegister()first
This is test noise, not a production concern. Could be addressed in a follow-up if desired.
Re: Feature overlap with amp.internal.model
While amp.internal.model can override the model for "smart" mode, this PR addresses a different use case: Amp CLI modes like Oracle (requires OpenAI) and Librarian (requires Anthropic) are hardcoded to specific providers. Users without those subscriptions currently fall back to Amp credits. This mapping feature allows transparent routing to alternative local providers, which is a valid cost-optimization use case.
|
Hi @ben-vargas , Thanks for your review and very useful comments. However, amp.internal.model still has limitations because the model support list is limited to a specified list, where users may want to use another model for smart mode, which is not possible. In addition, as you have noticed, oracle and librarian cannot configure it. So I think this feature is necessary for user customization. |
feat(amp): add model mapping support for routing unavailable models to alternatives
feat(amp): add model mapping support for routing unavailable models to alternatives
Summary
Add model mapping feature for Amp CLI that allows routing requests for unavailable models to alternative models that are available locally.
Features
amp-model-mappingsconfig to define model routing rules (e.g.,claude-opus-4.5→claude-sonnet-4)Changes
Core Implementation (
2cd5980)AmpModelMappingconfig struct withfromandtofieldsModelMapperinterface andDefaultModelMapperimplementationFallbackHandlerto apply model mappings before falling back to ampcode.comconfig.example.yamlwith documentationDocumentation (
33a5656)Hot Reload Fix (
3409f4e)ampModulein Server struct for config update accessampModule.OnConfigUpdated()during config reloadExample Configuration
Files Changed
internal/api/modules/amp/model_mapping.go- New model mapper implementationinternal/api/modules/amp/model_mapping_test.go- Unit testsinternal/api/modules/amp/fallback_handlers.go- Apply mappings in routinginternal/api/modules/amp/amp.go- Hot reload supportinternal/config/config.go- Config struct additionsinternal/watcher/watcher.go- Fix atomic save detectioninternal/api/server.go- Wire up amp module config updatesdocs/amp-cli-integration.md- Documentationconfig.example.yaml- Example configuration