Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a centralized LLM provider/model normalization (provider alias mapping, lowercasing/trimming, and date-suffix stripping) and updates callers to use it, including adapter cache/key usage and the sync normalizer delegation. Tests added for normalization behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/cost/adapter/adapter.go (1)
179-181: Optional cleanup: extract the repeated provider/model normalization into one helper.This is working correctly; just a small DRY improvement so the normalization call shape stays consistent in one place.
♻️ Suggested refactor
+func normalizeProviderModel(provider, modelID string) (string, string) { + return llmcost.NormalizeModelID(modelID, provider) +} + // getLLMPrices ... - provider, modelID = llmcost.NormalizeModelID(modelID, provider) + provider, modelID = normalizeProviderModel(provider, modelID) // resolveLLMUnitCost ... - provider, modelID = llmcost.NormalizeModelID(modelID, provider) + provider, modelID = normalizeProviderModel(provider, modelID)Also applies to: 280-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/cost/adapter/adapter.go` around lines 179 - 181, Extract the repeated normalization call into a single helper to keep normalization logic DRY: create a helper function (e.g., normalizeProviderModel(provider, modelID) string,string) that internally calls llmcost.NormalizeModelID(modelID, provider) and returns (provider, modelID) in canonical order, then replace the direct calls to llmcost.NormalizeModelID in adapter.go (the occurrences around the current provider, modelID assignment and the similar block at the other site) with calls to normalizeProviderModel so both sites use the same wrapper; ensure the helper name is unique (normalizeProviderModel or NormalizeModelPair) and update any callers to use the new return ordering.openmeter/llmcost/normalize.go (1)
30-54: Small robustness tweak: normalize input insideNormalizeProvidertoo.At Line 30, direct usage like
NormalizeProvider(" Azure ")currently won’t map as expected. Since this is exported, it’s safer if it handles trim/lowercase itself.🛠️ Suggested change
func NormalizeProvider(provider string) string { + provider = strings.ToLower(strings.TrimSpace(provider)) + switch provider { case "openai", "azure", "azure_ai": return "openai"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/normalize.go` around lines 30 - 54, NormalizeProvider should sanitize its input before matching: update the NormalizeProvider function to call strings.TrimSpace and strings.ToLower on the provider parameter (e.g., provider = strings.ToLower(strings.TrimSpace(provider))) so values like " Azure " or "AZURE" map correctly; import the strings package if needed and then use the normalized provider variable in the existing switch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/cost/adapter/adapter.go`:
- Around line 179-181: Extract the repeated normalization call into a single
helper to keep normalization logic DRY: create a helper function (e.g.,
normalizeProviderModel(provider, modelID) string,string) that internally calls
llmcost.NormalizeModelID(modelID, provider) and returns (provider, modelID) in
canonical order, then replace the direct calls to llmcost.NormalizeModelID in
adapter.go (the occurrences around the current provider, modelID assignment and
the similar block at the other site) with calls to normalizeProviderModel so
both sites use the same wrapper; ensure the helper name is unique
(normalizeProviderModel or NormalizeModelPair) and update any callers to use the
new return ordering.
In `@openmeter/llmcost/normalize.go`:
- Around line 30-54: NormalizeProvider should sanitize its input before
matching: update the NormalizeProvider function to call strings.TrimSpace and
strings.ToLower on the provider parameter (e.g., provider =
strings.ToLower(strings.TrimSpace(provider))) so values like " Azure " or
"AZURE" map correctly; import the strings package if needed and then use the
normalized provider variable in the existing switch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d340e3b8-0ca2-47c5-8676-67be868acec4
📒 Files selected for processing (5)
openmeter/cost/adapter/adapter.goopenmeter/llmcost/normalize.goopenmeter/llmcost/normalize_test.goopenmeter/llmcost/sync/normalizer.goopenmeter/llmcost/sync/normalizer_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/llmcost/normalize_test.go (1)
9-81: Add direct tests for exported NormalizeProviderThese tests validate aliases through
NormalizeModelID, butNormalizeProvideris exported and should have its own direct contract tests (especially for case/whitespace behavior if you apply the fix above).As per coding guidelines "`**/*_test.go`: Make sure the tests are comprehensive and cover the changes."Suggested test addition
+func TestNormalizeProviderDirect(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + {"alias", "azure", "openai"}, + {"unknown passthrough", "unknown_provider", "unknown_provider"}, + {"case and trim", " Azure_AI ", "openai"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, NormalizeProvider(tt.input)) + }) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/llmcost/normalize_test.go` around lines 9 - 81, Add direct unit tests for the exported NormalizeProvider function (separate from NormalizeModelID tests) that assert provider alias mapping, case-folding, and whitespace trimming. Specifically create tests calling NormalizeProvider with inputs like "OpenAI", " openai ", "azure", "vertex_ai", "aws", "facebook", "mistralai", and "unknown_provider" and assert the returned provider equals "openai", "openai", "openai", "google", "amazon", "meta", "mistral", and "unknown_provider" respectively; also include a table-driven test for these aliases and a small negative test for preserved unknown providers. Ensure tests reference NormalizeProvider by name and use the same assertion style as existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/llmcost/normalize.go`:
- Around line 30-54: NormalizeProvider currently treats input as case- and
whitespace-sensitive; update it to trim whitespace and lower-case the input
before the switch so calls like NormalizeProvider(" Azure ") map to "openai". At
the top of NormalizeProvider, compute a normalized variable (e.g., p :=
strings.ToLower(strings.TrimSpace(provider))) and use p in the switch cases
while keeping the same canonical return values; import the strings package if
not already present.
---
Nitpick comments:
In `@openmeter/llmcost/normalize_test.go`:
- Around line 9-81: Add direct unit tests for the exported NormalizeProvider
function (separate from NormalizeModelID tests) that assert provider alias
mapping, case-folding, and whitespace trimming. Specifically create tests
calling NormalizeProvider with inputs like "OpenAI", " openai ", "azure",
"vertex_ai", "aws", "facebook", "mistralai", and "unknown_provider" and assert
the returned provider equals "openai", "openai", "openai", "google", "amazon",
"meta", "mistral", and "unknown_provider" respectively; also include a
table-driven test for these aliases and a small negative test for preserved
unknown providers. Ensure tests reference NormalizeProvider by name and use the
same assertion style as existing tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adcb46f0-f60c-441c-ab4a-075f895219a9
📒 Files selected for processing (4)
openmeter/cost/adapter/adapter.goopenmeter/llmcost/normalize.goopenmeter/llmcost/normalize_test.goopenmeter/llmcost/sync/normalizer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/cost/adapter/adapter.go
- openmeter/llmcost/sync/normalizer.go
Comment on lines
+30
to
+54
| func NormalizeProvider(provider string) string { | ||
| switch provider { | ||
| case "openai", "azure", "azure_ai": | ||
| return "openai" | ||
| case "anthropic": | ||
| return "anthropic" | ||
| case "google", "vertex_ai", "gemini": | ||
| return "google" | ||
| case "amazon", "aws", "bedrock": | ||
| return "amazon" | ||
| case "meta", "facebook": | ||
| return "meta" | ||
| case "deepseek": | ||
| return "deepseek" | ||
| case "mistral", "mistralai": | ||
| return "mistral" | ||
| case "cohere": | ||
| return "cohere" | ||
| case "xai": | ||
| return "xai" | ||
| case "minimax": | ||
| return "minimax" | ||
| default: | ||
| return provider | ||
| } |
Contributor
There was a problem hiding this comment.
NormalizeProvider should normalize its own input
Right now NormalizeProvider is exported but case/whitespace-sensitive (e.g. NormalizeProvider(" Azure ") won’t map to openai). That makes direct usage easy to misuse and inconsistent with NormalizeModelID behavior (Line 16-27).
Suggested fix
func NormalizeProvider(provider string) string {
+ provider = strings.ToLower(strings.TrimSpace(provider))
+
switch provider {
case "openai", "azure", "azure_ai":
return "openai"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NormalizeProvider(provider string) string { | |
| switch provider { | |
| case "openai", "azure", "azure_ai": | |
| return "openai" | |
| case "anthropic": | |
| return "anthropic" | |
| case "google", "vertex_ai", "gemini": | |
| return "google" | |
| case "amazon", "aws", "bedrock": | |
| return "amazon" | |
| case "meta", "facebook": | |
| return "meta" | |
| case "deepseek": | |
| return "deepseek" | |
| case "mistral", "mistralai": | |
| return "mistral" | |
| case "cohere": | |
| return "cohere" | |
| case "xai": | |
| return "xai" | |
| case "minimax": | |
| return "minimax" | |
| default: | |
| return provider | |
| } | |
| func NormalizeProvider(provider string) string { | |
| provider = strings.ToLower(strings.TrimSpace(provider)) | |
| switch provider { | |
| case "openai", "azure", "azure_ai": | |
| return "openai" | |
| case "anthropic": | |
| return "anthropic" | |
| case "google", "vertex_ai", "gemini": | |
| return "google" | |
| case "amazon", "aws", "bedrock": | |
| return "amazon" | |
| case "meta", "facebook": | |
| return "meta" | |
| case "deepseek": | |
| return "deepseek" | |
| case "mistral", "mistralai": | |
| return "mistral" | |
| case "cohere": | |
| return "cohere" | |
| case "xai": | |
| return "xai" | |
| case "minimax": | |
| return "minimax" | |
| default: | |
| return provider | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/llmcost/normalize.go` around lines 30 - 54, NormalizeProvider
currently treats input as case- and whitespace-sensitive; update it to trim
whitespace and lower-case the input before the switch so calls like
NormalizeProvider(" Azure ") map to "openai". At the top of NormalizeProvider,
compute a normalized variable (e.g., p :=
strings.ToLower(strings.TrimSpace(provider))) and use p in the switch cases
while keeping the same canonical return values; import the strings package if
not already present.
tothandras
approved these changes
Mar 17, 2026
This was referenced Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Normalize LLM Model IDs to stripe dates.
Summary by CodeRabbit
Improvements
Refactor
Tests