feat: add Vault and multi-backend secrets integration#48
Merged
sami-marreed merged 11 commits intomainfrom Mar 4, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a secrets subsystem (env/db/AWS/Vault) with encrypted DB store and Vault Helm chart, runtime LLM override/config-from-UI, server secrets and model-list endpoints, frontend secrets UI and LLM controls, deployment/script updates for Vault, dependency additions, and tests for LLM publish/override behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend/User
participant API as Backend API
participant Resolver as Secret Resolver
participant Backends as Secret Backend (DB/Vault/AWS)
participant LLMManager as LLMManager / Cache
participant LLM as External LLM Provider
Frontend->>API: Publish or load LLM config (may include api_key ref)
API->>Resolver: resolve_secret(api_key_ref)
Resolver->>Backends: get(path, field)
Backends-->>Resolver: secret_value
Resolver-->>API: resolved_api_key
API->>LLMManager: check model cache / decide create_llm_from_config
alt llm_config provided
API->>API: create_llm_from_config(llm_config)
else
API->>LLMManager: get_model(settings)
end
API->>LLM: initialize/invoke model (with resolved credentials)
LLM-->>API: response
API-->>Frontend: result / applied config
sequenceDiagram
participant User as Frontend UI
participant UI as SecretsManager
participant API as Backend Secrets API
participant Resolver as Secret Resolver
participant Vault as Vault Backend
participant DB as DB Backend
User->>UI: Open Secrets Manager
UI->>API: GET /api/secrets
API->>Resolver: enumerate/resolve refs
Resolver->>Vault: get(vault://...)
Resolver->>DB: get(db://...)
Vault-->>API: vault secrets
DB-->>API: db secrets
API-->>UI: secrets list + mode
User->>UI: Create secret
UI->>API: POST /api/secrets (id, value)
alt vault mode
API->>Vault: set(path, value)
else
API->>DB: set_secret(id, encrypted_value)
end
Vault-->>API: success
DB-->>API: success
API-->>UI: db://ref (or confirmation)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cuga/backend/server/main.py (1)
107-154:⚠️ Potential issue | 🔴 CriticalInitialize
current_llmin app state before first stream request.
event_streamaccessesapp_state.current_llmdirectly, butAppStatedoes not initialize that attribute. If no startup config is applied, this can raiseAttributeErroron live requests.🔧 Proposed fix
class AppState: @@ def __init__(self): @@ self.tools_include_by_app: Optional[Dict[str, List[str]]] = None self.tools_include_version: int = 0 + self.current_llm: Optional[Any] = None self.initialize_sdk() @@ class DraftAppState: @@ def __init__(self): @@ self.policy_system: Optional[Any] = None self.policy_filesystem_sync: Optional[Any] = None # PolicyFilesystemSync instance for draft + self.current_llm: Optional[Any] = None @@ - current_llm=app_state.current_llm if agent is None else getattr(draft_app_state, "current_llm", None), + current_llm=getattr(app_state, "current_llm", None) + if agent is None + else getattr(draft_app_state, "current_llm", None),Also applies to: 173-182, 938-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/main.py` around lines 107 - 154, AppState lacks a current_llm attribute which event_stream reads, causing AttributeError on first stream request; update AppState.__init__ to define self.current_llm = None (or a sensible default LLM object) so the attribute always exists, and ensure any code paths that replace or use current_llm (e.g., initialize_sdk, event_stream) handle the None/default safely; reference the AppState class and the current_llm attribute to locate where to add the initialization.
🟠 Major comments (19)
deployment/helm/vault/values.yaml-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorVault is configured insecure-by-default (TLS disabled).
Line 7 and Line 26 disable TLS by default. That weakens token/secret transport guarantees and is risky if this values file is reused beyond local demos.
Also applies to: 23-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/helm/vault/values.yaml` at line 7, The values file currently disables TLS via the tlsDisable key (appears twice), which makes Vault insecure; change tlsDisable to false and configure proper TLS certificates/secrets (e.g., provide tls.cert/tls.key or reference an existing Kubernetes TLS secret) so Vault listens over TLS, and remove any fallback that allows plain HTTP; update both occurrences of tlsDisable and ensure corresponding ingress/Service/helm chart settings point to the TLS secret/cert names so the chart deploys with TLS enabled.deployment/helm/vault/templates/NOTES.txt-7-12 (1)
7-12:⚠️ Potential issue | 🟠 MajorDo not recommend root tokens for app runtime configuration.
Line 9 currently suggests using a root token. Please update this guidance to require a least-privilege app token (or auth method such as Kubernetes/AppRole) and avoid root-token usage in environment files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/helm/vault/templates/NOTES.txt` around lines 7 - 12, Update the NOTES guidance to stop recommending use of a root token and instead require a least-privilege app token or an auth method (e.g., Kubernetes auth or AppRole); change the line that mentions VAULT_TOKEN=<root-or-app-token> to advise creating a scoped application token or using an auth method, and ensure the DYNACONF_SECRETS__VAULT_TOKEN_ENV and VAULT_ADDR examples remain but reference a non-root credential (or an auth method) so callers of VAULT_TOKEN and DYNACONF_SECRETS__VAULT_TOKEN_ENV are explicitly instructed to use least-privilege credentials.src/cuga/settings.toml-88-88 (1)
88-88:⚠️ Potential issue | 🟠 Major
force_env = truedefault effectively disables Vault/DB/AWS refs.With current logic in
src/cuga/backend/llm/models.py(Line 740-Line 742), this default forces env-only behavior and bypasses backend-based refs unless users explicitly flip it. That undermines the new multi-backend secret flow out of the box.Suggested fix
-force_env = true # if true, always resolve from os.environ (ignores UI overrides and Vault) +force_env = false # opt-in only; keep backend refs enabled by default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/settings.toml` at line 88, The configuration default force_env = true causes environment-only resolution and bypasses backend refs; change the default to false in the settings (set force_env = false) and update the resolution logic that checks force_env (the branch in the models resolver that currently forces env lookup) so it only short-circuits to os.environ when force_env is true, otherwise allow Vault/DB/AWS ref resolution to run; ensure the symbol names referenced are the force_env config key and the resolver function in the models module that performs env vs backend lookup so the change affects the existing lookup path.src/cuga/backend/llm/models.py-17-27 (1)
17-27:⚠️ Potential issue | 🟠 MajorProcess-global LLM override can leak across concurrent requests.
This override is shared across the whole process. In multi-request execution, one request can overwrite another request’s override (including provider/API-key refs), causing cross-tenant/config bleed.
Suggested fix
+from contextvars import ContextVar ... -_current_llm_override: Optional[Dict[str, Any]] = None +_current_llm_override: ContextVar[Optional[Dict[str, Any]]] = ContextVar( + "_current_llm_override", default=None +) def get_current_llm_override() -> Optional[Dict[str, Any]]: - return _current_llm_override + return _current_llm_override.get() def set_current_llm_override(override: Optional[Dict[str, Any]]) -> None: - global _current_llm_override - _current_llm_override = override + _current_llm_override.set(override.copy() if isinstance(override, dict) else override)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/llm/models.py` around lines 17 - 27, The module-level _current_llm_override leaks overrides across concurrent requests; replace it with a request-scoped ContextVar and update get_current_llm_override and set_current_llm_override to read/write that ContextVar (keeping the same Optional[Dict[str, Any]] type), so overrides are tied to the current context instead of the whole process; update imports to include contextvars.ContextVar and ensure set_current_llm_override accepts None to clear the context variable.deployment/helm/deploy-openshift.sh-237-243 (1)
237-243:⚠️ Potential issue | 🟠 MajorDon’t unconditionally require
VAULT_TOKENenv when enabling Vault mode.These lines always set
DYNACONF_SECRETS__VAULT_TOKEN_ENV=VAULT_TOKEN, but token injection is optional earlier. That can start the app in vault mode without usable auth material.🛠️ Proposed fix
if [[ "$WITH_VAULT" == true ]]; then VAULT_INTERNAL_ADDR="http://vault.${NAMESPACE}.svc.cluster.local:8200" HELM_ARGS+=( "--set" "env.DYNACONF_SECRETS__MODE=vault" "--set" "env.DYNACONF_SECRETS__VAULT_ADDR=${VAULT_ADDR:-${VAULT_INTERNAL_ADDR}}" - "--set" "env.DYNACONF_SECRETS__VAULT_TOKEN_ENV=VAULT_TOKEN" ) + if [[ -n "${VAULT_TOKEN:-}" ]]; then + HELM_ARGS+=("--set" "env.DYNACONF_SECRETS__VAULT_TOKEN_ENV=VAULT_TOKEN") + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/helm/deploy-openshift.sh` around lines 237 - 243, The current block unconditionally adds "--set env.DYNACONF_SECRETS__VAULT_TOKEN_ENV=VAULT_TOKEN" causing vault mode to require a VAULT_TOKEN even when token injection is optional; update the WITH_VAULT handling so HELM_ARGS only receives the DYNACONF_SECRETS__VAULT_TOKEN_ENV entry if a VAULT_TOKEN is actually provided (e.g., check if VAULT_TOKEN is non-empty with [[ -n "${VAULT_TOKEN:-}" ]] and append the "--set" "env.DYNACONF_SECRETS__VAULT_TOKEN_ENV=VAULT_TOKEN" only in that branch), leaving other HELM_ARGS entries (env.DYNACONF_SECRETS__MODE and env.DYNACONF_SECRETS__VAULT_ADDR) unchanged.src/cuga/backend/secrets/backends/env_backend.py-18-19 (1)
18-19:⚠️ Potential issue | 🟠 MajorPreserve empty-string environment secrets.
Line 19 converts
""toNone, which makes an existing env secret look missing and may trigger fallback to another backend.🐛 Proposed fix
) -> str | None: val = os.environ.get(path) - return val if val else None + return val🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/backends/env_backend.py` around lines 18 - 19, The code currently treats an empty-string secret as missing by using a truthiness check on val; change the return logic so that values returned from os.environ.get(path) are preserved when they are empty strings — i.e., return val when val is not None, otherwise return None — by replacing the truthy check around val (the line using "return val if val else None") with an explicit None check referencing val and the os.environ.get(path) call.src/frontend_workspaces/frontend/src/AddToolModal.tsx-193-224 (1)
193-224:⚠️ Potential issue | 🟠 MajorRequire a selected secret when “Use saved secret” is enabled.
Current logic can submit
tool.authwithoutvaluewhenuseSavedSecretis true and nothing is selected, causing silent auth misconfiguration.🔧 Suggested fix
if (authType !== "none" && (needsKey ? authKey.trim() : true)) { let authValueFinal = authValue.trim(); + if (useSavedSecret && !authValueFinal) return; if (useSavedSecret && authValueFinal && authValueFinal.startsWith("db://")) { tool.auth = { type: authType, ...(needsKey && { key: authKey.trim() }), value: authValueFinal, @@ - const valid = type === "openapi" - ? url.trim().length > 0 - : isCommandMcp - ? command.trim().length > 0 - : url.trim().length > 0; + const authValid = + authType === "none" || + ((needsKey ? authKey.trim().length > 0 : true) && + (!useSavedSecret || authValue.trim().length > 0)); + + const valid = ( + type === "openapi" + ? url.trim().length > 0 + : isCommandMcp + ? command.trim().length > 0 + : url.trim().length > 0 + ) && authValid;Also applies to: 231-236, 536-537
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 193 - 224, The code can set tool.auth with no value when useSavedSecret is true but no secret is selected; update the conditional logic around authValueFinal and the branches that set tool.auth (references: authType, needsKey, authKey, useSavedSecret, authValueFinal, saveAsNewSecret, saveAsNewSecretKey, api.createSecret, tool.auth) so that when useSavedSecret is true you require a non-empty authValueFinal (or a created secret ref) before assigning tool.auth, and skip/throw validation otherwise; similarly adjust the other duplicated branches (around lines handling saveAsNewSecret and the later 231-236/536-537 regions) to only set tool.auth when value is present or a valid db:// ref, and ensure UI/validation surfaces the missing selection rather than silently creating an auth object with no value.src/cuga/backend/secrets/secret_resolver.py-102-130 (1)
102-130:⚠️ Potential issue | 🟠 MajorDo not retry explicitly scoped refs across all backends.
After scheme-specific lookup, Line 119 retries the same
pathon every backend. For explicit refs likevault://...ordb://..., this can return a value from the wrong backend.🔧 Suggested fix
scheme, path = parse_ref(ref) if scheme == "plain": return path + explicit_ref = ref.startswith(("vault://", "db://", "aws://", "env://")) backends = _active_backends() for backend in backends: if getattr(backend, "scheme", None) == scheme: try: val = backend.get( @@ except Exception as e: logger.debug("Secret backend {} get failed: {}", backend.scheme, e) + if explicit_ref: + return None for backend in backends: try: val = backend.get( path,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/secret_resolver.py` around lines 102 - 130, The code retries a scheme-scoped secret across all backends (risking returning a secret from the wrong backend) because after parse_ref returns a non-plain scheme you still run the global fallback loop; modify the logic in secret_resolver.py so the second "for backend in backends" fallback is executed only when the ref is unscoped (i.e., scheme is None/empty/unset) — leave the initial scheme-specific lookup (using parse_ref, _active_backends, and backend.get) as-is and skip the global retry when parse_ref returned an explicit scheme (use getattr(backend, "scheme") to match and do not run the fallback loop for scoped refs).src/frontend_workspaces/frontend/src/AddToolModal.tsx-480-482 (1)
480-482:⚠️ Potential issue | 🟠 MajorInline refresh stores the wrong shape into
secretsList.
setSecretsList(d.secrets || d.overrides || [])drops the requiredreffield used at Line 431. After the first inline save, dropdown option values can become invalid/empty.🔧 Suggested fix
- api.getSecrets().then((r) => { - if (r.ok) r.json().then((d) => setSecretsList(d.secrets || d.overrides || [])); - }); + setSecretsList((prev) => [ + ...prev, + { + id: slug, + description: `Auth for ${name.trim() || "tool"}`, + ref: `db://${slug}`, + }, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 480 - 482, The inline refresh is overwriting secretsList with a shape that drops the required ref field; update the promise handler where api.getSecrets() is used to pass the full secret objects into setSecretsList (e.g., setSecretsList(d.secrets || d.overrides || [])) ensuring you do not map to or extract only values so the ref property remains intact; locate the call in AddToolModal.tsx (the api.getSecrets().then(...) block that calls setSecretsList) and ensure it assigns the original secret objects (including ref) or falls back to an empty array.src/cuga/backend/secrets/seed.py-59-65 (1)
59-65:⚠️ Potential issue | 🟠 Major
seed_secrets_from_env_synccan return before seeding completes.Line 63 schedules seeding and exits immediately in a running loop, so startup callers can observe unseeded state. The task reference is also dropped.
🔧 Suggested fix
+_seed_tasks: set[asyncio.Task] = set() + def seed_secrets_from_env_sync() -> None: """Sync wrapper — runs in a new event loop if none is running.""" try: loop = asyncio.get_running_loop() - loop.create_task(seed_secrets_from_env()) + task = loop.create_task(seed_secrets_from_env()) + _seed_tasks.add(task) + task.add_done_callback(_seed_tasks.discard) except RuntimeError: asyncio.run(seed_secrets_from_env())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/seed.py` around lines 59 - 65, seed_secrets_from_env_sync currently schedules seed_secrets_from_env() with loop.create_task and returns immediately; change it to ensure seeding completes before returning by awaiting the coroutine when called from an async context and by running it to completion with asyncio.run when no loop is running. Concretely, in seed_secrets_from_env_sync detect a running loop, create the task for seed_secrets_from_env(), await that task (or call asyncio.get_running_loop().create_task(...) and await it inside an async wrapper) so the function does not return early; when no loop is running keep using asyncio.run(seed_secrets_from_env()). Ensure you keep a reference to the task so it is awaited rather than dropped.src/cuga/backend/server/manage_routes.py-406-409 (1)
406-409:⚠️ Potential issue | 🟠 MajorClear
agent.llm_configwhenllmis removed from config.The current truthy check keeps the previous
llm_configalive, so clearing LLM settings in UI won’t actually clear runtime graph config.🔧 Proposed fix
- llm_cfg = (config or {}).get("llm") or {} - if llm_cfg: - draft_agent.llm_config = llm_cfg + llm_cfg = (config or {}).get("llm") + draft_agent.llm_config = llm_cfg if isinstance(llm_cfg, dict) and llm_cfg else None @@ - llm_cfg = (config or {}).get("llm") or {} - if llm_cfg: - prod_agent.llm_config = llm_cfg + llm_cfg = (config or {}).get("llm") + prod_agent.llm_config = llm_cfg if isinstance(llm_cfg, dict) and llm_cfg else NoneAlso applies to: 487-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/manage_routes.py` around lines 406 - 409, The code only assigns draft_agent.llm_config when llm_cfg is truthy, leaving old settings when llm was removed; change the logic around the llm_cfg assignment in the block that sets llm_cfg, draft_agent.llm_config, and calls draft_agent.build_graph so that if (config or {}).get("llm") returns None or an empty dict you explicitly set draft_agent.llm_config = {} (or None per your design) to clear previous values before calling draft_agent.build_graph; apply the same change to the analogous block that handles llm at the other occurrence (the block that assigns llm_cfg and then awaits draft_agent.build_graph).src/cuga/backend/server/secrets_routes.py-192-204 (1)
192-204:⚠️ Potential issue | 🟠 MajorPreserve
agent_id/versionscope in update/delete flows.
create_secretallows scoped secrets, but update/delete read and write with default*scope, so agent/version-scoped secrets are not correctly updated/deleted.🔧 Proposed fix
`@router.put`("/{secret_id}") async def update_secret( secret_id: str, body: SecretUpdate, + agent_id: Optional[str] = None, + version: Optional[str] = None, current_user: Optional[UserInfo] = Depends(require_auth), ) -> dict[str, Any]: @@ - meta = await secrets_store.get_secret_metadata(secret_id) + scope_agent = agent_id or "*" + scope_version = version or "*" + meta = await secrets_store.get_secret_metadata(secret_id, agent_id=scope_agent, version=scope_version) @@ await secrets_store.set_secret( secret_id, body.value, description=body.description if body.description is not None else meta.get("description"), tags=body.tags if body.tags is not None else meta.get("tags"), + agent_id=scope_agent, + version=scope_version, created_by=creator, ) @@ `@router.delete`("/{secret_id}") async def delete_secret( secret_id: str, + agent_id: Optional[str] = None, + version: Optional[str] = None, current_user: Optional[UserInfo] = Depends(require_auth), ) -> dict[str, Any]: @@ - meta = await secrets_store.get_secret_metadata(secret_id) + scope_agent = agent_id or "*" + scope_version = version or "*" + meta = await secrets_store.get_secret_metadata(secret_id, agent_id=scope_agent, version=scope_version) @@ - ok = await secrets_store.delete_secret(secret_id) + ok = await secrets_store.delete_secret(secret_id, agent_id=scope_agent, version=scope_version)Also applies to: 236-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 192 - 204, The update/delete flows currently ignore secret scope and use the default `*` scope; when fetching metadata via secrets_store.get_secret_metadata(secret_id) extract the scope fields (e.g., agent_id and version) from meta and pass them through to secrets_store.set_secret (and the delete call) so the same scoped secret is updated/deleted; preserve the original created_by check via _user_id(current_user) and keep using meta.get("description")/meta.get("tags") but add meta.get("agent_id") and meta.get("version") (or whatever scope keys exist) as explicit parameters to set_secret and to the deletion method in the corresponding block (also apply the same change in the other block around lines 236-244).src/cuga/backend/server/secrets_routes.py-155-156 (1)
155-156:⚠️ Potential issue | 🟠 MajorUse configured Vault mount when returning refs.
Returned refs hardcode
vault://secret/...; ifsettings.secrets.vault_mountis different, the returned reference won’t resolve.🔧 Proposed fix
+def _vault_mount() -> str: + try: + from cuga.config import settings + sec = getattr(settings, "secrets", None) + return (getattr(sec, "vault_mount", "") or "secret") if sec else "secret" + except Exception: + return "secret" @@ - ref = f"vault://secret/{body.id}#value" + ref = f"vault://{_vault_mount()}/{body.id}#value" @@ - return {"ref": f"vault://secret/{secret_id}#value", "id": secret_id} + return {"ref": f"vault://{_vault_mount()}/{secret_id}#value", "id": secret_id}Also applies to: 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 155 - 156, The returned Vault ref is hardcoded to "vault://secret/..." so it will break when a custom mount is configured; update the code that builds the ref (where ref = f"vault://secret/{body.id}#value" and the similar occurrence around line 191) to use the configured mount from settings.secrets.vault_mount (e.g., build ref = f"vault://{settings.secrets.vault_mount}/{body.id}#value"), ensuring both places that construct the ref (the handler that returns {"ref": ref, "id": body.id} and the second occurrence) are changed to reference settings.secrets.vault_mount instead of the hardcoded "secret".tests/integration/test_llm_config_publish.py-38-47 (1)
38-47:⚠️ Potential issue | 🟠 MajorRestore env mutations in fixture teardown to keep tests isolated.
reset_llm_stateresets manager/override, but not env vars changed by_apply_published_config, so later tests can inherit stale model/SSL settings.🔧 Proposed fix
+import os import pytest from unittest.mock import patch, MagicMock @@ `@pytest.fixture`(autouse=True) def reset_llm_state(): + tracked_env = ("MODEL_NAME", "MODEL_TEMPERATURE", "CUGA_DISABLE_SSL") + env_snapshot = {k: os.environ.get(k) for k in tracked_env} mgr = LLMManager() mgr._models.clear() mgr._pre_instantiated_model = None set_current_llm_override(None) yield mgr._models.clear() mgr._pre_instantiated_model = None set_current_llm_override(None) + for k, v in env_snapshot.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_llm_config_publish.py` around lines 38 - 47, The fixture reset_llm_state currently clears LLMManager state but doesn’t revert environment changes done by _apply_published_config; modify reset_llm_state to snapshot os.environ (e.g., orig_env = os.environ.copy()) before yield and restore it in teardown (clear os.environ then os.environ.update(orig_env)) so any model/SSL/env vars set by _apply_published_config are fully restored; keep the existing LLMManager resets (mgr._models.clear(), mgr._pre_instantiated_model = None, set_current_llm_override(None)) and perform the env restore alongside them.src/cuga/backend/secrets/backends/vault_backend.py-40-46 (1)
40-46:⚠️ Potential issue | 🟠 MajorDo not infer mount from the first path segment unconditionally.
For paths like
team/service/key, this logic usesteamas mount instead of default mount + nested secret path, causing reads/writes against the wrong mount.🔧 Proposed fix
def _split_mount_and_path(full_path: str, default_mount: str = "secret") -> tuple[str, str]: - parts = full_path.strip().split("/") - if len(parts) >= 2: - return parts[0], "/".join(parts[1:]) - if len(parts) == 1 and parts[0]: - return default_mount, parts[0] + path = full_path.strip().strip("/") + if not path: + return default_mount, "" + prefix = f"{default_mount}/" + if path.startswith(prefix): + return default_mount, path[len(prefix):] + return default_mount, path - return default_mount, ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/backends/vault_backend.py` around lines 40 - 46, _split_mount_and_path is incorrectly treating the first path segment as the vault mount for any multi-segment full_path (e.g., "team/service/key"); change it so it only treats the first segment as a mount when that segment is a known/explicit mount (e.g., equals default_mount or is in an allowed_mounts set) — otherwise return default_mount and the entire full_path as the secret path. Update the function signature to accept an optional allowed_mounts collection or compare parts[0] to default_mount, and use that check in the branch that currently returns parts[0], "/".join(parts[1:]) so generic multi-segment names like "team/service/key" do not become mounts.src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py-742-746 (1)
742-746:⚠️ Potential issue | 🟠 Major
model_settingsis currently a no-op.
create_cuga_lite_graph(..., model_settings=...)passes this through, butcreate_call_model_nodenever uses it. That silently ignores caller-provided model configuration.💡 Proposed fix
-def create_call_model_node(base_model, base_callbacks, model_settings=None): +def create_call_model_node( + base_model, + base_callbacks, + model_settings: Optional[Dict[str, Any]] = None, +): @@ - active_model = configurable.get("llm") or base_model + active_model = configurable.get("llm") + if active_model is None and model_settings: + active_model = llm_manager.get_model(model_settings) + if active_model is None: + active_model = base_modelAlso applies to: 1176-1176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py` around lines 742 - 746, The create_call_model_node factory currently ignores the model_settings parameter (passed from create_cuga_lite_graph), so caller-provided model configuration is dropped; update create_call_model_node to accept and merge/apply model_settings when constructing the call_model node: locate create_call_model_node and the call site where it builds/returns the node (and similar code around line 1176), thread model_settings into whatever config/kwargs are passed to the underlying model invocation (e.g., merge model_settings with base_model/base_callbacks or with config['configurable']['llm'] precedence rules), and ensure the returned node uses that merged configuration so provided settings are honored.src/frontend_workspaces/frontend/src/ManagePage.tsx-398-399 (1)
398-399:⚠️ Potential issue | 🟠 MajorSecret/tool operations are not agent-scoped.
These paths currently hardcode or omit agent identity, so data can be fetched/saved against the wrong agent instead of the route-selected one.
💡 Proposed fix
+ const resolvedAgentId = agentId ?? agentContext?.agent_id ?? "cuga-default"; @@ - const [secretsRes, configRes] = await Promise.all([api.getSecrets(), api.getSecretsConfig()]); + const [secretsRes, configRes] = await Promise.all([ + api.getSecrets(resolvedAgentId), + api.getSecretsConfig(), + ]); @@ - agentId={"cuga-default"} + agentId={resolvedAgentId} @@ - <SecretsManager open={secretsModalOpen} onClose={() => { setSecretsModalOpen(false); refreshSecrets(); }} agentId="cuga-default" /> + <SecretsManager + open={secretsModalOpen} + onClose={() => { + setSecretsModalOpen(false); + refreshSecrets(); + }} + agentId={resolvedAgentId} + />Also applies to: 1041-1041, 1341-1341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 398 - 399, The calls to api.getSecrets() and api.getSecretsConfig() in ManagePage.tsx are not scoped to the currently selected agent/route, so secrets/tools can be read or written for the wrong agent; update every API call (e.g., api.getSecrets(), api.getSecretsConfig(), and other secret/tool calls found later in this file such as save/delete/update secret/tool usages around the other occurrences) to pass the selected agent/route identifier (for example selectedAgent.id or route.id) as an explicit parameter or option so the backend request is agent-scoped; locate the API calls in ManagePage.tsx (symbols: api.getSecrets, api.getSecretsConfig and any api.saveSecret/api.deleteSecret/api.updateSecret usages) and change their signatures/call sites to include the agent id and adjust the API client methods to accept and forward that id to the server.src/cuga/backend/storage/secrets_store.py-177-180 (1)
177-180:⚠️ Potential issue | 🟠 Major
get_secret_synccan falsely returnNoneafter timeout.If the worker thread exceeds 5s, this returns
Noneeven though retrieval may still complete later, causing nondeterministic false negatives.💡 Proposed fix
- t = threading.Thread(target=run) + t = threading.Thread(target=run, daemon=True) t.start() - done.wait(timeout=5.0) + if not done.wait(timeout=5.0): + raise TimeoutError("get_secret_sync timed out after 5 seconds") + t.join(timeout=0) return result[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/storage/secrets_store.py` around lines 177 - 180, get_secret_sync currently starts a worker thread (run) and calls done.wait(timeout=5.0) which can return False and lead to returning result[0] == None even though the background retrieval later succeeds; change the synchronization so we wait until the worker signals completion instead of using a 5s timeout: replace done.wait(timeout=5.0) with a blocking done.wait() (or equivalent loop that waits until done.is_set()), then return result[0]; you may also join the thread (t.join()) after the event to ensure cleanup. Ensure you update references in get_secret_sync to use done, result, and run accordingly and do not return until the event is set.src/frontend_workspaces/frontend/src/ManagePage.tsx-900-908 (1)
900-908:⚠️ Potential issue | 🟠 MajorInline secret creation always stores a
db://reference.At Line 900 this hardcodes DB refs, which breaks vault mode where refs should be vault-formatted.
💡 Proposed fix
- const ref = `db://${slug}`; + const ref = + llmSecretsMode === "vault" + ? `vault://secret/${slug}#value` + : `db://${slug}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 900 - 908, The code hardcodes a DB secret ref (`db://${slug}`) when creating inline secrets; change it to build the ref using the app's current secret-store mode instead of "db://". Locate the inline-create block (references: ref variable, updateLlm("api_key", ref), saveLlmDraft, refreshSecrets) and replace the literal with a mode-aware helper (e.g., derive prefix from the existing secret store state or a helper such as getSecretRefPrefix()/secretStoreType) so refs become "db://{slug}" for DB mode and the appropriate "vault://{slug}" (or other) for vault mode, then proceed with refreshSecrets(), updateLlm(...) and saveLlmDraft() as before.
🟡 Minor comments (4)
src/frontend_workspaces/agentic_chat/src/CugaHeader.tsx-220-232 (1)
220-232:⚠️ Potential issue | 🟡 MinorAvoid rendering duplicate separators in the user panel.
With
onOpenSecretsset, both Line 229 and Line 232 render dividers back-to-back.✂️ Suggested cleanup
- {onOpenSecrets && ( - <> - <SwitcherItem - aria-label="Manage Secrets" - onClick={() => { onOpenSecrets(); setUserPanelOpen(false); }} - > - <Password size={16} style={{ marginRight: "0.5rem", flexShrink: 0 }} /> - Manage Secrets - </SwitcherItem> - <SwitcherDivider /> - </> - )} - <SwitcherDivider /> + {onOpenSecrets && ( + <> + <SwitcherItem + aria-label="Manage Secrets" + onClick={() => { onOpenSecrets(); setUserPanelOpen(false); }} + > + <Password size={16} style={{ marginRight: "0.5rem", flexShrink: 0 }} /> + Manage Secrets + </SwitcherItem> + </> + )} + <SwitcherDivider />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/agentic_chat/src/CugaHeader.tsx` around lines 220 - 232, The user panel currently renders two adjacent SwitcherDivider elements when onOpenSecrets is truthy (one inside the onOpenSecrets conditional and one immediately after), so remove the duplicate by rendering a single divider; update the JSX around SwitcherItem and SwitcherDivider so that either the divider is only rendered inside the onOpenSecrets block (after SwitcherItem) or the outer SwitcherDivider is rendered conditionally when onOpenSecrets is falsy—refer to onOpenSecrets, SwitcherItem, SwitcherDivider, and setUserPanelOpen to locate and adjust the rendering logic.src/cuga/backend/llm/models.py-734-735 (1)
734-735:⚠️ Potential issue | 🟡 MinorBroad exception swallowing hides config resolution failures.
These blocks silently discard failures and make debugging secret/model selection issues difficult. Please narrow exception scope and log context.
Suggested fix
- try: + try: max_tokens = settings.agent.code.model.get("max_tokens", 16000) - except Exception: + except (AttributeError, TypeError, ValueError) as exc: + logger.debug("create_llm_from_config: fallback max_tokens due to settings read error: {}", exc) max_tokens = 16000 ... - except Exception: - pass + except (AttributeError, TypeError, ValueError) as exc: + logger.debug("create_llm_from_config: unable to read agent.code TOML override: {}", exc)Also applies to: 775-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/llm/models.py` around lines 734 - 735, The except block that silently sets max_tokens = 16000 should not swallow all Exceptions; replace the bare except with specific exception types that can occur during config/model resolution (e.g., KeyError, ValueError, LookupError or your ConfigError) and log the error and context (include model identifier/secret name and any resolving inputs) via the module logger before applying a safe fallback; for truly unexpected exceptions re-raise them so they surface. Apply the same change to the similar block that sets max_tokens at the other occurrence (the block at the second mentioned location), ensuring you reference the same variable name max_tokens and the surrounding model/secret resolution logic when adding the logger call and narrowed exception list.src/cuga/backend/server/demo_manage_setup.py-135-140 (1)
135-140:⚠️ Potential issue | 🟡 MinorAvoid silent failure when resolving demo LLM API-key ref.
The broad
except ... passcan mask real setup issues. Please at least log the failure and narrow exception scope.Suggested fix
+ from loguru import logger ... - except Exception: - pass + except (ImportError, AttributeError, RuntimeError) as exc: + logger.debug("setup_demo_manage_config: could not resolve llm api key ref: {}", exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/demo_manage_setup.py` around lines 135 - 140, The try/except around resolve_llm_api_key_ref is silently swallowing errors; change it to catch specific exceptions (e.g., ImportError and the specific resolution error your resolver raises) instead of a bare except, and log the failure with a module or process logger (include the exception message/stack) while leaving llm_api_key_ref unset or None on failure; target the resolve_llm_api_key_ref import and call site to add the narrow exception types and a processLogger or logging.getLogger(__name__).error(...) call that includes the exception details.src/cuga/backend/secrets/secret_resolver.py-97-99 (1)
97-99:⚠️ Potential issue | 🟡 MinorFalsy non-string refs are dropped unexpectedly.
At Line 98,
0andFalsebecomeNoneinstead of"0"/"False".🔧 Suggested fix
if ref is None: return None if not isinstance(ref, str): - return str(ref) if ref else None + return str(ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/secret_resolver.py` around lines 97 - 99, The current logic treats any falsy non-string (e.g., 0 or False) as None because of "if not isinstance(ref, str): return str(ref) if ref else None"; change it so only None becomes None and all other non-strings are converted to strings: check "if ref is None: return None", then "if not isinstance(ref, str): ref = str(ref)" and after that perform "ref = ref.strip()" and treat empty-string as None if that behavior is desired; update the code around the "ref" handling in secret_resolver.py accordingly.
🧹 Nitpick comments (4)
tests/system/test_manager_api_integration.py (1)
387-390: Replace fixed 15s delay with readiness polling.This fixed sleep is still timing-sensitive and makes the suite slower. Prefer bounded polling for actual draft+MCP readiness so tests fail fast when broken and don’t over-wait when healthy.
Proposed refactor
- logger.info("Waiting 15 seconds for agent rebuild and MCP servers to fully initialize...") - time.sleep(15) + logger.info("Waiting for draft registry reload + MCP startup readiness...") + deadline = time.time() + 30 + while time.time() < deadline: + probe = http_client.post( + STREAM_API_URL, + json={"query": "List the files in the ./test_workspace directory"}, + headers={"X-Use-Draft": "true"}, + ) + if probe.status_code == 200 and ( + "sample.txt" in probe.text.lower() or "sample" in probe.text.lower() + ): + break + time.sleep(1) + else: + raise AssertionError("Draft runtime did not become ready within 30 seconds")deployment/helm/deploy-openshift.sh (1)
193-197: Array-based approach is more idiomatic, but the current code is not broken.Lines 196 in its current form works correctly with properly quoted content, as the inner quotes protect the argument from word splitting. While building arguments via an array (as proposed) is a better shell scripting practice and more maintainable, the original code does not actually produce malformed arguments.
Proposed improvement
if [[ "$WITH_VAULT" == true ]]; then echo "[${STEP}/${TOTAL_STEPS}] Deploying HashiCorp Vault" VAULT_CHART_PATH="${SCRIPT_DIR}/vault" + VAULT_HELM_ARGS=() + if [[ -n "${VAULT_TOKEN:-}" ]]; then + VAULT_HELM_ARGS+=(--set "vault.server.extraEnvironmentVars.VAULT_DEV_ROOT_TOKEN_ID=${VAULT_TOKEN}") + fi + helm upgrade --install vault "${VAULT_CHART_PATH}" \ --namespace "${NAMESPACE}" \ -f "${VAULT_CHART_PATH}/values.openshift.yaml" \ - ${VAULT_TOKEN:+--set "vault.server.extraEnvironmentVars.VAULT_DEV_ROOT_TOKEN_ID=${VAULT_TOKEN}"} + "${VAULT_HELM_ARGS[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/helm/deploy-openshift.sh` around lines 193 - 197, Convert the inline, concatenated helm argument into an array-based args build to be more idiomatic and robust: create an array (e.g., ARGS) initialized with the fixed helm arguments including "${VAULT_CHART_PATH}", "--namespace", and "${NAMESPACE}", then if VAULT_TOKEN is set append the --set "vault.server.extraEnvironmentVars.VAULT_DEV_ROOT_TOKEN_ID=${VAULT_TOKEN}" entry to that array, and finally call helm upgrade --install vault "${ARGS[@]}" (preserving the -f value for "${VAULT_CHART_PATH}/values.openshift.yaml"); keep STEP++ unchanged. Ensure all expansions remain quoted when added to the array so word-splitting and globbing are avoided.tests/integration/test_llm_config_publish.py (1)
167-269: Consider isolating live-provider auth tests from default CI runs.These tests call real provider clients and can become slow/flaky due network/runtime variance. Prefer marking them as external (or mocking transport-level auth failure) so the default suite remains deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_llm_config_publish.py` around lines 167 - 269, These integration tests (test_openai_bad_key_override_raises_auth_error, test_openai_bad_key_override_raises_auth_error_async, test_groq_bad_key_override_raises_auth_error) hit real provider clients via set_current_llm_override → LLMManager → model.invoke/ainvoke and should be isolated from default CI; update each test to either (a) mark with a skip/marker like pytest.mark.external or pytest.mark.live and skip unless an env var (e.g., RUN_LIVE_PROVIDER_TESTS) is set, or (b) replace the network call by mocking the transport/auth failure so invoke/ainvoke raises the expected auth exception (mock the provider client used by LLMManager or the HTTP layer), ensuring the test still asserts on error_msg.src/cuga/backend/secrets/backends/db_backend.py (1)
15-68: Extract shared DB lookup logic to reduce backend drift.
EnvOverrideBackendandDbBackendcurrently duplicateavailable()andget()behavior. A small shared base keeps future changes consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/backends/db_backend.py` around lines 15 - 68, EnvOverrideBackend and DbBackend duplicate available() and get() logic; create a shared base class (e.g., BaseDbBackend) that implements available() (calling cuga.backend.storage.secrets_store._fernet) and a generic get() that converts path via _path_to_slug and calls get_secret_sync with tenant_id/instance_id/agent_id, then have EnvOverrideBackend and DbBackend inherit from that base and only set their scheme or override behavior if needed; ensure references to _path_to_slug, get_secret_sync, and _fernet remain unchanged so callers keep same semantics.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/frontend_workspaces/frontend/dist/index.htmlis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.9591b3c88a9fff9ce83b.jsis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.bad73e1b2f37a12c44ab.jsis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/vendors.1d0248ed8e71fecf29d7.jsis excluded by!**/dist/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.secrets.baselinedeployment/helm/deploy-openshift.shdeployment/helm/openshift.example.envdeployment/helm/vault/Chart.yamldeployment/helm/vault/templates/NOTES.txtdeployment/helm/vault/values.openshift.yamldeployment/helm/vault/values.yamlpyproject.tomlsrc/cuga/backend/cuga_graph/graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/prompt_utils.pysrc/cuga/backend/cuga_graph/utils/agent_loop.pysrc/cuga/backend/llm/models.pysrc/cuga/backend/secrets/__init__.pysrc/cuga/backend/secrets/backends/__init__.pysrc/cuga/backend/secrets/backends/aws_backend.pysrc/cuga/backend/secrets/backends/base.pysrc/cuga/backend/secrets/backends/db_backend.pysrc/cuga/backend/secrets/backends/env_backend.pysrc/cuga/backend/secrets/backends/vault_backend.pysrc/cuga/backend/secrets/models.pysrc/cuga/backend/secrets/secret_resolver.pysrc/cuga/backend/secrets/seed.pysrc/cuga/backend/server/demo_manage_setup.pysrc/cuga/backend/server/main.pysrc/cuga/backend/server/manage_routes.pysrc/cuga/backend/server/secrets_routes.pysrc/cuga/backend/storage/secrets_store.pysrc/cuga/backend/tools_env/registry/mcp_manager/adapter.pysrc/cuga/backend/tools_env/registry/mcp_manager/mcp_manager.pysrc/cuga/config.pysrc/cuga/settings.tomlsrc/frontend_workspaces/agentic_chat/src/CugaHeader.tsxsrc/frontend_workspaces/frontend/src/AddToolModal.csssrc/frontend_workspaces/frontend/src/AddToolModal.tsxsrc/frontend_workspaces/frontend/src/CugaHeader.tsxsrc/frontend_workspaces/frontend/src/ManagePage.tsxsrc/frontend_workspaces/frontend/src/SecretsManager.tsxsrc/frontend_workspaces/frontend/src/ToolsConfig.csssrc/frontend_workspaces/frontend/src/ToolsConfig.tsxsrc/frontend_workspaces/frontend/src/api.tstests/integration/test_llm_config_publish.pytests/system/test_manager_api_integration.pytests/unit/test_llm_override.py
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/cuga/backend/server/manage_routes.py (1)
220-229:⚠️ Potential issue | 🔴 CriticalDo not forward generic
Authorizationas provider API keyThis endpoint can unintentionally forward unrelated bearer tokens (e.g., app auth) to external provider APIs. Restrict to
X-LLM-API-Key(or server-side secret ref) and resolve refs explicitly.Suggested fix
- auth_header = request.headers.get("Authorization") or request.headers.get("X-LLM-API-Key") - if auth_header and auth_header.lower().startswith("bearer "): - api_key = auth_header[7:].strip() - elif auth_header: - api_key = auth_header.strip() - else: - from cuga.backend.secrets import resolve_secret - - key_ref = _PROVIDER_API_KEY_REF.get(provider_key, "OPENAI_API_KEY") - api_key = resolve_secret(key_ref) + from cuga.backend.secrets import resolve_secret + + raw_key = (request.headers.get("X-LLM-API-Key") or "").strip() + if raw_key: + api_key = resolve_secret(raw_key) or raw_key + else: + key_ref = _PROVIDER_API_KEY_REF.get(provider_key, "OPENAI_API_KEY") + api_key = resolve_secret(key_ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/manage_routes.py` around lines 220 - 229, The current auth extraction in manage_routes.py reads a generic Authorization header and may forward unrelated bearer tokens; change the logic in the block that sets api_key (using auth_header, _PROVIDER_API_KEY_REF, resolve_secret, provider_key) to only accept the explicit X-LLM-API-Key header for client-supplied provider API keys and ignore Authorization entirely, and otherwise fall back to resolving the server-side secret via _PROVIDER_API_KEY_REF.get(provider_key, "OPENAI_API_KEY") with resolve_secret(provider_key_ref); update the code that reads request.headers to check request.headers.get("X-LLM-API-Key") first and never use Authorization as the provider API key.src/cuga/backend/storage/secrets_store.py (1)
199-203:⚠️ Potential issue | 🔴 Critical
set_secretcan leak the relational store handle on missing keyThe store is opened before Fernet key validation; if key is missing, it raises before
finallycan close that handle.Suggested fix
- store = _get_store() fernet = _fernet() if not fernet: raise RuntimeError("CUGA_SECRET_KEY must be set to use the DB secrets backend") + store = _get_store()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/storage/secrets_store.py` around lines 199 - 203, The code opens the DB handle via _get_store() before validating the Fernet key, which can leak the store if validation raises; fix by validating the Fernet key first (call _fernet() and ensure it's truthy) before calling _get_store(), or if you prefer to keep the current order, wrap the store usage in a try/finally (or context manager) that always closes the store when set_secret (or the surrounding function) raises — refer to _get_store, _fernet, and the set_secret flow to locate where to reorder or add the try/finally to ensure store.close() runs on errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cuga/backend/cuga_graph/graph.py`:
- Around line 179-185: The code builds the runtime model from self.llm_config
via create_llm_from_config but then reconstructs model_config from
settings.agent.code.model for the supervisor path, causing inconsistent
credentials; update the supervisor/model settings path to derive its
model_config from the same source when self.llm_config is present: reuse or copy
self.llm_config (or transform it into the expected model_config shape) instead
of reading settings.agent.code.model in the block around create_llm_from_config
and the analogous block at lines 322-324, ensuring LLMManager is instantiated
with the propagated config so supervisor and runtime use identical
provider/model credentials.
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 420-423: The code only sets draft_agent.llm_config when llm_cfg is
truthy, so if the incoming config removes "llm" the previous llm_config remains;
modify the draft path around llm_cfg and draft_agent.build_graph to explicitly
clear draft_agent.llm_config (e.g., set to None or {}) when (config or
{}).get("llm") is falsy, and apply the same explicit-clear logic to the publish
path where publish_agent.llm_config is handled (the block referenced at lines
~501-503) so removed LLM settings no longer persist across builds.
In `@src/cuga/backend/storage/secrets_store.py`:
- Around line 177-180: get_secret_sync currently waits up to 5s with
done.wait(timeout=5.0) but always returns result[0] even when wait timed out;
change the logic to check the boolean returned by done.wait and handle timeout
explicitly (e.g., raise a TimeoutError or return a clear sentinel) instead of
returning possibly-None result[0]; locate get_secret_sync and the usage of
threading.Thread/run/done/result, call done.wait(timeout=5.0) into a variable
(e.g., finished = done.wait(...)), if not finished then raise or return a
timeout error, otherwise safely return result[0]; optionally join the thread
(t.join(timeout=0)) or set it as daemon to avoid leaking threads.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Around line 396-399: refreshSecrets, ToolsConfig, and SecretsManager are
currently operating on a hardcoded agent context (cuga-default) instead of the
current route's agentId; update refreshSecrets to call api.getSecrets and
api.getSecretsConfig with the current agentId (from the route or component prop)
and propagate that agentId into ToolsConfig and SecretsManager props so their
CRUD/UI calls use the correct agent; locate references to refreshSecrets,
ToolsConfig, SecretsManager, and api.getSecrets / api.getSecretsConfig and
replace hardcoded agent identifiers with the dynamic agentId from useParams or
the page's context and ensure any child components accept and use the agentId
when calling api methods.
- Around line 412-417: The ref mapping incorrectly falls back to `db://` for AWS
secrets; update the conditional that constructs the ref (the ternary using
s.source, mode, and the ref property) to add a branch for s.source === "aws" and
return the correct AWS reference (for example `aws://secret/${s.id}` or your
project's canonical AWS secret URI) instead of `db://${s.id}` so AWS-backed
secrets resolve properly.
---
Duplicate comments:
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 220-229: The current auth extraction in manage_routes.py reads a
generic Authorization header and may forward unrelated bearer tokens; change the
logic in the block that sets api_key (using auth_header, _PROVIDER_API_KEY_REF,
resolve_secret, provider_key) to only accept the explicit X-LLM-API-Key header
for client-supplied provider API keys and ignore Authorization entirely, and
otherwise fall back to resolving the server-side secret via
_PROVIDER_API_KEY_REF.get(provider_key, "OPENAI_API_KEY") with
resolve_secret(provider_key_ref); update the code that reads request.headers to
check request.headers.get("X-LLM-API-Key") first and never use Authorization as
the provider API key.
In `@src/cuga/backend/storage/secrets_store.py`:
- Around line 199-203: The code opens the DB handle via _get_store() before
validating the Fernet key, which can leak the store if validation raises; fix by
validating the Fernet key first (call _fernet() and ensure it's truthy) before
calling _get_store(), or if you prefer to keep the current order, wrap the store
usage in a try/finally (or context manager) that always closes the store when
set_secret (or the surrounding function) raises — refer to _get_store, _fernet,
and the set_secret flow to locate where to reorder or add the try/finally to
ensure store.close() runs on errors.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.secrets.baselinesrc/cuga/backend/cuga_graph/graph.pysrc/cuga/backend/llm/models.pysrc/cuga/backend/server/manage_routes.pysrc/cuga/backend/storage/secrets_store.pysrc/frontend_workspaces/frontend/src/ManagePage.tsxsrc/frontend_workspaces/frontend/src/api.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/frontend_workspaces/frontend/src/ManagePage.tsx (1)
417-422:⚠️ Potential issue | 🟠 MajorAWS secret sources are still mapped to
db://refs.This duplicates a previously reported issue: AWS entries need an
aws://reference branch, otherwise AWS-backed secrets resolve incorrectly.Suggested fix
setLlmSecretsList(raw.map((s) => ({ id: s.id, description: s.description, ref: s.source === "vault" || mode === "vault" ? `vault://secret/${s.id}#value` + : s.source === "aws" + ? `aws://${s.id}` : s.source === "env" ? s.id : `db://${s.id}`, })));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 417 - 422, The ref construction in ManagePage.tsx wrongly maps AWS-backed secrets to the db:// branch; update the ternary that builds ref for each secret (the expression using s.source and mode) to handle s.source === "aws" (and when mode === "vault" with s.source === "aws" if relevant) by returning an aws://... reference (e.g., aws://secret/${s.id}#value) instead of falling through to db:// so AWS secrets resolve correctly; adjust the branches where ref is computed (the ternary checking s.source === "vault" || mode === "vault", s.source === "env", else ...) to include an s.source === "aws" case that produces the aws:// scheme.src/cuga/backend/server/manage_routes.py (1)
226-236:⚠️ Potential issue | 🔴 CriticalDo not treat request
Authorizationas provider API key.Because this route is already auth-protected, Line 226 can pick up the app auth token and forward it to OpenAI/Groq. Use only a dedicated header (or server-side secret) for provider credentials.
🔒 Proposed fix
- auth_header = request.headers.get("Authorization") or request.headers.get("X-LLM-API-Key") - if auth_header and auth_header.lower().startswith("bearer "): - api_key = auth_header[7:].strip() - elif auth_header: - api_key = auth_header.strip() + api_key_header = request.headers.get("X-LLM-API-Key") + if api_key_header and api_key_header.lower().startswith("bearer "): + api_key = api_key_header[7:].strip() + elif api_key_header: + api_key = api_key_header.strip() else: from cuga.backend.secrets import resolve_secret🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/manage_routes.py` around lines 226 - 236, The code currently reads Authorization and may forward an app auth token as the provider API key; change the logic in the route handler that sets auth_header/api_key so it ignores request.headers.get("Authorization") entirely and only checks a dedicated header (e.g., "X-LLM-API-Key") and then falls back to server-side secret resolution via _PROVIDER_API_KEY_REF and resolve_secret; update the code paths referencing auth_header, api_key, and request.headers so Authorization is never used as a provider credential.src/cuga/backend/storage/secrets_store.py (1)
201-204:⚠️ Potential issue | 🔴 CriticalOpen connection leak on missing encryption key in
set_secret.Line 201 opens the relational store before key validation, but Line 204 can raise before the
try/finallyclose block starts (Line 208), leaking the handle.🔧 Proposed fix
- store = _get_store() fernet = _fernet() if not fernet: raise RuntimeError("CUGA_SECRET_KEY must be set to use the DB secrets backend") + store = _get_store()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/storage/secrets_store.py` around lines 201 - 204, The code in set_secret opens the relational store via _get_store() before validating the encryption key from _fernet(), which can raise RuntimeError and leak the store handle; either move the _fernet() call and its missing-key validation to happen before calling _get_store(), or ensure the store is closed on all paths by wrapping the open/usage in a try/finally that calls store.close() (or the appropriate close/dispose method) so that if _fernet() raises the handle is not leaked; refer to the set_secret function and symbols _get_store, _fernet, and store.close (or the existing close pattern) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cuga/backend/llm/models.py`:
- Around line 812-818: The preflight key resolution for platforms in the branch
that sets apikey_ref/resolved_key incorrectly always falls back to
"OPENAI_API_KEY"; update the logic in the block that checks use_env and platform
(where apikey_ref, resolved_key, _normalize_secret and resolve_secret are used)
so that when platform == "openrouter" the fallback secret/env var is
"OPENROUTER_API_KEY" (keep the existing resolve_secret/_normalize_secret flow),
otherwise retain the existing OPENAI_API_KEY fallback; ensure apikey_ref
selection still prefers api_key or llm_cfg["apikey_name"] before falling back to
the platform-specific default.
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 106-109: The current logic only sets os.environ["MODEL_NAME"] and
os.environ["MODEL_TEMPERATURE"] when llm_cfg provides values, but does not clear
previously set env vars when a later config omits those keys; update the
handling around llm_cfg in manage_routes.py so that for the "model" and
"temperature" keys you explicitly remove/clear the corresponding environment
variables (MODEL_NAME, MODEL_TEMPERATURE) when the key is missing or its value
is None/empty (use os.environ.pop("MODEL_NAME", None) /
os.environ.pop("MODEL_TEMPERATURE", None) or del if present), and apply the same
change to the other occurrence referenced (lines 133-138) to ensure old
overrides are cleared when configs remove those fields.
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx`:
- Around line 121-130: The mapping that builds the secrets list in AddToolModal
(where raw is derived and setSecretsList is called) treats any non-vault/non-env
source as `db://`, which incorrectly handles AWS secrets; update the mapping
logic inside the setSecretsList call to explicitly handle s.source === "aws"
(and/or mode === "aws" if appropriate) and produce the correct AWS ref (e.g., an
aws://... style ref) while preserving the existing vault (`vault://secret/...`)
and env (plain id) branches so AWS-backed secrets resolve correctly instead of
becoming db:// entries.
- Around line 475-482: After inline creation the code directly sets the raw API
objects into state (in the api.getSecrets(...) callback) but the dropdown
expects each secret to include a ref field, so normalize the returned secrets
before calling setSecretsList: in the api.getSecrets(agentId).then(...) handler
map (d.secrets || d.overrides || []) and produce a normalized array where each
item has a ref property (e.g., set ref = item.ref || item.key || item.name ||
`${item.id}`) and preserve other fields, then pass that normalized array to
setSecretsList and close the inline create UI; update references to secretsList
selection code if it assumes other field names.
- Around line 203-209: The secret creation call isn't scoping the secret to an
agent: update the create flow so api.createSecret is called with the current
agentId (pass the agent identifier used by this modal) to ensure secrets are
saved under the correct agent. Locate the block building baseSlug/slug
(variables saveAsNewSecretKey, name, authType, slug) and change the
api.createSecret(slug, authValueFinal) invocation to include the agentId
parameter (e.g., api.createSecret(agentId, slug, authValueFinal) or the API's
expected signature), ensuring any caller/context values for agentId used
elsewhere in this component are passed through.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Around line 903-906: The createSecret call in the inline LLM secret flow
should be agent-scoped and use the backend-returned ref instead of hardcoding
db://; update the api.createSecret invocation to include the effectiveAgentId
(pass the effectiveAgentId variable/prop) and, after the await, read the
returned ref from the response payload (e.g., res.body.ref or res.ref depending
on the API shape) and use that value when setting the selected ref before
calling setLlmInlineCreate(false); keep the existing variables slug and
llmInlineCreateValue.trim() but replace the hardcoded `db://${slug}` with the
ref returned by the backend and ensure you only proceed when res.ok is true.
---
Duplicate comments:
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 226-236: The code currently reads Authorization and may forward an
app auth token as the provider API key; change the logic in the route handler
that sets auth_header/api_key so it ignores request.headers.get("Authorization")
entirely and only checks a dedicated header (e.g., "X-LLM-API-Key") and then
falls back to server-side secret resolution via _PROVIDER_API_KEY_REF and
resolve_secret; update the code paths referencing auth_header, api_key, and
request.headers so Authorization is never used as a provider credential.
In `@src/cuga/backend/storage/secrets_store.py`:
- Around line 201-204: The code in set_secret opens the relational store via
_get_store() before validating the encryption key from _fernet(), which can
raise RuntimeError and leak the store handle; either move the _fernet() call and
its missing-key validation to happen before calling _get_store(), or ensure the
store is closed on all paths by wrapping the open/usage in a try/finally that
calls store.close() (or the appropriate close/dispose method) so that if
_fernet() raises the handle is not leaked; refer to the set_secret function and
symbols _get_store, _fernet, and store.close (or the existing close pattern)
when making the change.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Around line 417-422: The ref construction in ManagePage.tsx wrongly maps
AWS-backed secrets to the db:// branch; update the ternary that builds ref for
each secret (the expression using s.source and mode) to handle s.source ===
"aws" (and when mode === "vault" with s.source === "aws" if relevant) by
returning an aws://... reference (e.g., aws://secret/${s.id}#value) instead of
falling through to db:// so AWS secrets resolve correctly; adjust the branches
where ref is computed (the ternary checking s.source === "vault" || mode ===
"vault", s.source === "env", else ...) to include an s.source === "aws" case
that produces the aws:// scheme.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/frontend_workspaces/frontend/dist/index.htmlis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.faea0157f0dd4261ffdc.jsis excluded by!**/dist/**
📒 Files selected for processing (8)
.secrets.baselinesrc/cuga/backend/cuga_graph/graph.pysrc/cuga/backend/llm/models.pysrc/cuga/backend/server/manage_routes.pysrc/cuga/backend/storage/secrets_store.pysrc/frontend_workspaces/frontend/src/AddToolModal.tsxsrc/frontend_workspaces/frontend/src/ManagePage.tsxsrc/frontend_workspaces/frontend/src/ToolsConfig.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cuga/backend/server/main.py`:
- Line 154: DraftAppState is missing the current_llm attribute which AppState
defines; add a typed attribute initialization for current_llm in
DraftAppState.__init__ (e.g., self.current_llm: Optional[Any] = None) so the
field exists statically instead of relying on dynamic creation by
_apply_published_config and the getattr(draft_app_state, "current_llm", None)
usage.
offerakrabi
approved these changes
Mar 3, 2026
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
src/cuga/backend/llm/models.py (1)
822-828:⚠️ Potential issue | 🟠 MajorProvider preflight fallback uses the wrong default key reference.
At Lines 826-828, fallback always checks
OPENAI_API_KEY. Foropenrouterandazure, this should be provider-specific (OPENROUTER_API_KEYandAZURE_OPENAI_API_KEY).🔧 Proposed fix
if not use_env and platform in ("openai", "azure", "openrouter"): + provider_default_ref = ( + "OPENROUTER_API_KEY" if platform == "openrouter" + else "AZURE_OPENAI_API_KEY" if platform == "azure" + else "OPENAI_API_KEY" + ) apikey_ref = api_key or llm_cfg.get("apikey_name") resolved_key = _normalize_secret(resolve_secret(apikey_ref)) if apikey_ref else None if not resolved_key: - resolved_key = _normalize_secret(resolve_secret("OPENAI_API_KEY")) or os.environ.get( - "OPENAI_API_KEY" + resolved_key = _normalize_secret(resolve_secret(provider_default_ref)) or os.environ.get( + provider_default_ref )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/llm/models.py` around lines 822 - 828, The fallback always looks up "OPENAI_API_KEY" regardless of platform; update the resolution logic in the code block that computes apikey_ref/resolved_key (variables and helpers: use_env, platform, apikey_ref, resolved_key, _normalize_secret, resolve_secret) to pick a provider-specific env var when apikey_ref is missing — e.g., map platform "azure" -> "AZURE_OPENAI_API_KEY", "openrouter" -> "OPENROUTER_API_KEY", otherwise default to "OPENAI_API_KEY" — then attempt resolve_secret on that chosen name and fall back to os.environ.get(...) of the same provider-specific variable.src/frontend_workspaces/frontend/src/ManagePage.tsx (2)
909-912:⚠️ Potential issue | 🟠 MajorUse agent-scoped secret creation and persist the backend-returned ref.
This flow still creates secrets without agent scope and always selects
db://..., which is incorrect for non-DB backends.🔧 Proposed fix
- const res = await api.createSecret(slug, llmInlineCreateValue.trim(), "LLM API Key"); + const res = await api.createSecret( + slug, + llmInlineCreateValue.trim(), + "LLM API Key", + undefined, + effectiveAgentId + ); if (res.ok) { - const ref = `db://${slug}`; + const data = await res.json().catch(() => ({})); + const ref = data.ref || `db://${slug}`; setLlmInlineCreate(false); setLlmInlineCreateValue(""); setLlmInlineCreateKey("");Also applies to: 918-919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 909 - 912, The code is hardcoding a db:// ref and not using agent-scoped creation; change the call flow around api.createSecret so you pass the agent scope (or agent identifier) when creating the secret and then read and persist the backend-returned secret reference from the response instead of constructing `db://${slug}`; specifically update the logic around api.createSecret (the res variable), setLlmInlineCreate, and the ref assignment to use the returned ref field from res (e.g., res.ref or res.data.ref as provided by the API) and ensure the create call includes the agent scope flag/param required for agent-scoped secrets.
423-427:⚠️ Potential issue | 🟠 MajorNormalize secret reference handling across source mapping and UI state.
awssecrets are still falling back todb://..., andenvsecrets are stored as raw IDs while saved-secret detection expects URI-like refs. This causes incorrect resolver refs and inconsistent UI mode toggling.🔧 Proposed fix
setLlmSecretsList(raw.map((s) => ({ id: s.id, description: s.description, ref: s.source === "vault" || mode === "vault" ? `vault://secret/${s.id}#value` + : s.source === "aws" + ? `aws://${s.id}` : s.source === "env" - ? s.id + ? `env://${s.id}` : `db://${s.id}`, }))); @@ - setLlmUseSavedSecret( - typeof key === "string" && (key.startsWith("db://") || key.startsWith("vault://") || key.startsWith("aws://")) - ); + setLlmUseSavedSecret(typeof key === "string" && isSecretRef(key)); @@ - value={(llm.api_key ?? "").startsWith("db://") ? "" : (llm.api_key ?? "")} + value={isSecretRef(llm.api_key ?? "") ? "" : (llm.api_key ?? "")}Also applies to: 437-441, 934-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 423 - 427, The secret-ref branch logic should normalize all source types into URI-like refs so saved-secret detection and UI toggles work consistently: change the ternary that builds ref (using s.source, mode, and s.id) to treat "aws" the same as "vault" (produce vault://secret/{s.id}#value), produce env://{s.id} for env secrets (instead of raw s.id), and default to db://{s.id} for others; update the identical mapping sites that use the same logic (the other occurrences around the code referenced in the comment) so all places (the ref builder using s.source/mode and any saved-secret detection inputs) use the same normalized URI scheme.src/cuga/backend/server/manage_routes.py (2)
105-109:⚠️ Potential issue | 🟠 MajorClear MODEL overrides when values are removed from config.
The code sets
MODEL_NAME/MODEL_TEMPERATUREwhen present but never clears them when omitted, so old overrides persist across saves.🔧 Proposed fix
if force_env: if "model" in llm_cfg and llm_cfg["model"]: os.environ["MODEL_NAME"] = str(llm_cfg["model"]) + else: + os.environ.pop("MODEL_NAME", None) if "temperature" in llm_cfg and llm_cfg["temperature"] is not None: os.environ["MODEL_TEMPERATURE"] = str(llm_cfg["temperature"]) + else: + os.environ.pop("MODEL_TEMPERATURE", None) @@ if llm_cfg.get("model"): os.environ["MODEL_NAME"] = str(llm_cfg["model"]) + else: + os.environ.pop("MODEL_NAME", None)Also applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/manage_routes.py` around lines 105 - 109, When force_env handling of llm_cfg sets MODEL_NAME / MODEL_TEMPERATURE but never clears them, old values persist; update the block that uses force_env and llm_cfg (same pattern at the second occurrence around lines 133-135) to explicitly remove the environment overrides when the config omits or clears values: if "model" is missing or falsy, remove os.environ["MODEL_NAME"] (use os.environ.pop or del if present), and if "temperature" is missing or None, remove os.environ["MODEL_TEMPERATURE"]; keep the existing behavior of setting the vars when values are present.
291-303:⚠️ Potential issue | 🔴 CriticalValidate LiteLLM model-list target URL before server-side fetch.
For
litellm, the outboundurlis built from saved config and fetched without host/scheme validation, which keeps an SSRF path open.🔒 Proposed hardening
+ from urllib.parse import urlparse + import ipaddress @@ if provider_key == "litellm": @@ if base_url.endswith('/v1'): url = f"{base_url}/models" else: url = f"{base_url}/v1/models" + + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + raise HTTPException(status_code=400, detail="Invalid LiteLLM URL scheme") + host = parsed.hostname or "" + try: + ip = ipaddress.ip_address(host) + if ip.is_private or ip.is_loopback or ip.is_link_local: + raise HTTPException(status_code=400, detail="LiteLLM host is not allowed") + except ValueError: + if host in ("localhost",): + raise HTTPException(status_code=400, detail="LiteLLM host is not allowed")Also applies to: 371-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/manage_routes.py` around lines 291 - 303, The code builds an outbound LiteLLM model-list URL from provider_key/base_url without validating host or scheme, enabling SSRF; update the logic in the manage_routes handler where provider_key == "litellm" (and the similar block at the other occurrence) to parse base_url (e.g., via urllib.parse.urlparse), ensure a safe scheme (http or https), normalize the netloc, and reject or raise HTTPException(400) for missing scheme/host, for local/loopback/private IPs, and for hostnames that resolve to internal addresses; only then construct url (with /v1/models) and proceed with the server-side fetch.
🧹 Nitpick comments (1)
src/cuga/backend/secrets/seed.py (1)
118-124:seed_secrets_from_env_sync()uses fire-and-forgetcreate_task()but is not used during startup.The function at line 122 uses
loop.create_task()which is fire-and-forget and would cause nondeterministic seeding if called during startup. However, the actual startup path inmain.py(lifespan context manager, lines 246) usesawait seed_secrets_from_env()directly, which properly awaits completion before continuing.If this function is being used elsewhere in synchronous contexts where startup ordering matters, the fire-and-forget behavior is a concern. Otherwise, consider documenting the expected use cases or removing the function if unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/secrets/seed.py` around lines 118 - 124, seed_secrets_from_env_sync currently does a fire-and-forget loop.create_task(seed_secrets_from_env()) which yields nondeterministic behavior; make it deterministic by rejecting use from an active event loop and forcing callers to await the coroutine: change the try branch to raise a RuntimeError (or return an explicit error) instructing callers to await seed_secrets_from_env() when an event loop is running, and keep the except branch using asyncio.run(seed_secrets_from_env()) to run to completion in synchronous code; reference the functions seed_secrets_from_env_sync and seed_secrets_from_env so callers know to switch to the awaited coroutine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cuga/backend/llm/config.py`:
- Line 71: The LiteLLMConfig.url field override removed the inherited alias for
base_url causing instantiation with base_url to fail; update the
LiteLLMConfig.url Field definition to re-declare the alias by using Field(...,
alias="base_url", description="LiteLLM proxy/gateway URL (required)") so
Pydantic will accept base_url when constructing LiteLLMConfig (refer to the
LiteLLMConfig.url field in config.py).
In `@src/cuga/backend/secrets/seed.py`:
- Around line 130-146: The provider hint lookup in provider_hints wrongly
prioritizes generic "gpt" over "azure", causing azure model names (e.g.,
azure-gpt-4o) to resolve to the openai key; in the block that iterates
provider_hints (the provider_hints dict and the for hint, slug in
provider_hints.items() loop in seed.py), ensure more specific providers like
"azure" are checked before generic ones like "gpt" — either reorder
provider_hints so "azure" appears before "gpt" or implement
longest-prefix/most-specific matching for model_name (e.g., sort hints by length
descending before matching) so "azure" maps to "db://azure-openai-api-key"
correctly.
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 535-537: The call to await
_apply_published_config(state_to_update, config or {}) must be guarded so it
only runs when there is a valid draft_app_state; update the code around where
state_to_update/draft_app_state is set (referencing state_to_update and
_apply_published_config) to check if state_to_update is not None before calling
_apply_published_config (e.g., if state_to_update: await
_apply_published_config(state_to_update, config or {})), ensuring no attempt to
apply runtime config when draft_app_state is unavailable.
In `@src/cuga/backend/server/secrets_routes.py`:
- Around line 102-103: The handlers in secrets_routes.py currently raise
HTTPException with detail=str(e), which leaks internal errors to clients;
instead, catch the exception, log the full exception server-side (e.g.,
logger.exception or logger.error with trace) and re-raise a generic client-safe
HTTPException like HTTPException(status_code=500, detail="Internal server
error"); replace all occurrences of HTTPException(status_code=500,
detail=str(e)) (and similar detail=str(e) on other status codes) with this
pattern and ensure you retain server-side logging before raising.
- Around line 159-167: create_secret currently calls secrets_store.set_secret
with body.id and created_by=_user_id(current_user) allowing overwrite of secrets
owned by others; fix by checking existing secret ownership before writing: call
the store's lookup (e.g., secrets_store.get_secret or equivalent) for body.id,
and if a secret exists and its created_by does not match _user_id(current_user)
then abort with a 403/HTTPException; only call secrets_store.set_secret to
create or update when the secret is either absent or owned by the current user
(or implement an explicit upsert path that enforces ownership).
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx`:
- Around line 475-478: The code is currently forcing a db://... ref after
calling api.createSecret which discards backend-specific refs; change the
success branch for api.createSecret (the call in createSecret(..., agentId)) to
read the ref returned by the API response (e.g., res.ref or res.data?.ref) and
pass that into setAuthValue instead of synthesizing `db://${slug}`; fall back to
`db://${slug}` only if the response does not contain a ref. Keep
setInlineCreateOpen(false) and existing trimming logic.
- Around line 309-310: Select components in AddToolModal.tsx use defaultValue
(e.g., the Select bound to type with onChange calling setType) which makes them
uncontrolled and causes UI desync; change those Selects to controlled components
by replacing defaultValue with value (use the corresponding state variables like
type, protocol, or authMode) while preserving their onChange handlers (e.g.,
setType, setProtocol, setAuthMode) so programmatic state updates (applyTemplate,
auth mode changes) immediately reflect in the dropdowns; apply this replacement
for the Selects referenced around the setType handler and the other Selects at
the indicated locations.
---
Duplicate comments:
In `@src/cuga/backend/llm/models.py`:
- Around line 822-828: The fallback always looks up "OPENAI_API_KEY" regardless
of platform; update the resolution logic in the code block that computes
apikey_ref/resolved_key (variables and helpers: use_env, platform, apikey_ref,
resolved_key, _normalize_secret, resolve_secret) to pick a provider-specific env
var when apikey_ref is missing — e.g., map platform "azure" ->
"AZURE_OPENAI_API_KEY", "openrouter" -> "OPENROUTER_API_KEY", otherwise default
to "OPENAI_API_KEY" — then attempt resolve_secret on that chosen name and fall
back to os.environ.get(...) of the same provider-specific variable.
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 105-109: When force_env handling of llm_cfg sets MODEL_NAME /
MODEL_TEMPERATURE but never clears them, old values persist; update the block
that uses force_env and llm_cfg (same pattern at the second occurrence around
lines 133-135) to explicitly remove the environment overrides when the config
omits or clears values: if "model" is missing or falsy, remove
os.environ["MODEL_NAME"] (use os.environ.pop or del if present), and if
"temperature" is missing or None, remove os.environ["MODEL_TEMPERATURE"]; keep
the existing behavior of setting the vars when values are present.
- Around line 291-303: The code builds an outbound LiteLLM model-list URL from
provider_key/base_url without validating host or scheme, enabling SSRF; update
the logic in the manage_routes handler where provider_key == "litellm" (and the
similar block at the other occurrence) to parse base_url (e.g., via
urllib.parse.urlparse), ensure a safe scheme (http or https), normalize the
netloc, and reject or raise HTTPException(400) for missing scheme/host, for
local/loopback/private IPs, and for hostnames that resolve to internal
addresses; only then construct url (with /v1/models) and proceed with the
server-side fetch.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Around line 909-912: The code is hardcoding a db:// ref and not using
agent-scoped creation; change the call flow around api.createSecret so you pass
the agent scope (or agent identifier) when creating the secret and then read and
persist the backend-returned secret reference from the response instead of
constructing `db://${slug}`; specifically update the logic around
api.createSecret (the res variable), setLlmInlineCreate, and the ref assignment
to use the returned ref field from res (e.g., res.ref or res.data.ref as
provided by the API) and ensure the create call includes the agent scope
flag/param required for agent-scoped secrets.
- Around line 423-427: The secret-ref branch logic should normalize all source
types into URI-like refs so saved-secret detection and UI toggles work
consistently: change the ternary that builds ref (using s.source, mode, and
s.id) to treat "aws" the same as "vault" (produce vault://secret/{s.id}#value),
produce env://{s.id} for env secrets (instead of raw s.id), and default to
db://{s.id} for others; update the identical mapping sites that use the same
logic (the other occurrences around the code referenced in the comment) so all
places (the ref builder using s.source/mode and any saved-secret detection
inputs) use the same normalized URI scheme.
---
Nitpick comments:
In `@src/cuga/backend/secrets/seed.py`:
- Around line 118-124: seed_secrets_from_env_sync currently does a
fire-and-forget loop.create_task(seed_secrets_from_env()) which yields
nondeterministic behavior; make it deterministic by rejecting use from an active
event loop and forcing callers to await the coroutine: change the try branch to
raise a RuntimeError (or return an explicit error) instructing callers to await
seed_secrets_from_env() when an event loop is running, and keep the except
branch using asyncio.run(seed_secrets_from_env()) to run to completion in
synchronous code; reference the functions seed_secrets_from_env_sync and
seed_secrets_from_env so callers know to switch to the awaited coroutine.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/frontend_workspaces/frontend/dist/index.htmlis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.e34d461e2994be4a0196.jsis excluded by!**/dist/**
📒 Files selected for processing (11)
.secrets.baselinesrc/cuga/backend/llm/config.pysrc/cuga/backend/llm/models.pysrc/cuga/backend/secrets/seed.pysrc/cuga/backend/server/manage_routes.pysrc/cuga/backend/server/secrets_routes.pysrc/frontend_workspaces/frontend/src/AddToolModal.csssrc/frontend_workspaces/frontend/src/AddToolModal.tsxsrc/frontend_workspaces/frontend/src/ManagePage.tsxsrc/frontend_workspaces/frontend/src/ToolsConfig.csssrc/frontend_workspaces/frontend/src/ToolsConfig.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend_workspaces/frontend/src/ToolsConfig.css
Comment on lines
+475
to
+478
| const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId); | ||
| if (res.ok) { | ||
| setAuthValue(`db://${slug}`); | ||
| setInlineCreateOpen(false); |
There was a problem hiding this comment.
Inline secret creation should use the backend-returned ref.
At Line 477, forcing db://... discards source-specific refs returned by the API (e.g., vault), which can break secret resolution in non-local backends.
🔧 Proposed fix
const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId);
if (res.ok) {
- setAuthValue(`db://${slug}`);
+ const data = await res.json();
+ setAuthValue(data.ref || `db://${slug}`);
setInlineCreateOpen(false);
setInlineCreateKey("");
api.getSecrets(agentId).then((r) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId); | |
| if (res.ok) { | |
| setAuthValue(`db://${slug}`); | |
| setInlineCreateOpen(false); | |
| const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId); | |
| if (res.ok) { | |
| const data = await res.json(); | |
| setAuthValue(data.ref || `db://${slug}`); | |
| setInlineCreateOpen(false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 475 -
478, The code is currently forcing a db://... ref after calling api.createSecret
which discards backend-specific refs; change the success branch for
api.createSecret (the call in createSecret(..., agentId)) to read the ref
returned by the API response (e.g., res.ref or res.data?.ref) and pass that into
setAuthValue instead of synthesizing `db://${slug}`; fall back to `db://${slug}`
only if the response does not contain a ref. Keep setInlineCreateOpen(false) and
existing trimming logic.
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (3)
src/frontend_workspaces/frontend/src/AddToolModal.tsx (2)
475-478:⚠️ Potential issue | 🟠 MajorInline secret creation should use the backend-returned
ref.Line 477 forces
db://..., discarding source-specific refs returned by/api/secrets(e.g., vault/aws).Suggested fix
const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId); if (res.ok) { - setAuthValue(`db://${slug}`); + const created = await res.json(); + setAuthValue(created.ref || `db://${slug}`); setInlineCreateOpen(false); setInlineCreateKey("");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 475 - 478, The code currently constructs a db://... ref manually after calling api.createSecret; instead use the secret reference returned by the backend response instead of forcing `db://${slug}`. Update the block around api.createSecret(...) to extract the returned ref from the response (e.g., res.ref or res.data.ref depending on the API shape), pass that value to setAuthValue(...), and still call setInlineCreateOpen(false) on success; optionally fall back to `db://${slug}` only if the returned ref is missing.
146-151:⚠️ Potential issue | 🟠 MajorAWS secrets are still normalized to
db://refs.Lines 146-151 and 493-497 do not handle
s.source === "aws", so AWS-backed entries get incorrect refs and fail resolution.Suggested fix
ref: s.source === "vault" || mode === "vault" ? `vault://secret/${s.id}#value` + : s.source === "aws" + ? `aws://${s.id}` : s.source === "env" ? s.id : `db://${s.id}`,Also applies to: 493-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 146 - 151, The ref normalization in AddToolModal.tsx misses the AWS case so s.source === "aws" falls through to the db:// branch; update the ref conditional (the object building s.ref) to add an explicit s.source === "aws" branch (similar to the vault branch) and produce the correct AWS-style ref (e.g. `aws://secret/${s.id}#value` or your project's AWS URI format), and apply the same change to the second occurrence of the same conditional later in the file (the block around the other ref generation at lines ~493-497) so AWS-backed entries resolve correctly.src/cuga/backend/server/manage_routes.py (1)
385-397:⚠️ Potential issue | 🔴 Critical
/llm/modelsstill allows server-side fetch to a config-controlled LiteLLM URL (SSRF risk).Lines 385-397 compose
urldirectly from draftllm.urlforlitellm, then Line 466 performs the request. That keeps an SSRF path open through editable config.Suggested hardening
+ from urllib.parse import urlparse + import ipaddress @@ if provider_key == "litellm": if not base_url: raise HTTPException( status_code=400, detail="LiteLLM requires url/base_url in config", ) base_url = base_url.rstrip('/') + parsed = urlparse(base_url) + if parsed.scheme != "https": + raise HTTPException(status_code=400, detail="Only https LiteLLM URLs are allowed") + host = parsed.hostname or "" + if host in {"localhost", "127.0.0.1", "::1"}: + raise HTTPException(status_code=400, detail="Loopback hosts are not allowed") + try: + ip = ipaddress.ip_address(host) + if ip.is_private or ip.is_loopback or ip.is_link_local: + raise HTTPException(status_code=400, detail="Private/link-local hosts are not allowed") + except ValueError: + pass if base_url.endswith('/v1'): url = f"{base_url}/models" else: url = f"{base_url}/v1/models"Also applies to: 465-467
🧹 Nitpick comments (2)
tests/system/test_manager_api_integration.py (1)
387-390: Prefer readiness polling over fixed 15s sleep.A fixed delay increases suite time and still leaves flake risk under slower/faster environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system/test_manager_api_integration.py` around lines 387 - 390, Replace the fixed 15s delay (the logger.info(...) + time.sleep(15) block) with a readiness-polling loop that repeatedly checks the actual readiness conditions (e.g., the agent graph rebuild visibility via the manager API or the MCP subprocess/servers responding on their health or API endpoints) until success or a configurable timeout; implement this in tests/system/test_manager_api_integration.py as a small helper (e.g., wait_for_readiness) that polls at short intervals (with a max timeout and backoff), logs progress via logger, and raises on timeout instead of sleeping. Ensure the polling checks the same signals used elsewhere (agent/manager endpoints or subprocess health checks) so the test no longer relies on a fixed sleep.src/frontend_workspaces/frontend/src/ManagePage.tsx (1)
449-452: ReuseisSecretReffor saved-secret detection to avoid prefix drift.This logic currently duplicates protocol checks and omits
env://, whileisSecretRefalready centralizes that behavior.Suggested refactor
useEffect(() => { const key = llmConfig?.api_key ?? ""; - setLlmUseSavedSecret( - typeof key === "string" && (key.startsWith("db://") || key.startsWith("vault://") || key.startsWith("aws://")) - ); + setLlmUseSavedSecret(isSecretRef(key)); }, [llmConfig?.api_key]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 449 - 452, The saved-secret detection in ManagePage.tsx duplicates protocol checks and misses env://; replace the manual startsWith checks on the llmConfig api_key with a call to the centralized helper isSecretRef to determine saved-secret usage. In the block where key is derived from llmConfig?.api_key and passed to setLlmUseSavedSecret, keep the typeof key === "string" guard and call setLlmUseSavedSecret(isSecretRef(key)) instead of the explicit startsWith checks so detection stays consistent with the isSecretRef implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cuga/backend/llm/models.py`:
- Around line 822-829: The preflight fallback picks the wrong env var for Azure;
inside the block that computes apikey_ref/resolved_key (referencing use_env,
platform, apikey_ref, resolved_key, fallback_key, _normalize_secret and
resolve_secret) change the fallback selection so that when platform == "azure"
you set fallback_key = "AZURE_OPENAI_API_KEY", when platform == "openrouter" use
"OPENROUTER_API_KEY", otherwise use "OPENAI_API_KEY"; keep the subsequent
resolution logic the same so resolved_key tries resolve_secret(fallback_key)
then os.environ.get(fallback_key).
In `@src/cuga/backend/secrets/seed.py`:
- Around line 43-54: Dynamic env scanning in _build_dynamic_seed_map currently
seeds any non-empty env var matching _DYNAMIC_PATTERN
(KEY/SECRET/TOKEN/PASSWORD), which is too broad; restrict it by adding allowlist
and denylist checks: update _DYNAMIC_PATTERN usage in _build_dynamic_seed_map
(and the similar scanner function around the second occurrence) to first reject
well-known infra/CI/cloud prefixes (extend _SKIP_PREFIXES or add a new
_INFRA_PREFIXES set) and require either a service-specific prefix (e.g.,
startswith one of allowed application prefixes) or match a stricter pattern
(e.g., only variables ending with _KEY/_TOKEN/_SECRET or matching
^APP_[A-Z0-9_]+_(KEY|TOKEN|SECRET|PASSWORD)$); keep existing exclusions for
_STATIC_ENV_SEED_MAP and empty values, and when accepted continue to map via
_env_var_to_slug. Ensure tests/assertions are added or updated to cover rejected
infra vars and accepted app-specific vars.
- Around line 121-123: The background task created in the running-loop branch
loses its reference (loop.create_task(seed_secrets_from_env())), so retain the
Task handle by storing it in a module-level set (e.g., _background_tasks) and
add a done callback that removes the task from the set and logs or re-raises
unhandled exceptions; update the except RuntimeError branch to add the created
Task to _background_tasks and attach the cleanup callback so the
seed_secrets_from_env task lifecycle and errors are tracked.
In `@src/cuga/backend/server/main.py`:
- Around line 521-527: The startup code is invoking the published-config applier
_apply_published_config(draft_app_state, draft_config) which mutates
process-wide environment and can leak draft LLM settings (especially when
force_env is on); replace this call with a non-mutating draft applier (e.g.,
implement or call _apply_draft_config) that applies draft_config into
draft_app_state without touching os.environ or other global state, or run the
existing applier inside a sandbox/context manager that snapshots and restores
environment variables and global runtime state around the call; ensure
references to draft_config and draft_app_state are preserved and do not call
functions in manage_routes that set process-wide env vars.
In `@src/cuga/backend/server/manage_routes.py`:
- Around line 313-316: The endpoint function that has the signature including
request: Request, disable_ssl: bool = Query(False, alias="disable_ssl"),
agent_id: Optional[str] = None should also read the advertised X-LLM-API-Key
header when the API key is not provided via query/body; update the handler to
check request.headers.get("X-LLM-API-Key") and use that value as the API key (or
merge/override according to existing precedence logic) before performing the
existing validation that currently returns 400, ensuring clients sending the key
in the header don’t get rejected.
In `@src/cuga/backend/server/secrets_routes.py`:
- Around line 163-175: The update/delete handlers in secrets_routes.py drop the
secret scope (agent_id/version) when calling secrets_store methods, which can
cause operations to target the wrong secret when IDs are duplicated across
scopes; update all calls to secrets_store.set_secret,
secrets_store.delete_secret (and any other persistence/deletion calls in the
same handlers) to pass through the incoming body.agent_id and body.version (or
the resolved scope from get_secret_metadata) instead of defaulting to "*" or
omitting them, and ensure get_secret_metadata, the update route that calls
set_secret, and the delete route consistently compute and supply the same scoped
identifiers so updates/deletes operate on the exact secret expected.
- Around line 153-161: The vault branches currently bypass the creator-ownership
checks used in local mode; update the handlers that call _vault_write,
_vault_read and _vault_delete so they perform the same authorization check
(verify current_user.id or equivalent creator field) before returning early for
vault operations, and raise HTTPException(403) when the authenticated user is
not the creator; apply this check in the create/update (where _vault_write is
used), read (where _vault_read is used) and delete (where _vault_delete is used)
code paths so vault operations enforce the same ownership restrictions as local
mode.
In `@src/frontend_workspaces/agentic_chat/src/PoliciesConfig.tsx`:
- Around line 380-387: The code posts a merged config even when the pre-load
fetch (api.getManageConfig) fails, risking overwriting unrelated persisted
sections; change the logic in the component around loadResponse, loadData,
existingConfig and fullConfig so that if loadResponse.ok is false you abort the
save (do not call api.postManageConfig) and surface an error to the user/console
instead; specifically, check loadResponse.ok before constructing
fullConfig/normalizedConfig and call postManageConfig only when the
existingConfig has been successfully loaded, otherwise return early or throw to
avoid destructive partial writes.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Line 497: The forEach callback on errs (errs.forEach(...)) currently uses a
concise arrow expression that returns a value; change it to a block-bodied arrow
function so it does not implicitly return an expression—e.g. replace the
single-expression callback with (e: unknown) => { addToast("warning", "Policy
error", typeof e === "string" ? e : String(e)); }—and make the same change for
the other occurrence at the later location (line referenced in the comment).
- Around line 1067-1069: The current onChange uses Number(value) || 0.1 which
treats 0 as falsy and prevents setting temperature to 0; change it to parse the
numeric value and only fall back to 0.1 when the parsed result is NaN (e.g.
const v = Number(value); updateLlmTemperature(isNaN(v) ? 0.1 : v)); update the
onChange handler that calls updateLlmTemperature so it preserves valid 0 values
while still defaulting on invalid input.
- Around line 938-953: The inline secret-creation click handler (using
llmInlineCreateKey, llmInlineCreateValue and calling api.createSecret) only
processes res.ok and silently ignores failures or JSON parse errors; modify the
onClick handler to explicitly handle non-OK responses and JSON parse exceptions
by reading and logging/returning the error body (e.g., await res.text() or
try/catch around res.json()), surfacing a user-visible error state (e.g., set an
error message state or show a toast) when creation fails, and only on success
proceed with refreshSecrets(), updateLlm("api_key", ref) and saveLlmDraft;
ensure the handler cleans up UI state (setLlmInlineCreate(false), reset inputs)
only after a confirmed success and preserve inputs when there is an error so the
user can retry.
---
Duplicate comments:
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx`:
- Around line 475-478: The code currently constructs a db://... ref manually
after calling api.createSecret; instead use the secret reference returned by the
backend response instead of forcing `db://${slug}`. Update the block around
api.createSecret(...) to extract the returned ref from the response (e.g.,
res.ref or res.data.ref depending on the API shape), pass that value to
setAuthValue(...), and still call setInlineCreateOpen(false) on success;
optionally fall back to `db://${slug}` only if the returned ref is missing.
- Around line 146-151: The ref normalization in AddToolModal.tsx misses the AWS
case so s.source === "aws" falls through to the db:// branch; update the ref
conditional (the object building s.ref) to add an explicit s.source === "aws"
branch (similar to the vault branch) and produce the correct AWS-style ref (e.g.
`aws://secret/${s.id}#value` or your project's AWS URI format), and apply the
same change to the second occurrence of the same conditional later in the file
(the block around the other ref generation at lines ~493-497) so AWS-backed
entries resolve correctly.
---
Nitpick comments:
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx`:
- Around line 449-452: The saved-secret detection in ManagePage.tsx duplicates
protocol checks and misses env://; replace the manual startsWith checks on the
llmConfig api_key with a call to the centralized helper isSecretRef to determine
saved-secret usage. In the block where key is derived from llmConfig?.api_key
and passed to setLlmUseSavedSecret, keep the typeof key === "string" guard and
call setLlmUseSavedSecret(isSecretRef(key)) instead of the explicit startsWith
checks so detection stays consistent with the isSecretRef implementation.
In `@tests/system/test_manager_api_integration.py`:
- Around line 387-390: Replace the fixed 15s delay (the logger.info(...) +
time.sleep(15) block) with a readiness-polling loop that repeatedly checks the
actual readiness conditions (e.g., the agent graph rebuild visibility via the
manager API or the MCP subprocess/servers responding on their health or API
endpoints) until success or a configurable timeout; implement this in
tests/system/test_manager_api_integration.py as a small helper (e.g.,
wait_for_readiness) that polls at short intervals (with a max timeout and
backoff), logs progress via logger, and raises on timeout instead of sleeping.
Ensure the polling checks the same signals used elsewhere (agent/manager
endpoints or subprocess health checks) so the test no longer relies on a fixed
sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd2eb775-4715-4319-ae75-c7751d8f72e7
⛔ Files ignored due to path filters (2)
src/frontend_workspaces/frontend/dist/index.htmlis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.011f0159cefa443bf651.jsis excluded by!**/dist/**
📒 Files selected for processing (13)
.secrets.baselinesrc/cuga/backend/llm/config.pysrc/cuga/backend/llm/models.pysrc/cuga/backend/secrets/seed.pysrc/cuga/backend/server/main.pysrc/cuga/backend/server/manage_routes.pysrc/cuga/backend/server/secrets_routes.pysrc/frontend_workspaces/agentic_chat/src/PoliciesConfig.tsxsrc/frontend_workspaces/frontend/src/AddToolModal.tsxsrc/frontend_workspaces/frontend/src/ManagePage.tsxsrc/frontend_workspaces/frontend/src/ToolsConfig.tsxsrc/frontend_workspaces/frontend/src/api.tstests/system/test_manager_api_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cuga/backend/llm/config.py
Comment on lines
+822
to
+829
| if not use_env and platform in ("openai", "azure", "openrouter"): | ||
| apikey_ref = api_key or llm_cfg.get("apikey_name") | ||
| resolved_key = _normalize_secret(resolve_secret(apikey_ref)) if apikey_ref else None | ||
| if not resolved_key: | ||
| # Use platform-specific fallback | ||
| fallback_key = "OPENROUTER_API_KEY" if platform == "openrouter" else "OPENAI_API_KEY" | ||
| resolved_key = _normalize_secret(resolve_secret(fallback_key)) or os.environ.get(fallback_key) | ||
| if not resolved_key: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/cuga/backend/llm && cat -n models.py | sed -n '800,850p'Repository: cuga-project/cuga-agent
Length of output: 2789
🌐 Web query:
langchain_openai.AzureChatOpenAI default environment variables API key endpoint
💡 Result:
For Python langchain_openai.AzureChatOpenAI, the defaults are:
- API key:
AZURE_OPENAI_API_KEY - Endpoint:
AZURE_OPENAI_ENDPOINT(e.g.https://<your-resource-name>.openai.azure.com/) [1]
For JavaScript/TypeScript AzureChatOpenAI (from @langchain/openai), the common env vars are:
AZURE_OPENAI_API_KEYAZURE_OPENAI_API_INSTANCE_NAMEAZURE_OPENAI_API_DEPLOYMENT_NAMEAZURE_OPENAI_API_VERSION
(and optionallyAZURE_OPENAI_BASE_PATHfor nonstandard domains) [2]
Sources: [1] [2]
🏁 Script executed:
rg "AZURE_OPENAI" src/cuga/backend/llm/models.pyRepository: cuga-project/cuga-agent
Length of output: 49
Azure preflight fallback uses the wrong default environment variable.
At line 827, when platform == "azure", the code falls through to "OPENAI_API_KEY" instead of checking AZURE_OPENAI_API_KEY. This causes Azure-only deployments to fail preflight verification even when Azure credentials are properly configured, since langchain_openai's AzureChatOpenAI defaults to AZURE_OPENAI_API_KEY.
🔧 Suggested fix
- fallback_key = "OPENROUTER_API_KEY" if platform == "openrouter" else "OPENAI_API_KEY"
+ if platform == "openrouter":
+ fallback_key = "OPENROUTER_API_KEY"
+ elif platform == "azure":
+ fallback_key = "AZURE_OPENAI_API_KEY"
+ else:
+ fallback_key = "OPENAI_API_KEY"
resolved_key = _normalize_secret(resolve_secret(fallback_key)) or os.environ.get(fallback_key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cuga/backend/llm/models.py` around lines 822 - 829, The preflight
fallback picks the wrong env var for Azure; inside the block that computes
apikey_ref/resolved_key (referencing use_env, platform, apikey_ref,
resolved_key, fallback_key, _normalize_secret and resolve_secret) change the
fallback selection so that when platform == "azure" you set fallback_key =
"AZURE_OPENAI_API_KEY", when platform == "openrouter" use "OPENROUTER_API_KEY",
otherwise use "OPENAI_API_KEY"; keep the subsequent resolution logic the same so
resolved_key tries resolve_secret(fallback_key) then
os.environ.get(fallback_key).
Comment on lines
+43
to
+54
| def _build_dynamic_seed_map() -> dict[str, str]: | ||
| extra: dict[str, str] = {} | ||
| for env_var, value in os.environ.items(): | ||
| if not value: | ||
| continue | ||
| if env_var in _STATIC_ENV_SEED_MAP: | ||
| continue | ||
| if any(env_var.startswith(p) for p in _SKIP_PREFIXES): | ||
| continue | ||
| if _DYNAMIC_PATTERN.search(env_var): | ||
| extra[env_var] = _env_var_to_slug(env_var) | ||
| return extra |
There was a problem hiding this comment.
Dynamic env scanning is overly broad and can ingest unrelated infrastructure credentials.
The current dynamic matcher seeds any non-empty env var containing KEY/SECRET/TOKEN/PASSWORD into the DB store. That can unintentionally copy platform/CI/cloud credentials into application-managed secrets.
🔧 Suggested hardening
def _build_dynamic_seed_map() -> dict[str, str]:
extra: dict[str, str] = {}
+ allow_pattern = os.environ.get("CUGA_SEED_ALLOW_REGEX")
+ if not allow_pattern:
+ return extra
+ allow_re = re.compile(allow_pattern, re.IGNORECASE)
+
for env_var, value in os.environ.items():
if not value:
continue
if env_var in _STATIC_ENV_SEED_MAP:
continue
if any(env_var.startswith(p) for p in _SKIP_PREFIXES):
continue
- if _DYNAMIC_PATTERN.search(env_var):
+ if _DYNAMIC_PATTERN.search(env_var) and allow_re.search(env_var):
extra[env_var] = _env_var_to_slug(env_var)
return extraAlso applies to: 97-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cuga/backend/secrets/seed.py` around lines 43 - 54, Dynamic env scanning
in _build_dynamic_seed_map currently seeds any non-empty env var matching
_DYNAMIC_PATTERN (KEY/SECRET/TOKEN/PASSWORD), which is too broad; restrict it by
adding allowlist and denylist checks: update _DYNAMIC_PATTERN usage in
_build_dynamic_seed_map (and the similar scanner function around the second
occurrence) to first reject well-known infra/CI/cloud prefixes (extend
_SKIP_PREFIXES or add a new _INFRA_PREFIXES set) and require either a
service-specific prefix (e.g., startswith one of allowed application prefixes)
or match a stricter pattern (e.g., only variables ending with
_KEY/_TOKEN/_SECRET or matching ^APP_[A-Z0-9_]+_(KEY|TOKEN|SECRET|PASSWORD)$);
keep existing exclusions for _STATIC_ENV_SEED_MAP and empty values, and when
accepted continue to map via _env_var_to_slug. Ensure tests/assertions are added
or updated to cover rejected infra vars and accepted app-specific vars.
Comment on lines
+121
to
+123
| loop = asyncio.get_running_loop() | ||
| loop.create_task(seed_secrets_from_env()) | ||
| except RuntimeError: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cuga/backend/secrets/seed.py | sed -n '110,135p'Repository: cuga-project/cuga-agent
Length of output: 1147
🏁 Script executed:
rg -nP --type=py 'create_task\s*\(' src/cuga/backend/secrets/seed.pyRepository: cuga-project/cuga-agent
Length of output: 120
🏁 Script executed:
rg -nP --type=py '^\s*(?![A-Za-z_][A-Za-z0-9_]*\s*=).*?\bcreate_task\s*\(' srcRepository: cuga-project/cuga-agent
Length of output: 947
🏁 Script executed:
rg -nB5 -A5 'seed_secrets_from_env_sync' src/cuga/backend/secrets/seed.pyRepository: cuga-project/cuga-agent
Length of output: 448
🏁 Script executed:
rg -nB3 -A3 'seed_secrets_from_env_sync' --type=py srcRepository: cuga-project/cuga-agent
Length of output: 572
🏁 Script executed:
rg -nB2 -A10 'async def seed_secrets_from_env' src/cuga/backend/secrets/seed.pyRepository: cuga-project/cuga-agent
Length of output: 675
🏁 Script executed:
# Check how seed_secrets_from_env_sync is used in the codebase
rg -nB2 -A2 'seed_secrets_from_env_sync\(' src --type=pyRepository: cuga-project/cuga-agent
Length of output: 374
🏁 Script executed:
# Find where seed_secrets_from_env_sync is called/imported
rg 'seed_secrets_from_env_sync' src --type=py -B2 -A2Repository: cuga-project/cuga-agent
Length of output: 354
🏁 Script executed:
# Get the full context of the seed_secrets_from_env_sync function and what comes after
cat -n src/cuga/backend/secrets/seed.py | sed -n '1,30p'Repository: cuga-project/cuga-agent
Length of output: 1142
🏁 Script executed:
# Check the entire seed.py file structure to see if there are module-level variables
cat -n src/cuga/backend/secrets/seed.py | head -50Repository: cuga-project/cuga-agent
Length of output: 1733
🏁 Script executed:
# Check if there's a module-level _background_tasks or similar tracking
rg '_background_tasks|_tasks\s*[:=]' src/cuga/backend/secrets/seed.pyRepository: cuga-project/cuga-agent
Length of output: 49
🏁 Script executed:
# Look at how other files handle background task retention
rg -A3 'add_done_callback|_background_tasks.discard' src --type=py | head -30Repository: cuga-project/cuga-agent
Length of output: 390
Retain a strong reference for the background seed task.
The running-loop branch creates a task without keeping a reference, which allows exceptions and lifecycle changes to be silently missed. Retain the task handle in a module-level set with a done callback to ensure proper cleanup.
🔧 Suggested fix
+_background_tasks: set[asyncio.Task] = set()
+
def seed_secrets_from_env_sync() -> None:
"""Sync wrapper — runs in a new event loop if none is running."""
try:
loop = asyncio.get_running_loop()
- loop.create_task(seed_secrets_from_env())
+ task = loop.create_task(seed_secrets_from_env())
+ _background_tasks.add(task)
+ task.add_done_callback(_background_tasks.discard)
except RuntimeError:
asyncio.run(seed_secrets_from_env())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| loop = asyncio.get_running_loop() | |
| loop.create_task(seed_secrets_from_env()) | |
| except RuntimeError: | |
| _background_tasks: set[asyncio.Task] = set() | |
| def seed_secrets_from_env_sync() -> None: | |
| """Sync wrapper — runs in a new event loop if none is running.""" | |
| try: | |
| loop = asyncio.get_running_loop() | |
| task = loop.create_task(seed_secrets_from_env()) | |
| _background_tasks.add(task) | |
| task.add_done_callback(_background_tasks.discard) | |
| except RuntimeError: | |
| asyncio.run(seed_secrets_from_env()) |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 122-122: Store a reference to the return value of loop.create_task
(RUF006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cuga/backend/secrets/seed.py` around lines 121 - 123, The background task
created in the running-loop branch loses its reference
(loop.create_task(seed_secrets_from_env())), so retain the Task handle by
storing it in a module-level set (e.g., _background_tasks) and add a done
callback that removes the task from the set and logs or re-raises unhandled
exceptions; update the except RuntimeError branch to add the created Task to
_background_tasks and attach the cleanup callback so the seed_secrets_from_env
task lifecycle and errors are tracked.
Comment on lines
+521
to
+527
| if draft_config: | ||
| try: | ||
| from cuga.backend.server.manage_routes import _apply_published_config | ||
|
|
||
| await _apply_published_config(draft_app_state, draft_config) | ||
| except Exception as _e: | ||
| logger.debug("Startup: failed to apply draft LLM config: %s", _e) |
There was a problem hiding this comment.
Draft startup is using a published-config applier that mutates global runtime state.
Line 525 calls _apply_published_config(draft_app_state, draft_config). That helper mutates process-wide env vars, so draft LLM settings can bleed into published behavior (especially when force_env is enabled).
Suggested fix
- if draft_config:
- try:
- from cuga.backend.server.manage_routes import _apply_published_config
-
- await _apply_published_config(draft_app_state, draft_config)
- except Exception as _e:
- logger.debug("Startup: failed to apply draft LLM config: %s", _e)
+ if draft_config:
+ try:
+ from cuga.backend.server.manage_routes import _apply_llm_to_draft_state
+ _apply_llm_to_draft_state(draft_app_state, _draft_llm_cfg)
+ except Exception as _e:
+ logger.debug("Startup: failed to apply draft LLM config: %s", _e)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 526-526: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cuga/backend/server/main.py` around lines 521 - 527, The startup code is
invoking the published-config applier _apply_published_config(draft_app_state,
draft_config) which mutates process-wide environment and can leak draft LLM
settings (especially when force_env is on); replace this call with a
non-mutating draft applier (e.g., implement or call _apply_draft_config) that
applies draft_config into draft_app_state without touching os.environ or other
global state, or run the existing applier inside a sandbox/context manager that
snapshots and restores environment variables and global runtime state around the
call; ensure references to draft_config and draft_app_state are preserved and do
not call functions in manage_routes that set process-wide env vars.
Comment on lines
+313
to
+316
| request: Request, | ||
| disable_ssl: bool = Query(False, alias="disable_ssl"), | ||
| agent_id: Optional[str] = None, | ||
| ): |
There was a problem hiding this comment.
X-LLM-API-Key is advertised but never read.
Line 441 tells clients to set X-LLM-API-Key, and frontend sends it, but this endpoint never reads request.headers. That causes unnecessary 400s when key is provided only via header.
Suggested fix
- custom_auth_header = None
- api_key = None
+ custom_auth_header = None
+ api_key = (request.headers.get("X-LLM-API-Key") or "").strip() or None
@@
- if not api_key and not custom_auth_header:
+ if not api_key and not custom_auth_header:
logger.error(f"No authentication available for provider {provider_key}")
raise HTTPException(
status_code=400,
detail="API key required: set X-LLM-API-Key header or configure in config/secrets",
)Also applies to: 431-442
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 313-313: Unused function argument: request
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cuga/backend/server/manage_routes.py` around lines 313 - 316, The
endpoint function that has the signature including request: Request,
disable_ssl: bool = Query(False, alias="disable_ssl"), agent_id: Optional[str] =
None should also read the advertised X-LLM-API-Key header when the API key is
not provided via query/body; update the handler to check
request.headers.get("X-LLM-API-Key") and use that value as the API key (or
merge/override according to existing precedence logic) before performing the
existing validation that currently returns 400, ensuring clients sending the key
in the header don’t get rejected.
| } | ||
| if (data.policy_errors) { | ||
| const errs = Array.isArray(data.policy_errors) ? data.policy_errors : [data.policy_errors]; | ||
| errs.forEach((e: unknown) => addToast("warning", "Policy error", typeof e === "string" ? e : String(e))); |
There was a problem hiding this comment.
Fix lint-blocking forEach callbacks that currently return expression values.
These callbacks should use block bodies. Biome flags both as errors, and this can fail CI.
Suggested fix
- errs.forEach((e: unknown) => addToast("warning", "Policy error", typeof e === "string" ? e : String(e)));
+ errs.forEach((e: unknown) => {
+ addToast("warning", "Policy error", typeof e === "string" ? e : String(e));
+ });
- ([toolName, err]) => addToast("warning", `Tool: ${toolName}`, err?.error || err?.message || "Unknown error")
+ ([toolName, err]) => {
+ addToast("warning", `Tool: ${toolName}`, err?.error || err?.message || "Unknown error");
+ }Also applies to: 554-554
🧰 Tools
🪛 Biome (2.4.4)
[error] 497-497: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` at line 497, The forEach
callback on errs (errs.forEach(...)) currently uses a concise arrow expression
that returns a value; change it to a block-bodied arrow function so it does not
implicitly return an expression—e.g. replace the single-expression callback with
(e: unknown) => { addToast("warning", "Policy error", typeof e === "string" ? e
: String(e)); }—and make the same change for the other occurrence at the later
location (line referenced in the comment).
Comment on lines
+938
to
+953
| onClick={async () => { | ||
| const slug = llmInlineCreateKey.trim().toLowerCase().replace(/\s+/g, "-").replace(/[^a-z0-9-]/g, "-") || `llm-api-key-${Date.now()}`; | ||
| const res = await api.createSecret(slug, llmInlineCreateValue.trim(), "LLM API Key", undefined, effectiveAgentId); | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| const ref = data.ref || `db://${slug}`; | ||
| setLlmInlineCreate(false); | ||
| setLlmInlineCreateValue(""); | ||
| setLlmInlineCreateKey(""); | ||
| // Refresh list first so the new secret is available in the dropdown | ||
| await refreshSecrets(); | ||
| // Then select it and persist | ||
| updateLlm("api_key", ref); | ||
| setTimeout(saveLlmDraft, 0); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Handle failed/invalid secret-create responses explicitly.
The inline create path only handles res.ok; non-OK responses and JSON parse failures currently fail silently for users.
Suggested fix
<Button
size="sm"
style={{ marginTop: "auto" }}
disabled={!llmInlineCreateValue.trim()}
onClick={async () => {
const slug = llmInlineCreateKey.trim().toLowerCase().replace(/\s+/g, "-").replace(/[^a-z0-9-]/g, "-") || `llm-api-key-${Date.now()}`;
- const res = await api.createSecret(slug, llmInlineCreateValue.trim(), "LLM API Key", undefined, effectiveAgentId);
- if (res.ok) {
- const data = await res.json();
- const ref = data.ref || `db://${slug}`;
- setLlmInlineCreate(false);
- setLlmInlineCreateValue("");
- setLlmInlineCreateKey("");
- // Refresh list first so the new secret is available in the dropdown
- await refreshSecrets();
- // Then select it and persist
- updateLlm("api_key", ref);
- setTimeout(saveLlmDraft, 0);
- }
+ try {
+ const res = await api.createSecret(slug, llmInlineCreateValue.trim(), "LLM API Key", undefined, effectiveAgentId);
+ if (!res.ok) {
+ addToast("error", "Secret create failed", `Failed to create secret (${res.status} ${res.statusText})`);
+ return;
+ }
+ const data = await res.json().catch(() => ({}));
+ const ref = data.ref || `db://${slug}`;
+ setLlmInlineCreate(false);
+ setLlmInlineCreateValue("");
+ setLlmInlineCreateKey("");
+ await refreshSecrets();
+ updateLlm("api_key", ref);
+ setTimeout(saveLlmDraft, 0);
+ } catch (error) {
+ addToast("error", "Secret create failed", error instanceof Error ? error.message : "Network error");
+ }
}}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 938 - 953,
The inline secret-creation click handler (using llmInlineCreateKey,
llmInlineCreateValue and calling api.createSecret) only processes res.ok and
silently ignores failures or JSON parse errors; modify the onClick handler to
explicitly handle non-OK responses and JSON parse exceptions by reading and
logging/returning the error body (e.g., await res.text() or try/catch around
res.json()), surfacing a user-visible error state (e.g., set an error message
state or show a toast) when creation fails, and only on success proceed with
refreshSecrets(), updateLlm("api_key", ref) and saveLlmDraft; ensure the handler
cleans up UI state (setLlmInlineCreate(false), reset inputs) only after a
confirmed success and preserve inputs when there is an error so the user can
retry.
Comment on lines
1067
to
1069
| onChange={(_e: unknown, { value }: { value: number | string }) => | ||
| updateLlmTemperature(Number(value) || 0.7) | ||
| updateLlmTemperature(Number(value) || 0.1) | ||
| } |
There was a problem hiding this comment.
Preserve valid temperature=0 instead of forcing 0.1.
Number(value) || 0.1 treats 0 as falsy, so users can’t set the lower bound.
Suggested fix
- onChange={(_e: unknown, { value }: { value: number | string }) =>
- updateLlmTemperature(Number(value) || 0.1)
- }
+ onChange={(_e: unknown, { value }: { value: number | string }) => {
+ const parsed = Number(value);
+ updateLlmTemperature(Number.isFinite(parsed) ? parsed : 0.1);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onChange={(_e: unknown, { value }: { value: number | string }) => | |
| updateLlmTemperature(Number(value) || 0.7) | |
| updateLlmTemperature(Number(value) || 0.1) | |
| } | |
| onChange={(_e: unknown, { value }: { value: number | string }) => { | |
| const parsed = Number(value); | |
| updateLlmTemperature(Number.isFinite(parsed) ? parsed : 0.1); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend_workspaces/frontend/src/ManagePage.tsx` around lines 1067 -
1069, The current onChange uses Number(value) || 0.1 which treats 0 as falsy and
prevents setting temperature to 0; change it to parse the numeric value and only
fall back to 0.1 when the parsed result is NaN (e.g. const v = Number(value);
updateLlmTemperature(isNaN(v) ? 0.1 : v)); update the onChange handler that
calls updateLlmTemperature so it preserves valid 0 values while still defaulting
on invalid input.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/frontend_workspaces/frontend/src/AddToolModal.tsx (2)
143-151:⚠️ Potential issue | 🟠 MajorHandle AWS-backed secrets explicitly in ref normalization.
AWS source entries are currently coerced to
db://..., which breaks non-DB secret resolution paths.Suggested fix
+const toSecretRef = (s: { id: string; source?: string }, mode: string) => + s.source === "vault" || mode === "vault" + ? `vault://secret/${s.id}#value` + : s.source === "aws" + ? `aws://${s.id}` + : s.source === "env" + ? s.id + : `db://${s.id}`; - ref: s.source === "vault" || mode === "vault" - ? `vault://secret/${s.id}#value` - : s.source === "env" - ? s.id - : `db://${s.id}`, + ref: toSecretRef(s, mode),Also applies to: 499-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 143 - 151, The ref normalization in the setSecretsList mapping incorrectly treats AWS-backed secrets as DB entries; update the mapping inside the raw.map used by setSecretsList so that when s.source === "aws" (or when mode === "aws") you build an AWS-style ref (e.g., `aws://secret/${s.id}` or `aws-secretsmanager://...`) instead of falling through to `db://...`; apply the same change to the duplicate mapping instance referenced around lines 499-507 so both locations explicitly handle s.source === "aws" and mode === "aws".
484-487:⚠️ Potential issue | 🟠 MajorUse the backend-returned secret
refafter inline create.Line 486 hardcodes
db://..., which discards backend-specific refs (Vault/AWS) returned by the API.Suggested fix
const res = await api.createSecret(slug, inlineCreateValue.trim(), `Auth for ${name.trim() || "tool"}`, undefined, agentId); if (res.ok) { - setAuthValue(`db://${slug}`); + const data = await res.json(); + setAuthValue(data.ref || `db://${slug}`); setInlineCreateOpen(false); setInlineCreateKey("");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 484 - 487, The inline secret creation currently hardcodes setAuthValue to `db://${slug}` which ignores the backend-returned secret reference; after calling api.createSecret(slug, inlineCreateValue.trim(), ..., agentId) (the call named createSecret), extract the returned secret ref (e.g., res.ref or res.secret.ref depending on the API shape) and pass that to setAuthValue when present, falling back to `db://${slug}` only if no ref is returned, then close the inline create UI via setInlineCreateOpen(false).src/cuga/backend/server/secrets_routes.py (3)
183-186:⚠️ Potential issue | 🟠 MajorInternal runtime error details are exposed to clients.
Line 185 and Line 186 return
detail=str(e), which can leak backend internals. Log server-side and return client-safe messages.🛡️ Proposed fix
- except RuntimeError as e: - if "CUGA_SECRET_KEY" in str(e): - raise HTTPException(status_code=503, detail=str(e)) - raise HTTPException(status_code=400, detail=str(e)) + except RuntimeError as e: + logger.exception("Failed to create secret due to runtime error") + if "CUGA_SECRET_KEY" in str(e): + raise HTTPException(status_code=503, detail="Secrets backend is not configured") from e + raise HTTPException(status_code=400, detail="Invalid secret request") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 183 - 186, The except RuntimeError handler in secrets_routes.py currently returns the raw exception text to clients; change it to log the full exception server-side (using the module's logger, e.g., logger.exception or similar) and return a client-safe message in the HTTPException detail instead of str(e); preserve the status codes (503 when the error message mentions "CUGA_SECRET_KEY", otherwise 400) but replace the detail with a generic message like "Service temporarily unavailable" for 503 and "Bad request" for 400 while ensuring the original exception is recorded in logs for debugging.
165-176:⚠️ Potential issue | 🔴 CriticalScoped ownership check is performed against the wrong record.
Line 165 fetches metadata without
agent_id/version, but Line 175-Line 176 writes with scoped values. This can allow overwrite of an existing scoped secret without validating the scoped creator.🔧 Proposed fix
- meta = await secrets_store.get_secret_metadata(body.id) + scope_agent_id = body.agent_id or "*" + scope_version = body.version or "*" + meta = await secrets_store.get_secret_metadata( + body.id, + agent_id=scope_agent_id, + version=scope_version, + ) if meta: creator = meta.get("created_by") or "" if creator and _user_id(current_user) != creator: raise HTTPException(status_code=403, detail="Only the creator can update this secret") await secrets_store.set_secret( body.id, body.value, description=body.description, tags=body.tags, - agent_id=body.agent_id or "*", - version=body.version or "*", + agent_id=scope_agent_id, + version=scope_version, created_by=_user_id(current_user), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 165 - 176, The ownership check fetches metadata without scope, so get_secret_metadata(body.id) can return the unscoped record while set_secret writes a scoped secret (agent_id/version), allowing unauthorized overwrites; update the check to fetch metadata using the same scope values as set_secret (use body.agent_id or "*" and body.version or "*") before comparing creator via _user_id(current_user) to ensure the creator of the exact scoped secret is verified; keep existing behavior for missing metadata (allow create) and raise the same 403 when the scoped creator mismatches.
155-163:⚠️ Potential issue | 🔴 CriticalVault mode still bypasses creator authorization and drops scope isolation.
In vault branches, handlers return early without creator checks, and Vault keys are derived only from
secret_id(ignoringagent_id/version). This allows cross-scope overwrite/delete and breaks parity with local-mode authorization.🔒 Fix direction
+def _vault_scoped_id(secret_id: str, agent_id: str | None, version: str | None) -> str: + return f"{agent_id or '*'}:{version or '*'}:{secret_id}" @@ - if mode == "vault": - ok = _vault_write(body.id, body.value, description=body.description) + if mode == "vault": + scope_agent_id = body.agent_id or "*" + scope_version = body.version or "*" + meta = await secrets_store.get_secret_metadata( + body.id, + agent_id=scope_agent_id, + version=scope_version, + ) + creator = (meta or {}).get("created_by") or "" + if creator and _user_id(current_user) != creator: + raise HTTPException(status_code=403, detail="Only the creator can update this secret") + ok = _vault_write( + _vault_scoped_id(body.id, scope_agent_id, scope_version), + body.value, + description=body.description, + )Apply the same scoped key + creator check pattern in
update_secretanddelete_secret.Also applies to: 201-206, 262-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 155 - 163, The vault branches in update_secret and delete_secret mirror this snippet (using _vault_write/_vault_delete and returning vault://... refs) but skip the creator authorization and build Vault keys only from body.id; change those branches to reuse the same scoped-key and creator-check logic used in the local-mode path: derive the Vault key using the scope-aware identifier (include agent_id and version/namespace the same way local-mode composes it), verify the requestor is the creator before performing _vault_write/_vault_delete, and if the creator mismatches raise the same HTTPException used by local-mode; update the vault ref construction (ref = f"vault://secret/{scoped_key}#value" or similar) to maintain parity.
🧹 Nitpick comments (1)
src/cuga/backend/server/secrets_routes.py (1)
81-82: Avoid silentexcept ...: passon Vault operations.These blocks suppress failures and make incident diagnosis difficult. Emit at least debug-level logs before fallback behavior.
🧰 Proposed logging pattern
- except Exception: - pass + except Exception: + logger.debug("Vault list failed while aggregating secrets", exc_info=True) @@ - except Exception: - pass + except Exception: + logger.debug("Vault write failed", exc_info=True) @@ - except Exception: - return False + except Exception: + logger.debug("Vault delete failed", exc_info=True) + return FalseAlso applies to: 142-143, 248-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cuga/backend/server/secrets_routes.py` around lines 81 - 82, Replace each silent "except Exception: pass" in secrets_routes.py (the Vault operation handlers where Vault reads/fetches secrets) with an error-aware log and preserve exception details; capture the exception as "except Exception as e" and call the module logger (e.g., logger.debug or logger.exception) with a short, contextual message that includes the Vault operation and identifying info (secret path/name) and include exc_info=True so the stacktrace is available; do this for all occurrences that currently swallow exceptions so fallback logic remains but failures are recorded for diagnosis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx`:
- Around line 196-208: The code currently swallows failures from
api.createSecret and then continues using the plaintext authValueFinal; change
this so a failed secret creation prevents falling back to storing raw
credentials: in the submit flow around saveAsNewSecret, api.createSecret,
authValueFinal, and the surrounding try/catch in AddToolModal.tsx, surface the
error instead of ignoring it (e.g., set a form error/state and abort submission
or throw), and ensure the alternate block (lines ~210-215) uses the same
behavior so that when api.createSecret fails you do not assign or persist the
plaintext value and the user is prompted to fix or retry secret creation.
---
Duplicate comments:
In `@src/cuga/backend/server/secrets_routes.py`:
- Around line 183-186: The except RuntimeError handler in secrets_routes.py
currently returns the raw exception text to clients; change it to log the full
exception server-side (using the module's logger, e.g., logger.exception or
similar) and return a client-safe message in the HTTPException detail instead of
str(e); preserve the status codes (503 when the error message mentions
"CUGA_SECRET_KEY", otherwise 400) but replace the detail with a generic message
like "Service temporarily unavailable" for 503 and "Bad request" for 400 while
ensuring the original exception is recorded in logs for debugging.
- Around line 165-176: The ownership check fetches metadata without scope, so
get_secret_metadata(body.id) can return the unscoped record while set_secret
writes a scoped secret (agent_id/version), allowing unauthorized overwrites;
update the check to fetch metadata using the same scope values as set_secret
(use body.agent_id or "*" and body.version or "*") before comparing creator via
_user_id(current_user) to ensure the creator of the exact scoped secret is
verified; keep existing behavior for missing metadata (allow create) and raise
the same 403 when the scoped creator mismatches.
- Around line 155-163: The vault branches in update_secret and delete_secret
mirror this snippet (using _vault_write/_vault_delete and returning vault://...
refs) but skip the creator authorization and build Vault keys only from body.id;
change those branches to reuse the same scoped-key and creator-check logic used
in the local-mode path: derive the Vault key using the scope-aware identifier
(include agent_id and version/namespace the same way local-mode composes it),
verify the requestor is the creator before performing
_vault_write/_vault_delete, and if the creator mismatches raise the same
HTTPException used by local-mode; update the vault ref construction (ref =
f"vault://secret/{scoped_key}#value" or similar) to maintain parity.
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx`:
- Around line 143-151: The ref normalization in the setSecretsList mapping
incorrectly treats AWS-backed secrets as DB entries; update the mapping inside
the raw.map used by setSecretsList so that when s.source === "aws" (or when mode
=== "aws") you build an AWS-style ref (e.g., `aws://secret/${s.id}` or
`aws-secretsmanager://...`) instead of falling through to `db://...`; apply the
same change to the duplicate mapping instance referenced around lines 499-507 so
both locations explicitly handle s.source === "aws" and mode === "aws".
- Around line 484-487: The inline secret creation currently hardcodes
setAuthValue to `db://${slug}` which ignores the backend-returned secret
reference; after calling api.createSecret(slug, inlineCreateValue.trim(), ...,
agentId) (the call named createSecret), extract the returned secret ref (e.g.,
res.ref or res.secret.ref depending on the API shape) and pass that to
setAuthValue when present, falling back to `db://${slug}` only if no ref is
returned, then close the inline create UI via setInlineCreateOpen(false).
---
Nitpick comments:
In `@src/cuga/backend/server/secrets_routes.py`:
- Around line 81-82: Replace each silent "except Exception: pass" in
secrets_routes.py (the Vault operation handlers where Vault reads/fetches
secrets) with an error-aware log and preserve exception details; capture the
exception as "except Exception as e" and call the module logger (e.g.,
logger.debug or logger.exception) with a short, contextual message that includes
the Vault operation and identifying info (secret path/name) and include
exc_info=True so the stacktrace is available; do this for all occurrences that
currently swallow exceptions so fallback logic remains but failures are recorded
for diagnosis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae61ad2a-6e38-4f9c-8218-383e9b66bfea
⛔ Files ignored due to path filters (2)
src/frontend_workspaces/frontend/dist/index.htmlis excluded by!**/dist/**src/frontend_workspaces/frontend/dist/main.23c23b26c0e1fa3b7c0e.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
src/cuga/backend/server/secrets_routes.pysrc/frontend_workspaces/agentic_chat/src/PoliciesConfig.tsxsrc/frontend_workspaces/frontend/src/AddToolModal.tsx
Comment on lines
+196
to
+208
| if (saveAsNewSecret && authValueFinal) { | ||
| const baseSlug = saveAsNewSecretKey.trim() | ||
| ? saveAsNewSecretKey.trim().toLowerCase().replace(/\s+/g, "-").replace(/[^a-z0-9-]/g, "-") | ||
| : `${name.trim() || "tool"}-${authType}-${Date.now()}`.toLowerCase().replace(/[^a-z0-9-]/g, "-"); | ||
| const slug = baseSlug; | ||
| try { | ||
| const res = await api.createSecret(slug, authValueFinal, `Auth for ${name.trim() || "tool"}`, undefined, agentId); | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| authValueFinal = data.ref || `db://${slug}`; | ||
| } | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
Don’t silently fall back to plaintext auth when “save as new secret” fails.
If secret creation fails, submit continues and stores the raw secret value in tool.auth.value. This is a security/UX footgun.
Suggested fix
if (saveAsNewSecret && authValueFinal) {
const baseSlug = saveAsNewSecretKey.trim()
@@
try {
const res = await api.createSecret(slug, authValueFinal, `Auth for ${name.trim() || "tool"}`, undefined, agentId);
- if (res.ok) {
- const data = await res.json();
- authValueFinal = data.ref || `db://${slug}`;
- }
- } catch (_) {}
+ if (!res.ok) return; // block submit; show UI error in follow-up
+ const data = await res.json();
+ if (!data.ref) return; // block submit; avoid plaintext fallback
+ authValueFinal = data.ref;
+ } catch (_) {
+ return; // block submit; avoid plaintext fallback
+ }
}Also applies to: 210-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend_workspaces/frontend/src/AddToolModal.tsx` around lines 196 -
208, The code currently swallows failures from api.createSecret and then
continues using the plaintext authValueFinal; change this so a failed secret
creation prevents falling back to storing raw credentials: in the submit flow
around saveAsNewSecret, api.createSecret, authValueFinal, and the surrounding
try/catch in AddToolModal.tsx, surface the error instead of ignoring it (e.g.,
set a form error/state and abort submission or throw), and ensure the alternate
block (lines ~210-215) uses the same behavior so that when api.createSecret
fails you do not assign or persist the plaintext value and the user is prompted
to fix or retry secret creation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature Pull Request
Related Issue
Closes #
Description
Adds HashiCorp Vault integration alongside a pluggable secrets backend system. Secrets can be resolved from Vault, AWS Secrets Manager, database, or environment variables. Includes a Secrets Manager UI in the manage page, API routes for CRUD operations, Helm chart for OpenShift deployment, and integration with LLM config and MCP tools.
Type of Changes
Testing
Documentation
Checklist
Summary by CodeRabbit
New Features
Chores
Tests