fix(web): providers page reported a missing API key for every provider
Nightly Build / build (push) Successful in 4m35s

The card tested `p.api_key` on a DTO that has never carried it, so the badge
was falsy for every provider and always read "API key missing".

The list and the new detail DTO now expose `has_api_key: bool` — the key
value itself never reaches the browser, where the edit form used to prefill
it in plain text. Since the form can no longer send the stored key back, an
empty `api_key` on update means "keep the one on file" instead of erasing it,
which is what the field's placeholder already promised.
This commit is contained in:
Daniele
2026-09-01 21:01:45 +01:00
parent c1a8227e11
commit 65e0f24326
6 changed files with 69 additions and 8 deletions
+12
View File
@@ -8,6 +8,18 @@ release PR may merge — and a section is closed at the commit that bumps it.
## [Unreleased]
### Fixed
- The **Providers** page said *API key missing* on every provider, including the ones
with a perfectly good key. It now reports the real state. Editing a provider no longer
shows the saved key in the form either — leave the field blank and the existing key is
kept, type a new one to replace it.
### Security
- An LLM provider's API key is never sent to the browser any more: the provider list and
the edit form receive only whether a key is stored, not its value.
## [0.3.0] - 2026-08-24
### Added
+1
View File
@@ -314,6 +314,7 @@ impl LlmManager {
provider: p.provider.clone(),
base_url: p.base_url.clone(),
description: p.description.clone(),
has_api_key: p.api_key.as_deref().is_some_and(|k| !k.trim().is_empty()),
supported_types,
}
}).collect()
+4 -1
View File
@@ -75,7 +75,8 @@ pub fn dtl_mode_from_format(fmt: &str) -> DtlMode {
// ── Provider ──────────────────────────────────────────────────────────────────
/// Public provider metadata (no api_key).
/// Public provider metadata. The api_key itself never leaves the server: the UI
/// only needs to know **whether** one is stored, so this carries a boolean.
#[derive(Debug, Clone, serde::Serialize)]
pub struct LlmProviderInfo {
pub id: i64,
@@ -84,6 +85,8 @@ pub struct LlmProviderInfo {
pub provider: String,
pub base_url: Option<String>,
pub description: Option<String>,
/// True when a non-empty api_key is stored for this provider.
pub has_api_key: bool,
/// Service types this provider supports (from ProviderRegistry at runtime).
pub supported_types: Vec<ServiceType>,
}
+8
View File
@@ -10,6 +10,14 @@
LLM client abstraction (OpenAI-compat, Anthropic, Ollama…). OpenAI-compatible provider *types* are runtime data, not code: `providers/declared.rs` loads `providers.yaml` at boot (see Config in [../CLAUDE.md](../CLAUDE.md)); only non-OpenAI-compatible or bespoke providers (anthropic, ollama, openai, openrouter) stay native. **Retriability** (`Model::is_retriable`, `agent-loop`) keys on the real HTTP status carried by `ModelError { status }`, **not** a substring of the message — a model id/token count containing "404"/"401" cannot mis-classify; 401/403/404/422 don't retry, 400/429/5xx/network do. **Request logging** is the `logging.rs::LoggingModel` decorator, attached by the *caller's* `ModelSelector` (`loop_adapters/selector.rs::SkaldSelector::with_log`) — never by `LlmManager`, which builds one shared client per model and cannot know whose traffic it serves. The decorator's `RequestLogTarget` carries the owner: metadata → `llm_requests` in the registry (`user_id`, the column the UI filters on), payload bodies/headers → `llm_request_payloads` in that user's own encrypted DB, keyed by `request_id`; session + frame come from the request's own `conversation`/`frame`, so kernel rounds, sub-agent frames and compaction summaries are all attributed with no extra plumbing (`ModelRequest::log` is unused here)
## The provider API surface — the key is a boolean, never a value
**No provider endpoint ever returns a stored `api_key`, and the trap is that omitting it silently reads as "no key".** `LlmProviderInfo` (list) and `ProviderDetail` (`src/frontend/api/llm.rs`, the detail DTO — deliberately *not* `LlmProviderRecord`, which does carry the secret) both expose **`has_api_key: bool`** instead. That is the whole contract: the UI needs to know *whether* a key is on file, never what it is, and the browser is where a leaked key would end up in a devtools tab or a screenshot.
It shipped broken in exactly the way this shape invites: `list_providers_info` never carried `api_key` (correctly), while the card tested `Boolean(p.api_key)` — always `undefined` — so every provider was badged "API key missing" even with a working key. A missing field is falsy, not an error; nothing logs, nothing fails to build. If you add a provider surface, read `has_api_key`, and if you add a field to either DTO, keep the secret out by construction rather than by remembering to strip it.
The consequence on the write path is load-bearing: since the edit form can no longer prefill the key, **an empty `api_key` in the `PUT` payload means "keep the stored one"**`update_provider` re-reads the record and carries the old value over, because a blind `UPDATE … SET api_key=NULL` would wipe a working provider on any unrelated edit (a renamed description). The i18n placeholder (`providers.modal.api_key_ph`) already promised this behaviour before the backend implemented it. Side effect to know about: there is no longer a way to *clear* a key from the form — deleting the provider is the escape hatch.
## Token streaming & reasoning display
The chat streams tokens live, as a **parallel best-effort side-channel** that never alters the turn's authoritative flow: the final `Done` (or `Thinking`) event still carries the complete content and the frontend treats it as truth.
+39 -3
View File
@@ -98,12 +98,43 @@ pub async fn create_provider(
Ok(StatusCode::CREATED)
}
/// One provider, as the edit form sees it. Deliberately **not** `LlmProviderRecord`:
/// the stored api_key never travels to the browser — the form only needs to know
/// whether one exists, so it can offer "leave blank to keep it".
#[derive(Serialize)]
pub struct ProviderDetail {
pub id: i64,
pub name: String,
#[serde(rename = "type")]
pub provider: String,
pub has_api_key: bool,
pub base_url: Option<String>,
pub description: Option<String>,
}
impl From<LlmProviderRecord> for ProviderDetail {
fn from(r: LlmProviderRecord) -> Self {
ProviderDetail {
id: r.id,
name: r.name,
provider: r.provider,
has_api_key: has_key(&r.api_key),
base_url: r.base_url,
description: r.description,
}
}
}
fn has_key(key: &Option<String>) -> bool {
key.as_deref().is_some_and(|k| !k.trim().is_empty())
}
pub async fn get_provider(
State(skald): State<Arc<Skald>>,
axum::extract::Path(id): axum::extract::Path<i64>,
) -> Result<Json<LlmProviderRecord>, ApiError> {
) -> Result<Json<ProviderDetail>, ApiError> {
skald.llm_manager().get_provider(id).await
.map(Json)
.map(|r| Json(ProviderDetail::from(r)))
.ok_or_else(|| ApiError::not_found(format!("provider {id} not found")))
}
@@ -113,7 +144,12 @@ pub async fn update_provider(
Json(payload): Json<ProviderPayload>,
) -> Result<StatusCode, ApiError> {
validate_provider_type(&skald, &payload.provider)?;
let record = LlmProviderRecord::from(payload);
let mut record = LlmProviderRecord::from(payload);
// The form never receives the stored key, so it cannot send it back: an empty
// api_key means "keep the one on file", not "erase it".
if !has_key(&record.api_key) {
record.api_key = skald.llm_manager().get_provider(id).await.and_then(|r| r.api_key);
}
skald.llm_manager().update_provider(id, record).await?;
Ok(StatusCode::NO_CONTENT)
}
+5 -4
View File
@@ -98,14 +98,15 @@ export class LlmProvidersPage extends LightElement {
const res = await fetch(`/api/llm/providers/${provider.id}`);
if (!res.ok) throw new Error(`HTTP ${res.status}`);
const record = await res.json();
// The server never sends the stored key back — an empty box means "keep it".
this._form = {
name: record.name,
type: record.type,
api_key: record.api_key ?? '',
api_key: '',
base_url: record.base_url ?? '',
description: record.description ?? '',
};
this._modal = { mode: 'edit', id: record.id };
this._modal = { mode: 'edit', id: record.id, hasKey: Boolean(record.has_api_key) };
} catch (e) {
this._error = e.message;
}
@@ -172,7 +173,7 @@ export class LlmProvidersPage extends LightElement {
const icon = meta.icon;
const label = meta.display_name;
const count = this._modelCounts[String(p.id)];
const hasKey = Boolean(p.api_key);
const hasKey = Boolean(p.has_api_key);
const needsUrl = meta.fields.some(f => f.key === 'base_url');
return html`
@@ -268,7 +269,7 @@ export class LlmProvidersPage extends LightElement {
<label class="form-label fw-semibold" style="font-size:0.82rem">${t('providers.modal.api_key')}</label>
<input type="password" class="form-control form-control-sm" .value=${f.api_key}
autocomplete="new-password"
placeholder=${isEdit ? t('providers.modal.api_key_ph') : ''}
placeholder=${isEdit && this._modal?.hasKey ? t('providers.modal.api_key_ph') : ''}
@input=${(e) => this._setField('api_key', e.target.value)} />
</div>
` : ''}