Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions crates/buzz-agent/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
pub mod endpoint;

use std::time::Duration;

pub const PROTOCOL_VERSION: u32 = 2;
Expand Down Expand Up @@ -639,7 +641,6 @@ pub struct Config {

impl Config {
pub fn from_env() -> Result<Self, String> {
let databricks_host = env("DATABRICKS_HOST");
let databricks_model = env("DATABRICKS_MODEL");
let provider = resolve_provider(
env("BUZZ_AGENT_PROVIDER").as_deref(),
Expand All @@ -660,15 +661,14 @@ impl Config {
// Databricks borrows api_key as the *optional* `DATABRICKS_TOKEN` escape
// hatch — empty means "use OAuth PKCE." Legacy Databricks encodes the
// model in the URL path; Databricks v2 keeps it in the request body.
let (api_key, model, base_url, openai_api) = match provider {
let (api_key, model, openai_api) = match provider {
Provider::Anthropic => (
req("ANTHROPIC_API_KEY")?,
resolve_model(
buzz_agent_model.as_deref(),
env("ANTHROPIC_MODEL").as_deref(),
)
.ok_or_else(|| "config: ANTHROPIC_MODEL required".to_string())?,
env_or("ANTHROPIC_BASE_URL", "https://api.anthropic.com"),
OpenAiApi::Auto, // unused for Anthropic
),
Provider::OpenAi => (
Expand All @@ -678,14 +678,12 @@ impl Config {
env("OPENAI_COMPAT_MODEL").as_deref(),
)
.ok_or_else(|| "config: OPENAI_COMPAT_MODEL required".to_string())?,
env_or("OPENAI_COMPAT_BASE_URL", "https://api.openai.com/v1"),
parse_openai_api(env("OPENAI_COMPAT_API").as_deref())?,
),
Provider::Databricks | Provider::DatabricksV2 => (
env("DATABRICKS_TOKEN").unwrap_or_default(),
resolve_model(buzz_agent_model.as_deref(), databricks_model.as_deref())
.ok_or_else(|| "config: DATABRICKS_MODEL required".to_string())?,
databricks_host.ok_or_else(|| "config: DATABRICKS_HOST required".to_string())?,
OpenAiApi::Chat, // only read by OpenAI/legacy Databricks dispatch
),
Provider::OpenRouter => (
Expand All @@ -695,10 +693,10 @@ impl Config {
env("OPENROUTER_MODEL").as_deref(),
)
.ok_or_else(|| "config: OPENROUTER_MODEL required".to_string())?,
env_or("OPENROUTER_BASE_URL", "https://openrouter.ai/api/v1"),
OpenAiApi::Chat, // OpenRouter uses Chat Completions only
),
};
let base_url = endpoint::provider_base_url(&provider, env)?;
let system_prompt = match (env("BUZZ_AGENT_SYSTEM_PROMPT"), env("BUZZ_AGENT_SYSTEM_PROMPT_FILE")) {
(Some(_), Some(_)) => return Err(
"config: BUZZ_AGENT_SYSTEM_PROMPT and BUZZ_AGENT_SYSTEM_PROMPT_FILE are mutually exclusive".into()),
Expand Down
65 changes: 65 additions & 0 deletions crates/buzz-agent/src/config/endpoint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//! Endpoint resolution shared by agent startup and desktop security suggestions.
use super::Provider;

pub fn provider_base_url(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Document the public endpoint resolver

provider_base_url is newly exposed through the public config::endpoint module but has no Rustdoc describing its lookup contract, provider defaults, or the required Databricks host. Add a doc comment, or restrict its visibility if it is not intended for external launchers, to satisfy the repository requirement that every new public API be documented.

AGENTS.md reference: AGENTS.md:L193-L196

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overkill for a lookup?

provider: &Provider,
lookup: impl Fn(&str) -> Option<String>,
) -> Result<String, String> {
let (key, default) = match provider {
Provider::Anthropic => ("ANTHROPIC_BASE_URL", Some("https://api.anthropic.com")),
Provider::OpenAi => ("OPENAI_COMPAT_BASE_URL", Some("https://api.openai.com/v1")),
Provider::OpenRouter => ("OPENROUTER_BASE_URL", Some("https://openrouter.ai/api/v1")),
Provider::Databricks | Provider::DatabricksV2 => ("DATABRICKS_HOST", None),
};
lookup(key)
.or_else(|| default.map(str::to_owned))
.ok_or_else(|| format!("config: {key} required"))
}

/// Resolve without fetching credentials, signing in, or making network requests.
pub fn configured_endpoint(lookup: impl Fn(&str) -> Option<String>) -> Option<String> {
Comment on lines +19 to +20

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 I think this one needs a sentence or two of contract. My guess is that's why the bot keeps flagging it, though I agree full Rustdoc on provider_base_url would be overkill. Nothing in the repo calls configured_endpoint, and nothing in #7943 or #7944 does either. The module comment says desktop uses it, but I couldn't find a caller there. If the plan is for Sandpit to depend on buzz-agent as a crate, I'd say that here: it's for launchers, it returns the configured base URL (not a validated host or a full allowlist), overrides are returned as-is, and None means the provider is missing or unsupported, or it's Databricks with no host. The caller also has to pass the child's actual effective env.

Related: for an allowlist use case, OPENAI_COMPAT_BASE_URL="" coming back as Some("") is probably not what a launcher wants. I'd treat empty or whitespace-only values as unset here and leave from_env alone, since it already behaves that way on main.

let provider = super::resolve_provider(
lookup("BUZZ_AGENT_PROVIDER").as_deref(),
Some("configured"),
Some("configured"),
Some("configured"),
)
Comment on lines +21 to +26

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 The "configured" placeholders work because they happen to satisfy the three key-presence checks in resolve_provider, but that ties this function to resolve_provider's current parameters. If a provider later requires a different key, this would silently lie about it. I think it'd be cleaner to pull the name → Provider match out of resolve_provider into a small pure fn and have both call it.

.ok()?;
provider_base_url(&provider, lookup).ok()
}

#[cfg(test)]
mod tests {
use super::*;
#[test]
fn endpoints_follow_selected_provider_and_overrides() {
for (provider, expected) in [
("anthropic", "https://api.anthropic.com"),
("openai-compat", "https://api.openai.com/v1"),
("openrouter", "https://openrouter.ai/api/v1"),
] {
assert_eq!(
configured_endpoint(|key| (key == "BUZZ_AGENT_PROVIDER").then(|| provider.into()))
.as_deref(),
Some(expected)
);
}
assert_eq!(
configured_endpoint(|key| match key {
"BUZZ_AGENT_PROVIDER" => Some("databricks_v2".into()),
"DATABRICKS_HOST" => Some("https://workspace.example.com".into()),
_ => None,
})
.as_deref(),
Some("https://workspace.example.com")
);
assert!(configured_endpoint(|_| None).is_none());
assert_eq!(
provider_base_url(&Provider::OpenAi, |_| Some(
"http://localhost:1234/v1".into()
))
.unwrap(),
"http://localhost:1234/v1"
);
Comment on lines +57 to +63

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This lookup returns the custom URL for any key, so if OpenAI's override key accidentally became ANTHROPIC_BASE_URL this would still pass. Could this use a key-sensitive lookup, ideally with a different override set for each provider at once, and assert that only the selected provider's value wins? A table-driven test would also make it easy to add a couple of cases that aren't checked today: Databricks with no DATABRICKS_HOST (should be None), and a blank or unsupported BUZZ_AGENT_PROVIDER.

}
}
Loading