fix: support keyless openai-compatible endpoints - #5909
Conversation
7d5509d to
f2ea1b7
Compare
themiguelamador
left a comment
There was a problem hiding this comment.
I found four issues that need to be incorporated before this is ready:
- The edit dialog's new base-URL validity check ignores the persona env layer for an existing agent that already inherits its harness. Backend readiness accepts the inherited
OPENAI_COMPAT_BASE_URL, but the dialog computescompatibleBaseUrlInheritedfrom agent/global/file state only, so Save stays disabled. The fix recognizes persona inheritance while preserving an explicit local blank as a shadow. - The persona provider transition still uses the presence of a managed API-key field as proof that discovery requires a key. Selecting
openai-compatwith no key therefore clears an existing custom model even though this PR makes that key optional. The fix consults the provider's actual required-env-key contract. - Desktop readiness treats whitespace-only env values as present, while the runtime trims and rejects the compatible base URL. This reports an agent as ready even though it will fail at launch. The fix aligns Buzz Agent and Goose readiness/file checks with the runtime's blank-value semantics.
crates/buzz-agent/README.mdstill tells vLLM/llama.cpp/Ollama users to selectopenai, omitsopenai-compatfrom the provider list, and documents a default compatible URL rather than the new required explicit endpoint. The fix updates examples, the configuration table, provider matrix, and authentication behavior.
Verified fix: Complear/buzz branch review/pr-5909-fix, commit 9eaf3ac545dff4d1d5b0c6614c43a01326e42380.
Validation: full Desktop frontend suite (4,960 tests), Desktop TypeScript/static/file-size checks, focused Tauri readiness test, full buzz-agent suite, strict buzz-agent clippy, and Rust formatting all pass.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 combined feedback from two independent review passes. Both passes independently confirmed all four issues in the existing review (the inherited-URL gate in the edit dialog, the model-clearing on switch to openai-compat, the untrimmed readiness checks, and the stale buzz-agent README) — treating that convergence as strong confirmation those are real, so not re-litigating them here. The inline comments below are the additional findings that aren't on the PR yet. Also worth noting the branch is currently conflicting with main and needs a rebase.
| ), | ||
| "openai" if present_nonempty(openai_key) => Ok(Provider::OpenAi), | ||
| "openai" => Err("config: OPENAI_COMPAT_API_KEY required".into()), | ||
| "openai-compat" => Ok(Provider::OpenAiCompat), |
There was a problem hiding this comment.
🤖 I think this creates a silent break for existing records: on main, openai-compat resolves to Provider::OpenAi with the https://api.openai.com/v1 default, but after this change it hard-requires OPENAI_COMPAT_BASE_URL with no default. An agent saved today with provider=openai-compat + key and no base URL works, then flips to NotReady/startup-error after upgrade. There's precedent for boot migrations (the databricks v1→v2 rewrite) — either migrate openai-compat-without-URL records to openai, or call out the intentional break in the PR description.
| parse_openai_api(env("OPENAI_COMPAT_API").as_deref())?, | ||
| ), | ||
| Provider::OpenAiCompat => ( | ||
| env("OPENAI_COMPAT_API_KEY") |
There was a problem hiding this comment.
🤖 now that openai and openai-compat are distinct providers with distinct trust boundaries, they still share OPENAI_COMPAT_API_KEY. Global-defaults env merges into per-agent effective env, and the dialogs deliberately preserve credential env vars across provider switches — so a key configured for api.openai.com gets bearer-sent to whatever custom base URL a compat agent points at. It's pre-existing exposure, but this PR is the moment the providers split, so it feels like the right time to either give compat its own key var or scrub the inherited key unless it was set explicitly for the compat agent. If the shared var is intentional, a note on the accepted risk would do.
| } else { | ||
| base_url.unwrap_or_else(|| "https://api.openai.com/v1".to_string()) | ||
| }; | ||
| validate_openai_compat_base_url(&base_url)?; |
There was a problem hiding this comment.
🤖 small asymmetry: this now runs validate_openai_compat_base_url on the official openai path too, but runtime Provider::OpenAi still reads the URL via plain env_or with no validation. So discovery rejects a query/fragment URL that inference would happily use. Probably fine to validate both, but as written discovery fails where the runtime works.
| } | ||
|
|
||
| #[test] | ||
| fn optional_compat_key_allows_explicit_blank_to_shadow_process_env() { |
There was a problem hiding this comment.
🤖 nit: this test mutates process env with set_var without taking the env/path lock other tests in this crate use. The unique key name makes a collision unlikely, but it's inconsistent with the crate's own pattern for env-mutating tests.
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Clear provider-owned endpoint state when leaving openai-compatible configurations and reject base URLs with ambiguous or secret-bearing components. Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
bad9d97 to
e2ef27d
Compare
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz>
Summary
OpenAiCompatruntime provider with an explicit HTTP(S) endpoint and optional API keyValidation
buzz-agentconfiguration and request-header regression tests passedKnown unrelated failure
The full
buzz-agentpackage still has one MCP cancellation regression that reproduces on currentorigin/main; the provider-specific runtime tests pass. This PR does not modify that cancellation path.