Agent config api key management#1195
Merged
Merged
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:
📝 WalkthroughWalkthroughThe PR adds API-key listing and management support for LLM providers, LLM proxies, MCP proxies, and per-environment agent configs. It also updates shared models, generated clients, console pages, backend services and routes, and deployment scope allowlists. ChangesAPI key management across agent manager and console
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/services/mcp_proxy_service.go (1)
385-397: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not revoke keys when redeploy fails.
If
redeployMCPProxyToCurrentGatewaysfails, the live gateway may still be enforcing the old API-key security config. The new revoke block then removes the user-managed keys clients still need, causing an outage. Gate revocation on a successful redeploy or return the redeploy failure.Proposed fix
- if err := s.redeployMCPProxyToCurrentGateways(ctx, updated, orgUUID); err != nil { + redeployed := true + if err := s.redeployMCPProxyToCurrentGateways(ctx, updated, orgUUID); err != nil { + redeployed = false s.logger.Warn("Failed to redeploy updated MCP proxy", "proxyID", updated.UUID, "orgName", orgUUID, "error", err) } @@ - if apiKeyAuthDisabled { + if apiKeyAuthDisabled && redeployed {🤖 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 `@agent-manager-service/services/mcp_proxy_service.go` around lines 385 - 397, Prevent the key-revocation path in mcp_proxy_service.go from running when redeployMCPProxyToCurrentGateways fails. Update the update flow around redeployMCPProxyToCurrentGateways and apiKeyBroadcaster.broadcastRevokeUserManaged so that a failed redeploy either returns immediately with the error or otherwise skips the apiKeyAuthDisabled revoke block, ensuring user-managed keys are only revoked after a successful redeploy.
🧹 Nitpick comments (4)
console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts (1)
138-145: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider scoping cache invalidation to the specific proxy.
The
onSuccesshandlers invalidate all["mcp-proxy-api-keys"]queries. Since the list query key includesorgNameandproxyId, mutations will correctly refetch only the affected proxy's list if you invalidate the exact key["mcp-proxy-api-keys", orgName, proxyId]rather than the broad prefix.Using the broad prefix is acceptable if multiple proxy key lists may be active simultaneously, but it can cause unnecessary refetches across all open MCP proxy tabs. If the mutation hooks accepted the specific params, you could narrow the invalidation.
🤖 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 `@console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts` around lines 138 - 145, The mutation success handlers are invalidating the broad mcp-proxy API key cache instead of the specific proxy list. Update the relevant mutation hooks and their onSuccess invalidation logic to use the exact query key shape from useListMCPProxyAPIKeys, including orgName and proxyId, so only the affected proxy’s list refetches. Locate the cache invalidation calls tied to the mcp-proxy-api-keys query key and pass through the specific params from the mutation context or inputs.agent-manager-service/controllers/llm_provider_apikey_controller.go (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Debugfor the request-start log.This list endpoint will fire on every tab load/refetch, so logging the happy-path start at
Infowill add a lot of noise. KeepInfo/Errorfor rarer or higher-signal events and drop this one toDebug. As per coding guidelines, "Log with correlation context such as org, resource ID, and request ID; use Debug for hot paths, Info for rare events, and Error for destructive operations."🤖 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 `@agent-manager-service/controllers/llm_provider_apikey_controller.go` at line 60, Change the request-start log in ListLLMProviderAPIKeys from Info to Debug so this hot-path endpoint does not create noisy production logs. Keep the same correlation fields already used there, and make sure any other start-of-request logs in the same controller follow the same Debug-for-hot-path pattern while reserving Info/Error for higher-signal events.Source: Coding guidelines
console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts (1)
180-183: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScope cache invalidation to the mutated config/environment.
invalidateQueries({ queryKey: [LLM_CONFIG_API_KEY_QUERY_KEY] })will stale every config-scoped API-key list. These mutations already receiveorgName,projName,agentName,configId, andenvName, so invalidate the full key to avoid unnecessary refetches when multiple config tabs are mounted.Also applies to: 199-202, 213-216
🤖 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 `@console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts` around lines 180 - 183, The cache invalidation in the mutation success handlers is too broad and currently stales every API-key list for all configs. Update the onSuccess logic in the agent model config mutations to invalidate the full query key built from the existing mutation context values (orgName, projName, agentName, configId, and envName) instead of only [LLM_CONFIG_API_KEY_QUERY_KEY], so only the mutated config/environment is refetched. Apply the same scoped invalidation pattern in all affected handlers in agent-model-configs.console/workspaces/libs/api-client/src/hooks/llm-providers.ts (1)
477-480: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInvalidate the exact API-key query instead of the whole namespace.
These
invalidateQueriescalls use only the prefix key, so TanStack Query will mark every provider/proxy API-key list stale. Since the mutation variables already include the resource identifiers, invalidate the full scoped key instead to avoid unrelated tabs refetching.Suggested pattern
- onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["llm-provider-api-keys"] }); + onSuccess: (_data, variables) => { + queryClient.invalidateQueries({ + queryKey: [ + "llm-provider-api-keys", + variables.params.orgName, + variables.params.providerId, + ], + }); },- onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["llm-proxy-api-keys"] }); + onSuccess: (_data, variables) => { + queryClient.invalidateQueries({ + queryKey: [ + "llm-proxy-api-keys", + variables.params.orgName, + variables.params.projName, + variables.params.proxyId, + ], + }); },Also applies to: 494-496, 507-509, 535-537, 551-553, 563-565
🤖 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 `@console/workspaces/libs/api-client/src/hooks/llm-providers.ts` around lines 477 - 480, The `onSuccess` invalidations in the `llm-providers` hooks are using broad prefix keys, which causes every API-key list under that namespace to refetch. Update each affected mutation callback to invalidate the fully scoped API-key query key using the resource identifiers already available in the mutation variables, and apply the same fix in the other matching `onSuccess` blocks in this file so only the relevant provider/proxy cache entry is refreshed.
🤖 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 `@agent-manager-service/api/llm_provider_apikey_routes.go`:
- Line 27: The API key listing route is currently protected by the generic
provider read permission, which allows read-only provider viewers to enumerate
masked key metadata. Update the route registration for ListAPIKeys in
llm_provider_apikey_routes.go to use the API-key-specific permission already
present for this surface, or introduce a dedicated API-key read permission and
register it here. Keep the authorization enforced at route-registration time
alongside the other routes in this file.
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 961-963: The rotate-key handlers in
agent_configuration_controller.go are swallowing all JSON decode errors, so
malformed request bodies are treated like empty optional bodies and can silently
drop fields such as displayName and expiresAt. Update the request parsing around
spec.RotateLLMAPIKeyRequest to accept only io.EOF as the no-body case, and
return a 400 for any other json.NewDecoder(r.Body).Decode failure. Apply the
same validation to both rotate endpoints referenced by the shared decode
pattern, keeping the controller HTTP-only by mapping parse errors to HTTP
responses and leaving rotation logic outside the controller.
In `@agent-manager-service/db_migrations/migration_list.go`:
- Line 19: Update the migration gating so `latestVersion` does not enable
`migration026` while it still reclassifies all non-Agent artifacts as
`ConsoleManaged`. Adjust the `migration026` logic in
`migration026`/`026_reclassify_api_key_purpose.go` so historical provider,
proxy, and MCP keys that were user-facing are marked `UserManaged` instead of
hidden, and keep the list behavior in `migration_list.go` aligned with the
user-managed filter rollout.
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 5947-5961: The list API keys response schema is inconsistent with
the actual handler output, so update the OpenAPI contract for
listLLMProviderAPIKeys to return the structured ListAPIKeysResponse with masked
APIKeyInfo entries instead of an array of StoredAPIKey. Make the same schema
correction anywhere the API is duplicated in docs/api_v1_openapi.yaml so the
generated spec matches the backend and the console client expectations. Use the
listLLMProviderAPIKeys operation and ListAPIKeysResponse/APIKeyInfo as the
identifying symbols when updating the contract.
In `@agent-manager-service/services/agent_configuration_service.go`:
- Around line 558-560: The proxy identity lookup is using the display name
instead of the handle, so update the relevant return/serialization paths in
AgentConfigurationService to use the proxy handle from LLMProxyInfo rather than
Configuration.Name. In the matching logic around the EnvMappings loop, and the
other affected spots in this service, make sure you reference the proxy’s handle
field consistently with the existing key-management code that already uses
proxy.Handle.
- Around line 210-219: The config-scoped API key flows in
AgentConfigurationService currently let managed/internal agent proxies be
treated like external ones, so add a validation gate before Create/Rotate/Revoke
operations proceed. Update agentProxyAPIKeyPurpose and the related service
methods to return a dedicated sentinel when the target config is not external,
then have the controller map that sentinel to the appropriate response instead
of allowing user-managed key actions for internal agents. Ensure the check is
applied consistently across the Create*ConfigAPIKey, Rotate*, and Revoke* paths
in AgentConfigurationService.
In `@agent-manager-service/services/apikey_broadcaster.go`:
- Around line 199-217: `broadcastRevokeUserManaged` is doing repository reads
and gateway broadcasts without carrying request cancellation/deadlines. Update
`apiKeyBroadcaster.broadcastRevokeUserManaged` to accept `context.Context` as
the first argument, then pass it through to `apiKeyRepo.ListByArtifact`,
`gatewayRepo.GetByOrganizationID`, and `broadcastRevokeToGateways` so the whole
revoke flow can be canceled consistently.
In `@agent-manager-service/services/llm_provider_apikey_service.go`:
- Around line 56-70: The new API-key and revoke flow is not propagating request
context, so update `ListAPIKeys` and the bulk revoke path to pass `ctx` through
every I/O call. Add or switch to context-aware repository methods for
`providerRepo.GetByUUID`, `apiKeyRepo.ListByArtifact`, and the revoke helper
`broadcastRevokeUserManaged`, and thread `ctx` into those calls so DB/gateway
work respects cancellation and deadlines.
In `@agent-manager-service/services/llm_provider_service.go`:
- Around line 394-400: The pre-update provider lookup in the update flow is
being swallowed, which can leave apiKeyAuthWasEnabled at false and skip revoking
user-managed keys when API-key auth is disabled. In the provider update path
around s.resolveProvider and apiKeyAuthWasEnabled, stop treating the lookup as
best-effort and return the resolveProvider error immediately if it fails so the
update does not proceed with a stale snapshot. Keep the existing security
snapshot logic, but make the failure from the initial provider read visible to
the caller instead of continuing silently.
In `@agent-manager-service/services/llm_proxy_apikey_service.go`:
- Around line 60-69: `ListAPIKeys` is losing request cancellation because the
new repository calls in `LLMProxyAPIKeyService` do not pass the incoming
`context.Context`. Update the service method to thread `ctx` into both
`proxyRepo.GetByID` and `apiKeyRepo.ListByArtifact`, and ensure the repository
interfaces/implementations for those methods accept `context.Context` as the
first parameter so the DB I/O remains cancelable end to end.
- Around line 56-60: The proxy lookup in LLMProxyAPIKeyService.ListAPIKeys is
missing project scoping and currently resolves by orgID + proxyID only. Thread
projName from the controller through ListAPIKeys into the repository call, and
update the proxyRepo lookup to match org + project + proxy together so the
returned keys are limited to the caller’s project. Keep the existing
authorization flow intact, but ensure the target resource is validated against
the project identity rather than treating it as a wildcard.
In `@agent-manager-service/services/mcp_proxy_service.go`:
- Around line 448-456: ListAPIKeys currently ignores ctx when calling
apiKeyRepo.ListByArtifact, so cancellations and deadlines do not propagate to
the DB read. Add a context-aware repository method (with context.Context as the
first parameter) and update MCPProxyService.ListAPIKeys to call that new method
with ctx while keeping the existing proxy lookup and error handling intact.
In `@agent-manager-service/spec/api_mcpapi_keys.go`:
- Around line 205-336: The ListMCPProxyAPIKeysExecute client method is decoding
the API response into []StoredAPIKey, but the MCPProxyService.ListAPIKeys
endpoint returns a wrapped ListAPIKeysResponse object with a keys field. Update
the OpenAPI contract in docs/api_v1_openapi.yaml so the list endpoint uses the
wrapped response type, then regenerate the client so
ApiListMCPProxyAPIKeysRequest.Execute and ListMCPProxyAPIKeysExecute return and
decode the correct model instead of a raw slice.
In
`@console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx`:
- Around line 90-109: The expiration timestamp in APIKeysManager’s handleCreate
is forcing the date picker value to UTC by appending Z, which shifts the
selected local day for users in non-UTC time zones. Update the logic that builds
expiresAtRFC3339 so the selected date is interpreted as local end-of-day instead
of UTC, and keep the change scoped to the date handling in APIKeysManager while
preserving the existing defaultExpiry and canSubmit flow.
In
`@console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx`:
- Around line 51-82: The transient UI state in SingleAPIKeyManager is not scoped
to the current resource, so newKeyValue, deleteOpen, and regenerateOpen can
carry over when the backing config/env changes. Update SingleAPIKeyManager to
reset those states whenever the active resource changes, or add a scope key prop
that forces a remount/reset on boundary changes. Use the
SingleAPIKeyManagerProps interface and the component’s internal state handlers
as the main places to implement the reset, and have config-scoped callers pass a
stable key such as a config/env identifier.
In
`@console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx`:
- Around line 81-87: The LLMProviderAPIKeysTab logic is treating undefined
provider data as if API key auth were disabled, which shows the wrong
remediation when the provider fetch fails. Update LLMProviderAPIKeysTab to guard
on providerData before the apiKeyEnabled check, or pass the fetch error from
ViewLLMProvider and render an explicit error state instead of the Security-tab
message. Use the existing ViewLLMProvider and LLMProviderAPIKeysTab flow to
distinguish “no data because fetch failed” from “provider exists but API keys
are disabled.”
In `@console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx`:
- Around line 183-186: The AddMCPProxyForm flow is overwriting the fetched proxy
version with a hardcoded default, which causes the create payload to use the
wrong metadata. Update the logic in the form initialization path to derive
proxyVersion from result.serverInfo.version using getServerInfoValue, and only
fall back to a default when no version is present; keep the preview and payload
values consistent by changing the setProxyVersion call in AddMCPProxyForm rather
than hardcoding "v1.0.0".
In
`@console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx`:
- Around line 318-320: Gate the “No invoke URLs available” empty state in
MCPProxyOverviewTab behind the gateway query status so it only appears after
useListGateways has finished and succeeded. Update the branching around the
invoke URL fallback in MCPProxyOverviewTab to check the query’s loading/error
state first, and only render that Alert once gateway data has settled and there
are still no URLs to show.
In
`@console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsx`:
- Around line 186-231: The API key security form in MCPProxySecurityTab should
handle the "cookie" APIKeyLocation explicitly instead of treating every
non-query value as a header. Update the conditional rendering around the apiKey
branch so keyIn can show and preserve cookie-backed configs with an appropriate
label and input context, and avoid labeling cookie as "Header Key". Use the
existing symbols authenticationType, keyIn, KEY_LOCATION_OPTIONS, and the
keyValue field logic to make sure cookie values render correctly and are not
accidentally overwritten when the user edits the form.
---
Outside diff comments:
In `@agent-manager-service/services/mcp_proxy_service.go`:
- Around line 385-397: Prevent the key-revocation path in mcp_proxy_service.go
from running when redeployMCPProxyToCurrentGateways fails. Update the update
flow around redeployMCPProxyToCurrentGateways and
apiKeyBroadcaster.broadcastRevokeUserManaged so that a failed redeploy either
returns immediately with the error or otherwise skips the apiKeyAuthDisabled
revoke block, ensuring user-managed keys are only revoked after a successful
redeploy.
---
Nitpick comments:
In `@agent-manager-service/controllers/llm_provider_apikey_controller.go`:
- Line 60: Change the request-start log in ListLLMProviderAPIKeys from Info to
Debug so this hot-path endpoint does not create noisy production logs. Keep the
same correlation fields already used there, and make sure any other
start-of-request logs in the same controller follow the same Debug-for-hot-path
pattern while reserving Info/Error for higher-signal events.
In `@console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts`:
- Around line 180-183: The cache invalidation in the mutation success handlers
is too broad and currently stales every API-key list for all configs. Update the
onSuccess logic in the agent model config mutations to invalidate the full query
key built from the existing mutation context values (orgName, projName,
agentName, configId, and envName) instead of only
[LLM_CONFIG_API_KEY_QUERY_KEY], so only the mutated config/environment is
refetched. Apply the same scoped invalidation pattern in all affected handlers
in agent-model-configs.
In `@console/workspaces/libs/api-client/src/hooks/llm-providers.ts`:
- Around line 477-480: The `onSuccess` invalidations in the `llm-providers`
hooks are using broad prefix keys, which causes every API-key list under that
namespace to refetch. Update each affected mutation callback to invalidate the
fully scoped API-key query key using the resource identifiers already available
in the mutation variables, and apply the same fix in the other matching
`onSuccess` blocks in this file so only the relevant provider/proxy cache entry
is refreshed.
In `@console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts`:
- Around line 138-145: The mutation success handlers are invalidating the broad
mcp-proxy API key cache instead of the specific proxy list. Update the relevant
mutation hooks and their onSuccess invalidation logic to use the exact query key
shape from useListMCPProxyAPIKeys, including orgName and proxyId, so only the
affected proxy’s list refetches. Locate the cache invalidation calls tied to the
mcp-proxy-api-keys query key and pass through the specific params from the
mutation context or inputs.
🪄 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: 7b74697b-753a-4391-a0b2-cdf3f4ef6775
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (67)
agent-manager-service/api/agent_config_routes.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/db_migrations/026_reclassify_api_key_purpose.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/agent_configuration_dto.goagent-manager-service/models/apikey.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcp_proxies.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/client.goagent-manager-service/spec/model_mcp_policy.goagent-manager-service/spec/model_mcp_policy_availability_response.goagent-manager-service/spec/model_mcp_policy_available_item.goagent-manager-service/spec/model_mcp_proxy_capabilities.goagent-manager-service/spec/model_mcp_proxy_list_item.goagent-manager-service/spec/model_mcp_proxy_list_response.goagent-manager-service/spec/model_mcp_proxy_request.goagent-manager-service/spec/model_mcp_proxy_response.goagent-manager-service/spec/model_mcp_server_info_fetch_request.goagent-manager-service/spec/model_mcp_server_info_fetch_response.goagent-manager-service/wiring/wire_gen.goconsole/workspaces/libs/api-client/src/apis/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/apis/agent-model-configs.tsconsole/workspaces/libs/api-client/src/apis/llm-providers.tsconsole/workspaces/libs/api-client/src/apis/mcp-proxies.tsconsole/workspaces/libs/api-client/src/hooks/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/hooks/agent-model-configs.tsconsole/workspaces/libs/api-client/src/hooks/llm-providers.tsconsole/workspaces/libs/api-client/src/hooks/mcp-proxies.tsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/types/src/api/agent-mcp-configs.tsconsole/workspaces/libs/types/src/api/agent-model-configs.tsconsole/workspaces/libs/types/src/api/common.tsconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/libs/types/src/api/mcp-proxies.tsconsole/workspaces/pages/agent-security/src/Security.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsx
💤 Files with no reviewable changes (1)
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsx
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
128d324 to
10f3d1d
Compare
jhivandb
reviewed
Jun 26, 2026
jhivandb
left a comment
jhivandb
left a comment
Contributor
There was a problem hiding this comment.
🤖 Automated review by an agent (Claude Code /simplify) — backend Go only; console/UI and generated code skipped. These are quality/simplification observations (reuse, dedup, efficiency), not correctness-bug findings. Four inline comments below.
jhivandb
reviewed
Jun 26, 2026
jhivandb
left a comment
jhivandb
left a comment
Contributor
There was a problem hiding this comment.
🤖 Automated review by an agent (Claude Code) — follow-up. Two deeper, larger-scope suggestions (altitude/efficiency). Unlike the previous batch these touch code beyond the immediate diff (an interface addition / a wiring change), so treat them as design notes rather than quick cleanups.
Oshanath
force-pushed
the
agent-config-api-key-management
branch
2 times, most recently
from
d5224cb to
6c80680
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/services/platform_gateway_service.go (1)
681-706: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
ctxthrough the single-gateway branch too.Lines 691-694 still call
gatewayRepo.GetByUUIDwithoutctx, so requests with a specificgatewayIDremain uncancelable even though the list path was fixed at Line 706. Threadctxinto that repository API as well.As per coding guidelines, "Every I/O method must take
context.Contextas the first parameter and propagate it."🤖 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 `@agent-manager-service/services/platform_gateway_service.go` around lines 681 - 706, The single-gateway lookup path in GetGatewayStatus still calls gatewayRepo.GetByUUID without passing ctx, so it bypasses cancellation/timeouts unlike the organization list path. Update the GetGatewayStatus branch that handles gatewayID to thread ctx into the repository call, and make sure the gatewayRepo.GetByUUID API itself accepts context.Context as its first parameter and propagates it through the data access layer.Source: Coding guidelines
agent-manager-service/services/llm_proxy_apikey_service.go (1)
112-124: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the same project scoping to create, revoke, and rotate.
The list path now validates
(org, project, proxy), but these lifecycle operations still resolve byorgID + proxyID. ThreadprojNamethrough these methods and reuseresolveProjectScopedProxybefore broadcasting.As per coding guidelines,
Validate caller organization against the target resource for multi-tenant safety; missing tenant identity is an error, not a wildcard.Also applies to: 128-139, 144-156
🤖 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 `@agent-manager-service/services/llm_proxy_apikey_service.go` around lines 112 - 124, CreateAPIKey still resolves the proxy using only orgID and proxyID, so it bypasses the project-scoped validation now used by the list flow. Update CreateAPIKey, RevokeAPIKey, and RotateAPIKey to accept projName, call resolveProjectScopedProxy before any broadcaster method, and pass the resolved proxy UUID into broadcastCreate/broadcastRevoke/broadcastRotate. Keep the existing not-found/error behavior aligned with the project-scoped lookup so tenant identity is always validated.Source: Coding guidelines
♻️ Duplicate comments (2)
agent-manager-service/services/llm_proxy_apikey_service.go (1)
79-79: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
ctxinto the project-scoped proxy lookup.
resolveProjectScopedProxyreceivesctx, but this DB lookup drops it, so cancellation/deadlines stop before the repository call.Suggested fix
- proxy, err := s.proxyRepo.GetByIDAndProject(proxyID, orgID, project.UUID) + proxy, err := s.proxyRepo.GetByIDAndProject(ctx, proxyID, orgID, project.UUID)As per coding guidelines,
Every I/O method must take context.Context as the first parameter and propagate it.🤖 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 `@agent-manager-service/services/llm_proxy_apikey_service.go` at line 79, The project-scoped proxy lookup in resolveProjectScopedProxy is ignoring the incoming context, so cancellation and deadlines are not propagated into the repository call. Update the GetByIDAndProject invocation on s.proxyRepo to pass ctx as the first argument, matching the context-first convention used by I/O methods, and ensure the rest of the lookup flow in resolveProjectScopedProxy continues to use that same ctx.Source: Coding guidelines
agent-manager-service/services/llm_provider_apikey_service.go (1)
59-67: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep provider lookups cancelable.
These new paths take
ctx, but the provider repository lookup still does not receive it. Switch to a context-awareGetByUUID(ctx, ...)before listing or bulk-revoking keys.Suggested fix
- provider, err := s.providerRepo.GetByUUID(providerID, orgID) + provider, err := s.providerRepo.GetByUUID(ctx, providerID, orgID)As per coding guidelines,
Every I/O method must take context.Context as the first parameter and propagate it.Also applies to: 112-119
🤖 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 `@agent-manager-service/services/llm_provider_apikey_service.go` around lines 59 - 67, The provider lookup in the LLM key service is still not cancelable because it calls providerRepo.GetByUUID without the ctx that was already added to this flow. Update the lookup in the relevant service method(s) to use the context-aware GetByUUID(ctx, ...) before any key listing or bulk-revocation work, and propagate ctx consistently through the affected paths so the repository I/O follows the coding guideline. Use the existing symbols providerRepo, GetByUUID, ListByArtifact, and the bulk-revoke code in the same service to locate and update the related call sites.Source: Coding guidelines
🧹 Nitpick comments (1)
agent-manager-service/services/mcp_proxy_apikey_service.go (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftInject a narrow gateway-events interface instead of
*GatewayEventsService.This constructor couples the new service to another concrete service implementation, which makes the service layer harder to test and wire independently. As per coding guidelines,
agent-manager-service/services/**: Services must contain business logic only: validation gates, orchestration, and error mapping; depend on repository/client interfaces, never concrete types.🤖 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 `@agent-manager-service/services/mcp_proxy_apikey_service.go` around lines 39 - 53, The constructor currently depends on the concrete GatewayEventsService, which makes MCPProxyAPIKeyService tightly coupled and harder to test. Update NewMCPProxyAPIKeyService and the apiKeyBroadcaster wiring to accept a narrow gateway-events interface instead, and have the broadcaster depend only on that interface plus the repository interfaces it already uses. Keep the change localized to the MCPProxyAPIKeyService and apiKeyBroadcaster symbols so the service layer remains decoupled from concrete implementations.Source: Coding guidelines
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 924-932: The list endpoints in the agent configuration controller
are not translating ErrAgentConfigNotExternal, so they fall through to a 500
instead of a 403. Update the ListMCPConfigAPIKeys flow and the matching
ListLLMConfigAPIKeys handler to use the same sentinel-to-HTTP mapping as
create/rotate/revoke, either by reusing the shared error mapping helper or by
adding an explicit errors.Is(err, utils.ErrAgentConfigNotExternal) branch before
the generic internal-server fallback.
In `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go`:
- Around line 131-132: The MCP rotate handler is ignoring JSON decode errors and
can proceed with an empty specReq, so update the request parsing in
mcp_proxy_apikey_controller.go to validate the body in the controller. In the
rotate API key handler, treat io.EOF as the only acceptable empty-body case, but
return HTTP 400 for any other json.Decoder.Decode failure before calling the
rotate logic. Use the existing rotate request types and controller method that
handles the request to ensure malformed bodies never reach the update path.
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 3852-4051: The OpenAPI contract is missing the model-config
API-key endpoints that correspond to the controller’s
List/Create/Rotate/RevokeLLMConfigAPIKey operations. Add the matching
/orgs/{orgName}/projects/{projName}/agents/{agentName}/model-configs/{configId}/environments/{envName}/api-keys
and /{keyName} paths with the same path parameters, tags, request bodies,
operationIds, and response schemas as the existing MCP API-key routes, then
regenerate clients from docs/api_v1_openapi.yaml.
In `@agent-manager-service/repositories/apikey_repository.go`:
- Line 33: Update the ListByArtifact repository contract to require the caller
organization explicitly, because artifact UUID alone is not sufficient for
multi-tenant safety. Modify the ApikeyRepository interface and its
implementation method ListByArtifact to accept orgName alongside artifactUUID,
then update the query logic in the repository method to filter by both org
identity and artifact_uuid instead of treating tenant scope as implicit. Ensure
any call sites are updated to pass the caller org, using the existing repository
symbols ListByArtifact and ApikeyRepository as the main points of change.
In `@agent-manager-service/repositories/llm_proxy_repository.go`:
- Line 35: The new repository lookup method on LLMProxyRepository does not
follow the standard contract because GetByIDAndProject is missing
context.Context, does not use WithContext, and returns raw GORM errors. Update
GetByIDAndProject and its implementation in the repository to accept context as
the first parameter, call the query through r.db.WithContext(ctx), map
gorm.ErrRecordNotFound to the repo’s not-found sentinel, and wrap any other
database errors with a contextual fmt.Errorf using the method name so callers
only see repository-level errors.
In `@console/apps/web-ui/public/config.js`:
- Line 43: The rbacEnabled entry in config.js is using a self-comparison that
always evaluates to true and violates the noSelfCompare lint rule. Update the
config object to use a boolean literal directly for rbacEnabled instead of
comparing the same string value, and keep the change localized to the config
constant/object where rbacEnabled is defined.
In `@deployments/helm-charts/wso2-agent-manager/values.yaml`:
- Line 317: The console OAuth scope list in values.yaml is missing the MCP
API-key management permission, causing role-granted MCP proxy API-key actions to
be denied when rbacEnabled is true. Update the scopes value to include
amp:mcp-server:api-key-manage alongside the other MCP-related scopes, keeping
the existing scope string format intact.
In
`@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml`:
- Around line 1752-1770: The role lookup still hardcodes the old built-in admin
role, so update the bootstrap flow to resolve the renamed AMP role instead. In
the role-fetching block that sets ADMIN_ROLE_ID, change the search logic from
matching "name":"admin" to matching the AMP-specific "Agent Manager Admin" role
created by 61-amp-default-roles.sh, and keep the assignment call using that
resolved ID so the Agents Manager Admins group is attached to the scoped AMP
role.
- Around line 1593-1607: The role definitions in the bootstrap template now
include mcp-server:api-key-manage, but the corresponding MCP action is not being
provisioned yet. Update the resource setup in 60-amp-resource-server.sh to
create/register the mcp-server:api-key-manage permission before any calls to
set_role_permissions that assign it to roles such as Developer and AI Lead. Make
sure the new action is added in the same place and style as the other mcp-server
permissions so role validation and scope assignment both work.
---
Outside diff comments:
In `@agent-manager-service/services/llm_proxy_apikey_service.go`:
- Around line 112-124: CreateAPIKey still resolves the proxy using only orgID
and proxyID, so it bypasses the project-scoped validation now used by the list
flow. Update CreateAPIKey, RevokeAPIKey, and RotateAPIKey to accept projName,
call resolveProjectScopedProxy before any broadcaster method, and pass the
resolved proxy UUID into broadcastCreate/broadcastRevoke/broadcastRotate. Keep
the existing not-found/error behavior aligned with the project-scoped lookup so
tenant identity is always validated.
In `@agent-manager-service/services/platform_gateway_service.go`:
- Around line 681-706: The single-gateway lookup path in GetGatewayStatus still
calls gatewayRepo.GetByUUID without passing ctx, so it bypasses
cancellation/timeouts unlike the organization list path. Update the
GetGatewayStatus branch that handles gatewayID to thread ctx into the repository
call, and make sure the gatewayRepo.GetByUUID API itself accepts context.Context
as its first parameter and propagates it through the data access layer.
---
Duplicate comments:
In `@agent-manager-service/services/llm_provider_apikey_service.go`:
- Around line 59-67: The provider lookup in the LLM key service is still not
cancelable because it calls providerRepo.GetByUUID without the ctx that was
already added to this flow. Update the lookup in the relevant service method(s)
to use the context-aware GetByUUID(ctx, ...) before any key listing or
bulk-revocation work, and propagate ctx consistently through the affected paths
so the repository I/O follows the coding guideline. Use the existing symbols
providerRepo, GetByUUID, ListByArtifact, and the bulk-revoke code in the same
service to locate and update the related call sites.
In `@agent-manager-service/services/llm_proxy_apikey_service.go`:
- Line 79: The project-scoped proxy lookup in resolveProjectScopedProxy is
ignoring the incoming context, so cancellation and deadlines are not propagated
into the repository call. Update the GetByIDAndProject invocation on s.proxyRepo
to pass ctx as the first argument, matching the context-first convention used by
I/O methods, and ensure the rest of the lookup flow in resolveProjectScopedProxy
continues to use that same ctx.
---
Nitpick comments:
In `@agent-manager-service/services/mcp_proxy_apikey_service.go`:
- Around line 39-53: The constructor currently depends on the concrete
GatewayEventsService, which makes MCPProxyAPIKeyService tightly coupled and
harder to test. Update NewMCPProxyAPIKeyService and the apiKeyBroadcaster wiring
to accept a narrow gateway-events interface instead, and have the broadcaster
depend only on that interface plus the repository interfaces it already uses.
Keep the change localized to the MCPProxyAPIKeyService and apiKeyBroadcaster
symbols so the service layer remains decoupled from concrete implementations.
🪄 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: a4de78f8-82d1-4130-bd6f-68511605c42f
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (95)
agent-manager-service/api/agent_config_routes.goagent-manager-service/api/app.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/db_migrations/026_reclassify_api_key_purpose.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/agent_configuration_dto.goagent-manager-service/models/apikey.goagent-manager-service/rbac/permissions.goagent-manager-service/rbac/predefined_roles.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/mcp_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcp_proxies.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/client.goagent-manager-service/spec/model_api_key_info.goagent-manager-service/spec/model_list_api_keys_response.goagent-manager-service/spec/model_mcp_policy.goagent-manager-service/spec/model_mcp_policy_availability_response.goagent-manager-service/spec/model_mcp_policy_available_item.goagent-manager-service/spec/model_mcp_proxy_capabilities.goagent-manager-service/spec/model_mcp_proxy_list_item.goagent-manager-service/spec/model_mcp_proxy_list_response.goagent-manager-service/spec/model_mcp_proxy_request.goagent-manager-service/spec/model_mcp_proxy_response.goagent-manager-service/spec/model_mcp_server_info_fetch_request.goagent-manager-service/spec/model_mcp_server_info_fetch_response.goagent-manager-service/spec/model_security_summary.goagent-manager-service/utils/errors.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/workspaces/libs/api-client/src/apis/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/apis/agent-model-configs.tsconsole/workspaces/libs/api-client/src/apis/llm-providers.tsconsole/workspaces/libs/api-client/src/apis/mcp-proxies.tsconsole/workspaces/libs/api-client/src/hooks/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/hooks/agent-model-configs.tsconsole/workspaces/libs/api-client/src/hooks/llm-providers.tsconsole/workspaces/libs/api-client/src/hooks/mcp-proxies.tsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/types/src/api/agent-mcp-configs.tsconsole/workspaces/libs/types/src/api/agent-model-configs.tsconsole/workspaces/libs/types/src/api/common.tsconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/libs/types/src/api/mcp-proxies.tsconsole/workspaces/pages/agent-security/src/Security.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsxdeployments/docker-compose.ymldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamltest/e2e/framework/auth.go
💤 Files with no reviewable changes (4)
- test/e2e/framework/auth.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsx
- agent-manager-service/api/mcp_proxy_routes.go
- agent-manager-service/controllers/mcp_proxy_controller.go
✅ Files skipped from review due to trivial changes (16)
- agent-manager-service/spec/model_security_summary.go
- agent-manager-service/spec/model_mcp_policy_availability_response.go
- agent-manager-service/spec/model_list_api_keys_response.go
- agent-manager-service/spec/model_mcp_policy_available_item.go
- agent-manager-service/spec/model_mcp_server_info_fetch_response.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_capabilities.go
- agent-manager-service/spec/model_mcp_policy.go
- agent-manager-service/spec/model_mcp_proxy_response.go
- agent-manager-service/spec/model_mcp_server_info_fetch_request.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_request.go
- agent-manager-service/spec/model_mcp_proxy_list_item.go
- agent-manager-service/spec/api_mcp_proxies.go
- agent-manager-service/spec/api_mcpapi_keys.go
🚧 Files skipped from review as they are similar to previous changes (42)
- agent-manager-service/api/llm_proxy_apikey_routes.go
- console/workspaces/libs/shared-component/src/components/index.ts
- agent-manager-service/db_migrations/migration_list.go
- agent-manager-service/db_migrations/026_reclassify_api_key_purpose.go
- agent-manager-service/api/agent_config_routes.go
- console/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsx
- console/workspaces/libs/types/src/api/common.ts
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsx
- agent-manager-service/spec/client.go
- console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
- agent-manager-service/controllers/llm_provider_apikey_controller.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsx
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- console/workspaces/libs/types/src/api/mcp-proxies.ts
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsx
- agent-manager-service/spec/model_mcp_proxy_list_response.go
- console/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsx
- console/workspaces/libs/api-client/src/apis/llm-providers.ts
- console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsx
- agent-manager-service/services/llm_proxy_provisioner.go
- console/workspaces/libs/types/src/api/agent-mcp-configs.ts
- agent-manager-service/models/agent_configuration_dto.go
- console/workspaces/libs/api-client/src/apis/mcp-proxies.ts
- console/workspaces/pages/agent-security/src/Security.Component.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx
- console/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsx
- console/workspaces/libs/api-client/src/hooks/agent-mcp-configs.ts
- console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsx
- console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsx
- console/workspaces/libs/types/src/api/agent-model-configs.ts
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/models/apikey.go
- console/workspaces/libs/api-client/src/apis/agent-model-configs.ts
- console/workspaces/libs/api-client/src/apis/agent-mcp-configs.ts
- console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts
- console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts
- console/workspaces/libs/api-client/src/hooks/llm-providers.ts
- agent-manager-service/services/agent_configuration_service.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent-manager-service/services/platform_gateway_service.go (1)
681-706: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
ctxthrough the single-gateway branch too.Lines 691-694 still call
gatewayRepo.GetByUUIDwithoutctx, so requests with a specificgatewayIDremain uncancelable even though the list path was fixed at Line 706. Threadctxinto that repository API as well.As per coding guidelines, "Every I/O method must take
context.Contextas the first parameter and propagate it."🤖 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 `@agent-manager-service/services/platform_gateway_service.go` around lines 681 - 706, The single-gateway lookup path in GetGatewayStatus still calls gatewayRepo.GetByUUID without passing ctx, so it bypasses cancellation/timeouts unlike the organization list path. Update the GetGatewayStatus branch that handles gatewayID to thread ctx into the repository call, and make sure the gatewayRepo.GetByUUID API itself accepts context.Context as its first parameter and propagates it through the data access layer.Source: Coding guidelines
agent-manager-service/services/llm_proxy_apikey_service.go (1)
112-124: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply the same project scoping to create, revoke, and rotate.
The list path now validates
(org, project, proxy), but these lifecycle operations still resolve byorgID + proxyID. ThreadprojNamethrough these methods and reuseresolveProjectScopedProxybefore broadcasting.As per coding guidelines,
Validate caller organization against the target resource for multi-tenant safety; missing tenant identity is an error, not a wildcard.Also applies to: 128-139, 144-156
🤖 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 `@agent-manager-service/services/llm_proxy_apikey_service.go` around lines 112 - 124, CreateAPIKey still resolves the proxy using only orgID and proxyID, so it bypasses the project-scoped validation now used by the list flow. Update CreateAPIKey, RevokeAPIKey, and RotateAPIKey to accept projName, call resolveProjectScopedProxy before any broadcaster method, and pass the resolved proxy UUID into broadcastCreate/broadcastRevoke/broadcastRotate. Keep the existing not-found/error behavior aligned with the project-scoped lookup so tenant identity is always validated.Source: Coding guidelines
♻️ Duplicate comments (2)
agent-manager-service/services/llm_proxy_apikey_service.go (1)
79-79: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
ctxinto the project-scoped proxy lookup.
resolveProjectScopedProxyreceivesctx, but this DB lookup drops it, so cancellation/deadlines stop before the repository call.Suggested fix
- proxy, err := s.proxyRepo.GetByIDAndProject(proxyID, orgID, project.UUID) + proxy, err := s.proxyRepo.GetByIDAndProject(ctx, proxyID, orgID, project.UUID)As per coding guidelines,
Every I/O method must take context.Context as the first parameter and propagate it.🤖 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 `@agent-manager-service/services/llm_proxy_apikey_service.go` at line 79, The project-scoped proxy lookup in resolveProjectScopedProxy is ignoring the incoming context, so cancellation and deadlines are not propagated into the repository call. Update the GetByIDAndProject invocation on s.proxyRepo to pass ctx as the first argument, matching the context-first convention used by I/O methods, and ensure the rest of the lookup flow in resolveProjectScopedProxy continues to use that same ctx.Source: Coding guidelines
agent-manager-service/services/llm_provider_apikey_service.go (1)
59-67: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep provider lookups cancelable.
These new paths take
ctx, but the provider repository lookup still does not receive it. Switch to a context-awareGetByUUID(ctx, ...)before listing or bulk-revoking keys.Suggested fix
- provider, err := s.providerRepo.GetByUUID(providerID, orgID) + provider, err := s.providerRepo.GetByUUID(ctx, providerID, orgID)As per coding guidelines,
Every I/O method must take context.Context as the first parameter and propagate it.Also applies to: 112-119
🤖 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 `@agent-manager-service/services/llm_provider_apikey_service.go` around lines 59 - 67, The provider lookup in the LLM key service is still not cancelable because it calls providerRepo.GetByUUID without the ctx that was already added to this flow. Update the lookup in the relevant service method(s) to use the context-aware GetByUUID(ctx, ...) before any key listing or bulk-revocation work, and propagate ctx consistently through the affected paths so the repository I/O follows the coding guideline. Use the existing symbols providerRepo, GetByUUID, ListByArtifact, and the bulk-revoke code in the same service to locate and update the related call sites.Source: Coding guidelines
🧹 Nitpick comments (1)
agent-manager-service/services/mcp_proxy_apikey_service.go (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftInject a narrow gateway-events interface instead of
*GatewayEventsService.This constructor couples the new service to another concrete service implementation, which makes the service layer harder to test and wire independently. As per coding guidelines,
agent-manager-service/services/**: Services must contain business logic only: validation gates, orchestration, and error mapping; depend on repository/client interfaces, never concrete types.🤖 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 `@agent-manager-service/services/mcp_proxy_apikey_service.go` around lines 39 - 53, The constructor currently depends on the concrete GatewayEventsService, which makes MCPProxyAPIKeyService tightly coupled and harder to test. Update NewMCPProxyAPIKeyService and the apiKeyBroadcaster wiring to accept a narrow gateway-events interface instead, and have the broadcaster depend only on that interface plus the repository interfaces it already uses. Keep the change localized to the MCPProxyAPIKeyService and apiKeyBroadcaster symbols so the service layer remains decoupled from concrete implementations.Source: Coding guidelines
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 924-932: The list endpoints in the agent configuration controller
are not translating ErrAgentConfigNotExternal, so they fall through to a 500
instead of a 403. Update the ListMCPConfigAPIKeys flow and the matching
ListLLMConfigAPIKeys handler to use the same sentinel-to-HTTP mapping as
create/rotate/revoke, either by reusing the shared error mapping helper or by
adding an explicit errors.Is(err, utils.ErrAgentConfigNotExternal) branch before
the generic internal-server fallback.
In `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go`:
- Around line 131-132: The MCP rotate handler is ignoring JSON decode errors and
can proceed with an empty specReq, so update the request parsing in
mcp_proxy_apikey_controller.go to validate the body in the controller. In the
rotate API key handler, treat io.EOF as the only acceptable empty-body case, but
return HTTP 400 for any other json.Decoder.Decode failure before calling the
rotate logic. Use the existing rotate request types and controller method that
handles the request to ensure malformed bodies never reach the update path.
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 3852-4051: The OpenAPI contract is missing the model-config
API-key endpoints that correspond to the controller’s
List/Create/Rotate/RevokeLLMConfigAPIKey operations. Add the matching
/orgs/{orgName}/projects/{projName}/agents/{agentName}/model-configs/{configId}/environments/{envName}/api-keys
and /{keyName} paths with the same path parameters, tags, request bodies,
operationIds, and response schemas as the existing MCP API-key routes, then
regenerate clients from docs/api_v1_openapi.yaml.
In `@agent-manager-service/repositories/apikey_repository.go`:
- Line 33: Update the ListByArtifact repository contract to require the caller
organization explicitly, because artifact UUID alone is not sufficient for
multi-tenant safety. Modify the ApikeyRepository interface and its
implementation method ListByArtifact to accept orgName alongside artifactUUID,
then update the query logic in the repository method to filter by both org
identity and artifact_uuid instead of treating tenant scope as implicit. Ensure
any call sites are updated to pass the caller org, using the existing repository
symbols ListByArtifact and ApikeyRepository as the main points of change.
In `@agent-manager-service/repositories/llm_proxy_repository.go`:
- Line 35: The new repository lookup method on LLMProxyRepository does not
follow the standard contract because GetByIDAndProject is missing
context.Context, does not use WithContext, and returns raw GORM errors. Update
GetByIDAndProject and its implementation in the repository to accept context as
the first parameter, call the query through r.db.WithContext(ctx), map
gorm.ErrRecordNotFound to the repo’s not-found sentinel, and wrap any other
database errors with a contextual fmt.Errorf using the method name so callers
only see repository-level errors.
In `@console/apps/web-ui/public/config.js`:
- Line 43: The rbacEnabled entry in config.js is using a self-comparison that
always evaluates to true and violates the noSelfCompare lint rule. Update the
config object to use a boolean literal directly for rbacEnabled instead of
comparing the same string value, and keep the change localized to the config
constant/object where rbacEnabled is defined.
In `@deployments/helm-charts/wso2-agent-manager/values.yaml`:
- Line 317: The console OAuth scope list in values.yaml is missing the MCP
API-key management permission, causing role-granted MCP proxy API-key actions to
be denied when rbacEnabled is true. Update the scopes value to include
amp:mcp-server:api-key-manage alongside the other MCP-related scopes, keeping
the existing scope string format intact.
In
`@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml`:
- Around line 1752-1770: The role lookup still hardcodes the old built-in admin
role, so update the bootstrap flow to resolve the renamed AMP role instead. In
the role-fetching block that sets ADMIN_ROLE_ID, change the search logic from
matching "name":"admin" to matching the AMP-specific "Agent Manager Admin" role
created by 61-amp-default-roles.sh, and keep the assignment call using that
resolved ID so the Agents Manager Admins group is attached to the scoped AMP
role.
- Around line 1593-1607: The role definitions in the bootstrap template now
include mcp-server:api-key-manage, but the corresponding MCP action is not being
provisioned yet. Update the resource setup in 60-amp-resource-server.sh to
create/register the mcp-server:api-key-manage permission before any calls to
set_role_permissions that assign it to roles such as Developer and AI Lead. Make
sure the new action is added in the same place and style as the other mcp-server
permissions so role validation and scope assignment both work.
---
Outside diff comments:
In `@agent-manager-service/services/llm_proxy_apikey_service.go`:
- Around line 112-124: CreateAPIKey still resolves the proxy using only orgID
and proxyID, so it bypasses the project-scoped validation now used by the list
flow. Update CreateAPIKey, RevokeAPIKey, and RotateAPIKey to accept projName,
call resolveProjectScopedProxy before any broadcaster method, and pass the
resolved proxy UUID into broadcastCreate/broadcastRevoke/broadcastRotate. Keep
the existing not-found/error behavior aligned with the project-scoped lookup so
tenant identity is always validated.
In `@agent-manager-service/services/platform_gateway_service.go`:
- Around line 681-706: The single-gateway lookup path in GetGatewayStatus still
calls gatewayRepo.GetByUUID without passing ctx, so it bypasses
cancellation/timeouts unlike the organization list path. Update the
GetGatewayStatus branch that handles gatewayID to thread ctx into the repository
call, and make sure the gatewayRepo.GetByUUID API itself accepts context.Context
as its first parameter and propagates it through the data access layer.
---
Duplicate comments:
In `@agent-manager-service/services/llm_provider_apikey_service.go`:
- Around line 59-67: The provider lookup in the LLM key service is still not
cancelable because it calls providerRepo.GetByUUID without the ctx that was
already added to this flow. Update the lookup in the relevant service method(s)
to use the context-aware GetByUUID(ctx, ...) before any key listing or
bulk-revocation work, and propagate ctx consistently through the affected paths
so the repository I/O follows the coding guideline. Use the existing symbols
providerRepo, GetByUUID, ListByArtifact, and the bulk-revoke code in the same
service to locate and update the related call sites.
In `@agent-manager-service/services/llm_proxy_apikey_service.go`:
- Line 79: The project-scoped proxy lookup in resolveProjectScopedProxy is
ignoring the incoming context, so cancellation and deadlines are not propagated
into the repository call. Update the GetByIDAndProject invocation on s.proxyRepo
to pass ctx as the first argument, matching the context-first convention used by
I/O methods, and ensure the rest of the lookup flow in resolveProjectScopedProxy
continues to use that same ctx.
---
Nitpick comments:
In `@agent-manager-service/services/mcp_proxy_apikey_service.go`:
- Around line 39-53: The constructor currently depends on the concrete
GatewayEventsService, which makes MCPProxyAPIKeyService tightly coupled and
harder to test. Update NewMCPProxyAPIKeyService and the apiKeyBroadcaster wiring
to accept a narrow gateway-events interface instead, and have the broadcaster
depend only on that interface plus the repository interfaces it already uses.
Keep the change localized to the MCPProxyAPIKeyService and apiKeyBroadcaster
symbols so the service layer remains decoupled from concrete implementations.
🪄 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: a4de78f8-82d1-4130-bd6f-68511605c42f
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (95)
agent-manager-service/api/agent_config_routes.goagent-manager-service/api/app.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/db_migrations/026_reclassify_api_key_purpose.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/agent_configuration_dto.goagent-manager-service/models/apikey.goagent-manager-service/rbac/permissions.goagent-manager-service/rbac/predefined_roles.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/mcp_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcp_proxies.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/client.goagent-manager-service/spec/model_api_key_info.goagent-manager-service/spec/model_list_api_keys_response.goagent-manager-service/spec/model_mcp_policy.goagent-manager-service/spec/model_mcp_policy_availability_response.goagent-manager-service/spec/model_mcp_policy_available_item.goagent-manager-service/spec/model_mcp_proxy_capabilities.goagent-manager-service/spec/model_mcp_proxy_list_item.goagent-manager-service/spec/model_mcp_proxy_list_response.goagent-manager-service/spec/model_mcp_proxy_request.goagent-manager-service/spec/model_mcp_proxy_response.goagent-manager-service/spec/model_mcp_server_info_fetch_request.goagent-manager-service/spec/model_mcp_server_info_fetch_response.goagent-manager-service/spec/model_security_summary.goagent-manager-service/utils/errors.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/workspaces/libs/api-client/src/apis/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/apis/agent-model-configs.tsconsole/workspaces/libs/api-client/src/apis/llm-providers.tsconsole/workspaces/libs/api-client/src/apis/mcp-proxies.tsconsole/workspaces/libs/api-client/src/hooks/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/hooks/agent-model-configs.tsconsole/workspaces/libs/api-client/src/hooks/llm-providers.tsconsole/workspaces/libs/api-client/src/hooks/mcp-proxies.tsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/types/src/api/agent-mcp-configs.tsconsole/workspaces/libs/types/src/api/agent-model-configs.tsconsole/workspaces/libs/types/src/api/common.tsconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/libs/types/src/api/mcp-proxies.tsconsole/workspaces/pages/agent-security/src/Security.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsxdeployments/docker-compose.ymldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamltest/e2e/framework/auth.go
💤 Files with no reviewable changes (4)
- test/e2e/framework/auth.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsx
- agent-manager-service/api/mcp_proxy_routes.go
- agent-manager-service/controllers/mcp_proxy_controller.go
✅ Files skipped from review due to trivial changes (16)
- agent-manager-service/spec/model_security_summary.go
- agent-manager-service/spec/model_mcp_policy_availability_response.go
- agent-manager-service/spec/model_list_api_keys_response.go
- agent-manager-service/spec/model_mcp_policy_available_item.go
- agent-manager-service/spec/model_mcp_server_info_fetch_response.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_capabilities.go
- agent-manager-service/spec/model_mcp_policy.go
- agent-manager-service/spec/model_mcp_proxy_response.go
- agent-manager-service/spec/model_mcp_server_info_fetch_request.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_request.go
- agent-manager-service/spec/model_mcp_proxy_list_item.go
- agent-manager-service/spec/api_mcp_proxies.go
- agent-manager-service/spec/api_mcpapi_keys.go
🚧 Files skipped from review as they are similar to previous changes (42)
- agent-manager-service/api/llm_proxy_apikey_routes.go
- console/workspaces/libs/shared-component/src/components/index.ts
- agent-manager-service/db_migrations/migration_list.go
- agent-manager-service/db_migrations/026_reclassify_api_key_purpose.go
- agent-manager-service/api/agent_config_routes.go
- console/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsx
- console/workspaces/libs/types/src/api/common.ts
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsx
- agent-manager-service/spec/client.go
- console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
- agent-manager-service/controllers/llm_provider_apikey_controller.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsx
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- console/workspaces/libs/types/src/api/mcp-proxies.ts
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsx
- agent-manager-service/spec/model_mcp_proxy_list_response.go
- console/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsx
- console/workspaces/libs/api-client/src/apis/llm-providers.ts
- console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsx
- agent-manager-service/services/llm_proxy_provisioner.go
- console/workspaces/libs/types/src/api/agent-mcp-configs.ts
- agent-manager-service/models/agent_configuration_dto.go
- console/workspaces/libs/api-client/src/apis/mcp-proxies.ts
- console/workspaces/pages/agent-security/src/Security.Component.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx
- console/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsx
- console/workspaces/libs/api-client/src/hooks/agent-mcp-configs.ts
- console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsx
- console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsx
- console/workspaces/libs/types/src/api/agent-model-configs.ts
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/models/apikey.go
- console/workspaces/libs/api-client/src/apis/agent-model-configs.ts
- console/workspaces/libs/api-client/src/apis/agent-mcp-configs.ts
- console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts
- console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts
- console/workspaces/libs/api-client/src/hooks/llm-providers.ts
- agent-manager-service/services/agent_configuration_service.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
🛑 Comments failed to post (9)
agent-manager-service/controllers/agent_configuration_controller.go (1)
924-932: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Map non-external config errors on list endpoints too.
Create/rotate/revoke translate
ErrAgentConfigNotExternalto403, but both list handlers fall through to500. Use the shared error mapping or handle this sentinel explicitly.Suggested fix
if err != nil { if errors.Is(err, utils.ErrAgentConfigNotFound) { utils.WriteErrorResponse(w, http.StatusNotFound, "Configuration not found") return } + if errors.Is(err, utils.ErrAgentConfigNotExternal) { + utils.WriteErrorResponse(w, http.StatusForbidden, "API key management is only available for external agents") + return + } log.Error("ListMCPConfigAPIKeys: failed to list API keys", "error", err)Apply the same branch in
ListLLMConfigAPIKeys.As per coding guidelines,
agent-manager-service/controllers/**: Controllers must translate sentinel errors to HTTP errors.Also applies to: 1027-1035
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 924 - 932, The list endpoints in the agent configuration controller are not translating ErrAgentConfigNotExternal, so they fall through to a 500 instead of a 403. Update the ListMCPConfigAPIKeys flow and the matching ListLLMConfigAPIKeys handler to use the same sentinel-to-HTTP mapping as create/rotate/revoke, either by reusing the shared error mapping helper or by adding an explicit errors.Is(err, utils.ErrAgentConfigNotExternal) branch before the generic internal-server fallback.Source: Coding guidelines
agent-manager-service/controllers/mcp_proxy_apikey_controller.go (1)
131-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject malformed rotate request bodies.
Invalid JSON currently rotates the key with empty update fields. Accept only
io.EOFfor the optional-body case; return400for other decode errors.Suggested fix
import ( "encoding/json" "errors" + "io" "log/slog" "net/http" @@ var specReq spec.RotateLLMAPIKeyRequest - _ = json.NewDecoder(r.Body).Decode(&specReq) + if err := json.NewDecoder(r.Body).Decode(&specReq); err != nil && !errors.Is(err, io.EOF) { + utils.WriteErrorResponse(w, http.StatusBadRequest, "Invalid request body") + return + }As per coding guidelines,
agent-manager-service/controllers/**: Controllers must parse/validate requests and map results to status codes.📝 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.var specReq spec.RotateLLMAPIKeyRequest if err := json.NewDecoder(r.Body).Decode(&specReq); err != nil && !errors.Is(err, io.EOF) { utils.WriteErrorResponse(w, http.StatusBadRequest, "Invalid request body") return }🤖 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 `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go` around lines 131 - 132, The MCP rotate handler is ignoring JSON decode errors and can proceed with an empty specReq, so update the request parsing in mcp_proxy_apikey_controller.go to validate the body in the controller. In the rotate API key handler, treat io.EOF as the only acceptable empty-body case, but return HTTP 400 for any other json.Decoder.Decode failure before calling the rotate logic. Use the existing rotate request types and controller method that handles the request to ensure malformed bodies never reach the update path.Source: Coding guidelines
agent-manager-service/docs/api_v1_openapi.yaml (1)
3852-4051: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add the missing model-config API-key routes to the contract.
This spec only adds
/mcp-configs/{configId}/.../api-keys, but the controller also exposesList/Create/Rotate/RevokeLLMConfigAPIKey. Add the matching/model-configs/{configId}/environments/{envName}/api-keysand{keyName}operations before regenerating clients.As per coding guidelines,
agent-manager-service/docs/api_v1_openapi.yaml: Usedocs/api_v1_openapi.yamlas the API contract source of truth for adding or changing an API resource.🤖 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 `@agent-manager-service/docs/api_v1_openapi.yaml` around lines 3852 - 4051, The OpenAPI contract is missing the model-config API-key endpoints that correspond to the controller’s List/Create/Rotate/RevokeLLMConfigAPIKey operations. Add the matching /orgs/{orgName}/projects/{projName}/agents/{agentName}/model-configs/{configId}/environments/{envName}/api-keys and /{keyName} paths with the same path parameters, tags, request bodies, operationIds, and response schemas as the existing MCP API-key routes, then regenerate clients from docs/api_v1_openapi.yaml.Source: Coding guidelines
agent-manager-service/repositories/apikey_repository.go (1)
33-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope
ListByArtifactto the caller org.Line 76 only filters by
artifact_uuid, so this new repository API can enumerate another tenant’s keys if a mis-scoped artifact UUID reaches it. AddorgNameto the method contract and enforce it in the query instead of treating tenant identity as implicit.Suggested fix
-type APIKeyRepository interface { +type APIKeyRepository interface { Upsert(key *models.StoredAPIKey) error Delete(artifactUUID, name string) error - ListByArtifact(ctx context.Context, artifactUUID string) ([]models.StoredAPIKey, error) + ListByArtifact(ctx context.Context, orgName, artifactUUID string) ([]models.StoredAPIKey, error)-func (r *APIKeyRepo) ListByArtifact(ctx context.Context, artifactUUID string) ([]models.StoredAPIKey, error) { +func (r *APIKeyRepo) ListByArtifact(ctx context.Context, orgName, artifactUUID string) ([]models.StoredAPIKey, error) { var keys []models.StoredAPIKey - err := r.db.WithContext(ctx).Where("artifact_uuid = ?", artifactUUID).Find(&keys).Error + err := r.db.WithContext(ctx). + Joins("JOIN artifacts a ON api_keys.artifact_uuid = a.uuid"). + Where("api_keys.artifact_uuid = ? AND a.organization_name = ?", artifactUUID, orgName). + Find(&keys).Error return keys, err }As per coding guidelines, "Validate caller organization against the target resource for multi-tenant safety; missing tenant identity is an error, not a wildcard."
Also applies to: 74-76
🤖 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 `@agent-manager-service/repositories/apikey_repository.go` at line 33, Update the ListByArtifact repository contract to require the caller organization explicitly, because artifact UUID alone is not sufficient for multi-tenant safety. Modify the ApikeyRepository interface and its implementation method ListByArtifact to accept orgName alongside artifactUUID, then update the query logic in the repository method to filter by both org identity and artifact_uuid instead of treating tenant scope as implicit. Ensure any call sites are updated to pass the caller org, using the existing repository symbols ListByArtifact and ApikeyRepository as the main points of change.Source: Coding guidelines
agent-manager-service/repositories/llm_proxy_repository.go (1)
35-35: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bring the new repository lookup onto the standard repo contract.
This new DB method is missing
context.Context, usesr.dbinstead ofWithContext, and returns raw GORM errors. That leaves the lookup uncancelable and leaks persistence-layer details past the repository boundary.As per coding guidelines, "Every I/O method must take
context.Contextas the first parameter and propagate it" and "gorm.ErrRecordNotFound[must map] to the appropriate sentinel not-found error, wrap all other unexpected errors with context usingfmt.Errorf(\"...: %w\", err)."Also applies to: 164-179
🤖 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 `@agent-manager-service/repositories/llm_proxy_repository.go` at line 35, The new repository lookup method on LLMProxyRepository does not follow the standard contract because GetByIDAndProject is missing context.Context, does not use WithContext, and returns raw GORM errors. Update GetByIDAndProject and its implementation in the repository to accept context as the first parameter, call the query through r.db.WithContext(ctx), map gorm.ErrRecordNotFound to the repo’s not-found sentinel, and wrap any other database errors with a contextual fmt.Errorf using the method name so callers only see repository-level errors.Source: Coding guidelines
console/apps/web-ui/public/config.js (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the self-comparison with a boolean literal.
Line 43 evaluates to
trueunconditionally and is already tripping Biome'snoSelfComparerule. Use the boolean literal directly so this file stays lint-clean. Based on static analysis, Biome reportedlint/suspicious/noSelfCompareon Line 43.Suggested fix
- rbacEnabled: "true" === "true", + rbacEnabled: true,📝 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.rbacEnabled: true,🧰 Tools
🪛 Biome (2.5.1)
[error] 43-43: This comparison uses the same expression on both sides.
(lint/suspicious/noSelfCompare)
🤖 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 `@console/apps/web-ui/public/config.js` at line 43, The rbacEnabled entry in config.js is using a self-comparison that always evaluates to true and violates the noSelfCompare lint rule. Update the config object to use a boolean literal directly for rbacEnabled instead of comparing the same string value, and keep the change localized to the config constant/object where rbacEnabled is defined.Source: Linters/SAST tools
deployments/helm-charts/wso2-agent-manager/values.yaml (1)
317-317: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Request the MCP API-key management scope in the console client.
Line 169 advertises
amp:mcp-server:api-key-manage, but the console OAuth scopes omit it here. WithrbacEnabled: "true", MCP proxy API-key actions can be denied even for users whose roles grant the permission.Suggested fix
- scopes: "openid profile email ... amp:mcp-server:connect amp:mcp-server:read ..." + scopes: "openid profile email ... amp:mcp-server:connect amp:mcp-server:api-key-manage amp:mcp-server:read ..."📝 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.scopes: "openid profile email amp:llm-proxy:deploy amp:mcp-server:create amp:org:invite-member amp:role:delete amp:monitor:read amp:agent:deploy-production amp:git-secret:delete amp:llm-provider:delete amp:mcp-server:configure-guardrail amp:agent:promote amp:deployment-pipeline:create amp:deployment-pipeline:read amp:deployment-pipeline:update amp:deployment-pipeline:delete amp:gateway:delete amp:group:delete amp:llm-proxy:update amp:monitor:score-read amp:llm-provider-template:update amp:agent:build amp:agent:rollback amp:agent:token-manage amp:evaluator:read amp:llm-provider-template:read amp:monitor:create amp:agent:suspend amp:gateway:token-manage amp:git-secret:read amp:mcp-server:connect amp:mcp-server:api-key-manage amp:mcp-server:read amp:observability:project-dashboard amp:group:create amp:observability:org-dashboard amp:org:remove-member amp:agent:deploy-non-production amp:group:read amp:llm-provider-template:create amp:project:delete amp:agent:create amp:evaluator:create amp:llm-provider:read amp:org:modify-settings amp:catalog:read amp:environment:update amp:group:update amp:llm-proxy:delete amp:project:read amp:data-plane:read amp:agent:read amp:environment:read amp:llm-provider:configure-guardrail amp:observability:guardrail-metric amp:org:assign-role amp:project:update amp:mcp-server:delete amp:environment:delete amp:evaluator:update amp:git-secret:create amp:monitor:execute amp:llm-provider:api-key-manage amp:gateway:read amp:agent:update amp:llm-provider-template:delete amp:monitor:delete amp:org:view amp:llm-provider:create amp:mcp-server:update amp:org:manage-idp amp:role:read amp:role:update amp:agent:delete amp:gateway:create amp:llm-provider:deploy amp:llm-proxy:api-key-manage amp:monitor:update amp:org:manage-service-account amp:role:create amp:evaluator:delete amp:llm-provider:connect amp:llm-provider:update amp:llm-proxy:read amp:monitor:score-publish amp:observability:infra-metric amp:project:create amp:repository:read amp:gateway:update amp:environment:create amp:llm-proxy:create amp:agent:api-key-manage amp:agent-kind:read amp:agent-kind:create amp:agent-kind:update amp:agent-kind:delete amp:profile:read amp:profile:update-attributes"🤖 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 `@deployments/helm-charts/wso2-agent-manager/values.yaml` at line 317, The console OAuth scope list in values.yaml is missing the MCP API-key management permission, causing role-granted MCP proxy API-key actions to be denied when rbacEnabled is true. Update the scopes value to include amp:mcp-server:api-key-manage alongside the other MCP-related scopes, keeping the existing scope string format intact.deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml (2)
1593-1607: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provision
mcp-server:api-key-managebefore assigning it to roles.These role payloads now reference
mcp-server:api-key-manage, but60-amp-resource-server.shstill never creates that MCP action. Role updates will either fail validation or succeed without granting the scope, so the new MCP API-key endpoints remain unusable.Suggested fix
create_action "$RS_ID" "$R_MCP" "Configure Guardrail" "configure-guardrail" "Configure guardrails on an MCP server" create_action "$RS_ID" "$R_MCP" "Connect" "connect" "Connect an agent to an MCP server" + create_action "$RS_ID" "$R_MCP" "Manage API Key" "api-key-manage" "Create, update, and delete MCP server API keys" @@ - log_info "Agent Manager resource server registration complete (97 permissions registered)." + log_info "Agent Manager resource server registration complete (98 permissions registered)."🤖 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 `@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml` around lines 1593 - 1607, The role definitions in the bootstrap template now include mcp-server:api-key-manage, but the corresponding MCP action is not being provisioned yet. Update the resource setup in 60-amp-resource-server.sh to create/register the mcp-server:api-key-manage permission before any calls to set_role_permissions that assign it to roles such as Developer and AI Lead. Make sure the new action is added in the same place and style as the other mcp-server permissions so role validation and scope assignment both work.
1752-1770: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Look up the renamed AMP admin role here, not
"admin".
61-amp-default-roles.shnow createsAgent Manager Admin, but this block still resolves"name":"admin". That either fails the bootstrap or, worse, assigns the group to Thunder's broader built-in admin role instead of the scoped AMP role.Suggested fix
- log_info "Looking up admin role..." + log_info "Looking up Agent Manager Admin role..." @@ - ADMIN_ROLE_ID=$(echo "$BODY" | sed 's/},{/}\n{/g' | grep '"name":"admin"' | grep -o '"id":"[^"]*"' | head -1 | cut -d'"' -f4) + ADMIN_ROLE_ID=$(echo "$BODY" | sed 's/},{/}\n{/g' | grep '"name":"Agent Manager Admin"' | grep -o '"id":"[^"]*"' | head -1 | cut -d'"' -f4) if [[ -z "$ADMIN_ROLE_ID" ]]; then - log_error "Could not find 'admin' role" + log_error "Could not find 'Agent Manager Admin' role" exit 1 fi - log_info "Found 'admin' role (id: $ADMIN_ROLE_ID)" + log_info "Found 'Agent Manager Admin' role (id: $ADMIN_ROLE_ID)"📝 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.log_info "Looking up Agent Manager Admin role..." RESPONSE=$(api_call GET "/roles") HTTP_CODE="${RESPONSE: -3}" BODY="${RESPONSE%???}" if [[ "$HTTP_CODE" != "200" ]]; then log_error "Failed to fetch roles (HTTP $HTTP_CODE)" exit 1 fi ADMIN_ROLE_ID=$(echo "$BODY" | sed 's/},{/}\n{/g' | grep '"name":"Agent Manager Admin"' | grep -o '"id":"[^"]*"' | head -1 | cut -d'"' -f4) if [[ -z "$ADMIN_ROLE_ID" ]]; then log_error "Could not find 'Agent Manager Admin' role" exit 1 fi log_info "Found 'Agent Manager Admin' role (id: $ADMIN_ROLE_ID)" log_info "Assigning Agent Manager Admin role to Agent Manager Admins group..." ASSIGN_PAYLOAD='{"assignments":[{"id":"'${ADMINS_GROUP_ID}'","type":"group"}]}' RESPONSE=$(api_call POST "/roles/$ADMIN_ROLE_ID/assignments/add" "$ASSIGN_PAYLOAD")🤖 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 `@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml` around lines 1752 - 1770, The role lookup still hardcodes the old built-in admin role, so update the bootstrap flow to resolve the renamed AMP role instead. In the role-fetching block that sets ADMIN_ROLE_ID, change the search logic from matching "name":"admin" to matching the AMP-specific "Agent Manager Admin" role created by 61-amp-default-roles.sh, and keep the assignment call using that resolved ID so the Agents Manager Admins group is attached to the scoped AMP role.
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
6c80680 to
024533f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agent-manager-service/controllers/agent_configuration_controller.go (1)
881-891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep correlation fields in shared error logs.
writeConfigAPIKeyErroronly receivesoperationanderr, so unexpected API-key failures lose the available org/project/agent/config/env/key context from the call sites.Suggested direction
-func (c *agentConfigurationController) writeConfigAPIKeyError(w http.ResponseWriter, log *slog.Logger, operation string, err error) { +func (c *agentConfigurationController) writeConfigAPIKeyError(w http.ResponseWriter, log *slog.Logger, operation string, err error, attrs ...any) { switch { @@ default: - log.Error(operation+": failed", "error", err) + log.Error(operation+": failed", append(attrs, "error", err)...) utils.WriteErrorResponse(w, http.StatusInternalServerError, "Failed to manage API key") } }Then pass fields like
orgName,projName,agentName,configUUID,envName, andkeyNamefrom each caller.As per coding guidelines, logs should include correlation context such as org, resource ID, and request ID.
Also applies to: 963-963, 993-993, 1011-1011, 1066-1066, 1096-1096, 1114-1114
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 881 - 891, The shared API-key error logger in writeConfigAPIKeyError drops important correlation context on unexpected failures. Update writeConfigAPIKeyError to accept the available org/project/agent/config/env/key identifiers from its callers, and include those fields in the default log.Error call alongside operation and err. Then update each call site that currently passes only operation and err so the logger retains orgName, projName, agentName, configUUID, envName, and keyName context for debugging.Source: Coding guidelines
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 924-931: The list handlers currently only map
ErrAgentConfigNotFound, so ErrAgentConfigNotExternal and ErrGatewayNotFound
still fall through as 500s. Update ListMCPConfigAPIKeys and ListLLMConfigAPIKeys
in agent_configuration_controller.go to mirror the create/rotate/revoke handlers
by checking all known sentinel errors from c.agentConfigService before logging,
and return the appropriate HTTP error responses with utils.WriteErrorResponse
for each case.
---
Nitpick comments:
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 881-891: The shared API-key error logger in writeConfigAPIKeyError
drops important correlation context on unexpected failures. Update
writeConfigAPIKeyError to accept the available org/project/agent/config/env/key
identifiers from its callers, and include those fields in the default log.Error
call alongside operation and err. Then update each call site that currently
passes only operation and err so the logger retains orgName, projName,
agentName, configUUID, envName, and keyName context for debugging.
🪄 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: 883e4561-ec54-4c38-a739-393519a765ea
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (95)
agent-manager-service/api/agent_config_routes.goagent-manager-service/api/app.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/llm_provider_apikey_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/db_migrations/026_reclassify_api_key_purpose.goagent-manager-service/db_migrations/migration_list.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/agent_configuration_dto.goagent-manager-service/models/apikey.goagent-manager-service/rbac/permissions.goagent-manager-service/rbac/predefined_roles.goagent-manager-service/repositories/agent_configuration_repository.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_provider_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/llm_proxy_provisioner.goagent-manager-service/services/mcp_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcp_proxies.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/client.goagent-manager-service/spec/model_api_key_info.goagent-manager-service/spec/model_list_api_keys_response.goagent-manager-service/spec/model_mcp_policy.goagent-manager-service/spec/model_mcp_policy_availability_response.goagent-manager-service/spec/model_mcp_policy_available_item.goagent-manager-service/spec/model_mcp_proxy_capabilities.goagent-manager-service/spec/model_mcp_proxy_list_item.goagent-manager-service/spec/model_mcp_proxy_list_response.goagent-manager-service/spec/model_mcp_proxy_request.goagent-manager-service/spec/model_mcp_proxy_response.goagent-manager-service/spec/model_mcp_server_info_fetch_request.goagent-manager-service/spec/model_mcp_server_info_fetch_response.goagent-manager-service/spec/model_security_summary.goagent-manager-service/utils/errors.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/workspaces/libs/api-client/src/apis/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/apis/agent-model-configs.tsconsole/workspaces/libs/api-client/src/apis/llm-providers.tsconsole/workspaces/libs/api-client/src/apis/mcp-proxies.tsconsole/workspaces/libs/api-client/src/hooks/agent-mcp-configs.tsconsole/workspaces/libs/api-client/src/hooks/agent-model-configs.tsconsole/workspaces/libs/api-client/src/hooks/llm-providers.tsconsole/workspaces/libs/api-client/src/hooks/mcp-proxies.tsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/types/src/api/agent-mcp-configs.tsconsole/workspaces/libs/types/src/api/agent-model-configs.tsconsole/workspaces/libs/types/src/api/common.tsconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/libs/types/src/api/mcp-proxies.tsconsole/workspaces/pages/agent-security/src/Security.Component.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsxconsole/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsxdeployments/docker-compose.ymldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamltest/e2e/framework/auth.go
💤 Files with no reviewable changes (3)
- agent-manager-service/api/mcp_proxy_routes.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsx
- agent-manager-service/controllers/mcp_proxy_controller.go
✅ Files skipped from review due to trivial changes (19)
- console/workspaces/libs/types/src/api/common.ts
- agent-manager-service/spec/model_security_summary.go
- console/workspaces/libs/shared-component/src/components/index.ts
- agent-manager-service/spec/model_mcp_server_info_fetch_request.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/spec/model_mcp_proxy_capabilities.go
- agent-manager-service/spec/model_mcp_proxy_list_item.go
- agent-manager-service/spec/model_mcp_policy_availability_response.go
- agent-manager-service/spec/model_mcp_policy_available_item.go
- agent-manager-service/spec/model_list_api_keys_response.go
- agent-manager-service/spec/model_mcp_server_info_fetch_response.go
- agent-manager-service/spec/model_mcp_proxy_response.go
- agent-manager-service/spec/model_mcp_proxy_request.go
- agent-manager-service/spec/model_mcp_policy.go
- agent-manager-service/spec/model_mcp_proxy_list_response.go
- agent-manager-service/spec/model_api_key_info.go
- agent-manager-service/spec/api_mcp_proxies.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/spec/api_mcpapi_keys.go
🚧 Files skipped from review as they are similar to previous changes (70)
- agent-manager-service/api/llm_proxy_apikey_routes.go
- agent-manager-service/api/llm_provider_apikey_routes.go
- agent-manager-service/rbac/permissions.go
- deployments/helm-charts/wso2-amp-thunder-extension/values.yaml
- console/workspaces/pages/configure-agent/src/ViewMCPServer.Component.tsx
- agent-manager-service/models/agent_configuration_dto.go
- deployments/helm-charts/wso2-agent-manager/values.yaml
- test/e2e/framework/auth.go
- agent-manager-service/api/agent_config_routes.go
- agent-manager-service/controllers/gateway_controller.go
- agent-manager-service/utils/errors.go
- console/workspaces/libs/api-client/src/apis/mcp-proxies.ts
- deployments/docker-compose.yml
- agent-manager-service/db_migrations/migration_list.go
- agent-manager-service/rbac/predefined_roles.go
- console/workspaces/pages/configure-agent/src/ViewLLMProvider.Component.tsx
- console/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsx
- agent-manager-service/repositories/agent_configuration_repository.go
- console/workspaces/libs/types/src/api/agent-mcp-configs.ts
- console/workspaces/libs/types/src/api/mcp-proxies.ts
- agent-manager-service/api/mcp_proxy_apikey_routes.go
- agent-manager-service/services/ai_application_service.go
- console/workspaces/pages/mcp-proxies/src/subComponents/ViewMCPProxy.tsx
- agent-manager-service/wiring/params.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/services/agent_apikey_service.go
- console/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsx
- agent-manager-service/api/app.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsx
- console/apps/web-ui/public/config.js
- console/workspaces/libs/api-client/src/apis/agent-mcp-configs.ts
- agent-manager-service/controllers/llm_provider_apikey_controller.go
- agent-manager-service/repositories/llm_proxy_repository.go
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsx
- console/workspaces/pages/agent-security/src/Security.Component.tsx
- agent-manager-service/db_migrations/026_reclassify_api_key_purpose.go
- console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx
- agent-manager-service/services/platform_gateway_service.go
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyAPIKeysTab.tsx
- console/workspaces/libs/types/src/api/llm-providers.ts
- agent-manager-service/repositories/gateway_repository.go
- console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx
- agent-manager-service/services/llm_proxy_provisioner.go
- agent-manager-service/services/llm_provider_service.go
- agent-manager-service/services/mcp_proxy_apikey_service.go
- agent-manager-service/spec/client.go
- agent-manager-service/repositories/apikey_repository.go
- agent-manager-service/models/apikey.go
- console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx
- agent-manager-service/services/llm_provider_apikey_service.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- console/workspaces/libs/api-client/src/apis/agent-model-configs.ts
- console/workspaces/libs/api-client/src/hooks/agent-model-configs.ts
- console/workspaces/libs/api-client/src/apis/llm-providers.ts
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
- agent-manager-service/services/mcp_proxy_service.go
- agent-manager-service/services/apikey_broadcaster.go
- agent-manager-service/services/llm_proxy_apikey_service.go
- console/workspaces/libs/api-client/src/hooks/agent-mcp-configs.ts
- console/workspaces/libs/types/src/api/agent-model-configs.ts
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderOverviewTab.tsx
- console/workspaces/libs/api-client/src/hooks/mcp-proxies.ts
- console/workspaces/libs/api-client/src/hooks/llm-providers.ts
- deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml
- agent-manager-service/services/agent_configuration_service.go
- agent-manager-service/docs/api_v1_openapi.yaml
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
024533f to
e8f2132
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go`:
- Around line 76-80: The request parsing in CreateMCPProxyAPIKey and the other
controller decoder still accepts unknown fields and trailing JSON because it
only does a single json.NewDecoder(...).Decode. Update both handlers to use a
decoder configured with DisallowUnknownFields before decoding the request
struct, and then verify there is no extra JSON remaining after the first decode
so junk payloads are rejected. Keep the existing error handling/logging pattern
in the affected controller methods.
In `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Around line 4122-4152: Add the missing 403 response to the config API-key
operations in the OpenAPI contract so it matches writeConfigAPIKeyError’s
ErrAgentConfigNotExternal mapping. Update the response blocks for
createLLMConfigAPIKey, rotateLLMConfigAPIKey, revokeLLMConfigAPIKey, and the
MCP-config equivalents in api_v1_openapi.yaml to document 403 alongside the
existing 404/503/500 cases, using the shared ErrorResponse schema.
🪄 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: fa000e7e-c06d-4c88-8e4e-a47286d1606e
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (55)
agent-manager-service/api/app.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/gateway_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/apikey.goagent-manager-service/rbac/permissions.goagent-manager-service/rbac/predefined_roles.goagent-manager-service/repositories/apikey_repository.goagent-manager-service/repositories/gateway_repository.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/repomocks/apikey_repository_mock.goagent-manager-service/repositories/repomocks/gateway_repository_mock.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/services/agent_apikey_service.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/ai_application_service.goagent-manager-service/services/apikey_broadcaster.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/services/platform_gateway_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/model_api_key_info.goagent-manager-service/spec/model_list_api_keys_response.goagent-manager-service/spec/model_security_summary.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsxdeployments/docker-compose.ymldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamltest/e2e/framework/auth.go
💤 Files with no reviewable changes (3)
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyTable.tsx
- agent-manager-service/api/mcp_proxy_routes.go
- agent-manager-service/controllers/mcp_proxy_controller.go
✅ Files skipped from review due to trivial changes (8)
- deployments/helm-charts/wso2-amp-thunder-extension/values.yaml
- agent-manager-service/spec/model_security_summary.go
- agent-manager-service/repositories/repomocks/apikey_repository_mock.go
- agent-manager-service/spec/model_list_api_keys_response.go
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/spec/model_api_key_info.go
- agent-manager-service/spec/api_mcpapi_keys.go
🚧 Files skipped from review as they are similar to previous changes (39)
- agent-manager-service/services/ai_application_service.go
- agent-manager-service/api/app.go
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderSecurityTab.tsx
- agent-manager-service/api/llm_provider_apikey_routes.go
- agent-manager-service/api/llm_proxy_apikey_routes.go
- test/e2e/framework/auth.go
- console/apps/web-ui/public/config.js
- agent-manager-service/api/mcp_proxy_apikey_routes.go
- agent-manager-service/services/agent_apikey_service.go
- console/workspaces/libs/types/src/api/llm-providers.ts
- agent-manager-service/controllers/gateway_controller.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxySecurityTab.tsx
- agent-manager-service/rbac/predefined_roles.go
- agent-manager-service/services/platform_gateway_service.go
- agent-manager-service/rbac/permissions.go
- deployments/docker-compose.yml
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- agent-manager-service/repositories/llm_proxy_repository.go
- agent-manager-service/wiring/params.go
- console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx
- console/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsx
- agent-manager-service/wiring/wire.go
- agent-manager-service/repositories/gateway_repository.go
- agent-manager-service/services/mcp_proxy_apikey_service.go
- deployments/helm-charts/wso2-agent-manager/values.yaml
- agent-manager-service/repositories/apikey_repository.go
- agent-manager-service/models/apikey.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsx
- agent-manager-service/services/llm_provider_apikey_service.go
- agent-manager-service/services/llm_proxy_apikey_service.go
- agent-manager-service/repositories/repomocks/gateway_repository_mock.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsx
- agent-manager-service/services/mcp_proxy_service.go
- console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx
- console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx
- agent-manager-service/services/apikey_broadcaster.go
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
- agent-manager-service/services/agent_configuration_service.go
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
e8f2132 to
523cdb7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agent-manager-service/controllers/agent_configuration_controller.go (1)
881-891: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass resource context into config API-key error logs.
The shared helper logs unexpected failures with only
operationanderror, so these per-config failures cannot be correlated to org/project/agent/config/env/key. Thread the request identifiers through the helper, similar to the MCP proxy controller.As per coding guidelines,
agent-manager-service/**/*.go: “Log with correlation context such as org, resource ID, and request ID; use Debug for hot paths, Info for rare events, and Error for destructive operations.”Suggested direction
-func (c *agentConfigurationController) writeConfigAPIKeyError(w http.ResponseWriter, log *slog.Logger, operation string, err error) { +func (c *agentConfigurationController) writeConfigAPIKeyError( + w http.ResponseWriter, + log *slog.Logger, + operation string, + err error, + attrs ...any, +) { switch { @@ default: - log.Error(operation+": failed", "error", err) + log.Error("agent config API key operation failed", append([]any{ + "operation", operation, + "error", err, + }, attrs...)...) utils.WriteErrorResponse(w, http.StatusInternalServerError, "Failed to manage API key") } }Then pass fields at call sites, e.g.
orgName,projName,agentName,configID,envName, andkeyNamewhere available.Also applies to: 963-963, 993-993, 1011-1011, 1066-1066, 1096-1096, 1114-1114
🤖 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 `@agent-manager-service/controllers/agent_configuration_controller.go` around lines 881 - 891, The shared API-key error helper only logs operation and error, so unexpected config failures lack correlation context. Update writeConfigAPIKeyError to accept the request/resource identifiers used at the call sites, then include org/project/agent/config/env/key details in the log entry alongside the error. Thread those fields through each caller in agentConfigurationController where the helper is used, following the same correlation pattern as the MCP proxy controller.Source: Coding guidelines
agent-manager-service/controllers/mcp_proxy_apikey_controller.go (1)
82-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared create-key validation helper.
apiKeyCreateNameDisplayNamealready centralizes this unwrap/400 behavior in the same package. Reusing it here avoids the MCP proxy and config handlers drifting on validation text or semantics.Suggested refactor
- name := "" - if specReq.Name != nil { - name = *specReq.Name - } - displayName := "" - if specReq.DisplayName != nil { - displayName = *specReq.DisplayName - } - if name == "" && displayName == "" { - utils.WriteErrorResponse(w, http.StatusBadRequest, "At least one of 'name' or 'displayName' must be provided") + name, displayName, ok := apiKeyCreateNameDisplayName(w, specReq.Name, specReq.DisplayName) + if !ok { return }🤖 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 `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go` around lines 82 - 93, The MCP proxy create-key handler is duplicating name/displayName unwrapping and validation instead of using the shared helper. Update the create flow in the controller to call apiKeyCreateNameDisplayName for specReq.Name and specReq.DisplayName, and use its returned values/error directly so validation text and behavior stay aligned with the other handlers.
🤖 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.
Nitpick comments:
In `@agent-manager-service/controllers/agent_configuration_controller.go`:
- Around line 881-891: The shared API-key error helper only logs operation and
error, so unexpected config failures lack correlation context. Update
writeConfigAPIKeyError to accept the request/resource identifiers used at the
call sites, then include org/project/agent/config/env/key details in the log
entry alongside the error. Thread those fields through each caller in
agentConfigurationController where the helper is used, following the same
correlation pattern as the MCP proxy controller.
In `@agent-manager-service/controllers/mcp_proxy_apikey_controller.go`:
- Around line 82-93: The MCP proxy create-key handler is duplicating
name/displayName unwrapping and validation instead of using the shared helper.
Update the create flow in the controller to call apiKeyCreateNameDisplayName for
specReq.Name and specReq.DisplayName, and use its returned values/error directly
so validation text and behavior stay aligned with the other handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6ccd741-6dba-4698-9efe-417b5bb92593
⛔ Files ignored due to path filters (2)
cli/pkg/clients/amsvc/gen/client.gen.gois excluded by!**/gen/**cli/pkg/clients/amsvc/gen/types.gen.gois excluded by!**/gen/**
📒 Files selected for processing (34)
agent-manager-service/api/app.goagent-manager-service/api/mcp_proxy_apikey_routes.goagent-manager-service/api/mcp_proxy_routes.goagent-manager-service/controllers/agent_configuration_controller.goagent-manager-service/controllers/llm_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_apikey_controller.goagent-manager-service/controllers/mcp_proxy_controller.goagent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/apikey.goagent-manager-service/repositories/llm_proxy_repository.goagent-manager-service/repositories/repomocks/llm_proxy_repository_mock.goagent-manager-service/services/agent_configuration_service.goagent-manager-service/services/llm_provider_apikey_service.goagent-manager-service/services/llm_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_apikey_service.goagent-manager-service/services/mcp_proxy_service.goagent-manager-service/spec/api_llmapi_keys.goagent-manager-service/spec/api_mcpapi_keys.goagent-manager-service/spec/model_api_key_info.goagent-manager-service/spec/model_list_api_keys_response.goagent-manager-service/spec/model_security_summary.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/vite.config.tsconsole/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsxconsole/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsxconsole/workspaces/libs/types/src/api/llm-providers.tsconsole/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsxconsole/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsxconsole/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsxconsole/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsxconsole/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
💤 Files with no reviewable changes (3)
- agent-manager-service/api/mcp_proxy_routes.go
- agent-manager-service/controllers/mcp_proxy_controller.go
- agent-manager-service/services/mcp_proxy_service.go
✅ Files skipped from review due to trivial changes (7)
- agent-manager-service/spec/model_security_summary.go
- console/apps/web-ui/vite.config.ts
- agent-manager-service/spec/model_list_api_keys_response.go
- agent-manager-service/wiring/wire_gen.go
- agent-manager-service/spec/model_api_key_info.go
- agent-manager-service/spec/api_mcpapi_keys.go
- agent-manager-service/spec/api_llmapi_keys.go
🚧 Files skipped from review as they are similar to previous changes (22)
- agent-manager-service/api/mcp_proxy_apikey_routes.go
- agent-manager-service/api/app.go
- agent-manager-service/wiring/params.go
- agent-manager-service/wiring/wire.go
- agent-manager-service/repositories/llm_proxy_repository.go
- console/workspaces/libs/types/src/api/llm-providers.ts
- agent-manager-service/controllers/llm_proxy_apikey_controller.go
- console/workspaces/pages/configure-agent/src/Configure/subComponents/MCPProxyAPIKeysSection.tsx
- agent-manager-service/models/apikey.go
- console/workspaces/pages/mcp-proxies/src/subComponents/AddMCPProxyForm.tsx
- console/workspaces/pages/configure-agent/src/Configure/subComponents/LLMProxyAPIKeysSection.tsx
- agent-manager-service/repositories/repomocks/llm_proxy_repository_mock.go
- console/workspaces/pages/llm-providers/src/subComponents/ViewLLMProvider.tsx
- console/workspaces/pages/mcp-proxies/src/subComponents/MCPProxyOverviewTab.tsx
- agent-manager-service/services/llm_proxy_apikey_service.go
- agent-manager-service/services/llm_provider_apikey_service.go
- console/workspaces/libs/shared-component/src/components/APIKeysManager/SingleAPIKeyManager.tsx
- agent-manager-service/services/mcp_proxy_apikey_service.go
- console/workspaces/libs/shared-component/src/components/APIKeysManager/APIKeysManager.tsx
- agent-manager-service/services/agent_configuration_service.go
- agent-manager-service/docs/api_v1_openapi.yaml
- console/workspaces/pages/llm-providers/src/subComponents/LLMProviderAPIKeysTab.tsx
menakaj
previously approved these changes
Jul 1, 2026
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
523cdb7 to
46296a7
Compare
Add API key management support for LLM providers, MCP proxies, and external agent configurations. This includes the backend service/controller paths, generated API surface, and resource-specific handling needed to create, list, and revoke keys.
Improve the LLM provider and MCP proxy security tab experience, fix MCP proxy UI behavior, use the correct OAuth scopes for API key operations, and propagate request context through the API key management flow.
Scope LLM proxy API key listing to the caller project, regenerate the client for the wrapped API key list response, preserve expiry dates in non-UTC time zones, reset transient key UI by backing resource, show explicit provider fetch failures, derive MCP proxy versions from server info, avoid premature empty states, and remove the unsupported cookie key location from the contract.
Extract shared API key model conversion, config/environment resolution, controller error handling, and create-name helpers. Split MCP proxy API key handling into its own service/controller pair and inject broadcasting directly into the agent configuration service so LLM provider, LLM proxy, MCP proxy, and agent configuration keys follow the same structure.
The main app chunk sits at ~5 MB. PR builds bundle the branch merged with main, which pushes the largest chunk just past the 5000 kB limit; Rush treats the resulting Vite chunk-size warning as a build failure (allowWarningsInSuccessfulBuild=false). Raise the limit to 6000 kB to restore headroom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Oshanath
force-pushed
the
agent-config-api-key-management
branch
from
46296a7 to
6055a5b
Compare
menakaj
approved these changes
Jul 5, 2026
This was referenced Jul 6, 2026
Merged
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
3 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.
Summary
Adds API key management support for LLM providers, MCP proxies, and external agent configurations.
Changes
Summary by CodeRabbit