Model-change disclosure ignores requested_model when resolved_model is absent
- Lenguaje dominante
- Rust
- Estrellas
- 54.2k
- Forks
- 6.2k
- Merge medio
- 3 d 2 h
- PR fusionados (30 d)
- 262
Descripción
**Describe the bug**
`Message.metadata.inference` is meant to record which model produced each assistant message. It is
built in `Agent::reply_internal` from the provider's `ModelInfo`:
```rust
// crates/goose/src/agents/agent.rs:1831
let inference = provider
.fetch_model_info(&requested_model)
.await
.ok()
.and_then(|model_info| model_info.resolved_model) // <-- gates the whole struct
.map(|resolved_model| InferenceMetadata {
provider: provider_name,
requested_model,
resolved_model: Some(resolved_model),
});
```
The `.and_then(|model_info| model_info.resolved_model)` discards the entire `InferenceMetadata`
whenever `resolved_model` is `None` — including the `provider` and `requested_model` fields, which
are always known and never depend on it.
`resolved_model` is `None` for almost every provider. The default `fetch_model_info` returns
`model_info_for_provider_model`, which hardcodes it:
```rust
// crates/goose-provider-types/src/base.rs:308
ModelInfo {
name: model_name.to_string(),
resolved_model: None,
...
}
```
Only `crates/goose/src/providers/databricks.rs` overrides `fetch_model_info` to populate it. So for
every other provider, `inference` is `None`, and the three consumers downstream
(`agent.rs:2054`, `:2059`, and the `with_inference_if_assistant` call at `:2658` that stamps the
messages actually written to the session store) are all no-ops.
Confirmed against a real `sessions.db`: all 1346 messages carry
`{"userVisible":true,"agentVisible":true}` and no `inference` key.
**Two things break as a result.**
1. **Per-message model attribution is lost.** The session-level `sessions.model_config_json` is not
a substitute: `Agent::update_provider` (`agent.rs:2744`) overwrites it in place on every model
switch, and the UI offers a model picker inside the chat composer, scoped to the current session.
So a conversation where the user switched models part-way through is recorded as having used a
single model — the last one — with no trace of the others. Anything reconstructed from the
session store (transcripts, exports, support bundles) inherits that.
`llm_request.*.jsonl` is not a fallback: `LOGS_TO_KEEP = 10`
(`crates/goose/src/providers/utils.rs:215`) keeps only the last ten requests process-wide.
2. **A finished UI feature never renders.** `ProgressiveMessageList` has a complete inline
"model changed from X to Y" disclosure — `getResolvedModel` / `getPreviousResolvedModel` /
`renderModelChangeDisclosure` (`ui/desktop/src/components/ProgressiveMessageList.tsx:98-127`),
rendered at `:269-282`. It reads `message.metadata.inference?.resolvedModel`, so it is
unreachable for every provider except Databricks. Users get no indication that a mid-conversation
model switch happened.
**To Reproduce**
1. Configure any provider that does not override `fetch_model_info` (i.e. anything but Databricks).
2. Start a chat and send a message.
3. Switch model using the picker in the chat composer.
4. Send another message.
Observed:
- No "model changed" disclosure appears between the two turns.
- Every message row in `sessions.db` has `metadata_json` without an `inference` key:
```sql
SELECT json_extract(metadata_json, '$.inference') FROM messages WHERE session_id = '';
-- all NULL
```
- `sessions.model_config_json` holds only the second model.
Expected: each assistant message records the model that served it, and the disclosure marks the
switch point.
**Proposed fix**
Two independent changes.
*1. Don't let a missing `resolved_model` discard the whole struct* (`crates/goose/src/agents/agent.rs`):
```rust
let resolved_model = provider
.fetch_model_info(&requested_model)
.await
.ok()
.and_then(|model_info| model_info.resolved_model);
let inference = Some(InferenceMetadata {
provider: provider_name,
requested_model,
resolved_model,
});
```
`resolved_model` stays `Option` — that field genuinely is provider-specific — but its absence no
longer costs the attribution. No new cost on the hot path: the default `fetch_model_info` is a local
canonical-registry lookup, and the call already happens today.
This leaves `inference` as an `Option` that is now always `Some`, so the three consumer sites need
no changes. Collapsing it to a plain `InferenceMetadata` and dropping those `if let Some(...)`
branches would be tidier; happy to do that instead, or as a follow-up, if maintainers prefer — it
was kept out of this change to keep the diff reviewable.
*2. Let the disclosure fall back to the requested model*
(`ui/desktop/src/components/ProgressiveMessageList.tsx`):
```ts
return (
message.metadata.inference?.resolvedModel ??
message.metadata.inference?.requestedModel ??
null
);
```
Providers reporting an upstream name keep their current behaviour; the rest now surface the
requested id, which `getModelDisplayName` already maps through the configured model list.
Change 2 depends on change 1 to have any data to read.
**Test**
A regression test is straightforward with the existing harness — `CountingTextProvider` in
`agent.rs`'s test module already inherits the default `fetch_model_info`, so driving one turn
through `create_test_agent` / `reply` and asserting that both the streamed and the persisted
assistant messages carry `metadata.inference.requested_model` fails before the change and passes
after.
**Goose version**
1.41.1
**Additional context**
Happy to open a PR with both changes plus the test if this looks right.
Guía de contribución
Evaluación
Este issue todavía no se ha evaluado.