-
Notifications
You must be signed in to change notification settings - Fork 0
fix: prevent LiteLLM model id collisions #1254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Test & Lint Summary
Test files: 174 passed, 22 skipped (196 total) |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks!
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the list parsing—coverage looks good.
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignment looks good—thanks for the follow-up.
|
Tests & Lint Summary:
Lint finished without errors. |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration coverage and sync diagnostics look great—thanks for the comprehensive updates.
Test & Lint Summary
Scope: 5 of 6 workspace projects
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the alias work—just one blocker noted inline about normalizing canonical provider keys.
| if (alias) { | ||
| return alias; | ||
| } | ||
| return trimmed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] For canonical provider keys we need to normalize all inputs to the LiteLLM canonical identifier. Right now, if an operator enters (note the capital A) we fall through the alias map and return the original casing from . That value then propagates to / , so we would POST to LiteLLM instead of the required , and the sync will still fail. Please have return the lower-cased key (e.g. ) so even canonical names are normalized.
|
Re-ran with the provider casing changes:
Lint passed cleanly. Shared provider alias suite: 5 tests passed. Platform UI suite: 78 files, 377 tests passed. LLM settings service suite: 1 file, 29 tests passed. |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canonicalization fix looks solid—thanks for the quick turnaround.
|
Updated shared package exports to expose normalizeLiteLLMProvider as a named ESM export. Re-verified:
Lint passed. Shared suite: 1 file, 5 tests passed. Platform UI suite: 78 files, 377 tests passed. LLM settings service suite: 1 file, 29 tests passed. |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export map looks good—thanks for tightening up the shared package.
Lint & Test
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping the normalization logic server-side. I left two blockers in the UI mapping—alias support dropped, so existing LiteLLM data renders under "Unknown" providers. Once we restore alias-aware normalization there, we should be good.
Lint & Test
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alias handling looks good now—thanks for covering the UI paths with tests.
Lint & Test
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model operations now use LiteLLM identifiers, and the test dialog surfaces responses nicely. Thanks for adding the integration coverage.
|
Lint:
Tests:
|
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the follow-up—most of the flow looks solid. I called out one type-safety issue inline: the model form now sends a status to the dialog while tests run, but the prop signature still excludes that state. That narrows the union and breaks type-checking. Once the prop accepts the full status union (and we optionally reflect pending in the footer copy), we should be set.
packages/platform-ui/src/features/llmSettings/components/ModelFormDialog.tsx
Outdated
Show resolved
Hide resolved
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending state wiring looks good now—thanks for tightening up the messaging.
Lint & Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result view integration works well—the single dialog flow keeps context without spawning overlays. Tests cover the regressions. Nice clean-up.
Lint & Tests
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Streamlined modal flow looks solid—credential-level tests are gone, overrides removed, and the result view layout reads cleanly. Tests cover the new interactions.
Summary
Testing