feat(dashboard): show only the credential fields each provider type uses - #591
Conversation
The provider add/edit modal offered every field the wire format has, so an
OpenAI provider asked for Vertex project and service-account JSON, and the
only feedback on a bad save was one banner at the top of the form.
Provider registrations already know what they read, so the form is now built
from that. `DiscoveryConfig` gains `CredentialFields`; most types derive their
form from the existing discovery flags, and only the four that do not fit the
"API key against one endpoint" shape declare one. `GET
/admin/provider-credentials/types` returns those forms alongside each type
(`[]string` -> `[{type, default_base_url, fields[]}]`), and the dashboard
renders exactly them: Vertex shows no API key field, Azure marks the base URL
required, Ollama leads with the endpoint, enumerated fields become selects.
Upserts are validated against the same forms before anything is persisted.
Previously an unusable credential was written to the store and then reported
as "saved but not applied", leaving a broken row behind; it is now rejected
with a 400 whose `error.param` names the offending field, and the dashboard
attaches the message to that input, opening the advanced section and focusing
it when needed.
A field's `options` are the values a form should offer, not a whitelist: the
providers accept further spellings of each (`openai_compatible`, `compat`,
`adc`) and the docs use them, so the admin API must not be stricter than the
config.yaml and env vars it replaces. A stored value outside the list joins the
select rather than rendering blank, and a stored value the form does not render
at all is sent back untouched, so no schema gap can silently clear a working
setting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughProvider discovery now exposes credential schemas used by backend validation, structured admin API responses, and schema-driven dashboard forms. Upserts return field-specific errors before persistence, while provider registrations describe provider-specific fields and authentication options. ChangesProvider credential schema flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant AdminAPI
participant CredentialsService
participant ProviderFactory
participant CredentialStore
Dashboard->>AdminAPI: Request provider credential types
AdminAPI->>CredentialsService: Load credential schemas
CredentialsService->>ProviderFactory: Read discovery schemas
ProviderFactory-->>AdminAPI: Return structured provider fields
Dashboard->>AdminAPI: Submit provider credential
AdminAPI->>CredentialsService: Validate and upsert credential
CredentialsService->>CredentialStore: Persist validated row
CredentialStore-->>Dashboard: Return success or field-specific error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/credential_schema_test.go`:
- Around line 31-116: Convert the credential-schema tests, especially
TestCredentialSchemas_DerivesTheFormFromDiscoveryFlags and
TestCredentialSchemas_UsesTheRegistrationsDeclaredForm, into table-driven
coverage. Add cases for default configuration plus Bedrock, Mantle, Gemini, and
Vertex registration field mappings and options, asserting each generated schema
matches the admin contract and declared ordering. Reuse shared field and schema
assertions so registration metadata drift is detected consistently.
In `@web/dashboard/src/pages/providers-config/providersConfig.svelte.js`:
- Around line 174-181: Update selectType to clear values from the previous
provider type that are not rendered by the newly selected type, while preserving
fields needed for edit-mode value retention. Ensure stale values such as
vertex_project are removed before buildProviderCredentialPayload submits the
form, without clearing fields currently present in this.formFields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8d3379a-e264-4538-a323-be22a6c8b41e
⛔ Files ignored due to path filters (5)
internal/admin/dashboard/static/dist/assets/index-BSE-2hNO.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-BgPgxKXk.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-jpXFjnTT.cssis excluded by!**/dist/**internal/admin/dashboard/static/dist/assets/index-nDWTc8Kq.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (22)
CLAUDE.mdcmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/handler_provider_credentials.gointernal/admin/handler_provider_credentials_test.gointernal/providers/bedrock/bedrock.gointernal/providers/bedrockmantle/bedrock_mantle.gointernal/providers/credential_schema.gointernal/providers/credential_schema_test.gointernal/providers/credential_validate.gointernal/providers/credential_validate_test.gointernal/providers/credentials.gointernal/providers/factory.gointernal/providers/gemini/gemini.gointernal/providers/vertex/vertex.gotests/e2e/release-e2e-scenarios.mdweb/dashboard/src/pages/providers-config/ProviderCredentialEditor.svelteweb/dashboard/src/pages/providers-config/ProviderCredentialField.svelteweb/dashboard/src/pages/providers-config/providersConfig.svelte.jsweb/dashboard/src/pages/providers-config/providersConfigLogic.jsweb/dashboard/src/styles/dashboard.cssweb/dashboard/tests/providers-config.test.js
Confidence Score: 3/5This PR should not merge until Gemini save-time validation matches its runtime backend and authentication requirements. Gemini credentials can pass the new pre-persistence validation even though the provider later rejects them, leaving operators with a successfully saved but unusable provider. Files Needing Attention: internal/providers/credential_validate.go, internal/providers/gemini/gemini.go
 T-Rex LogsWhat T-Rex did
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
R[Provider registration] --> S[Credential schema]
S --> A[Admin types endpoint]
A --> D[Dashboard form]
D --> P[Credential PUT]
P --> V[Save-time validation]
V --> B[Provider construction]
B --> T[Credential persistence]
T --> G[Live registry]
Reviews (1): Last reviewed commit: "feat(dashboard): show only the credentia..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if raw := cred.toRawProviderConfig(); isVertexProviderConfig(raw) { | ||
| return validateGoogleCredential(cred) | ||
| } |
There was a problem hiding this comment.
When a Gemini credential omits backend, this check treats it as non-Vertex and the optional schema key passes validation, while runtime normalization defaults to AI Studio or infers Vertex from project fields. This allows configurations that later fail because AI Studio has no API key or Vertex is configured with API-key authentication.
Artifacts
- Evidence file captured while the check ran.
- The full command output behind this check.
There was a problem hiding this comment.
Good catch — fixed in 20a3139. The divergence is real: normalizeGeminiBackend infers Vertex from vertex_project/vertex_location alone, while isVertexProviderConfig (resolution) only recognizes an explicit backend.
One correction to the finding: such a row was not persisted. CredentialsService.Upsert builds the adapter before writing, so it was already rejected — but resolution blamed api_keys, telling an operator who deliberately configured Vertex to supply a key they do not have.
Validation now names the field that actually resolves the ambiguity:
PUT {"name":"g1","type":"gemini","vertex_project":"p","vertex_location":"us-central1"}
400 param: backend
set backend to "vertex" to authenticate with the Vertex project and location, or provide an API key
Adding "backend":"vertex" is accepted (200) and hands the row to the Google rules, which it satisfies. A row that carries an API key is left alone: it resolves and runs, and the adapter's own inference takes it from there.
The check keys off the schema (backend offered with a vertex option) rather than the type name, so it does not fire for the vertex type, which has no backend to choose. Covered by TestValidateCredential_VertexFieldsNeedAnExplicitBackend.
I deliberately did not change isVertexProviderConfig to mirror the adapter's inference: that predicate is shared with startup resolution for config.yaml/env providers, so widening it would change which declarative providers register — out of scope here and worth its own change.
The payload echoes back values the form does not render, so that editing a stored row cannot silently clear a setting its type's form has no field for. In the create form that same rule leaked: a value typed under one type — say a Vertex project entered while gemini was selected — stayed in the form after switching to openai and was submitted with it, invisible to the operator. If the gateway rejected it, `error.param` named a field that is not on screen and the message had nowhere to land. Changing type now resets every value the new type's form does not render. Only creating clears: an existing provider's type is immutable, so the echo still protects stored values there. Also pin the shipped registrations' credential forms in a table-driven test. It fails if a registration stops declaring a field its adapter reads — the regression that removed Vertex's api_mode from the form in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Gemini adapter infers the Vertex backend from vertex_project/vertex_location alone, but credential resolution only recognizes an explicit `backend` and so still demands an API key. A row with the Vertex fields filled in and no backend was therefore rejected for a missing key — asking the operator to produce a credential they deliberately were not using. Validation now names `backend` for that shape: set it to vertex, or provide a key. A row that carries a key is left alone, since it resolves and runs with the adapter's own inference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@run/providers_test.go`:
- Around line 106-111: Update the required-field assertions in the schema test
loop around schema.Field(name) to validate both directions: ensure fields listed
in tt.required are required and ensure fields not listed are not required.
Mirror the symmetric per-field checks used by
TestCredentialSchemas_DerivesTheFormFromDiscoveryFlags, including cases such as
openai base_url.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 39630332-b789-4d18-a25b-01a7c2b0f03b
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-BMU0Aq5G.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (5)
internal/providers/credential_schema_test.gorun/providers_test.goweb/dashboard/src/pages/providers-config/providersConfig.svelte.jsweb/dashboard/src/pages/providers-config/providersConfigLogic.jsweb/dashboard/tests/providers-config.test.js
| for _, name := range tt.required { | ||
| field, found := schema.Field(name) | ||
| if !found || !field.Required { | ||
| t.Errorf("%s.Required = %v (present=%v), want true", name, field.Required, found) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Required check only verifies the positive case — add the negative assertion.
This only fails if a listed field is not required; it won't catch a field incorrectly becoming required (e.g. base_url on openai). The sibling test in credential_schema_test.go (TestCredentialSchemas_DerivesTheFormFromDiscoveryFlags) already checks both directions for every field — mirror that here for symmetric coverage.
♻️ Proposed fix
- for _, name := range tt.required {
- field, found := schema.Field(name)
- if !found || !field.Required {
- t.Errorf("%s.Required = %v (present=%v), want true", name, field.Required, found)
- }
- }
+ for _, field := range schema.Fields {
+ want := slices.Contains(tt.required, field.Name)
+ if field.Required != want {
+ t.Errorf("%s.Required = %v, want %v", field.Name, field.Required, want)
+ }
+ }📝 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.
| for _, name := range tt.required { | |
| field, found := schema.Field(name) | |
| if !found || !field.Required { | |
| t.Errorf("%s.Required = %v (present=%v), want true", name, field.Required, found) | |
| } | |
| } | |
| for _, field := range schema.Fields { | |
| want := slices.Contains(tt.required, field.Name) | |
| if field.Required != want { | |
| t.Errorf("%s.Required = %v, want %v", field.Name, field.Required, want) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run/providers_test.go` around lines 106 - 111, Update the required-field
assertions in the schema test loop around schema.Field(name) to validate both
directions: ensure fields listed in tt.required are required and ensure fields
not listed are not required. Mirror the symmetric per-field checks used by
TestCredentialSchemas_DerivesTheFormFromDiscoveryFlags, including cases such as
openai base_url.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
What
The provider add/edit modal showed every field the wire format has, whatever the type: an OpenAI provider asked for Vertex project, GCP scope and service-account JSON. The only feedback on a bad save was a single banner at the top of the form.
Provider registrations already know which fields they read, so the form is now built from that knowledge instead of duplicating it.
openaiazureollama/vllmbedrockvertexgeminiHow
providers.DiscoveryConfiggainsCredentialFields. Most types derive their form from the flags already there (AllowAPIKeyless,RequireBaseURL,SupportsAPIVersion); only the four that do not fit the "API key against one endpoint" shape declare one. Deriving from the same struct that drives env/YAML resolution means the form and the resolver cannot disagree about what a type needs.GET /admin/provider-credentials/typesnow returns those forms alongside each type. Admin API shape change:[]string→[{type, default_base_url, fields[{name, required, advanced, options}]}]. The dashboard is the only consumer; the S196 e2e assertion and swagger are updated.field.nameis the same string in the schema, the PUT payload, and an error'sparam— that is what lets a server-side rejection land on the input that caused it.Error handling
Validation now runs before anything is persisted. Previously an unusable credential was written to the store and then reported as
provider "x" was saved but not applied, leaving a broken row behind. It is now rejected with a 400 whoseerror.paramnames the field, and nothing is stored:The dashboard attaches the message to that input, opens the Advanced section if the field lives there, and focuses it. The client validates only what the schema can decide (required fields, blank key rows, duplicate/slashed names, malformed service-account JSON, a scheme-less base URL); cross-field provider rules — project-or-base-URL, service-account-when-
gcp_service_account,api_keynot authenticating against Vertex — stay server-side so provider knowledge is not duplicated in JS.Not a whitelist
optionsare the values a form should offer, not the values the API accepts. Providers accept further spellings of each (openai_compatible,compat,adc,service_account) and the docs andconfig.example.yamluse them, so rejecting those would make the admin API stricter than theconfig.yamland env vars it exists to replace. Two consequences, both covered by tests:That last rule caught a real one in review: Vertex honours
api_mode(via the Gemini adapter it delegates translation to —VERTEX_API_MODEis documented), and the first draft of its form omitted it.Verified
go test ./...,golangci-lint, 354 dashboard tests,svelte-check, and the full pre-commit suite (race, dist-sync, perf guard) are green. Also driven against a running gateway: each type renders its own fields, a blank required key and a scheme-less base URL are caught locally, a Vertex-backed Gemini without a project comes back from the server ontovertex_project,openai_compatible/adcround-trip untouched, and create/list/edit works end to end.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
MCP_ENABLED.Bug Fixes
error.paramerrors and prevent invalid data from being saved.Documentation