feat(web): the AI providers experience - settings, picker, and run-path wiring - #6001
Conversation
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughProvider probing, persisted provider connections, connection-aware SDK resolution, and connection-first model selection were added across the API, SDK, shared entities, and web UI. The settings experience now manages provider connections instead of separate secret tables. ChangesProvider probing
SDK connection resolution
Shared provider entities
Provider management and model selection UI
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🟠 High · up to The current implementation can still run requests with the wrong provider connection in fallback and legacy-alias cases, potentially using the wrong account or credential, while credential payloads remain easy to expose through accidental logging or tracebacks. These are concrete merge-readiness risks that should be fixed or explicitly accepted before merge. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Inline annotations: each comment marks a decision point, the reason, and the revert path. The decisions summary comment on this PR lists them all in one place.
| """ | ||
|
|
||
| label: str = "OpenRouter" | ||
| key_url: str = "https://openrouter.ai/api/v1/key" |
There was a problem hiding this comment.
OpenRouter needs two calls by design: /api/v1/key (authenticated, free) decides the credential verdict, /api/v1/models only supplies the list. The models endpoint is public, so without this split a garbage key would read as valid. A unit test pins that a 200 from the public catalog cannot outrank a 401 from /key.
| return GuardedEndpoint( | ||
| url=urlunparse(parsed._replace(netloc=pinned_netloc)), | ||
| headers={"Host": host_header}, | ||
| extensions={"sni_hostname": parsed.hostname}, |
There was a problem hiding this comment.
SSRF guard for caller-supplied URLs (custom, azure): reuses the webhook egress policy, then resolves and pins the request to the validated IP (Host header + SNI preserved) so a DNS rebind between validation and send cannot redirect the probe. Same technique the webhook sender uses in production.
| * pinning today's defaults into the record, and a harness set that came out empty only because no | ||
| * harness declares this deployment goes out omitted rather than saying "no harness may use this". | ||
| */ | ||
| export const connectionPolicyForSave = ({ |
There was a problem hiding this comment.
Decision (review finding): a saved policy field is sent only when the user actually chose it. An untouched pre-checked card saves no models field, so the connection keeps following Agenta's defaults; an actively cleared list still saves an explicit empty list. Same rule for harnesses: a deployment no harness declares saves nothing rather than 'no harness may use this'.
| def get_provider_settings(model: str, scope: str = "all") -> Optional[Dict]: | ||
| """ | ||
| Builds the LLM request with appropriate kwargs based on the custom provider/model | ||
| def _resolve_provider_settings( |
There was a problem hiding this comment.
Resolution precedence, shared by both context paths: explicit slug wins (either vault kind; unknown slug raises a clean 400 naming known slugs, never silently another key); without a slug, the family mapping applies with a saved-model-membership tiebreak, then first record, which is the pre-connections behavior. A slug-selected standard record also verifies the model maps to its family, so an Anthropic connection asked for an OpenAI model fails loudly instead of sending the wrong key.
| # A project may hold several connections per provider family, so vault provider_key records | ||
| # are kept as a list. Keying them by family (as the locals still are) would drop every | ||
| # connection but the last, making a named second OpenAI key unreachable. | ||
| vault_standard = [] |
There was a problem hiding this comment.
Load-bearing fix found during implementation: vault provider_key records were keyed by family here, so two OpenAI connections reached the workflow as one and no resolver change could make the second reachable. Records are now a list; env-var locals still shadow per family. This touches every prompt run, so backward-compat is pinned by test_vault_secrets_multiple_connections.py.
| const mergedOptions = useMemo(() => { | ||
| const base = schemaOptions?.options ?? [] | ||
| const extra = llmProviderConfig?.extraOptionGroups ?? [] | ||
| const extra = withoutSlugBoundGroups(llmProviderConfig?.extraOptionGroups ?? []) |
There was a problem hiding this comment.
This control writes a single scalar and cannot persist a sibling connection slug, so connection-stamped standard groups are filtered out here; offering them would silently run on whichever key the family fallback picks. The judge and prompt editors go through ModelConfigEditor, which writes model + connection in one dispatch, so they get the groups there. Custom-provider groups stay: they resolve by model-key membership, not slug.
| * has to come back out as whatever the harness accepts, or the run fails on an id the harness | ||
| * never published. | ||
| */ | ||
| const harnessSpellings = ( |
There was a problem hiding this comment.
default_models arrive in each harness's own spelling, so a saved claude-fable-5 emits anthropic/claude-fable-5 under Pi and the bare alias under Claude Code; this indexes each harness's catalog by bare id to republish the id the runtime actually accepts. Pinned by test.
| // A picked connection row carries its model, provider family, connection and harness. All four | ||
| // land in ONE `onChange`: writing `llm` and `harness` through two calls would have the second | ||
| // overwrite the first, since both compose from the same (stale) `config`. | ||
| const applyPickerSelection = useCallback( |
There was a problem hiding this comment.
A picked row names a model AND its harness, so llm and harness.kind are composed into one onChange here; two sequential writes would both build on the same stale config and the second would drop the first. The same single-dispatch rule is why the prompt side writes model + connection together in ModelConfigEditor.
| // A subscription is a login mounted into the deployment; cloud has nowhere to mount one. | ||
| // isCloud is really isEE today, which would hide subscriptions on self-hosted EE, | ||
| // exactly where mounted logins exist. Ungated until a real cloud signal exists. | ||
| showSubscriptions={true} |
There was a problem hiding this comment.
Decision after QA: subscriptions stay visible on every self-hosted build. The available isCloud signal is really isEE (isDemo() returns isEE()), which would hide the section exactly where mounted logins exist. Same root cause as the pre-existing 'Unavailable in the cloud' badge showing on self-hosted EE; a real cloud signal is a recorded follow-up.
Decisions taken autonomously during this implementation (for review, each reversible)Delivery shape
Product/interface calls (beyond what the plan fixed)
Known issues deliberately deferred (with owners' notes in the PR body)
|
04ab09f to
85dc40e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Two more cosmetic items from the live-QA report, added to the deferred list for completeness: the Bedrock credential labels mix casing ("Access key ID" vs "Secret Access Key" in providerFields.ts), and the "This secret will be encrypted in transit and at rest." note repeats on three consecutive Bedrock fields. Also for the record: a transient "Add your model provider key" banner flashed once during an agent run on a valid connection and collapsed on its own; it looks like a loading-state gate in the banner condition, was not reproducible afterward, and is worth a look if it resurfaces. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
web/packages/agenta-entity-ui/src/secretProvider/ActiveModelsSection.tsx (1)
100-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider virtualizing the model list.
Provider discovery can return more than 100 model ids. This list renders every visible option inside a fixed-height scroll container, so the DOM node count grows with the provider's catalog. If you expect large catalogs, render the rows with a virtualized list.
As per coding guidelines: "Virtualize lists with 100 or more items and debounce or throttle search, filter, scroll, and resize handlers."
Source: Coding guidelines
web/packages/agenta-entity-ui/src/secretProvider/PlaygroundProviderSections.tsx (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not derive connection state from a display string.
connectedcomparescredentialSummary(connection)against the literal"—". That literal is presentation output from@agenta/entities/secret. If the placeholder text changes there, the status dot inverts for every row and no type error appears.Export a predicate (for example
hasCredential(connection)) or a shared placeholder constant from the entities package, and use it here.web/packages/agenta-entity-ui/tests/unit/connectionPicker.test.ts (1)
258-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an empty group list.
buildPickerGroupsis only exercised with one connection that yields options.ModelPickerControlbranches ongroups.length === 0and renders itsfallbackinstead of the picker. Pin that boundary here: a connection whose harness policy resolves to no supported harness must produce no group.Example:
standard("1", "openai", {harnesses: ["claude"]})already resolves to[]in theeffectiveHarnessessuite at Line 93.web/packages/agenta-entity-ui/src/secretProvider/ProviderDrawer.tsx (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass a project-scoped Settings href into
ProviderDrawer.@agenta/entity-uidoes not expose a workspace atom or depend on Next.js, so derive the href fromappIdentifiersAtomin the app layer and render it with Next.jsLinkthere. The current render-timewindow.locationfallback produces/settings?tab=llmsduring SSR, while the browser produces a project-scoped URL; the native<a>also causes a full page reload.api/oss/src/core/providers/adapters.py (1)
62-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the response body before parsing it.
_probe_catalogcallsclient.get(...)and thenresponse.json()with no size limit. ForOpenAICompatibleAdapterandAzureAdapterthe target host comes from the caller. A hostile or misconfigured endpoint can return a very large body. The API process then buffers and parses all of it. The 10-second timeout limits duration, not volume.Consider streaming the response and rejecting bodies above a fixed ceiling, and capping the extracted model list.
♻️ Sketch of a size ceiling
+MAX_CATALOG_BYTES = 2 * 1024 * 1024 +MAX_CATALOG_MODELS = 2000 + async def _probe_catalog(try: models = extract(response.json()) except (AttributeError, KeyError, TypeError, ValueError): return ProbeOutcome( credential=credential, discovery=DiscoveryResult(status=DiscoveryStatus.FAILED), ) + models = models[:MAX_CATALOG_MODELS]Add the byte ceiling by replacing
client.get(...)withclient.stream(...)and aborting onceMAX_CATALOG_BYTESis exceeded.sdks/python/oss/tests/pytest/unit/test_secrets_manager_connection_slug.py (1)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the known-slug assertion.
"openai" in error.messageis a substring of"openai-9", which the message already contains. The assertion passes even when the known-connections hint is missing. Assert the hint text instead.♻️ Proposed assertion
- # The message names what IS available, and never a key. - assert "openai" in error.message + # The message names what IS available, and never a key. + assert "Known connections: openai" in error.message assert "sk-first" not in error.messageweb/oss/src/hooks/useLLMProviderConfig.tsx (1)
34-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unnecessary
as ProviderGroup[]assertion.
PromptModelGroup[]is structurally assignable toProviderGroup[].ProviderGroup.keyis optional, whilePromptModelOption.keyis required.web/packages/agenta-entities/src/secret/api/probe.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack the Fern migration for this endpoint, and reuse one
CredentialStatusdefinition.Two points on this new API module:
- The coding guidelines require Fern client accessors instead of raw axios. The header comment documents the reason (the generated client has no
probeProvideryet), so this is an accepted temporary deviation. Add a tracked issue for the migration togetProvidersClient().probeProvider(...)so the deviation does not become permanent. I can open that issue if you want.CREDENTIAL_STATUSES/CredentialStatushere duplicate theCredentialStatusunion inweb/packages/agenta-entities/src/secret/core/connections.ts(Line 371). Two independent definitions of the same three states can drift. Import the core type here, or derive the core type fromCREDENTIAL_STATUSES.As per coding guidelines: "Frontend API code must use per-resource Fern client accessors from
@agenta/sdk/resources, never raw axios or the@agenta/sdkroot barrel."Also applies to: 22-23
Source: Coding guidelines
web/packages/agenta-entities/src/secret/state/connections.ts (1)
74-89: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInvalidate the vault query inside this atom after a successful save.
The save path does not invalidate
vaultSecretsQueryAtom, so every caller must refetch by itself.ModelPickerControldoes this throughonSaved={() => refetchVault()}, but any other caller that forgets it shows stale connections. Invalidate once here instead.The coding guidelines require invalidation after mutations for
atomWithQuerydata.As per coding guidelines: "Use
atomWithQuerywith TanStack Query for API data fetching; … invalidate after mutations".Source: Coding guidelines
web/packages/agenta-entities/src/secret/core/providerFields.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
fieldNoteForKindwith the documented fallback.The doc comment states that a kind absent from
notesByKindfalls back tonote. The implementation does not do that. WhennotesByKindexists, the function returnsfield.notesByKind[kind], which isundefinedfor an unlisted kind, and it never readsfield.note.No current field declares both
noteandnotesByKind, so behavior is correct today. A future field with both would silently lose its shared note.♻️ Proposed fix
export const fieldNoteForKind = (field: ProviderFieldConfig, kind: string): string | undefined => - field.notesByKind ? field.notesByKind[kind] : field.note + field.notesByKind?.[kind] ?? field.noteAlso applies to: 44-46
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 17556fa3-4010-4205-812c-f621055a530f
📒 Files selected for processing (78)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/providers/__init__.pyapi/oss/src/apis/fastapi/providers/models.pyapi/oss/src/apis/fastapi/providers/router.pyapi/oss/src/core/providers/__init__.pyapi/oss/src/core/providers/adapters.pyapi/oss/src/core/providers/dtos.pyapi/oss/src/core/providers/endpoints.pyapi/oss/src/core/providers/exceptions.pyapi/oss/src/core/providers/service.pyapi/oss/src/resources/evaluators/evaluators.pyapi/oss/tests/pytest/unit/providers/__init__.pyapi/oss/tests/pytest/unit/providers/test_provider_probe.pysdks/python/agenta/sdk/engines/running/errors.pysdks/python/agenta/sdk/engines/running/handlers.pysdks/python/agenta/sdk/engines/running/interfaces.pysdks/python/agenta/sdk/managers/secrets.pysdks/python/agenta/sdk/middlewares/running/vault.pysdks/python/agenta/sdk/utils/assets.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/test_auto_ai_critique_v0_connection.pysdks/python/oss/tests/pytest/unit/test_auto_ai_critique_v0_runtime.pysdks/python/oss/tests/pytest/unit/test_chat_v0_inputs.pysdks/python/oss/tests/pytest/unit/test_llm_v0_handler_flags_running.pysdks/python/oss/tests/pytest/unit/test_llm_v0_provider_key_binding.pysdks/python/oss/tests/pytest/unit/test_prompt_template_extensions.pysdks/python/oss/tests/pytest/unit/test_secrets_manager_connection_slug.pysdks/python/oss/tests/pytest/unit/test_supported_llm_models.pysdks/python/oss/tests/pytest/unit/test_vault_secrets_multiple_connections.pyweb/oss/src/components/AgentChatSlice/hooks/useChatSlashCommands.tsxweb/oss/src/components/pages/settings/AIProviders/AIProviders.tsxweb/oss/src/components/pages/settings/Secrets/SecretProviderTable/index.tsxweb/oss/src/components/pages/settings/Secrets/Secrets.tsxweb/oss/src/components/pages/settings/assets/navigation.tsweb/oss/src/hooks/useLLMProviderConfig.tsxweb/oss/src/pages/w/[workspace_id]/p/[project_id]/settings/index.tsxweb/packages/agenta-entities/src/runnable/evaluatorTransforms.tsweb/packages/agenta-entities/src/secret/api/index.tsweb/packages/agenta-entities/src/secret/api/probe.tsweb/packages/agenta-entities/src/secret/core/connectionSummary.tsweb/packages/agenta-entities/src/secret/core/connections.tsweb/packages/agenta-entities/src/secret/core/index.tsweb/packages/agenta-entities/src/secret/core/promptModelGroups.tsweb/packages/agenta-entities/src/secret/core/providerCatalog.tsweb/packages/agenta-entities/src/secret/core/providerFields.tsweb/packages/agenta-entities/src/secret/core/transforms.tsweb/packages/agenta-entities/src/secret/core/types.tsweb/packages/agenta-entities/src/secret/index.tsweb/packages/agenta-entities/src/secret/state/connections.tsweb/packages/agenta-entities/src/secret/state/index.tsweb/packages/agenta-entities/src/secret/state/persistence.tsweb/packages/agenta-entities/tests/unit/evaluator-connection.test.tsweb/packages/agenta-entities/tests/unit/prompt-model-groups.test.tsweb/packages/agenta-entities/tests/unit/provider-connections.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/GroupedChoiceControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ModelPickerControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useModelHarness.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionPicker.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/connectionUtils.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/index.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/ModelConfigEditor.tsxweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/useModelConfigurePopover.tsxweb/packages/agenta-entity-ui/src/DrillInView/index.tsweb/packages/agenta-entity-ui/src/secretProvider/ActiveModelsSection.tsxweb/packages/agenta-entity-ui/src/secretProvider/CustomProviderForm.tsxweb/packages/agenta-entity-ui/src/secretProvider/HarnessesSection.tsxweb/packages/agenta-entity-ui/src/secretProvider/PlaygroundProviderSections.tsxweb/packages/agenta-entity-ui/src/secretProvider/ProviderCatalogList.tsxweb/packages/agenta-entity-ui/src/secretProvider/ProviderConnectionCard.tsxweb/packages/agenta-entity-ui/src/secretProvider/ProviderDrawer.tsxweb/packages/agenta-entity-ui/src/secretProvider/index.tsweb/packages/agenta-entity-ui/src/secretProvider/providerIcon.tsweb/packages/agenta-entity-ui/tests/unit/connectionPicker.test.tsweb/packages/agenta-entity-ui/tests/unit/connectionUtils.test.tsweb/packages/agenta-shared/src/types/llmProvider.tsweb/packages/agenta-ui/src/SelectLLMProvider/SelectLLMProviderBase.tsxweb/packages/agenta-ui/src/SelectLLMProvider/types.tsweb/packages/agenta-ui/src/drill-in/context/DrillInUIContext.tsx
💤 Files with no reviewable changes (2)
- web/oss/src/components/pages/settings/Secrets/SecretProviderTable/index.tsx
- web/oss/src/components/pages/settings/Secrets/Secrets.tsx
| class ProviderCredentials(BaseModel): | ||
| """Credentials in transit only. Never persisted here, never logged, never echoed.""" | ||
|
|
||
| key: Optional[str] = None | ||
| url: Optional[str] = None | ||
| version: Optional[str] = None | ||
| extras: Optional[Dict[str, Any]] = None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make the credential hard to print by accident.
The docstring states the credential is never logged. Nothing in the type enforces that. Pydantic renders field values in repr() and str(), so any future log.info(..., credentials=credentials) or exception traceback that includes the model prints the raw key.
Consider SecretStr for key, or Field(repr=False) on key and extras. extras also carries secrets such as aws_secret_access_key. Adapters read credentials.key and credentials.extras directly, so SecretStr requires .get_secret_value() at those call sites in adapters.py.
🔒 Minimal hardening without touching call sites
class ProviderCredentials(BaseModel):
"""Credentials in transit only. Never persisted here, never logged, never echoed."""
- key: Optional[str] = None
+ key: Optional[str] = Field(default=None, repr=False)
url: Optional[str] = None
version: Optional[str] = None
- extras: Optional[Dict[str, Any]] = None
+ extras: Optional[Dict[str, Any]] = Field(default=None, repr=False)📝 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.
| class ProviderCredentials(BaseModel): | |
| """Credentials in transit only. Never persisted here, never logged, never echoed.""" | |
| key: Optional[str] = None | |
| url: Optional[str] = None | |
| version: Optional[str] = None | |
| extras: Optional[Dict[str, Any]] = None | |
| class ProviderCredentials(BaseModel): | |
| """Credentials in transit only. Never persisted here, never logged, never echoed.""" | |
| key: Optional[str] = Field(default=None, repr=False) | |
| url: Optional[str] = None | |
| version: Optional[str] = None | |
| extras: Optional[Dict[str, Any]] = Field(default=None, repr=False) |
There was a problem hiding this comment.
Accepted, with the stronger variant: key becomes SecretStr (unprintable through repr, str, and model_dump; adapters unwrap via .get_secret_value() at their call sites) and extras gets Field(repr=False) since typing every AWS/Vertex value would push SecretStr into a plain dict. The existing canary leak tests get three new assertions: repr, str, and model_dump of ProviderCredentials must not contain the canary. Landing with the next commit batch on this PR; the tree is currently frozen for the final QA gate run and this touches the probe path under test, so it applies immediately after the gate's verdict rather than mid-run.
There was a problem hiding this comment.
@mmabrouk, the stronger variant addresses the credential exposure risk.
SecretStr protects key in repr(), str(), and serialization. Hiding extras from repr() is appropriate for the untyped cloud-provider credential fields. The canary assertions cover the relevant accidental-output paths.
The thread remains open until the next commit batch includes the change and tests.
🐇
✏️ Learnings added
Learnt from: mmabrouk
Repo: Agenta-AI/agenta PR: 6001
File: api/oss/src/core/providers/dtos.py:34-40
Timestamp: 2026-08-13T13:52:06.919Z
Learning: For `api/oss/src/core/providers/dtos.py`, `ProviderCredentials.key` must use Pydantic `SecretStr`, and provider adapter call sites must unwrap it with `.get_secret_value()`. `ProviderCredentials.extras` contains untyped AWS and Vertex credential values and must use `Field(repr=False)`. Credential leak tests must verify that `repr()`, `str()`, and `model_dump()` of `ProviderCredentials` do not expose a canary secret.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| # First record per family wins, matching `SecretsManager._settings_by_family`. Letting | ||
| # the last one overwrite would run this path on a different key than every other | ||
| # resolution path picks once a project holds two connections for one provider. | ||
| key_attrs = { | ||
| "openai": "openai_key", | ||
| "anthropic": "anthropic_key", | ||
| "openrouter": "openrouter_key", | ||
| "cohere": "cohere_key", | ||
| "azure": "azure_key", | ||
| "groq": "groq_key", | ||
| } | ||
| bound: set = set() | ||
| for secret in secrets: | ||
| if secret.get("kind") != "provider_key": | ||
| continue | ||
| data = secret.get("data", {}) | ||
| kind = data.get("kind") | ||
| key = data.get("provider", {}).get("key") | ||
| if kind == "openai" and key: | ||
| litellm.openai_key = key | ||
| elif kind == "anthropic" and key: | ||
| litellm.anthropic_key = key | ||
| elif kind == "openrouter" and key: | ||
| litellm.openrouter_key = key | ||
| elif kind == "cohere" and key: | ||
| litellm.cohere_key = key | ||
| elif kind == "azure" and key: | ||
| litellm.azure_key = key | ||
| elif kind == "groq" and key: | ||
| litellm.groq_key = key | ||
| attr = key_attrs.get(kind) | ||
| if not attr or not key or kind in bound: | ||
| continue | ||
| setattr(litellm, attr, key) | ||
| bound.add(kind) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor connection in the llm_v0 fallback path.
Lines 3583-3605 bind one LiteLLM key per provider family before the loop. The loop then reads model but never reads llm_config["connection"]. If an LLM entry selects openai-2, this path still uses the first OpenAI credential. This can run a request on the wrong provider account.
Resolve provider settings for each LLM entry with its connection slug. Pass the resolved settings to the completion call. Add a test with two same-family connections and an LLM entry that sets connection.
There was a problem hiding this comment.
Confirmed major and accepted. Root cause of the seam: llm_v0 was deliberately left on the legacy per-family binding when the other three paths gained slug-first resolution, but a later contract fix declared connection on its entry schema, making the field writable while the handler ignores it: the wrong-account class this PR eliminates elsewhere. Fix: per-entry resolution through the shared slug-first resolver (SecretsManager), family fallback when no slug, with the two-same-family-connections test plus the slugless-entry determinism pin. Queued as a post-QA-gate pre-commit change together with the ProviderCredentials hardening (the tree is frozen mid-gate and this hot-reloads the service under test); both land in the next commit batch on this PR.
There was a problem hiding this comment.
@mmabrouk, thank you for the confirmation and the detailed root-cause analysis.
The planned fix addresses the finding. Per-entry SecretsManager resolution will ensure that llm_v0 uses the selected connection. The two-connection test and the slugless determinism test will cover both required behaviors.
I will keep this thread open until the commit is available for verification.
🐇
You are interacting with an AI system.
Railway Preview Environment
Updated at 2026-08-13T15:48:10.946Z |
|
I reviewed the current head,
Both issues are already acknowledged in their inline review threads, but the fixes are not present in the current head. I found no additional blocker in the static review. |
POST /providers/probe validates a credential and discovers models in one action while keeping the two results separate: a public catalog can refresh models but can never produce a key-valid claim, and a missing list endpoint is unsupported, not invalid. Adapters cover the standard API-key providers, OpenRouter's authenticated key check, Azure and Bedrock bearer paths, and OpenAI-compatible endpoints behind the shared egress guard with DNS pinning. Credentials are never persisted or logged; the vault's SecretSafeRoute keeps them out of 422 bodies.
… card The Settings tab becomes AI providers: a table of connections (provider, masked credential, active models, connected date) whose rows open the connection card directly, plus an Add provider button that opens the catalog drawer. The catalog lists every standard provider except the defunct Aleph Alpha, adds AWS Bedrock, Azure OpenAI, and Google Vertex AI as first-class rows with their own credential field sets, and ends with the OpenAI-compatible endpoint row. The card runs one Test action against the probe API and keeps its two results honest, pre-checks the default models and tags them, always accepts manual model IDs, saves a policy only when the user actually chose one, and never shows a modal or toast. The old Secrets settings surface is deleted; its custom provider form lives on for other consumers.
…onnection SecretsManager resolves a stored connection slug first, of either vault kind, and raises a clean 400 for an unknown slug instead of silently running on another key. Without a slug the old family mapping still applies, extended so a record whose saved model list contains the requested model wins over the bare family match. The vault middleware stops collapsing same-family records, which had made a second OpenAI connection unreachable on every prompt path. The prompt and judge model pickers show one group per named connection, suppress the ambiguous static family group when two or more connections claim it, and persist the slug beside the model in one dispatch. The prompt-side model catalog gains the current identifiers it was missing.
The picker's first level lists the project's connections and the two consumer subscriptions instead of provider families; the flyout lists each connection's models crossed with the harnesses that may drive them, with the harness as a tag and a cost hint when a model is reachable twice. A pick writes model, provider, connection slug, and harness in one dispatch. The drawer's playground context adds the Connected and Subscriptions sections around the shared catalog, and the closed pill goes dashed with Set up AI providers when nothing is connected. Custom connections are matched by their stored slug with a name fallback for legacy records, and the chat composer's /model picker shares the same rows.
One family-to-prefix table, mirrored in TypeScript and Python with cross-referencing drift tests, turns provider-native model ids into the spelling litellm routes. Pick-time translation keeps stored configs in litellm format, so pre-existing prompts are untouched by construction; the resolver keeps an idempotent safety net on both context paths and custom-provider records are structurally exempt, pinned by mutation- checked tests. The saved-models tiebreak compares normalized spellings so an explicit claim beats first-record. Unknown connection slugs fail as a clean 400 naming the candidates, never by running on another key.
From accepted review findings: ProviderCredentials.key becomes SecretStr and extras drops out of repr, so a stray log line or traceback cannot print a key; adapters unwrap at the single point of use and canary tests pin repr, str, and both dump forms. The llm_v0 fallback path stops pre-binding one key per family and resolves each entry through the shared slug-first resolver, so an entry naming a specific connection runs on that connection's key across the whole retry chain.
Three entry contexts with their exact titles and section sets, the catalog as the only scrolling region, folded connected rows with one composed subtitle, subscription rows as one pair per plan and harness derived from the runner's live status, the setup row and empty state verbatim from the spec, the pair card, and the connection card per the handoff: one secret line, green-dot result with fetch count, recommended tags, manual IDs always, drawer-body scrolling with the close control hard right and a truncation guard on long names. Saved policy fields are sent only when the user actually chose them.
One cascade component: search on top, connections left with counts and olive Subscription tags on rows named by plan, flyouts right with curated names, quiet default and cheapest hints, harness sections with logos and a via-header for single-harness connections, the ink harness tooltip, and one Manage model providers footer. The completion context strips every trace of harness. Uncataloged saved ids appear in the harness's own spelling, labelled exactly as the user typed them, and the closed pill resolves the same beautified label as the menu. Subscription rows consume the runner's live pairs with a static fallback, sharing one status query with the drawer. The tooltip link color becomes a real palette token.
The Fallback and Retry titles inherited the 16px body over 12px labels; they join the panel's own scale, and the header aligns with its tabs. One shared function now computes a no-list connection's effective models, so the settings table, the drawer count, and both pickers cannot disagree; the playground model row renders the resolved label instead of the raw stored id, with manual ids shown exactly as typed.
A hand-added model id absent from Pi's registry is registered into the per-run models.json with its base model's real metadata, so it runs as a first-class model; the operator's mounted Pi folder is never written. The Pi probe also names which provider logins the mount holds (OAuth entries only, family names only), the service passes them through a closed allow-list, and subscription pairs derive from the live answer, which is how a ChatGPT login in Pi becomes a visible pair.
The connect gate opens the providers drawer directly, and a connection created from onboarding auto-switches the agent to its first model and commits, leaving the user typing; a second connection added mid-session never touches the current pick. The dead duplicate gate hook is deleted rather than synced. The settings table matches its siblings (Provider and Name columns, Created, an actions menu), Preferences reads Feature flags with a Developer mode toggle describing exactly what it gates, the eval stepper's all-rows hover highlight is fixed at its CSS root, and the dev api gains the graceful-shutdown bound that ends the reload deadlock.
The onboarding auto-switch pulses the config section through the shared draft signal; the new origin keeps it out of the approval dock's Undo.
… and provider The public status proxy allow-listed state words and the `providers` list, but two runner-controlled strings still passed through untouched: the harness map's KEY, which becomes an object key in the browser's JSON, and the singular `provider`. A future or faulty runner could put an account name, a path, or a credential in either. Both are now closed sets. Keys must name a known harness (`HarnessKind`) and unknown ones are dropped, which also caps the map — a runner cannot push more entries than there are harnesses. `provider` gets the same treatment `providers` already had: a family the card cannot render is dropped rather than failing the entry, since the state word is still good. Both sets already match what the runner sends (its own SUBSCRIPTION_HARNESSES and its constant provider values), so this narrows the contract without changing any real response.
…el spelling Five review findings on the provider-connections surface: - The vault shadow filter compared provider kinds raw, so MISTRALAI_API_KEY survived next to a stored `mistral` connection and — locals coming first in the combined list — then won the resolver's tiebreak, beating the key the user saved. Both sides now normalize through a shared `normalize_provider_kind`, moved to `sdk/utils/providers.py` because the manager already imports the vault middleware and could not be imported back. - The family tiebreak fell through to the first record, which could be one the user narrowed away from the requested model while a later list-less record still offered it. A record with no saved list follows Agenta's defaults, so it now wins over a narrowed one; an explicit `[]` still means "offer nothing" and wins nothing. - The probe applied its 10s budget per httpx request, and OpenRouter issues two, so the worst case behind the button was double. The budget now covers the whole probe; running out of it maps to `unknown` + `failed`, which is what it honestly proved. - The playground picker offered a credential-set connection its bare saved slugs when it carried both spellings, but the SDK matches those connections on `model_keys` only, so the pick resolved to no provider settings. - `together_ai/moonshotai/Kimi-K2-Instruct` is gone from Together's serverless API and from the agent catalog; replaced with `Kimi-K2.6`, which the agent catalog carries. Plus a doc comment that stated the opposite of the save rule, and a catalog call moved inside its own try so an exception degrades to an empty set instead of failing module collection.
97994d0 to
5d4bd5d
Compare
59ab9a3 to
8f546a0
Compare
9b88dde
into
feat/runner-subscription-status
1 participant
Implements pull requests 2, 3, and 4 of the plan in #5987, as three commits on one branch. Stacked
on #5994 (runner subscription status), which stacks on #5995 (the vault connection contract). The
picker's subscription section reads the runner-status atoms, which is why this sits above #5994.
Context
The plan's target design gives Agenta one "AI providers" surface: a Settings page that manages
provider connections, a connection card with one honest Test action, a connection-first model
picker in the agent playground, and prompt, completion, chat, and LLM-as-a-judge runs that resolve
the exact connection the user picked. The vault contract for all of this landed in #5995; this
branch builds the product on top of it.
What changed, per commit
Commit 1: provider credential probe (
POST /providers/probe). One action validates acredential and discovers models while keeping two separate statuses, so a public catalog
(Perplexity, DeepInfra) can refresh models but can never produce a false "Key valid", and a
missing list endpoint reads "unsupported", never "invalid". Adapters cover the standard API-key
providers, OpenRouter's authenticated key check, Azure and Bedrock bearer paths, and
OpenAI-compatible endpoints behind the repo's egress guard with DNS pinning. Credentials are never
persisted or logged; the vault's SecretSafeRoute keeps them out of 422 bodies. Bedrock's SigV4 and
Vertex's token exchange are marked TODO rather than pulling boto3/google-auth into the API deps.
Commit 2: the AI providers settings experience. The Settings tab is renamed from "LLMs". A
connections table (provider, masked credential, active models, connected date) opens the
connection card directly from a row; "Add provider" opens the catalog drawer. The catalog drops
the defunct Aleph Alpha, adds AWS Bedrock, Azure OpenAI, and Google Vertex AI as first-class rows
whose cards render their real field sets, and ends with the OpenAI-compatible endpoint row. The
card pre-checks the default models (tagged "default"), always accepts manual model IDs, saves a
models/harnesses policy only when the user actually chose one (an untouched card keeps following
Agenta's defaults), and uses no modal and no toast anywhere. The old Secrets settings surface is
deleted.
Commit 3: prompt, completion, chat, and LLM-as-a-judge wiring.
SecretsManagerresolves astored connection slug first (either vault kind) and raises a clean 400 for an unknown slug
instead of silently running on another key; without a slug the legacy family mapping still
applies. The vault middleware stops collapsing same-family records, which had made a second OpenAI
connection unreachable on every prompt path. The prompt and judge pickers group models per named
connection, suppress the ambiguous static family group when several connections claim it, and
persist the slug beside the model in one dispatch.
Commit 4: the connection-first agent playground picker. Level 1 lists connections and
subscriptions; the flyout lists models crossed with the harnesses that may drive them, harness as
a tag, cost hint when a model is reachable twice. A pick writes model, provider, connection slug,
and harness in one update. The drawer gains its playground context (Connected, catalog,
Subscriptions, footer with count and "Manage in Settings"), and the closed pill goes dashed with
"Set up AI providers" on first run.
Scope / risk
lone connection per family behaves exactly as today, and deleting a referenced connection fails
loudly by design rather than falling back to a different key.
TODO. Regeneration is deferred until the series stabilizes.
isCloudsignal isreally
isEE, which would have hidden it exactly where mounted logins exist. A realcloud-versus-self-hosted signal is a known follow-up (same root cause as the pre-existing
"Unavailable in the cloud" badge on self-hosted EE).
catalog while the card counts against the live fetch, so the two totals can differ; the
Together AI brand icon is near-invisible in dark mode (pre-existing SVG); the prompt commit
diff shows the model change but not the connection change.
How to QA
Prerequisites: dev stack from this branch, a real OpenAI key, a logged-in user.
"OpenAI accepted this key.", a fetched model list with the three defaults pre-checked at the
top and tagged, "Active models: following Agenta defaults" until you touch the list. Done
saves; the table shows the masked key and counts.
free number).
key (401)." with Retry and Done disabled, no toast.
the second one; run. Expected: the run executes and the runner log line shows
connection=agenta:<slug-of-second-connection>.connection and run a completion. Expected 200, and the saved config carries
llm_config.connection.Expected: the judge executes and the stored parameters carry the connection slug.
connection and the known slugs, not a silent fallback.
Test commands:
cd api && uv run pytest oss/tests/pytest/unit/ -q(includes the 64 probe tests)cd sdks/python && uv run pytest ../../sdks/python/oss/tests/pytest/unit -q(2100 tests)cd web/packages/agenta-entities && ./node_modules/.bin/vitest run(1089 tests)cd web/packages/agenta-entity-ui && ./node_modules/.bin/vitest run(386 tests)Two full live-QA rounds ran against the dev stack: the first covered the settings flow, both
playgrounds, both themes, and key-leak scans (zero hits across all containers); the second
verified the judge binding end to end (a judge evaluation executed on a named connection),
the fetch-stable model ordering, the drawer section order, and the Azure/Vertex hint copy.
Addendum: the second day's commits (founder-driven iteration)
Nine further commits landed after live review by the founder, each individually reviewed, tested,
and live-QA'd; the final QA gate ran on a frozen tree with a GO verdict:
cross-referencing drift tests; pick-time translation keeps stored configs in LiteLLM format so
pre-existing prompts are untouched by construction; custom providers structurally exempt.
llm_v0resolution (accepted review findings).ChatGPT-via-Pi from the runner's provider map), the connection card per the handoff brief.
ids run via per-run Pi model registration and display exactly as typed.
renames, the eval-stepper CSS fix, and the dev API graceful-shutdown bound.
Deferred, recorded: picker clipping under 570px viewports; the misleading "add your key" message
when a provider rejects an existing credential; create-evaluator stacking under the evaluation
modal; screen-reader exposure of picker rows; a dark-mode clipped row in the model list; Fern
regeneration (#6012); slug backfill TOCTOU (#6015). Open naming call for the founder: the
Settings tab says "AI providers" while the drawer titles say "Model providers" per the final
handoff; both rulings honored literally, one word decides.