Create agent llm-provider configuration when creating the agent#605
Open
Create agent llm-provider configuration when creating the agent#605
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds optional per-environment LLM model configuration to agent creation: OpenAPI/schema and generated Go models added, agent manager creates LLM configs with rollback on failure, DI wiring adjusted, and frontend UI, form state, and payload builder updated to select and submit provider/guardrail/env-variable mappings. Changes
Sequence DiagramsequenceDiagram
participant UI as Client/UI
participant Form as InternalAgentForm
participant Builder as buildAgentPayload
participant AMS as AgentManagerService
participant ACS as AgentConfigurationService
participant OC as OpenChoreo
UI->>Form: submit(formData + llmProviders)
Form->>Builder: buildAgentCreationPayload(...)
Builder-->>UI: CreateAgentRequest (includes modelConfig)
UI->>AMS: POST CreateAgent(req)
AMS->>AMS: create base agent
alt modelConfig present
AMS->>ACS: createAgentLLMConfigs(ModelConfig[])
ACS-->>AMS: success
else LLM config error
ACS-->>AMS: error
AMS->>AMS: best-effort cleanup secrets
AMS->>OC: DeleteComponent(agent)
OC-->>AMS: deleted
AMS-->>UI: error
end
AMS->>OC: publish agent
OC-->>AMS: result
AMS-->>UI: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
menakaj
force-pushed
the
agent-llm-config-create
branch
from
01ac35c to
5f1dedc
Compare
rasika2012
reviewed
Mar 20, 2026
console/workspaces/Untitled
Outdated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
console/workspaces/Untitled (1)
1-1: Remove accidental workspace artifact from the PR.
console/workspaces/Untitledappears to be a stray file unrelated to the feature and should not be committed.Suggested cleanup
-API Key Generated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/Untitled` at line 1, Remove the accidental workspace artifact named "Untitled" from the PR: delete the file from the branch (unstage and remove from git), amend or create a new commit without it, and push the updated commit to the PR; also check for any other stray console workspace files and, if these files are generated regularly, add an appropriate rule to .gitignore to prevent future accidental commits.console/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsx (1)
24-24: Unused import:LLMProviderFormEntry.The
LLMProviderFormEntrytype is imported but not used anywhere in this file. External agent flow doesn't include LLM provider configuration based on the PR design.🧹 Proposed fix to remove unused import
-import { ConnectAgentFormValues, LLMProviderFormEntry } from "../form/schema"; +import { ConnectAgentFormValues } from "../form/schema";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsx` at line 24, Remove the unused type import LLMProviderFormEntry from the import statement that currently imports ConnectAgentFormValues and LLMProviderFormEntry (the line importing from "../form/schema"); keep ConnectAgentFormValues only so the file no longer imports unused symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 6401-6406: The environmentVariables array schema lacks the same
duplicate-key validation note as
CreateAgentModelConfigRequest.environmentVariables; update the OpenAPI docs for
the environmentVariables property (the array under environmentVariables and its
items referencing EnvironmentVariableConfig) to document that duplicate keys are
rejected with a 400 error (in addition to uniqueItems: true), e.g., add a
description sentence or validation note stating "Duplicate environment variable
keys are rejected with 400" so the contract matches the standalone model-config
API.
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 332-349: The default env-var values for new provider entries are
not unique and not validated: update the logic that sets entry.urlVarName and
entry.apikeyVarName (the places that render TextField with
value={entry.urlVarName ?? `${agentNameUpper}_URL`} and
value={entry.apikeyVarName ?? `${agentNameUpper}_API_KEY`} as well as the
similar occurrences at the other noted locations) to generate unique,
provider-scoped defaults (e.g., include a unique provider index or provider id)
instead of reusing `${agentNameUpper}_URL`/`_API_KEY`, and add validation in the
onChange handlers (handleUrlVarChange and handleApikeyVarChange) to reject empty
or invalid names using the same env-var regex used for configurations.env;
ensure buildAgentCreationPayload receives sanitized, validated values only.
- Around line 419-425: The drawer currently hardcodes drawerEnvName =
environments[0]?.name which causes edits to always target the first env and
overwrites selectedProviderByEnv for every env; change the drawer to accept or
derive the actual active environment name (e.g., prop activeEnvName or from the
active tab) and use that value instead of environments[0] when computing
currentDrawerProviderUuid and when writing updates to
llmProviders[x].selectedProviderByEnv and modelConfig.envMappings; update the
other similar block (the logic around lines handling 463-470) so updates only
set the single key for the active env (e.g.,
selectedProviderByEnv[activeEnvName] = {...} or
modelConfig.envMappings[activeEnvName] = ...), not replace the entire mapping
object, preserving other environments' mappings.
---
Nitpick comments:
In `@console/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsx`:
- Line 24: Remove the unused type import LLMProviderFormEntry from the import
statement that currently imports ConnectAgentFormValues and LLMProviderFormEntry
(the line importing from "../form/schema"); keep ConnectAgentFormValues only so
the file no longer imports unused symbols.
In `@console/workspaces/Untitled`:
- Line 1: Remove the accidental workspace artifact named "Untitled" from the PR:
delete the file from the branch (unstage and remove from git), amend or create a
new commit without it, and push the updated commit to the PR; also check for any
other stray console workspace files and, if these files are generated regularly,
add an appropriate rule to .gitignore to prevent future accidental commits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f88de9a-8766-4d77-b69c-01c996ba42dc
📒 Files selected for processing (16)
agent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/services/agent_manager.goagent-manager-service/spec/model_create_agent_request.goagent-manager-service/spec/model_model_config_request.goagent-manager-service/wiring/wire_gen.goconsole/workspaces/Untitledconsole/workspaces/libs/types/src/api/agents.tsconsole/workspaces/pages/add-new-agent/package.jsonconsole/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsxconsole/workspaces/pages/add-new-agent/src/components/ExternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsxconsole/workspaces/pages/add-new-agent/src/form/schema.tsconsole/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.ts
Comment on lines
+6401
to
+6406
| environmentVariables: | ||
| type: array | ||
| description: Optional custom environment variable names exposed to the agent | ||
| uniqueItems: true | ||
| items: | ||
| $ref: "#/components/schemas/EnvironmentVariableConfig" |
Contributor
There was a problem hiding this comment.
Keep environmentVariables validation docs consistent with the standalone model-config API.
uniqueItems: true only rejects identical {key, name} objects. The existing CreateAgentModelConfigRequest.environmentVariables docs already say duplicate keys are rejected with 400, but that rule is omitted here, so the same field shape now has two different documented contracts.
📝 Suggested doc fix
environmentVariables:
type: array
- description: Optional custom environment variable names exposed to the agent
+ description: Optional custom environment variable names exposed to the agent. Duplicate keys are rejected with 400.
uniqueItems: true
items:
$ref: "#/components/schemas/EnvironmentVariableConfig"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agent-manager-service/docs/api_v1_openapi.yaml` around lines 6401 - 6406, The
environmentVariables array schema lacks the same duplicate-key validation note
as CreateAgentModelConfigRequest.environmentVariables; update the OpenAPI docs
for the environmentVariables property (the array under environmentVariables and
its items referencing EnvironmentVariableConfig) to document that duplicate keys
are rejected with a 400 error (in addition to uniqueItems: true), e.g., add a
description sentence or validation note stating "Duplicate environment variable
keys are rejected with 400" so the contract matches the standalone model-config
API.
Comment on lines
+636
to
+647
| // Create LLM configurations (applies to both internal and external agents) | ||
| if len(req.ModelConfig) > 0 { | ||
| if err := s.createAgentLLMConfigs(ctx, orgName, projectName, req); err != nil { | ||
| s.logger.Error("Failed to create LLM configurations for agent", "agentName", req.Name, "error", err) | ||
| if hasSecrets { | ||
| s.cleanupSecretsOnRollback(ctx, secretLocation) | ||
| } | ||
| if errDeletion := s.ocClient.DeleteComponent(ctx, orgName, projectName, req.Name); errDeletion != nil { | ||
| s.logger.Error("Failed to rollback agent after LLM config failure", "agentName", req.Name, "error", errDeletion) | ||
| } | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Roll back created LLM configs on every failure path.
Once createAgentLLMConfigs has created the first config, any later error here only removes the component and secrets. That leaves orphaned ${agent}-llm-config* records behind, so a retry can fail even though the component was rolled back. Please track the created config IDs and delete them in each rollback branch so create-agent stays atomic.
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx
Show resolved
Hide resolved
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (2)
246-257:⚠️ Potential issue | 🟠 MajorMake the env-var names unique and valid by default.
This still initializes every entry with the same
${agentNameUpper}_URL/${agentNameUpper}_API_KEYpair, and the change handlers store whatever the user types. Multiple entries collide immediately, and display names like123 Botcan produce defaults that violate the env-var regex already enforced forenv.Also applies to: 332-349, 455-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 246 - 257, The default env-var names are not unique/validated and handlers accept arbitrary input; update the initialization logic that sets urlVarName/apikeyVarName (and the similar blocks around the other occurrences) to generate unique, regex-safe defaults per entry (e.g., based on a sanitized agent name plus the entry index or a counter) and ensure handleUrlVarChange and handleApikeyVarChange sanitize and/or validate user input against the same env-var regex before calling onUpdateEntry (reject/normalize invalid characters and avoid collisions by appending index if needed), so look for the functions handleUrlVarChange and handleApikeyVarChange and the places initializing urlVarName/apikeyVarName and apply the uniqueness + validation logic consistently.
244-245:⚠️ Potential issue | 🟠 MajorKeep provider edits scoped to the active environment.
onOpenDraweronly carries the entry index,drawerEnvNameis fixed to the first environment, andhandleProviderSelectrewrites the wholeselectedProviderByEnvrecord. With multiple environments, opening the drawer from another tab still edits the first env and wipes any per-env selection differences.Also applies to: 419-425, 443-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 244 - 245, The drawer open flow is using a fixed drawerEnvName and onOpenDrawer(index) so opening the drawer from a non-first environment edits the first env and handleProviderSelect replaces the entire selectedProviderByEnv record; update the flow so the active environment is passed and used: change onOpenDrawer to accept (index, envName) or capture the current env in handleOpenDrawer (reference handleOpenDrawer, onOpenDrawer, drawerEnvName), ensure handleProviderSelect updates only selectedProviderByEnv[envName] (merge into the existing object rather than replacing it) so per-environment selections are preserved, and update any other callsites (see usages around the other mentions) to pass the correct envName and include envName in useCallback dependency lists.
🧹 Nitpick comments (2)
agent-manager-service/wiring/wire_gen.go (1)
70-70: Consider clarifying the dual repository/service access pattern.The
agentManagerServicenow holds bothagentConfigRepository(direct repo access) andagentConfigurationService(service layer). Per the PR objective of atomic creation with rollback, ensure that agent configuration operations during creation flow throughagentConfigurationServicerather than the direct repository, to maintain transactional guarantees and avoid bypassing business logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/wiring/wire_gen.go` at line 70, The wiring currently injects both agentConfigRepository and agentConfigurationService into NewAgentManagerService, which risks bypassing service-layer transactional logic; update the constructor call and AgentManagerService so creation/rollback use agentConfigurationService exclusively: remove the direct agentConfigRepository dependency from the NewAgentManagerService injection (or stop using it inside AgentManagerService), change any AgentManagerService methods that call agentConfigRepository to call the corresponding agentConfigurationService methods (e.g., CreateAgentConfig, DeleteAgentConfig/Rollback methods), and ensure NewAgentManagerService signature and AgentManagerService implementation only rely on agentConfigurationService for agent configuration operations to preserve atomic creation and rollback guarantees.console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (1)
103-109: Prefer theme-derived sizing in the new card UI.The avatar/logo dimensions here are hardcoded pixels in
sx. Using theme tokens keeps this section aligned with the console’s spacing scale.As per coding guidelines, "Use theme tokens via the
sxprop instead of hardcoded colors and spacing values".Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 103 - 109, The Avatar in LLMProviderSection.tsx uses hardcoded pixel values for sizing and also has other hardcoded spacing/colors nearby; update the Avatar's sx to use theme tokens instead of numeric literals (e.g., replace height: 32 and width: 32 with theme.spacing(...) via the sx callback such as (theme) => theme.spacing(4)) and ensure any nearby color/spacing uses theme.palette and theme.spacing tokens (refer to the Avatar component, its sx prop and the isSelected logic, and the similar block around lines 124-128) so spacing and colors align with the console theme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 386-388: The add flow is enabled before the backing queries settle
(useListEnvironments, useListCatalogLLMProviders, useListLLMProviderTemplates),
causing creation with missing env mappings and empty-state UI; fix by gating the
Add button and drawer behavior on the queries' loading/completed state (use
isLoading/isFetching or data === undefined) — disable the Add control until
environments and catalog finish loading, prevent creating a provider if
environments is undefined (check environments before creating env mappings in
the create handler used by the drawer), and swap the empty-state copy for a
loading spinner in the drawer while catalog/template queries are loading; apply
the same guards to the other affected sections that call those hooks (the blocks
around the other Add flows).
- Around line 437-441: handleDrawerClose resets search state but does not clear
a pending debounce timer, so a queued timeout can later restore stale
debouncedSearch and reopen the drawer; update handleDrawerClose to clear the
pending timer (e.g., clearTimeout on the timer id stored in a ref or state)
before resetting setProviderDrawerOpen, setProviderSearchQuery, and
setDebouncedSearch, and apply the same change to the other drawer-close handler
referenced at lines 499-504 (the other provider/drawer close function) so any
outstanding setTimeout is canceled on close.
---
Duplicate comments:
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 246-257: The default env-var names are not unique/validated and
handlers accept arbitrary input; update the initialization logic that sets
urlVarName/apikeyVarName (and the similar blocks around the other occurrences)
to generate unique, regex-safe defaults per entry (e.g., based on a sanitized
agent name plus the entry index or a counter) and ensure handleUrlVarChange and
handleApikeyVarChange sanitize and/or validate user input against the same
env-var regex before calling onUpdateEntry (reject/normalize invalid characters
and avoid collisions by appending index if needed), so look for the functions
handleUrlVarChange and handleApikeyVarChange and the places initializing
urlVarName/apikeyVarName and apply the uniqueness + validation logic
consistently.
- Around line 244-245: The drawer open flow is using a fixed drawerEnvName and
onOpenDrawer(index) so opening the drawer from a non-first environment edits the
first env and handleProviderSelect replaces the entire selectedProviderByEnv
record; update the flow so the active environment is passed and used: change
onOpenDrawer to accept (index, envName) or capture the current env in
handleOpenDrawer (reference handleOpenDrawer, onOpenDrawer, drawerEnvName),
ensure handleProviderSelect updates only selectedProviderByEnv[envName] (merge
into the existing object rather than replacing it) so per-environment selections
are preserved, and update any other callsites (see usages around the other
mentions) to pass the correct envName and include envName in useCallback
dependency lists.
---
Nitpick comments:
In `@agent-manager-service/wiring/wire_gen.go`:
- Line 70: The wiring currently injects both agentConfigRepository and
agentConfigurationService into NewAgentManagerService, which risks bypassing
service-layer transactional logic; update the constructor call and
AgentManagerService so creation/rollback use agentConfigurationService
exclusively: remove the direct agentConfigRepository dependency from the
NewAgentManagerService injection (or stop using it inside AgentManagerService),
change any AgentManagerService methods that call agentConfigRepository to call
the corresponding agentConfigurationService methods (e.g., CreateAgentConfig,
DeleteAgentConfig/Rollback methods), and ensure NewAgentManagerService signature
and AgentManagerService implementation only rely on agentConfigurationService
for agent configuration operations to preserve atomic creation and rollback
guarantees.
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 103-109: The Avatar in LLMProviderSection.tsx uses hardcoded pixel
values for sizing and also has other hardcoded spacing/colors nearby; update the
Avatar's sx to use theme tokens instead of numeric literals (e.g., replace
height: 32 and width: 32 with theme.spacing(...) via the sx callback such as
(theme) => theme.spacing(4)) and ensure any nearby color/spacing uses
theme.palette and theme.spacing tokens (refer to the Avatar component, its sx
prop and the isSelected logic, and the similar block around lines 124-128) so
spacing and colors align with the console theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d369e565-d046-4fbc-a3b6-32c77cc46184
⛔ Files ignored due to path filters (1)
console/common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
agent-manager-service/wiring/wire_gen.goconsole/workspaces/Untitledconsole/workspaces/libs/types/src/api/agents.tsconsole/workspaces/pages/add-new-agent/package.jsonconsole/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsxconsole/workspaces/pages/add-new-agent/src/components/ExternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsxconsole/workspaces/pages/add-new-agent/src/form/schema.tsconsole/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.ts
✅ Files skipped from review due to trivial changes (5)
- console/workspaces/Untitled
- console/workspaces/pages/add-new-agent/src/components/ExternalAgentFlow.tsx
- console/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsx
- console/workspaces/pages/add-new-agent/package.json
- console/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- console/workspaces/libs/types/src/api/agents.ts
- console/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsx
- console/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.ts
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx
Outdated
Show resolved
Hide resolved
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx
Show resolved
Hide resolved
menakaj
force-pushed
the
agent-llm-config-create
branch
3 times, most recently
from
a9afca9 to
e765179
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (2)
342-353:⚠️ Potential issue | 🟠 MajorUse unique env-var defaults per entry and validate env-var names before storing.
All entries default to the same
${agentNameUpper}_URL/${agentNameUpper}_API_KEYnames, and input handlers on Lines 249-259 accept arbitrary values. Since payload building forwards these names directly, collisions/invalid env var names are only caught downstream.Proposed patch
+const ENV_VAR_NAME_REGEX = /^[A-Z_][A-Z0-9_]*$/; @@ - const handleUrlVarChange = useCallback( + const handleUrlVarChange = useCallback( (e: React.ChangeEvent<HTMLInputElement>) => { - onUpdateEntry(index, { ...entry, urlVarName: e.target.value }); + const next = e.target.value.trim().toUpperCase(); + if (next !== "" && !ENV_VAR_NAME_REGEX.test(next)) return; + onUpdateEntry(index, { ...entry, urlVarName: next || undefined }); }, @@ - const handleApikeyVarChange = useCallback( + const handleApikeyVarChange = useCallback( (e: React.ChangeEvent<HTMLInputElement>) => { - onUpdateEntry(index, { ...entry, apikeyVarName: e.target.value }); + const next = e.target.value.trim().toUpperCase(); + if (next !== "" && !ENV_VAR_NAME_REGEX.test(next)) return; + onUpdateEntry(index, { ...entry, apikeyVarName: next || undefined }); }, @@ - urlVarName: `${agentNameUpper}_URL`, - apikeyVarName: `${agentNameUpper}_API_KEY`, + urlVarName: `${agentNameUpper}_${prev.length + 1}_URL`, + apikeyVarName: `${agentNameUpper}_${prev.length + 1}_API_KEY`,Also applies to: 465-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 342 - 353, Default env-var names for each LLM entry currently all use `${agentNameUpper}_URL` and `${agentNameUpper}_API_KEY` and the handlers (handleUrlVarChange, handleApikeyVarChange) accept arbitrary values; change this so each entry gets a unique default (e.g., derive from entry.id or index when rendering the TextField value for urlVarName/apikeyVarName) and add validation in handleUrlVarChange and handleApikeyVarChange to reject/normalize invalid env-var names (allow only A-Z0-9 and underscores, start with a letter, no collisions among entries) before updating entry state so only valid, collision-free names are stored and forwarded.
393-395:⚠️ Potential issue | 🟠 MajorBlock add/select until required queries settle, and prevent empty env-mappings creation.
environmentscan still be unresolved when the add flow is usable. On Line 458, creating a new entry with an emptyenvironmentsarray producesselectedProviderByEnv = {}, which breaks per-environment intent and can emit invalidmodelConfig.envMappings.[suggested fix: gate Add + guard creation path]
Proposed patch
- const { data: environments = [] } = useListEnvironments({ orgName: orgId }); - const { data: catalogData } = useListCatalogLLMProviders({ orgName: orgId }, { limit: 50 }); + const { data: environments = [], isLoading: isEnvironmentsLoading } = useListEnvironments({ orgName: orgId }); + const { data: catalogData, isLoading: isCatalogLoading } = useListCatalogLLMProviders({ orgName: orgId }, { limit: 50 }); @@ if (editingIndex === null) { + if (isEnvironmentsLoading || environments.length === 0) { + return prev; + } const selectedProviderByEnv: LLMProviderFormEntry["selectedProviderByEnv"] = {}; for (const env of environments) { selectedProviderByEnv[env.name] = { uuid: providerUuid, handle: providerHandle }; } @@ <Button @@ - disabled={providers.length === 0 && !!catalogData} + disabled={isEnvironmentsLoading || isCatalogLoading || providers.length === 0} > Add </Button>Also applies to: 437-470, 547-553, 597-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 393 - 395, Gate the Add/select flow until useListEnvironments (and related queries) are settled by checking the hook loading/error state and that environments is a non-empty array before enabling the Add button and before computing selectedProviderByEnv; in the onAdd/createNewEntry path, add a defensive guard that returns early (or shows validation) if environments is undefined/empty to avoid initializing selectedProviderByEnv = {} and emitting invalid modelConfig.envMappings. Ensure selectedProviderByEnv and any envMappings are constructed only after environments is available (derive them from environments.map(...) not from an empty default) so per-environment intent remains correct.
🧹 Nitpick comments (1)
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (1)
526-530: Prefer a stable key instead ofindexfor stateful entry cards.
EntryCardhas local state (selectedEnvIndex). Usingkey={index}can rebind state to the wrong entry after deletion/reorder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 526 - 530, The map over llmProviders uses key={index} which can cause EntryCard (which holds local state selectedEnvIndex) to lose or swap state after reorder/delete; change the key to a stable unique identifier from the entry object (e.g., entry.id or another immutable field like entry.providerKey or entry.name) in the llmProviders.map that renders EntryCard; if entries currently lack a stable id, add one when items are created (persisted or generated once) and use that property as the key so EntryCard instances keep stable identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 342-353: Default env-var names for each LLM entry currently all
use `${agentNameUpper}_URL` and `${agentNameUpper}_API_KEY` and the handlers
(handleUrlVarChange, handleApikeyVarChange) accept arbitrary values; change this
so each entry gets a unique default (e.g., derive from entry.id or index when
rendering the TextField value for urlVarName/apikeyVarName) and add validation
in handleUrlVarChange and handleApikeyVarChange to reject/normalize invalid
env-var names (allow only A-Z0-9 and underscores, start with a letter, no
collisions among entries) before updating entry state so only valid,
collision-free names are stored and forwarded.
- Around line 393-395: Gate the Add/select flow until useListEnvironments (and
related queries) are settled by checking the hook loading/error state and that
environments is a non-empty array before enabling the Add button and before
computing selectedProviderByEnv; in the onAdd/createNewEntry path, add a
defensive guard that returns early (or shows validation) if environments is
undefined/empty to avoid initializing selectedProviderByEnv = {} and emitting
invalid modelConfig.envMappings. Ensure selectedProviderByEnv and any
envMappings are constructed only after environments is available (derive them
from environments.map(...) not from an empty default) so per-environment intent
remains correct.
---
Nitpick comments:
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 526-530: The map over llmProviders uses key={index} which can
cause EntryCard (which holds local state selectedEnvIndex) to lose or swap state
after reorder/delete; change the key to a stable unique identifier from the
entry object (e.g., entry.id or another immutable field like entry.providerKey
or entry.name) in the llmProviders.map that renders EntryCard; if entries
currently lack a stable id, add one when items are created (persisted or
generated once) and use that property as the key so EntryCard instances keep
stable identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2faf873-6e0a-4530-8d2b-c2b414ff22e1
📒 Files selected for processing (4)
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.tsconsole/workspaces/pages/add-new-project/src/components/ProjectForm.tsx
✅ Files skipped from review due to trivial changes (1)
- console/workspaces/pages/add-new-project/src/components/ProjectForm.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- console/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.ts
- console/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (3)
556-561: Consider adding cleanup on unmount.The debounce timer is cleared on drawer close and provider select, but if the component unmounts while the drawer is open with a pending timer, it could fire after unmount. Adding a cleanup effect would be more robust.
🧹 Suggested cleanup effect
+import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; -import React, { useCallback, useMemo, useRef, useState } from "react"; @@ const handleSearchChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { const val = e.target.value; setProviderSearchQuery(val); if (searchTimerRef.current) clearTimeout(searchTimerRef.current); searchTimerRef.current = setTimeout(() => setDebouncedSearch(val), 250); }, []); + useEffect(() => { + return () => { + if (searchTimerRef.current) { + clearTimeout(searchTimerRef.current); + } + }; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 556 - 561, The debounce timer started in handleSearchChange (uses searchTimerRef and setDebouncedSearch) needs a cleanup on unmount to avoid firing after the component is destroyed; add a useEffect that returns a cleanup function which checks searchTimerRef.current and calls clearTimeout on it (and sets it to null) so any pending timer is cleared when the component unmounts.
272-281: Misleading variable nameguardrailsfor a boolean check.The variable
guardrailsis assigned the boolean result of.some(), but the name suggests it's an array. While the logic is correct (returning early if a duplicate exists), the naming is confusing.✏️ Suggested rename for clarity
const handleAddGuardrail = useCallback( (guardrail: GuardrailSelection) => { - const guardrails = entry.guardrails.some( + const isDuplicate = entry.guardrails.some( (g) => g.name === guardrail.name && g.version === guardrail.version) - if (guardrails) return; + if (isDuplicate) return; onUpdateEntry(index, { ...entry, guardrails: [...entry.guardrails, guardrail] }); }, [index, entry, onUpdateEntry], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 272 - 281, The boolean result of entry.guardrails.some(...) in handleAddGuardrail is named guardrails which is misleading; rename it to a clearer identifier like isDuplicate or alreadyExists (used in the duplicate-check and early return) and update the subsequent if check accordingly in the handleAddGuardrail callback so entry.guardrails, GuardrailSelection, onUpdateEntry, and index usages remain unchanged.
578-592: Consider using stable keys instead of array index.Using
indexas the React key can cause unexpected behavior when entries are removed, as React may incorrectly preserve state for shifted items. IfLLMProviderFormEntrydoesn't have a unique ID, consider generating one (e.g.,crypto.randomUUID()) when creating new entries inhandleProviderSelect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 578 - 592, The EntryCard list is using the array index as key which can lead to UI/state bugs; update the list to use a stable unique identifier (e.g., entry.id) as the key and ensure every LLMProviderFormEntry has that id: modify the provider creation flow in handleProviderSelect to assign a UUID (crypto.randomUUID()) to new entries, update any initialization code that constructs entries to also populate id for legacy/seeded entries, and keep EntryCard key={entry.id} so handleRemoveEntry/handleUpdateEntry and rendering remain stable when items are added/removed.console/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsx (1)
154-170: Consider extracting the conflict detection logic to avoid duplication.The logic for computing
agentNameUpperand deriving LLM variable names is duplicated here and inInternalAgentForm.tsx(lines 418-428). If these implementations drift apart, conflict detection could become inconsistent between the submit button state and the visual error indicators.♻️ Suggested extraction
Create a utility function in the
form/orutils/directory:// e.g., in utils/llmVarNames.ts export function deriveLLMVarNames( llmProviders: LLMProviderFormEntry[], displayName: string ): string[] { const agentNameUpper = displayName ? displayName.toUpperCase().replace(/[^A-Z0-9]/g, "_") : "AGENT"; return llmProviders.flatMap((e, i) => [ e.urlVarName ?? `${agentNameUpper}_${i + 1}_URL`, e.apikeyVarName ?? `${agentNameUpper}_${i + 1}_API_KEY`, ]); } export function hasLLMVarConflicts( llmProviders: LLMProviderFormEntry[], displayName: string, envKeys: string[] ): boolean { const llmNames = deriveLLMVarNames(llmProviders, displayName); const llmNameSet = new Set(llmNames); if (llmNames.length !== llmNameSet.size) return true; if (envKeys.length !== new Set(envKeys).size) return true; return envKeys.some((k) => llmNameSet.has(k)); }Then use it in both
InternalAgentFlow.tsxandInternalAgentForm.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsx` around lines 154 - 170, Extract the duplicated LLM variable name and conflict-detection logic into a shared utility (e.g., export deriveLLMVarNames and hasLLMVarConflicts) and replace the inline computation in InternalAgentFlow (the IIFE that reads llmProviders and formData.displayName, builds llmNames/llmNameSet and checks env keys) and the matching block in InternalAgentForm with calls to the new helpers; deriveLLMVarNames should accept (llmProviders, displayName) and return the flat list of var names, and hasLLMVarConflicts should accept (llmProviders, displayName, envKeys) and return the boolean used for hasLLMVarConflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@console/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsx`:
- Around line 154-170: Extract the duplicated LLM variable name and
conflict-detection logic into a shared utility (e.g., export deriveLLMVarNames
and hasLLMVarConflicts) and replace the inline computation in InternalAgentFlow
(the IIFE that reads llmProviders and formData.displayName, builds
llmNames/llmNameSet and checks env keys) and the matching block in
InternalAgentForm with calls to the new helpers; deriveLLMVarNames should accept
(llmProviders, displayName) and return the flat list of var names, and
hasLLMVarConflicts should accept (llmProviders, displayName, envKeys) and return
the boolean used for hasLLMVarConflicts.
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 556-561: The debounce timer started in handleSearchChange (uses
searchTimerRef and setDebouncedSearch) needs a cleanup on unmount to avoid
firing after the component is destroyed; add a useEffect that returns a cleanup
function which checks searchTimerRef.current and calls clearTimeout on it (and
sets it to null) so any pending timer is cleared when the component unmounts.
- Around line 272-281: The boolean result of entry.guardrails.some(...) in
handleAddGuardrail is named guardrails which is misleading; rename it to a
clearer identifier like isDuplicate or alreadyExists (used in the
duplicate-check and early return) and update the subsequent if check accordingly
in the handleAddGuardrail callback so entry.guardrails, GuardrailSelection,
onUpdateEntry, and index usages remain unchanged.
- Around line 578-592: The EntryCard list is using the array index as key which
can lead to UI/state bugs; update the list to use a stable unique identifier
(e.g., entry.id) as the key and ensure every LLMProviderFormEntry has that id:
modify the provider creation flow in handleProviderSelect to assign a UUID
(crypto.randomUUID()) to new entries, update any initialization code that
constructs entries to also populate id for legacy/seeded entries, and keep
EntryCard key={entry.id} so handleRemoveEntry/handleUpdateEntry and rendering
remain stable when items are added/removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 471a8c6e-4a11-41f3-8cfc-af36bbacb982
📒 Files selected for processing (5)
console/workspaces/pages/add-new-agent/src/components/CreateButtons.tsxconsole/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsxconsole/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- console/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsx
menakaj
force-pushed
the
agent-llm-config-create
branch
2 times, most recently
from
eadf7e1 to
b171e93
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
agent-manager-service/docs/api_v1_openapi.yaml (1)
6529-6534:⚠️ Potential issue | 🟡 MinorDocument duplicate-key rejection for
environmentVariablesto match existing API contract.Line 6531 still omits the duplicate-key behavior (
400) that is documented onCreateAgentModelConfigRequest.environmentVariables;uniqueItems: truealone does not communicate key-level uniqueness.📝 Suggested doc fix
environmentVariables: type: array - description: Optional custom environment variable names exposed to the agent + description: Optional custom environment variable names exposed to the agent. Duplicate keys are rejected with 400. uniqueItems: true items: $ref: "#/components/schemas/EnvironmentVariableConfig"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/docs/api_v1_openapi.yaml` around lines 6529 - 6534, Add documentation that environmentVariables enforces key-level uniqueness and duplicate keys will be rejected with a 400 error to match the existing API contract for CreateAgentModelConfigRequest.environmentVariables; update the schema block for environmentVariables (and/or its description) to explicitly state "duplicate keys are rejected (HTTP 400)" and ensure any relevant response examples or the CreateAgentModelConfigRequest schema mention the same behavior so the contract is consistent.agent-manager-service/services/agent_manager.go (1)
759-775:⚠️ Potential issue | 🟠 MajorGive these LLM configs a full rollback/delete lifecycle.
After the first
Createsucceeds, any later failure leaves the earlier${agent}-llm-config*rows behind because this helper only returnserror; the later rollback paths inCreateAgentandDeleteAgentstill only clean up the component, secrets, andagentConfigRepodata. Please either delete partial creations here or return the created config IDs so the caller can clean them up too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/services/agent_manager.go` around lines 759 - 775, createAgentLLMConfigs currently creates multiple agent LLM configs via agentConfigurationService.Create but on a later error leaves previously created `${agent}-llm-config*` rows orphaned; update createAgentLLMConfigs to either (A) track created config identifiers returned from agentConfigurationService.Create and return them to the caller (so CreateAgent/DeleteAgent can perform full rollback), or (B) perform immediate cleanup on failure by calling agentConfigurationService.Delete (or the appropriate delete method) for each successfully created config before returning the error; reference createAgentLLMConfigs, agentConfigurationService.Create, and the higher-level CreateAgent/DeleteAgent flows when making the change.console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx (1)
502-509:⚠️ Potential issue | 🟡 MinorGive provider rows a stable id.
The add path derives
urlVarName/apikeyVarNamefromprev.length + 1, so deleting an earlier row and adding another can recreate an existing suffix. The render path also useskey={index}, which letsEntryCard's localselectedEnvIndexstate jump to the wrong provider after a delete. A stable per-entry id fixes both problems.Also applies to: 579-580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx` around lines 502 - 509, The provider entries need a stable unique id to avoid colliding suffixes and key/index-related state jumps: when adding in the reducer where you build the new entry (the object containing selectedProviderByEnv, urlVarName, apikeyVarName, guardrails), generate and include a stable id field (e.g., id: uuid() or a monotonic counter) and use that id to derive urlVarName/apikeyVarName instead of using prev.length + 1; then change the render so EntryCard uses key={entry.id} (and anywhere relying on index for keeping selectedEnvIndex) to prevent selectedEnvIndex from jumping after deletes. Apply the same id usage where new entries are created in the other add path referenced around the 579-580 area.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@console/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsx`:
- Around line 95-103: Normalize env variable keys the same way
buildAgentCreationPayload does before running duplicate and reserved-name
checks: inside the envVariables.map block (where keyError is computed) compute a
normalizedKey by collapsing whitespace to underscores (matching
buildAgentCreationPayload) and use normalizedKey for checking against
llmReservedNames and sibling keys (build siblingNormalizedKeys from envVariables
excluding current index). Update the Duplicate key and LLM-provider conflict
checks to reference normalizedKey instead of the raw item.key so collisions like
"FOO BAR" vs "FOO_BAR" are caught early.
---
Duplicate comments:
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 6529-6534: Add documentation that environmentVariables enforces
key-level uniqueness and duplicate keys will be rejected with a 400 error to
match the existing API contract for
CreateAgentModelConfigRequest.environmentVariables; update the schema block for
environmentVariables (and/or its description) to explicitly state "duplicate
keys are rejected (HTTP 400)" and ensure any relevant response examples or the
CreateAgentModelConfigRequest schema mention the same behavior so the contract
is consistent.
In `@agent-manager-service/services/agent_manager.go`:
- Around line 759-775: createAgentLLMConfigs currently creates multiple agent
LLM configs via agentConfigurationService.Create but on a later error leaves
previously created `${agent}-llm-config*` rows orphaned; update
createAgentLLMConfigs to either (A) track created config identifiers returned
from agentConfigurationService.Create and return them to the caller (so
CreateAgent/DeleteAgent can perform full rollback), or (B) perform immediate
cleanup on failure by calling agentConfigurationService.Delete (or the
appropriate delete method) for each successfully created config before returning
the error; reference createAgentLLMConfigs, agentConfigurationService.Create,
and the higher-level CreateAgent/DeleteAgent flows when making the change.
In
`@console/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsx`:
- Around line 502-509: The provider entries need a stable unique id to avoid
colliding suffixes and key/index-related state jumps: when adding in the reducer
where you build the new entry (the object containing selectedProviderByEnv,
urlVarName, apikeyVarName, guardrails), generate and include a stable id field
(e.g., id: uuid() or a monotonic counter) and use that id to derive
urlVarName/apikeyVarName instead of using prev.length + 1; then change the
render so EntryCard uses key={entry.id} (and anywhere relying on index for
keeping selectedEnvIndex) to prevent selectedEnvIndex from jumping after
deletes. Apply the same id usage where new entries are created in the other add
path referenced around the 579-580 area.
🪄 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: 07a880eb-f132-4501-ba54-703ab89f6a5a
⛔ Files ignored due to path filters (1)
console/common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
agent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/services/agent_manager.goagent-manager-service/spec/model_create_agent_request.goagent-manager-service/spec/model_model_config_request.goagent-manager-service/wiring/wire_gen.goconsole/workspaces/libs/types/src/api/agents.tsconsole/workspaces/pages/add-new-agent/package.jsonconsole/workspaces/pages/add-new-agent/src/components/CreateButtons.tsxconsole/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsxconsole/workspaces/pages/add-new-agent/src/components/ExternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/LLMProviderSection.tsxconsole/workspaces/pages/add-new-agent/src/form/schema.tsconsole/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.tsconsole/workspaces/pages/add-new-project/src/components/ProjectForm.tsx
✅ Files skipped from review due to trivial changes (4)
- console/workspaces/pages/add-new-agent/src/components/ExternalAgentFlow.tsx
- console/workspaces/pages/add-new-agent/package.json
- console/workspaces/pages/add-new-project/src/components/ProjectForm.tsx
- console/workspaces/pages/add-new-agent/src/forms/ExternalAgentForm.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- console/workspaces/pages/add-new-agent/src/components/CreateButtons.tsx
- console/workspaces/libs/types/src/api/agents.ts
- console/workspaces/pages/add-new-agent/src/components/InternalAgentFlow.tsx
- agent-manager-service/spec/model_model_config_request.go
Comment on lines
+95
to
+103
| {envVariables.length ? envVariables.map((item, index) => { | ||
| const siblingKeys = new Set( | ||
| envVariables.flatMap((e, i) => (i !== index && e.key ? [e.key] : [])), | ||
| ); | ||
| const keyError = item.key && llmReservedNames.has(item.key) | ||
| ? "Already used as an LLM provider variable" | ||
| : item.key && siblingKeys.has(item.key) | ||
| ? "Duplicate key" | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
Normalize env keys before running duplicate/reserved-name checks.
buildAgentCreationPayload later serializes data.env[*].key with whitespace collapsed to _, but keyError compares the raw input here. FOO BAR and FOO_BAR therefore pass validation in this component and only collide in the submitted payload, including the LLM-provider conflict check.
Suggested fix
+const normalizeEnvKey = (key?: string) => key?.replace(/\s+/g, "_");
+
<Box display="flex" flexDirection="column" py={2} gap={2}>
{envVariables.length ? envVariables.map((item, index) => {
+ const normalizedKey = normalizeEnvKey(item.key);
const siblingKeys = new Set(
- envVariables.flatMap((e, i) => (i !== index && e.key ? [e.key] : [])),
+ envVariables.flatMap((e, i) =>
+ i !== index && e.key ? [normalizeEnvKey(e.key)!] : [],
+ ),
);
- const keyError = item.key && llmReservedNames.has(item.key)
+ const keyError = normalizedKey && llmReservedNames.has(normalizedKey)
? "Already used as an LLM provider variable"
- : item.key && siblingKeys.has(item.key)
+ : normalizedKey && siblingKeys.has(normalizedKey)
? "Duplicate key"
: undefined;
return (
<EnvVariableEditor
@@
onKeyChange={(value) => handleInitialEdit('key', value)}
onValueChange={(value) => handleInitialEdit('value', value)}
onSensitiveChange={(value: boolean) => handleInitialEdit('isSensitive', value)}
onRemove={() => handleRemove(0)}
- keyError={envVariables?.[0]?.key && llmReservedNames.has(envVariables[0].key!) ? "Already used as an LLM provider variable" : undefined}
+ keyError={(() => {
+ const normalizedKey = normalizeEnvKey(envVariables?.[0]?.key);
+ return normalizedKey && llmReservedNames.has(normalizedKey)
+ ? "Already used as an LLM provider variable"
+ : undefined;
+ })()}
/>
}Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@console/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsx`
around lines 95 - 103, Normalize env variable keys the same way
buildAgentCreationPayload does before running duplicate and reserved-name
checks: inside the envVariables.map block (where keyError is computed) compute a
normalizedKey by collapsing whitespace to underscores (matching
buildAgentCreationPayload) and use normalizedKey for checking against
llmReservedNames and sibling keys (build siblingNormalizedKeys from envVariables
excluding current index). Update the Duplicate key and LLM-provider conflict
checks to reference normalizedKey instead of the raw item.key so collisions like
"FOO BAR" vs "FOO_BAR" are caught early.
menakaj
force-pushed
the
agent-llm-config-create
branch
from
b171e93 to
527d4e5
Compare
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.
Purpose
Fixes #575
Env variable uniqueness validation
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Improvements
Documentation/Types