fix(providers): preserve image references and fail open on unknown vision support - #11548

@codex review

@codex review

@codex review

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: b24b3e3b9d

🤖 Reviewed at b24b3e3b9. CI fully green (all 17 required checks pass). The core of this PR is warranted and the fail-open flip is correct — but there's one clobbering bug in the new Databricks helper that I'd fix before merge.

Blocking: with_resolved_vision_support destroys an explicit capability when the alias resolves to an unknown upstream

crates/goose-providers/src/databricks.rs:190-198:

if config.supports_vision.is_none() || effective_model_name != model_config.model_name {
    config.supports_vision = Self::model_supports_vision(effective_model_name);
}

model_supports_vision returns None for anything absent from the canonical registry. So when the endpoint is an alias (effective_model_name != model_config.model_name) and the resolved upstream is not in the catalog, an explicit Some(false) is overwritten with None — the user's deliberate setting is destroyed rather than refined by better data.

Combined with the fail-open in this same PR, that Some(false) case is a live regression: an endpoint explicitly marked text-only now yields None → unwrap_or(true) → images get sent → provider 400. Pre-PR that config correctly stripped the image.

I proved it by compiling this against your head:

let alias = ModelConfig::new("team-chat").with_vision_support(false);
assert_eq!(alias.supports_vision, Some(false));
let resolved =
    DatabricksProvider::with_resolved_vision_support(&alias, "some-unlisted-endpoint");
assert_eq!(resolved.supports_vision, None); // passes — explicit Some(false) destroyed

Only overwrite when the lookup actually yields something. This keeps resolved-upstream precedence (both existing assertions in resolved_upstream_capability_takes_precedence_over_alias_canonicalization still hold) but never downgrades a known value to unknown:

if let Some(v) = Self::model_supports_vision(effective_model_name) {
    config.supports_vision = Some(v);
}

The new test at databricks.rs:840-848 only covers alias→known upstream, so this path is uncovered — worth adding the alias→unknown case with an explicit setting.

Non-blocking: rehydrate_canonical_capabilities no-ops when provider_name is None

crates/goose/src/model_config.rs:51-59 returns the config untouched when provider_name is None. The three agent-side callers pass session.provider_name.as_deref() (agents/agent.rs:1013, permission/permission_judge.rs:20, security/adversary_inspector.rs:23) — None for exactly the older/partially-initialised sessions this was added to repair. Such a session on openai/o3-mini keeps supports_vision: None, the gate now yields true, and the image is sent to a model that can't take it — an error that didn't happen pre-PR.

Note session/session_manager.rs:609 passes the live provider.get_name() instead. Using the live provider name (or falling back to Config::global().get_goose_provider()) at all four sites would close the hole and make the four call sites consistent.

Also worth a note

rehydrate_canonical_capabilities delegates straight to apply_canonical_limits, which also back-fills context_limit, max_tokens and reasoning, not just capabilities. A session that deliberately left context_limit unset for dynamic resolution now gets a pre-baked canonical value before provider.get_context_limit() at agent.rs:1834. Probably fine, but it's wider than the name suggests and isn't covered by the new test.

Confirmed good: the mime-type placeholders are consistent across all three formatters; all five gate sites were flipped (no stragglers); azure_foundry is correctly skipped; with_canonical_limits only writes into None fields so it can't clobber explicit settings; and the agent.rs refactor to model_config_for_session is behaviour-preserving with character-identical error strings.

All three points addressed in 5dcca42:

Blocking — databricks clobber: with_resolved_vision_support now only overwrites when the canonical lookup yields a value (if let Some(v) = model_supports_vision(...)), so an explicit Some(false) on an alias that resolves to an unlisted upstream survives instead of being downgraded to None → fail-open. Added the uncovered case to the test: team-chat with Some(false) resolving to some-unlisted-endpoint stays Some(false). Both precedence assertions still hold.

Non-blocking — provider_name None: rehydrate_canonical_defaults (renamed from ...capabilities since it delegates to apply_canonical_limits and back-fills context_limit/max_tokens/reasoning too) now falls back to Config::global().get_goose_provider() when the caller passes None, closing the hole for older/partially-initialised sessions at all four call sites.

Note — wider than the name: covered by the rename above + the model_config_for_session_rehydrates_legacy_capabilities test now asserts context_limit() == 128_000 (gpt-4o), documenting that canonical defaults beyond capabilities are back-filled.

@codex review

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: c32a6f3d22