Add custom LLM Provider Templates#2242
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughIntroduced custom LLM Provider Templates as a versioned, first-class resource across gateway, platform API, and AI Workspace.
WalkthroughThis PR adds group-version and version-aware LLM provider template handling across gateway, platform API, and portal code paths. Template schemas and default assets add Sequence Diagram(s)Platform API version creation: sequenceDiagram
participant Client
participant LLMHandler
participant LLMProviderTemplateService
participant LLMProviderTemplateRepository
participant Database
Client->>LLMHandler: POST /llm-provider-templates/{id}/versions
LLMHandler->>LLMProviderTemplateService: CreateVersion(...)
LLMProviderTemplateService->>LLMProviderTemplateRepository: CreateNewVersion(template)
LLMProviderTemplateRepository->>Database: insert version row and mark latest
Database-->>LLMProviderTemplateRepository: stored row
LLMProviderTemplateRepository-->>LLMProviderTemplateService: version record
LLMProviderTemplateService-->>LLMHandler: created version
LLMHandler-->>Client: 201 Created
Portal template version selection: sequenceDiagram
participant User
participant ServiceProviderNew
participant providerTemplateApis
participant LLMHandler
participant TemplateVersionDialog
User->>ServiceProviderNew: select template
ServiceProviderNew->>providerTemplateApis: getProviderTemplateVersions(templateId,...)
providerTemplateApis->>LLMHandler: GET /llm-provider-templates/{id}/versions
LLMHandler-->>providerTemplateApis: version list
providerTemplateApis-->>ServiceProviderNew: enabled versions
ServiceProviderNew->>TemplateVersionDialog: open picker
User->>TemplateVersionDialog: choose version
TemplateVersionDialog-->>ServiceProviderNew: selected version
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
|
@CodeRabbit please review |
|
✅ Action performedReview finished.
|
|
@CodeRabbit please review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/src/internal/service/llm.go (1)
445-493:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCreate providers from the requested enabled template version.
This path still resolves
req.TemplatewithGetByID, so provider creation always binds to the latest template UUID/OpenAPI fallback. With template version selection andenabled, non-latest selections are ignored and disabled latest versions remain usable through the API. Resolve the requested template version withGetByVersionand reject!tpl.Enabledbefore building the provider.🤖 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 `@platform-api/src/internal/service/llm.go` around lines 445 - 493, The template retrieval in the provider creation logic currently uses GetByID which always resolves to the latest template version, ignoring version selection. Replace the GetByID calls with GetByVersion to respect the requested template version from req.Template, and add a validation check after template retrieval to ensure tpl.Enabled is true before proceeding with provider creation, returning an appropriate error if the template is disabled. This ensures provider creation uses the exact requested template version and only enabled templates are usable.portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx (1)
82-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVersion switches do not reliably refresh template-derived defaults.
Line 83 compares only template ID. When users pick another version of the same template, ID is unchanged, so upstream/spec defaults may not refresh for the selected version.
Suggested fix
- const lastTemplateIdRef = useRef<string | null>(null); + const lastTemplateKeyRef = useRef<string | null>(null); @@ - const currentTemplateId = template?.id ?? null; - const templateChanged = lastTemplateIdRef.current !== currentTemplateId; - lastTemplateIdRef.current = currentTemplateId; + const currentTemplateKey = template + ? `${template.id ?? ''}:${template.version ?? ''}` + : null; + const templateChanged = lastTemplateKeyRef.current !== currentTemplateKey; + lastTemplateKeyRef.current = currentTemplateKey;Also applies to: 101-109
🤖 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 `@portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx` around lines 82 - 90, The template change detection in ServiceProviderNew.tsx only compares the template ID, but when users select a different version of the same template, the ID remains unchanged while the version and its metadata (like endpointUrl and spec) change. Update the templateChanged comparison logic to include the template version in addition to the template ID. Instead of only comparing currentTemplateId, create a composite key or comparison that includes both the template ID and version (or another version identifier) to ensure that version switches are detected as changes. Apply the same fix to all locations where this pattern appears, including the affected lines 101-109.
🟡 Minor comments (6)
gateway/gateway-controller/pkg/storage/memory.go-370-372 (1)
370-372:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake latest-template selection deterministic when timestamps are equal.
When two templates share the same
CreatedAt, map iteration order decides the winner, so latest-by-handle can vary across runs. Add a stable tie-breaker (for example, UUID).Proposed fix
- if latestID == "" || !t.CreatedAt.Before(latestAt) { + if latestID == "" || + t.CreatedAt.After(latestAt) || + (t.CreatedAt.Equal(latestAt) && id > latestID) { latestID = id latestAt = t.CreatedAt }🤖 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 `@gateway/gateway-controller/pkg/storage/memory.go` around lines 370 - 372, In the latest template selection logic within the condition at line 370-372, the tie-breaker for templates with equal CreatedAt timestamps is non-deterministic due to map iteration order. Modify the condition to add a stable secondary sort criteria: when two templates have the same CreatedAt timestamp, use the id (UUID) as a tie-breaker by comparing it lexicographically to ensure deterministic selection. Update the conditional logic to keep the current behavior when timestamps differ, but add an additional check for when timestamps are equal to compare the string IDs and select the one that comes first lexicographically (or use your preferred ordering).gateway/gateway-controller/pkg/utils/llm_deployment.go-646-649 (1)
646-649:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the DB-conflict fallback message version-aware.
Conflict detection now uses
(handle, version), but the fallback error returned afterSaveLLMProviderTemplatefailure still reports handle-only text, which makes conflict responses inconsistent across paths.Suggested fix
if err := s.db.SaveLLMProviderTemplate(stored); err != nil { if storage.IsConflictError(err) || strings.Contains(err.Error(), "already exists") { - return nil, fmt.Errorf("%w: template with handle '%s' already exists", storage.ErrConflict, tmpl.Metadata.Name) + return nil, fmt.Errorf( + "%w: template with handle '%s' and version '%s' already exists", + storage.ErrConflict, + stored.GetHandle(), + stored.GetVersion(), + ) } return nil, fmt.Errorf("failed to save template to database: %w", err) }🤖 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 `@gateway/gateway-controller/pkg/utils/llm_deployment.go` around lines 646 - 649, The validateTemplateConflict method now performs version-aware conflict detection using both handle and version parameters via stored.GetHandle() and stored.GetVersion(), but the fallback error message returned after SaveLLMProviderTemplate fails still only includes the handle in its conflict message. Update the error message that gets returned as a fallback after SaveLLMProviderTemplate failure to include both the handle and version information, ensuring consistency with the new version-aware conflict detection approach used in validateTemplateConflict.platform-api/src/internal/service/llm.go-345-371 (1)
345-371:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed version strings consistently.
GetVersion,SetVersionEnabled, andDeleteVersionpass malformed non-empty versions to the repository and return not-found. ReturnErrInvalidInputwhennormalizeTemplateVersionfails so these endpoints match create-version validation.Proposed fix
func (s *LLMProviderTemplateService) GetVersion(orgUUID, handle, version string) (*api.LLMProviderTemplate, error) { - v := strings.TrimSpace(version) - if handle == "" || v == "" { + v, ok := normalizeTemplateVersion(version) + if handle == "" || !ok { return nil, constants.ErrInvalidInput } - if normalized, ok := normalizeTemplateVersion(v); ok { - v = normalized - } m, err := s.repo.GetByVersion(handle, orgUUID, v) @@ func (s *LLMProviderTemplateService) SetVersionEnabled(orgUUID, handle, version string, enabled bool) (*api.LLMProviderTemplate, error) { - v := strings.TrimSpace(version) - if handle == "" || v == "" { + v, ok := normalizeTemplateVersion(version) + if handle == "" || !ok { return nil, constants.ErrInvalidInput } - if normalized, ok := normalizeTemplateVersion(v); ok { - v = normalized - } if err := s.repo.SetEnabled(handle, orgUUID, v, enabled); err != nil { @@ func (s *LLMProviderTemplateService) DeleteVersion(orgUUID, handle, version string) error { - v := strings.TrimSpace(version) - if handle == "" || v == "" { + v, ok := normalizeTemplateVersion(version) + if handle == "" || !ok { return constants.ErrInvalidInput } - if normalized, ok := normalizeTemplateVersion(v); ok { - v = normalized - } if err := s.repo.DeleteVersion(handle, orgUUID, v); err != nil {Also applies to: 401-409
🤖 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 `@platform-api/src/internal/service/llm.go` around lines 345 - 371, The GetVersion, SetVersionEnabled, and DeleteVersion methods are not rejecting malformed version strings consistently. When normalizeTemplateVersion returns false (indicating the version string is malformed), these methods continue processing and eventually return a not-found error instead of rejecting the invalid input. Fix this by checking the ok boolean returned from normalizeTemplateVersion in all three methods, and when ok is false, immediately return constants.ErrInvalidInput rather than proceeding with the unvalidated version string. This will make the version validation behavior consistent across all endpoints.portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsx-198-201 (1)
198-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen endpoint URL validation to enforce URL format.
Line 199 currently treats any non-empty string as valid. This allows malformed endpoint values through submission.
Suggested fix
+function isValidHttpUrl(value: string): boolean { + const raw = value.trim(); + if (!raw) return false; + try { + const u = new URL(raw); + return u.protocol === 'http:' || u.protocol === 'https:'; + } catch { + return false; + } +} @@ - const isEndpointValid = endpointUrl.trim().length > 0; + const isEndpointValid = isValidHttpUrl(endpointUrl);Also applies to: 437-443
🤖 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 `@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsx` around lines 198 - 201, The isEndpointValid variable currently only validates that the endpointUrl is non-empty after trimming, but does not enforce proper URL format validation. Replace the simple length check with a proper URL validation mechanism that verifies the endpoint is a well-formed URL (such as using URL constructor validation or a URL pattern regex test similar to how VERSION_PATTERN is used for version validation). Apply this same validation fix to the other location mentioned at lines 437-443 where similar endpoint validation occurs.portals/ai-workspace/src/utils/providerTemplateManifest.ts-63-76 (1)
63-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire a defined template ID before manifest generation.
Line 66 and Line 75 use
t.iddirectly even thoughProviderTemplate.idis optional. Ifidis missing, both the manifestmetadata.nameand download filename become invalid.Suggested fix
-export function buildTemplateManifestYaml(t: ProviderTemplate): string { +type IdentifiedProviderTemplate = ProviderTemplate & { id: string }; + +export function buildTemplateManifestYaml(t: IdentifiedProviderTemplate): string { @@ -export function templateManifestFileName(t: ProviderTemplate): string { +export function templateManifestFileName(t: IdentifiedProviderTemplate): string { @@ -export function downloadTemplateYaml(t: ProviderTemplate): string { +export function downloadTemplateYaml(t: IdentifiedProviderTemplate): string {🤖 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 `@portals/ai-workspace/src/utils/providerTemplateManifest.ts` around lines 63 - 76, The code uses t.id directly in two places without validating it exists first: in the manifest object's metadata.name property and in the templateManifestFileName function's return statement. Since ProviderTemplate.id is optional, add validation to ensure t.id is defined before using it in both locations. Either throw an error with a clear message if id is missing, or guard against it being undefined. Make sure this validation happens early in both the manifest creation logic and the templateManifestFileName function to prevent generating invalid manifest names and filenames.portals/ai-workspace/src/pages/appShell/AppSidebar.tsx-298-318 (1)
298-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisable footer navigation while intro lock is active.
When
showIntrois true, the main nav is intentionally non-interactive, but the footer remains clickable. This creates inconsistent onboarding behavior.Suggested fix
<Sidebar.Footer> - <Sidebar.Category> + <Box + style={ + showIntro ? { opacity: 0.25, pointerEvents: 'none' } : undefined + } + > + <Sidebar.Category> {hasPermission(SCOPES.LLM_TEMPLATE_MANAGE) && ( <NavLink to={settingsPath} style={navLinkStyle}> <Sidebar.Item id="settings"> @@ </Sidebar.Item> </NavLink> )} @@ - </Sidebar.Category> + </Sidebar.Category> + </Box> </Sidebar.Footer>🤖 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 `@portals/ai-workspace/src/pages/appShell/AppSidebar.tsx` around lines 298 - 318, The Sidebar.Footer with the settings navigation link remains interactive even when the intro lock (showIntro) is active, creating inconsistent onboarding behavior. Modify the Sidebar.Footer and its contents (the NavLink to settingsPath and the Settings Sidebar.Item) to be disabled or non-interactive when showIntro is true, similar to how the main navigation is handled. This can be done by either conditionally rendering the footer contents only when showIntro is false, or by adding appropriate disabled state/styling and onClick prevention when showIntro is true.
🤖 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 `@gateway/gateway-controller/pkg/storage/memory.go`:
- Around line 41-47: The templateIdByHandleVersion map currently uses a
string-joined key format (handle + "@" + version) which can be ambiguous when
handle or version values contain the "@" delimiter, potentially causing
incorrect lookups and breaking uniqueness guarantees. Replace this string
concatenation approach with a typed composite key structure (such as a struct
combining handle and version fields) or implement nested maps (for example, a
map of handles to maps of versions). This same refactoring should also be
applied to any other locations where this ambiguous key pattern is used for
handle-version indexing throughout the file.
In `@platform-api/src/api/generated.go`:
- Around line 4686-4689: The types GatewayStatusResponse,
RESTAPIValidationResponse, UnpublishFromDevPortalRequest, and
ValidateRESTAPIParams are manually restored in the generated.go file because
they are dropped by oapi-codegen during generation. This makes the output
non-reproducible. Create a new hand-written compatibility file in the same
package to define these types permanently, and remove them from generated.go.
When extracting these types, ensure you handle the dependency where
GatewayStatusResponse is referenced in GatewayStatusListResponse by keeping that
reference intact and properly importing from the new compatibility file if
needed.
In `@platform-api/src/internal/database/schema.postgres.sql`:
- Around line 313-318: The current unique constraint on (organization_uuid,
handle, version) prevents duplicate versions but does not enforce that only one
row per (organization_uuid, handle) pair can have is_latest set to TRUE. Add a
partial unique index after the table definition that ensures only one latest
version exists per organization and handle combination. Create a unique index
that covers the columns organization_uuid and handle but applies only to rows
where is_latest equals TRUE. This will guarantee that at most one row with
is_latest = TRUE exists for each (organization_uuid, handle) pair, maintaining
data consistency when promoting new versions.
In `@platform-api/src/internal/repository/llm.go`:
- Around line 306-314: The `List` method in `LLMProviderTemplateRepo` filters
for `is_latest = true`, returning only the latest version of templates. When the
seeder sync loop in `llm_template_seeder.go` uses `List` and `Update` methods to
sync built-in templates, it can overwrite user-created latest versions instead
of updating the base built-in version. Add a version or UUID-specific query and
update path in the repository for the seeder to explicitly target the base
built-in version of templates during sync, or modify the seeder to query for and
operate on the base version constraint when retrieving and updating built-in
template data.
- Around line 123-171: The `CreateNewVersion` function has a concurrency
vulnerability where multiple concurrent transactions could both read the same
latest record before either demotes it, violating the single-latest invariant.
Add row-level locking to the initial SELECT query that checks total count,
sameVersion, and createdBy by appending `FOR UPDATE` to the SQL statement before
scanning. This will lock the relevant rows in the transaction, preventing
concurrent modifications and ensuring that only one `is_latest = true` record
exists per organization and handle pair.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 9956-9962: The `enabled` field in the LLMProviderTemplate schema
is currently writable in POST/PUT request bodies, but the actual way to modify
it is through the PATCH `/llm-provider-templates/{id}/versions/{version}`
endpoint. To align the contract with the actual behavior, mark the `enabled`
field as `readOnly: true` in the LLMProviderTemplate schema definition. This
will indicate that the field cannot be set during creation or updates via
POST/PUT, and can only be modified through the documented PATCH endpoint for
versions.
- Around line 2920-2925: The requestBody schema for this endpoint is incorrectly
referencing the generic LLMProviderTemplate schema, which has conflicting field
requirements compared to what the endpoint actually needs. Create a new
version-specific request schema in the components/schemas section (for example,
CreateLLMProviderTemplateVersionRequest) that properly defines id as optional
and version as required. Update the $ref in the requestBody to point to this new
version-specific schema instead of the generic LLMProviderTemplate, ensuring
that generated clients and validators enforce the correct contract matching the
handler/service behavior.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsx`:
- Around line 195-201: The toTemplateId function can return an empty string when
the input contains only punctuation or non-alphanumeric characters, but the
isFormValid validation does not account for this case, allowing submission of
invalid template IDs. Update the form validation logic to ensure that
toTemplateId(name) produces a non-empty string before allowing the form to be
valid. Add a check in the isFormValid validation (or wherever the validation
logic is defined across the mentioned line ranges) to verify that the normalized
template ID is not empty after applying the toTemplateId transformation.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsx`:
- Around line 152-173: The fetchSpecFromUrl function lacks validation to ensure
the fetched content is actually a valid OpenAPI specification before processing
it. Add validation logic to check that the fetched content contains the required
OpenAPI structure (such as openapi or swagger fields) before accepting it. If
the content fails validation, show an error message via showSnackbar indicating
the fetched content is not a valid OpenAPI spec, and avoid setting the endpoint
URL or spec content. This validation should occur before calling
parseSpecServerUrl and setting any state values.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsx`:
- Around line 233-260: The handleSwitchVersion function updates selectedVersion
state before attempting to fetch the template data, which means if the fetch
fails, the UI will display a selected version that doesn't match the actual
template data being shown. Move the setSelectedVersion(version) call from its
current position at the beginning to after the successful data fetch,
specifically after the setTemplate(full) call inside the try block. This ensures
the selected version only changes when the new template data has been
successfully loaded.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsx`:
- Around line 293-306: The "Create" button in the ProviderTemplatesList
component is gated by the condition `templates.length > 0`, which only checks
for custom templates. This prevents the button from displaying when users only
have built-in templates available, blocking template creation. Modify the
condition in the Button component's rendering logic to show the "Create" button
whenever templates are available, whether they are built-in or custom templates.
This may require checking a boolean flag or prop that indicates whether built-in
templates exist in addition to checking the custom templates length.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/TemplateTokenMapping.tsx`:
- Around line 155-170: The resource identity used in toggleOverride, mappingFor,
and isOverridden functions is inconsistent with the rendering logic. These
functions use only the path parameter as the resource identifier, but the
rendering appears to use method-path combinations (as noted in lines 243-246).
This causes multiple method rows for the same path to incorrectly share a single
override entry. Update the resource identity to be consistent across both the
override state management functions and the rendering logic by either using only
path throughout (deduplicating rows by path level) or including method
information in the resource identifier consistently for both
mappingFor/toggleOverride operations and row rendering.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/TemplateVersionDialog.tsx`:
- Around line 63-88: In the useEffect hook that fetches provider template
versions, the selected version state is not being reset when the fetch operation
starts or fails, which can result in stale data being used. Add a call to
setSelected with an empty string right after setting setIsLoading and setError
to null at the beginning of the effect to clear any previous selection.
Additionally, in the catch block where errors are handled, also call setSelected
with an empty string to ensure no stale version is retained when the fetch
fails.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx`:
- Around line 433-445: The condition checking enabledVersions.length <= 1 treats
the case of zero enabled versions the same as one enabled version, causing the
code to fall back to template.version even when no versions are actually
enabled. Replace the <= 1 condition with === 1 to only apply the template
selection when there is exactly one enabled version, and add an explicit else
block or separate condition to handle the case when enabledVersions.length === 0
(no enabled versions) appropriately, such as showing an error or disabling the
action.
- Around line 351-353: The provider create payload is including a
templateVersion field by conditionally spreading selectedTemplateVersion into
the request object, but the backend LLMProvider model does not support template
versioning during provider creation. Remove the ternary operator that
conditionally adds templateVersion: selectedTemplateVersion to the payload,
keeping only the fields that the backend actually supports for provider
creation.
---
Outside diff comments:
In `@platform-api/src/internal/service/llm.go`:
- Around line 445-493: The template retrieval in the provider creation logic
currently uses GetByID which always resolves to the latest template version,
ignoring version selection. Replace the GetByID calls with GetByVersion to
respect the requested template version from req.Template, and add a validation
check after template retrieval to ensure tpl.Enabled is true before proceeding
with provider creation, returning an appropriate error if the template is
disabled. This ensures provider creation uses the exact requested template
version and only enabled templates are usable.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx`:
- Around line 82-90: The template change detection in ServiceProviderNew.tsx
only compares the template ID, but when users select a different version of the
same template, the ID remains unchanged while the version and its metadata (like
endpointUrl and spec) change. Update the templateChanged comparison logic to
include the template version in addition to the template ID. Instead of only
comparing currentTemplateId, create a composite key or comparison that includes
both the template ID and version (or another version identifier) to ensure that
version switches are detected as changes. Apply the same fix to all locations
where this pattern appears, including the affected lines 101-109.
---
Minor comments:
In `@gateway/gateway-controller/pkg/storage/memory.go`:
- Around line 370-372: In the latest template selection logic within the
condition at line 370-372, the tie-breaker for templates with equal CreatedAt
timestamps is non-deterministic due to map iteration order. Modify the condition
to add a stable secondary sort criteria: when two templates have the same
CreatedAt timestamp, use the id (UUID) as a tie-breaker by comparing it
lexicographically to ensure deterministic selection. Update the conditional
logic to keep the current behavior when timestamps differ, but add an additional
check for when timestamps are equal to compare the string IDs and select the one
that comes first lexicographically (or use your preferred ordering).
In `@gateway/gateway-controller/pkg/utils/llm_deployment.go`:
- Around line 646-649: The validateTemplateConflict method now performs
version-aware conflict detection using both handle and version parameters via
stored.GetHandle() and stored.GetVersion(), but the fallback error message
returned after SaveLLMProviderTemplate fails still only includes the handle in
its conflict message. Update the error message that gets returned as a fallback
after SaveLLMProviderTemplate failure to include both the handle and version
information, ensuring consistency with the new version-aware conflict detection
approach used in validateTemplateConflict.
In `@platform-api/src/internal/service/llm.go`:
- Around line 345-371: The GetVersion, SetVersionEnabled, and DeleteVersion
methods are not rejecting malformed version strings consistently. When
normalizeTemplateVersion returns false (indicating the version string is
malformed), these methods continue processing and eventually return a not-found
error instead of rejecting the invalid input. Fix this by checking the ok
boolean returned from normalizeTemplateVersion in all three methods, and when ok
is false, immediately return constants.ErrInvalidInput rather than proceeding
with the unvalidated version string. This will make the version validation
behavior consistent across all endpoints.
In
`@portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsx`:
- Around line 198-201: The isEndpointValid variable currently only validates
that the endpointUrl is non-empty after trimming, but does not enforce proper
URL format validation. Replace the simple length check with a proper URL
validation mechanism that verifies the endpoint is a well-formed URL (such as
using URL constructor validation or a URL pattern regex test similar to how
VERSION_PATTERN is used for version validation). Apply this same validation fix
to the other location mentioned at lines 437-443 where similar endpoint
validation occurs.
In `@portals/ai-workspace/src/pages/appShell/AppSidebar.tsx`:
- Around line 298-318: The Sidebar.Footer with the settings navigation link
remains interactive even when the intro lock (showIntro) is active, creating
inconsistent onboarding behavior. Modify the Sidebar.Footer and its contents
(the NavLink to settingsPath and the Settings Sidebar.Item) to be disabled or
non-interactive when showIntro is true, similar to how the main navigation is
handled. This can be done by either conditionally rendering the footer contents
only when showIntro is false, or by adding appropriate disabled state/styling
and onClick prevention when showIntro is true.
In `@portals/ai-workspace/src/utils/providerTemplateManifest.ts`:
- Around line 63-76: The code uses t.id directly in two places without
validating it exists first: in the manifest object's metadata.name property and
in the templateManifestFileName function's return statement. Since
ProviderTemplate.id is optional, add validation to ensure t.id is defined before
using it in both locations. Either throw an error with a clear message if id is
missing, or guard against it being undefined. Make sure this validation happens
early in both the manifest creation logic and the templateManifestFileName
function to prevent generating invalid manifest names and filenames.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67a3c5a3-6fae-403d-955e-cc0628e972eb
📒 Files selected for processing (60)
gateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/default-llm-provider-templates/anthropic-template.yamlgateway/gateway-controller/default-llm-provider-templates/awsbedrock-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureaifoundry-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureopenai-template.yamlgateway/gateway-controller/default-llm-provider-templates/gemini-template.yamlgateway/gateway-controller/default-llm-provider-templates/mistral-template.yamlgateway/gateway-controller/default-llm-provider-templates/openai-template.yamlgateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/models/llm_provider_template.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/utils/llm_deployment.gogateway/gateway-controller/pkg/utils/llm_deployment_test.goplatform-api/src/api/generated.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/llm.goplatform-api/src/internal/model/llm.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_template_seeder.goplatform-api/src/internal/utils/llm_provider_template_loader.goplatform-api/src/resources/default-llm-provider-templates/anthropic-template.yamlplatform-api/src/resources/default-llm-provider-templates/awsbedrock-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureaifoundry-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureopenai-template.yamlplatform-api/src/resources/default-llm-provider-templates/gemini-template.yamlplatform-api/src/resources/default-llm-provider-templates/mistral-template.yamlplatform-api/src/resources/default-llm-provider-templates/openai-template.yamlplatform-api/src/resources/openapi.yamlportals/ai-workspace/src/App.tsxportals/ai-workspace/src/apis/providerTemplateApis.tsportals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplateContext.tsxportals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplatesContext.tsxportals/ai-workspace/src/pages/appShell/AppSidebar.tsxportals/ai-workspace/src/pages/appShell/appShellMain.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/EditProviderTemplate.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/TemplateTokenMapping.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyProviderTab.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/ProviderTemplateSelector.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/TemplateVersionDialog.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ProvidersList.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProvidersSummaryCard.tsxportals/ai-workspace/src/pages/appShell/appShellPages/settings/Main.tsxportals/ai-workspace/src/utils/providerTemplateDisplay.tsportals/ai-workspace/src/utils/providerTemplateFields.tsportals/ai-workspace/src/utils/providerTemplateManifest.tsportals/ai-workspace/src/utils/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
platform-api/src/internal/database/schema.sqlite.sql (1)
301-319: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce one latest version per template family.
UNIQUE(organization_uuid, base_handle, version)prevents duplicate versions, but multiple rows with the same(organization_uuid, base_handle)pair can haveis_latest = 1. Add a partial unique index:CREATE UNIQUE INDEX IF NOT EXISTS llm_provider_templates_one_latest_per_base ON llm_provider_templates (organization_uuid, base_handle) WHERE is_latest = 1;🤖 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 `@platform-api/src/internal/database/schema.sqlite.sql` around lines 301 - 319, The llm_provider_templates table currently allows multiple rows with the same (organization_uuid, base_handle) pair to have is_latest = 1, which violates the requirement of having only one latest version per template family. Add a partial unique index to the table creation that enforces this constraint. Create a unique index named llm_provider_templates_one_latest_per_base on the (organization_uuid, base_handle) columns with a WHERE clause that only applies the uniqueness constraint when is_latest equals 1, ensuring only one row per template family can be marked as the latest version within an organization.platform-api/src/internal/database/schema.sql (1)
298-316: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce one latest version per template family.
UNIQUE(organization_uuid, base_handle, version)prevents duplicate versions, but multiple rows with the same(organization_uuid, base_handle)pair can haveis_latest = TRUE. Add a unique index to ensure only one latest version exists per family (syntax may vary by database dialect).🤖 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 `@platform-api/src/internal/database/schema.sql` around lines 298 - 316, The current UNIQUE constraint on (organization_uuid, base_handle, version) in the llm_provider_templates table prevents duplicate versions but does not enforce that only one row per template family can have is_latest set to TRUE. Add a unique index on the (organization_uuid, base_handle) columns with a WHERE clause filtering for is_latest = TRUE to ensure only one latest version exists per template family. This filtered unique index will prevent multiple rows with the same organization and base_handle from having is_latest = TRUE simultaneously while allowing multiple versions with is_latest = FALSE.platform-api/src/internal/database/schema.postgres.sql (1)
303-321: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce one latest version per template family.
UNIQUE(organization_uuid, base_handle, version)prevents duplicate versions, but multiple rows with the same(organization_uuid, base_handle)pair can haveis_latest = TRUE. Add a partial unique index to ensure only one latest version exists per family:CREATE UNIQUE INDEX IF NOT EXISTS llm_provider_templates_one_latest_per_base ON llm_provider_templates (organization_uuid, base_handle) WHERE is_latest;This prevents the race condition in
CreateNewVersionwhere concurrent transactions could both promote their version to latest.🤖 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 `@platform-api/src/internal/database/schema.postgres.sql` around lines 303 - 321, After the CREATE TABLE statement for llm_provider_templates, add a CREATE UNIQUE INDEX statement on the columns (organization_uuid, base_handle) with a WHERE clause filtering for is_latest = TRUE. This partial unique index ensures that only one row per template family (identified by organization_uuid and base_handle pair) can have is_latest set to TRUE, preventing the race condition where concurrent transactions could both promote their version to latest.platform-api/src/internal/repository/llm.go (1)
106-179: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd row-level locking to prevent latest-version race condition.
CreateNewVersiondemotes the current latest and inserts a new latest version in a transaction, but withoutFOR UPDATE, concurrent calls can read the same state before either updates, violating the single-latest invariant. Add row-level locking to the SELECT at lines 130-135:SELECT (SELECT COUNT(*) FROM llm_provider_templates WHERE base_handle = ? AND organization_uuid = ? FOR UPDATE), (SELECT COUNT(*) FROM llm_provider_templates WHERE base_handle = ? AND organization_uuid = ? AND version = ?), (SELECT created_by FROM llm_provider_templates WHERE base_handle = ? AND organization_uuid = ? AND is_latest = ? LIMIT 1 FOR UPDATE)Note: The FOR UPDATE syntax may need adjustment depending on your SQL dialect and how multiple subqueries are locked.
🤖 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 `@platform-api/src/internal/repository/llm.go` around lines 106 - 179, The CreateNewVersion method has a race condition where concurrent transactions can read the same "latest" version state before either demotes it, violating the single-latest invariant. Add row-level locking (FOR UPDATE) to the QueryRow statement that selects the total count, sameVersion count, and createdBy value from the llm_provider_templates table to prevent other concurrent transactions from reading this state while the current transaction is still in progress. Ensure the FOR UPDATE clause is applied to the subqueries appropriately for your SQL dialect so that rows remain locked until the transaction commits.
🤖 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 `@portals/ai-workspace/src/utils/providerTemplateManifest.ts`:
- Around line 72-75: The templateManifestFileName function uses t.id directly in
the filename without checking if it is undefined, which would result in
"undefined-template.yaml" as the filename. Add a guard at the start of the
templateManifestFileName function to handle the case when t.id is undefined or
empty, either by throwing an error with a descriptive message or by providing a
sensible default value. This ensures that only valid filenames are generated.
---
Duplicate comments:
In `@platform-api/src/internal/database/schema.postgres.sql`:
- Around line 303-321: After the CREATE TABLE statement for
llm_provider_templates, add a CREATE UNIQUE INDEX statement on the columns
(organization_uuid, base_handle) with a WHERE clause filtering for is_latest =
TRUE. This partial unique index ensures that only one row per template family
(identified by organization_uuid and base_handle pair) can have is_latest set to
TRUE, preventing the race condition where concurrent transactions could both
promote their version to latest.
In `@platform-api/src/internal/database/schema.sql`:
- Around line 298-316: The current UNIQUE constraint on (organization_uuid,
base_handle, version) in the llm_provider_templates table prevents duplicate
versions but does not enforce that only one row per template family can have
is_latest set to TRUE. Add a unique index on the (organization_uuid,
base_handle) columns with a WHERE clause filtering for is_latest = TRUE to
ensure only one latest version exists per template family. This filtered unique
index will prevent multiple rows with the same organization and base_handle from
having is_latest = TRUE simultaneously while allowing multiple versions with
is_latest = FALSE.
In `@platform-api/src/internal/database/schema.sqlite.sql`:
- Around line 301-319: The llm_provider_templates table currently allows
multiple rows with the same (organization_uuid, base_handle) pair to have
is_latest = 1, which violates the requirement of having only one latest version
per template family. Add a partial unique index to the table creation that
enforces this constraint. Create a unique index named
llm_provider_templates_one_latest_per_base on the (organization_uuid,
base_handle) columns with a WHERE clause that only applies the uniqueness
constraint when is_latest equals 1, ensuring only one row per template family
can be marked as the latest version within an organization.
In `@platform-api/src/internal/repository/llm.go`:
- Around line 106-179: The CreateNewVersion method has a race condition where
concurrent transactions can read the same "latest" version state before either
demotes it, violating the single-latest invariant. Add row-level locking (FOR
UPDATE) to the QueryRow statement that selects the total count, sameVersion
count, and createdBy value from the llm_provider_templates table to prevent
other concurrent transactions from reading this state while the current
transaction is still in progress. Ensure the FOR UPDATE clause is applied to the
subqueries appropriately for your SQL dialect so that rows remain locked until
the transaction commits.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e7da108-968a-413f-9146-eabd428bcb4c
📒 Files selected for processing (39)
docs/rest-apis/gateway/llm-provider-template-management.mdgateway/gateway-controller/pkg/storage/memory.goplatform-api/src/api/compat.goplatform-api/src/api/generated.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/handler/llm.goplatform-api/src/internal/model/llm.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_template_seeder.goplatform-api/src/internal/utils/llm_provider_template_loader.goplatform-api/src/resources/default-llm-provider-templates/anthropic-template.yamlplatform-api/src/resources/default-llm-provider-templates/awsbedrock-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureaifoundry-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureopenai-template.yamlplatform-api/src/resources/default-llm-provider-templates/gemini-template.yamlplatform-api/src/resources/default-llm-provider-templates/mistral-template.yamlplatform-api/src/resources/default-llm-provider-templates/openai-template.yamlplatform-api/src/resources/openapi.yamlportals/ai-workspace/src/Components/ResourceView/ResourceRow.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/EditProviderTemplate.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/TemplateTokenMapping.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyDefinitionTab.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/GuardrailsSection.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/ProviderTemplateSelector.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/TemplateVersionDialog.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderConnectionTab.tsxportals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsxportals/ai-workspace/src/utils/providerTemplateDisplay.tsportals/ai-workspace/src/utils/providerTemplateManifest.tsportals/ai-workspace/src/utils/types.ts
✅ Files skipped from review due to trivial changes (3)
- platform-api/src/resources/default-llm-provider-templates/anthropic-template.yaml
- docs/rest-apis/gateway/llm-provider-template-management.md
- platform-api/src/resources/default-llm-provider-templates/mistral-template.yaml
🚧 Files skipped from review as they are similar to previous changes (20)
- platform-api/src/resources/default-llm-provider-templates/awsbedrock-template.yaml
- platform-api/src/resources/default-llm-provider-templates/azureaifoundry-template.yaml
- platform-api/src/resources/default-llm-provider-templates/openai-template.yaml
- platform-api/src/resources/default-llm-provider-templates/gemini-template.yaml
- portals/ai-workspace/src/utils/providerTemplateDisplay.ts
- platform-api/src/resources/default-llm-provider-templates/azureopenai-template.yaml
- platform-api/src/internal/service/llm_template_seeder.go
- platform-api/src/internal/model/llm.go
- platform-api/src/internal/handler/llm.go
- platform-api/src/internal/repository/interfaces.go
- portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/AddNewProvider/ProviderTemplateSelector.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplateVersion.tsx
- portals/ai-workspace/src/utils/types.ts
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/EditProviderTemplate.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsx
- platform-api/src/resources/openapi.yaml
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/TemplateTokenMapping.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsx
- platform-api/src/internal/service/llm.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/resources/openapi.yaml (1)
2773-2797: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse an update-specific request schema for
PUT /llm-provider-templates/{id}.This endpoint is described as updating the latest version in place, but its request body still points at
LLMProviderTemplate, which makesidandversionrequired request fields. That exposes immutable identity fields as writable in generated clients and leaves mismatches between path/body semantics undefined. Point the request body to a dedicated update schema that omitsidandversion, or document and validate strict equality.Also applies to: 10081-10126
🤖 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 `@platform-api/src/resources/openapi.yaml` around lines 2773 - 2797, The PUT update operation in the OpenAPI spec still uses LLMProviderTemplate for the request body, which incorrectly requires immutable id and version fields. Update the requestBody for updateLLMProviderTemplate to reference a dedicated update schema that omits those fields, or explicitly document and enforce strict equality between path id and body values so generated clients do not treat them as writable.
🤖 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 `@gateway/gateway-controller/pkg/utils/llm_transformer.go`:
- Around line 98-101: The fallback in llm_transformer.go is too broad:
getTemplateByVersionedID(handle) currently falls back to
GetLLMProviderTemplateByHandle for any error. Update the lookup logic so
LLMTransformer.getTemplateByVersionedID only returns the handle-based template
lookup when the error is storage.ErrNotFound, and otherwise returns the original
error unchanged. Keep the behavior localized around getTemplateByVersionedID and
the call site that checks its error.
---
Outside diff comments:
In `@platform-api/src/resources/openapi.yaml`:
- Around line 2773-2797: The PUT update operation in the OpenAPI spec still uses
LLMProviderTemplate for the request body, which incorrectly requires immutable
id and version fields. Update the requestBody for updateLLMProviderTemplate to
reference a dedicated update schema that omits those fields, or explicitly
document and enforce strict equality between path id and body values so
generated clients do not treat them as writable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 467473b1-4309-4adf-8e42-b6414897962b
📒 Files selected for processing (15)
gateway/gateway-controller/pkg/utils/llm_provider_transformer_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/it/docker-compose.test.yamlgateway/it/features/llm-custom-template-invoke.featureplatform-api/src/api/generated.goplatform-api/src/internal/database/connection.goplatform-api/src/internal/database/schema.sqlserver.sqlplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_provider_template_test.goplatform-api/src/resources/openapi.yamlportals/ai-workspace/cypress/e2e/005-provider-templates/005-custom-provider-template.cy.jsportals/ai-workspace/cypress/support/commands.jsportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsxportals/ai-workspace/src/utils/providerTemplateManifest.ts
💤 Files with no reviewable changes (1)
- portals/ai-workspace/src/utils/providerTemplateManifest.ts
✅ Files skipped from review due to trivial changes (1)
- platform-api/src/api/generated.go
🚧 Files skipped from review as they are similar to previous changes (7)
- platform-api/src/internal/service/llm_provider_template_test.go
- portals/ai-workspace/cypress/support/commands.js
- portals/ai-workspace/cypress/e2e/005-provider-templates/005-custom-provider-template.cy.js
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplatesList.tsx
- platform-api/src/internal/database/schema.sqlserver.sql
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/ProviderTemplateOverview.tsx
- platform-api/src/internal/service/llm.go
…o 'managedBy', add 'groupVersionId' and fix test failures
…'groupVersionId', and fix test issues
3dc4f59 to
c363d46
Compare
73083c6 to
8f63c55
Compare
…date related tests
… up unused deny policy logic
|
@CodeRabbit please review |
|
I’ll review the PR changes now. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/gateway-controller/pkg/utils/mock_db_test.go (1)
141-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the newest matching template in the mock.
Production lookup resolves a group to the latest stored version, but this mock returns the first matching map entry. Once tests cover multiple versions per
groupId, this becomes nondeterministic and no longer matches runtime behavior.Suggested mock fix
func (m *testMockDB) GetLLMProviderTemplateByHandle(handle string) (*models.StoredLLMProviderTemplate, error) { + var latest *models.StoredLLMProviderTemplate for _, t := range m.templates { - if t.GetGroupID() == handle { - return t, nil + if t.GetGroupID() != handle { + continue + } + if latest == nil || t.CreatedAt.After(latest.CreatedAt) { + latest = t } } + if latest != nil { + return latest, nil + } return nil, storage.ErrNotFound }🤖 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 `@gateway/gateway-controller/pkg/utils/mock_db_test.go` around lines 141 - 148, The mock implementation of GetLLMProviderTemplateByHandle currently returns the first matching template, which can diverge from production behavior when multiple versions exist for the same group. Update testMockDB.GetLLMProviderTemplateByHandle to select the newest matching template from m.templates, using the stored version/recency field used by models.StoredLLMProviderTemplate rather than returning the first match, so tests mirror the runtime lookup behavior.gateway/gateway-controller/pkg/storage/sqlite_test.go (1)
1172-1183: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet explicit
groupIdandversionin the template fixture.This helper only populates
displayName, soGetGroupID()falls back tometadata.nameand the storage tests at Lines 478, 535, and 608 still pass even if explicitgroup_id/versionpersistence regresses. Use aspec.groupIdvalue that differs frommetadata.name, and setspec.version, so the new DB contract is actually exercised.Suggested test fixture update
func createTestLLMProviderTemplate() *models.StoredLLMProviderTemplate { llmTemplateCounter++ + groupID := fmt.Sprintf("template-family-%d", llmTemplateCounter) + version := "v1.0" return &models.StoredLLMProviderTemplate{ UUID: fmt.Sprintf("test-template-%d", llmTemplateCounter), Configuration: api.LLMProviderTemplate{ ApiVersion: api.LLMProviderTemplateApiVersionGatewayApiPlatformWso2Comv1, Kind: api.LLMProviderTemplateKindLlmProviderTemplate, Metadata: api.Metadata{Name: fmt.Sprintf("test-template-%d", llmTemplateCounter)}, Spec: api.LLMProviderTemplateData{ DisplayName: fmt.Sprintf("Test Template %d", llmTemplateCounter), + GroupId: &groupID, + Version: &version, }, }, CreatedAt: time.Now(), UpdatedAt: time.Now(), } }🤖 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 `@gateway/gateway-controller/pkg/storage/sqlite_test.go` around lines 1172 - 1183, The `createTestLLMProviderTemplate` fixture only sets `DisplayName`, so `GetGroupID()` still falls back to `Metadata.Name` and won’t detect regressions in explicit `group_id`/`version` persistence. Update this helper to populate `api.LLMProviderTemplateData` with a `GroupId` value that is different from `Metadata.Name`, and also set `Version`, so the storage tests exercise the real DB contract through `createTestLLMProviderTemplate` and `StoredLLMProviderTemplate`.
🤖 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 `@gateway/gateway-controller/pkg/storage/gateway-controller-db.sql`:
- Around line 143-146: The schema currently documents that the handle
(metadata.name) is unique per gateway, but the gateway-controller table
constraints only enforce the primary key and the (gateway_id, group_id, version)
uniqueness. Update the table definition in gateway-controller-db.sql to add a
uniqueness constraint for handle scoped by gateway_id, and make sure the
existing uniqueness rules remain unchanged so handle-based lookups in the
gateway controller stay unambiguous.
In `@gateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sql`:
- Around line 128-129: The SQL Server schema for the gateway controller is
missing the gateway-scoped uniqueness rule for handle, so align it with the
other schema variants by adding the handle constraint in the table definition
next to the existing versioned family uniqueness rule. Update the
gateway-controller DB schema where the PRIMARY KEY and UNIQUE constraint are
declared so the stored handle remains uniquely enforced per gateway, using the
same constraint naming/shape as the corresponding schema family.
---
Outside diff comments:
In `@gateway/gateway-controller/pkg/storage/sqlite_test.go`:
- Around line 1172-1183: The `createTestLLMProviderTemplate` fixture only sets
`DisplayName`, so `GetGroupID()` still falls back to `Metadata.Name` and won’t
detect regressions in explicit `group_id`/`version` persistence. Update this
helper to populate `api.LLMProviderTemplateData` with a `GroupId` value that is
different from `Metadata.Name`, and also set `Version`, so the storage tests
exercise the real DB contract through `createTestLLMProviderTemplate` and
`StoredLLMProviderTemplate`.
In `@gateway/gateway-controller/pkg/utils/mock_db_test.go`:
- Around line 141-148: The mock implementation of GetLLMProviderTemplateByHandle
currently returns the first matching template, which can diverge from production
behavior when multiple versions exist for the same group. Update
testMockDB.GetLLMProviderTemplateByHandle to select the newest matching template
from m.templates, using the stored version/recency field used by
models.StoredLLMProviderTemplate rather than returning the first match, so tests
mirror the runtime lookup behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62e0b933-7c0a-40f9-bee5-33151536af7c
📒 Files selected for processing (83)
docs/rest-apis/gateway/llm-provider-template-management.mdgateway/gateway-controller/README.mdgateway/gateway-controller/api/management-openapi.yamlgateway/gateway-controller/default-llm-provider-templates/anthropic-template.yamlgateway/gateway-controller/default-llm-provider-templates/awsbedrock-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureaifoundry-template.yamlgateway/gateway-controller/default-llm-provider-templates/azureopenai-template.yamlgateway/gateway-controller/default-llm-provider-templates/gemini-template.yamlgateway/gateway-controller/default-llm-provider-templates/mistral-template.yamlgateway/gateway-controller/default-llm-provider-templates/openai-template.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/handlers_test.gogateway/gateway-controller/pkg/api/handlers/list_operations_test.gogateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.gogateway/gateway-controller/pkg/api/handlers/resource_response.gogateway/gateway-controller/pkg/api/management/generated.gogateway/gateway-controller/pkg/eventlistener/llm_template_processor.gogateway/gateway-controller/pkg/eventlistener/llm_template_processor_test.gogateway/gateway-controller/pkg/models/llm_provider_template.gogateway/gateway-controller/pkg/models/llm_provider_template_test.gogateway/gateway-controller/pkg/storage/gateway-controller-db.postgres.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlgateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sqlgateway/gateway-controller/pkg/storage/loaders.gogateway/gateway-controller/pkg/storage/memory.gogateway/gateway-controller/pkg/storage/memory_test.gogateway/gateway-controller/pkg/storage/postgres_test.gogateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/storage/sqlite.gogateway/gateway-controller/pkg/storage/sqlite_test.gogateway/gateway-controller/pkg/storage/sqlserver_test.gogateway/gateway-controller/pkg/utils/llm_deployment.gogateway/gateway-controller/pkg/utils/llm_deployment_test.gogateway/gateway-controller/pkg/utils/llm_provider_template_loader.gogateway/gateway-controller/pkg/utils/llm_provider_template_loader_test.gogateway/gateway-controller/pkg/utils/llm_provider_transformer_test.gogateway/gateway-controller/pkg/utils/llm_transformer.gogateway/gateway-controller/pkg/utils/mock_db_test.gogateway/gateway-controller/tests/integration/schema_test.gogateway/it/LLM_PROVIDER_TEST_PLAN.mdgateway/it/README.mdgateway/it/docker-compose.test.yamlgateway/it/features/lazy-resources-xds.featuregateway/it/features/llm-custom-template-invoke.featuregateway/it/features/llm-provider-templates.featuregateway/it/steps_llm.goplatform-api/src/api/generated.goplatform-api/src/api/manual_types.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/connection.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/database/schema.sqlserver.sqlplatform-api/src/internal/handler/llm.goplatform-api/src/internal/model/llm.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/llm_test.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_provider_template_test.goplatform-api/src/internal/service/llm_template_seeder.goplatform-api/src/internal/service/llm_test.goplatform-api/src/internal/utils/llm_provider_template_loader.goplatform-api/src/resources/default-llm-provider-templates/anthropic-template.yamlplatform-api/src/resources/default-llm-provider-templates/awsbedrock-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureaifoundry-template.yamlplatform-api/src/resources/default-llm-provider-templates/azureopenai-template.yamlplatform-api/src/resources/default-llm-provider-templates/gemini-template.yamlplatform-api/src/resources/default-llm-provider-templates/mistral-template.yamlplatform-api/src/resources/default-llm-provider-templates/openai-template.yamlplatform-api/src/resources/openapi.yamlportals/ai-workspace/cypress/e2e/001-providers/001-provider-and-proxy.cy.jsportals/ai-workspace/cypress/e2e/005-provider-templates/005-custom-provider-template.cy.jsportals/ai-workspace/cypress/support/commands.jsportals/ai-workspace/src/App.tsxportals/ai-workspace/src/Components/ResourceView/ResourceRow.tsxportals/ai-workspace/src/apis/providerTemplateApis.tsportals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplateContext.tsxportals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplatesContext.tsxportals/ai-workspace/src/pages/appShell/AppSidebar.tsxportals/ai-workspace/src/pages/appShell/appShellMain.tsxportals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsx
💤 Files with no reviewable changes (38)
- platform-api/src/resources/default-llm-provider-templates/awsbedrock-template.yaml
- platform-api/src/resources/default-llm-provider-templates/openai-template.yaml
- platform-api/src/resources/default-llm-provider-templates/azureopenai-template.yaml
- platform-api/src/resources/default-llm-provider-templates/azureaifoundry-template.yaml
- platform-api/src/internal/service/llm_test.go
- platform-api/src/resources/default-llm-provider-templates/gemini-template.yaml
- platform-api/src/resources/default-llm-provider-templates/anthropic-template.yaml
- platform-api/src/api/manual_types.go
- portals/ai-workspace/src/pages/appShell/AppSidebar.tsx
- platform-api/src/internal/service/llm_template_seeder.go
- gateway/it/steps_llm.go
- platform-api/src/internal/utils/llm_provider_template_loader.go
- portals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplateContext.tsx
- portals/ai-workspace/src/contexts/llmProvider/providerTemplate/ProviderTemplatesContext.tsx
- platform-api/src/internal/database/schema.sql
- portals/ai-workspace/cypress/support/commands.js
- portals/ai-workspace/src/Components/ResourceView/ResourceRow.tsx
- platform-api/src/internal/model/llm.go
- portals/ai-workspace/src/App.tsx
- platform-api/src/internal/constants/error.go
- portals/ai-workspace/cypress/e2e/005-provider-templates/005-custom-provider-template.cy.js
- platform-api/src/internal/database/schema.sqlserver.sql
- platform-api/src/internal/database/schema.postgres.sql
- portals/ai-workspace/src/apis/providerTemplateApis.ts
- platform-api/src/resources/default-llm-provider-templates/mistral-template.yaml
- portals/ai-workspace/src/pages/appShell/appShellPages/providerTemplate/CreateProviderTemplate.tsx
- portals/ai-workspace/src/pages/appShell/appShellMain.tsx
- platform-api/src/internal/service/llm_provider_template_test.go
- platform-api/src/internal/database/connection.go
- portals/ai-workspace/cypress/e2e/001-providers/001-provider-and-proxy.cy.js
- platform-api/src/internal/handler/llm.go
- platform-api/src/internal/database/schema.sqlite.sql
- platform-api/src/internal/repository/llm_test.go
- platform-api/src/internal/repository/interfaces.go
- platform-api/src/internal/service/llm.go
- platform-api/src/api/generated.go
- platform-api/src/resources/openapi.yaml
- platform-api/src/internal/repository/llm.go
✅ Files skipped from review due to trivial changes (5)
- gateway/gateway-controller/tests/integration/schema_test.go
- gateway/it/LLM_PROVIDER_TEST_PLAN.md
- gateway/gateway-controller/pkg/eventlistener/llm_template_processor_test.go
- gateway/gateway-controller/pkg/storage/loaders.go
- gateway/it/features/lazy-resources-xds.feature
🚧 Files skipped from review as they are similar to previous changes (22)
- gateway/gateway-controller/pkg/storage/sqlite.go
- gateway/gateway-controller/pkg/api/handlers/resource_response.go
- gateway/it/features/llm-custom-template-invoke.feature
- gateway/gateway-controller/pkg/storage/sqlserver_test.go
- docs/rest-apis/gateway/llm-provider-template-management.md
- gateway/gateway-controller/pkg/api/handlers/handlers.go
- gateway/gateway-controller/api/management-openapi.yaml
- gateway/gateway-controller/pkg/utils/llm_provider_template_loader.go
- gateway/it/features/llm-provider-templates.feature
- gateway/gateway-controller/pkg/utils/llm_provider_template_loader_test.go
- gateway/it/docker-compose.test.yaml
- gateway/gateway-controller/pkg/utils/llm_transformer.go
- gateway/gateway-controller/pkg/eventlistener/llm_template_processor.go
- gateway/gateway-controller/pkg/utils/llm_provider_transformer_test.go
- gateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.go
- gateway/gateway-controller/pkg/models/llm_provider_template_test.go
- gateway/gateway-controller/pkg/api/handlers/handlers_test.go
- gateway/gateway-controller/pkg/utils/llm_deployment_test.go
- gateway/gateway-controller/pkg/utils/llm_deployment.go
- gateway/gateway-controller/pkg/api/handlers/list_operations_test.go
- gateway/gateway-controller/pkg/storage/sql_store.go
- gateway/gateway-controller/pkg/storage/memory.go
| -- Each (group_id, version) pair is unique per gateway; versions coexist. | ||
| -- The handle (metadata.name) is unique per gateway. | ||
| PRIMARY KEY (gateway_id, uuid), | ||
| UNIQUE(gateway_id, handle) | ||
| UNIQUE(gateway_id, group_id, version) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the documented handle uniqueness.
Line 144 says handle is unique per gateway, but the table only enforces (gateway_id, group_id, version). Keep the handle constraint to avoid ambiguous handle-based lookups.
Proposed fix
- UNIQUE(gateway_id, group_id, version)
+ UNIQUE(gateway_id, group_id, version),
+ UNIQUE(gateway_id, handle)📝 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.
| -- Each (group_id, version) pair is unique per gateway; versions coexist. | |
| -- The handle (metadata.name) is unique per gateway. | |
| PRIMARY KEY (gateway_id, uuid), | |
| UNIQUE(gateway_id, handle) | |
| UNIQUE(gateway_id, group_id, version) | |
| -- Each (group_id, version) pair is unique per gateway; versions coexist. | |
| -- The handle (metadata.name) is unique per gateway. | |
| PRIMARY KEY (gateway_id, uuid), | |
| UNIQUE(gateway_id, group_id, version), | |
| UNIQUE(gateway_id, handle) |
🤖 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 `@gateway/gateway-controller/pkg/storage/gateway-controller-db.sql` around
lines 143 - 146, The schema currently documents that the handle (metadata.name)
is unique per gateway, but the gateway-controller table constraints only enforce
the primary key and the (gateway_id, group_id, version) uniqueness. Update the
table definition in gateway-controller-db.sql to add a uniqueness constraint for
handle scoped by gateway_id, and make sure the existing uniqueness rules remain
unchanged so handle-based lookups in the gateway controller stay unambiguous.
| PRIMARY KEY (gateway_id, uuid), | ||
| UNIQUE(gateway_id, handle) | ||
| UNIQUE(gateway_id, group_id, version) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep handle uniqueness consistent across schema variants.
handle remains a stored identifier, but this schema no longer enforces gateway-scoped uniqueness for it. Add the handle constraint alongside the versioned family constraint.
Proposed fix
- UNIQUE(gateway_id, group_id, version)
+ UNIQUE(gateway_id, group_id, version),
+ UNIQUE(gateway_id, handle)📝 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.
| PRIMARY KEY (gateway_id, uuid), | |
| UNIQUE(gateway_id, handle) | |
| UNIQUE(gateway_id, group_id, version) | |
| PRIMARY KEY (gateway_id, uuid), | |
| UNIQUE(gateway_id, group_id, version), | |
| UNIQUE(gateway_id, handle) |
🤖 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 `@gateway/gateway-controller/pkg/storage/gateway-controller-db.sqlserver.sql`
around lines 128 - 129, The SQL Server schema for the gateway controller is
missing the gateway-scoped uniqueness rule for handle, so align it with the
other schema variants by adding the handle constraint in the table definition
next to the existing versioned family uniqueness rule. Update the
gateway-controller DB schema where the PRIMARY KEY and UNIQUE constraint are
declared so the stored handle remains uniquely enforced per gateway, using the
same constraint naming/shape as the corresponding schema family.
Purpose
Today, admins can only create LLM providers from a fixed, built-in set of provider templates (OpenAI, Anthropic, Azure OpenAI, AWS Bedrock, Gemini, Mistral, Azure AI Foundry). Any LLM provider that isn't on that list can't be onboarded without changes to the gateway itself.
This PR adds LLM Provider Templates as a first-class, user-managed resource: org admins can define their own custom templates (name, endpoint, OpenAPI spec, token/model extraction mapping) and immediately use them to create LLM providers, the same way they use the built-in ones.
Key points
managedBy: wso2) templates read-only and protected from edit/delete, while still letting admins fork a new custom version off a built-in template.User stories
UI Implementations