aaif-goose / aaif-goose/goose

Model-change disclosure ignores requested_model when resolved_model is absent

Abierto
#11,041 2 comentarios 0 reacciones 1 asignado Reclamado por @jbg Ver en GitHub
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

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.