diff --git a/crates/forge_config/src/config.rs b/crates/forge_config/src/config.rs index 7ccd5d513a..a289244189 100644 --- a/crates/forge_config/src/config.rs +++ b/crates/forge_config/src/config.rs @@ -57,6 +57,10 @@ pub struct ProviderUrlParam { /// UI. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub options: Vec, + /// Whether this parameter is optional. When `true`, the parameter may be + /// left blank without causing an error. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub optional: bool, } /// Source of models for a provider: either a URL to fetch them from or a diff --git a/crates/forge_domain/src/auth/new_types.rs b/crates/forge_domain/src/auth/new_types.rs index 579bf2b0ba..3968222eb9 100644 --- a/crates/forge_domain/src/auth/new_types.rs +++ b/crates/forge_domain/src/auth/new_types.rs @@ -83,6 +83,8 @@ pub struct URLParamValue(String); /// /// When `options` is `Some`, the UI presents a dropdown for selection. /// When `options` is `None`, the UI presents a free-text input. +/// When `optional` is `true`, the parameter may be left blank and missing +/// values are silently ignored during credential creation and URL rendering. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct URLParamSpec { /// The parameter name used as the template variable and credential map key. @@ -90,18 +92,27 @@ pub struct URLParamSpec { /// Optional list of allowed values. When present, the UI renders a /// dropdown. pub options: Option>, + /// Whether this parameter is optional. When `true`, the parameter may be + /// left blank without causing an error. + #[serde(default)] + pub optional: bool, } impl URLParamSpec { /// Creates a `URLParamSpec` with only a name, rendering as a free-text /// input. pub fn new(name: impl Into) -> Self { - Self { name: name.into(), options: None } + Self { name: name.into(), options: None, optional: false } } /// Creates a `URLParamSpec` with preset options, rendering as a dropdown. pub fn with_options(name: impl Into, options: Vec) -> Self { - Self { name: name.into(), options: Some(options) } + Self { name: name.into(), options: Some(options), optional: false } + } + + /// Creates an optional `URLParamSpec` that may be left blank. + pub fn optional(name: impl Into) -> Self { + Self { name: name.into(), options: None, optional: true } } } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 6e1ddda394..9ecd50fc41 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -3003,8 +3003,13 @@ impl A + Send + Sync> UI .prompt()? .context("Parameter selection cancelled")? } else { - // Free-text path (existing behavior) - let mut input = ForgeWidget::input(format!("Enter {}", param.name)); + // Free-text path + let label = if param.optional { + format!("Enter {} (optional, press Enter to skip)", param.name) + } else { + format!("Enter {}", param.name) + }; + let mut input = ForgeWidget::input(label); // Add default value if it exists in the credential if let Some(params) = existing_url_params @@ -3013,13 +3018,19 @@ impl A + Send + Sync> UI input = input.with_default(default_value.as_str()); } + if param.optional { + input = input.allow_empty(true); + } + let param_value = input.prompt()?.context("Parameter input cancelled")?; - anyhow::ensure!( - !param_value.trim().is_empty(), - "{} cannot be empty", - param.name - ); + if !param.optional { + anyhow::ensure!( + !param_value.trim().is_empty(), + "{} cannot be empty", + param.name + ); + } param_value.trim_end_matches('/').to_string() }; diff --git a/crates/forge_repo/src/provider/provider.json b/crates/forge_repo/src/provider/provider.json index 32ab9592cc..c445af2d5b 100644 --- a/crates/forge_repo/src/provider/provider.json +++ b/crates/forge_repo/src/provider/provider.json @@ -1228,11 +1228,11 @@ "url_param_vars": [ {"name": "VLLM_SSL_SCHEME", "options": ["http", "https"]}, "VLLM_HOST", - "VLLM_PORT" + {"name": "VLLM_PORT", "optional": true} ], "response_type": "OpenAI", - "url": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}:{{VLLM_PORT}}/v1/chat/completions", - "models": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}:{{VLLM_PORT}}/v1/models", + "url": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}{{#if VLLM_PORT}}:{{VLLM_PORT}}{{/if}}/v1/chat/completions", + "models": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}{{#if VLLM_PORT}}:{{VLLM_PORT}}{{/if}}/v1/models", "auth_methods": ["api_key"] }, { diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index b9e762b43c..81387c7027 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -27,8 +27,15 @@ enum Models { enum UrlParamVarConfig { /// A plain environment variable name with free-text UI input. Plain(String), - /// A parameter with a constrained set of options, rendered as a dropdown. - WithOptions { name: String, options: Vec }, + /// A parameter with a constrained set of options, rendered as a dropdown, + /// and an optional flag indicating the param may be left blank. + WithOptions { + name: String, + #[serde(default)] + options: Vec, + #[serde(default)] + optional: bool, + }, } impl UrlParamVarConfig { @@ -40,12 +47,26 @@ impl UrlParamVarConfig { } } + /// Returns whether this parameter is optional. + fn is_optional(&self) -> bool { + match self { + Self::Plain(_) => false, + Self::WithOptions { optional, .. } => *optional, + } + } + /// Converts into a `URLParamSpec` for use in the domain layer. fn into_spec(self) -> URLParamSpec { match self { Self::Plain(s) => URLParamSpec::new(URLParam::from(s)), - Self::WithOptions { name, options } => { - URLParamSpec::with_options(URLParam::from(name), options) + Self::WithOptions { name, options, optional } => { + let mut spec = if options.is_empty() { + URLParamSpec::new(URLParam::from(name)) + } else { + URLParamSpec::with_options(URLParam::from(name), options) + }; + spec.optional = optional; + spec } } } @@ -132,10 +153,14 @@ fn merge_configs(base: &mut Vec, other: Vec) { impl From for UrlParamVarConfig { fn from(param: forge_config::ProviderUrlParam) -> Self { - if param.options.is_empty() { + if param.options.is_empty() && !param.optional { UrlParamVarConfig::Plain(param.name) } else { - UrlParamVarConfig::WithOptions { name: param.name, options: param.options } + UrlParamVarConfig::WithOptions { + name: param.name, + options: param.options, + optional: param.optional, + } } } } @@ -397,6 +422,11 @@ impl< URLParam::from(name.to_string()), URLParamValue::from(value.to_string()), ); + } else if env_var.is_optional() { + // Optional param absent from env — omit from credential + // entirely. `render_url_template` injects null + // for absent optional params so `{{#if PARAM}}` + // evaluates to false. } else { return Err(Error::env_var_not_found(config.id.clone(), name).into()); } @@ -1820,4 +1850,49 @@ mod env_tests { .find(|c| c.id == ProviderId::OPEN_ROUTER); assert!(openrouter_config.is_some()); } + + #[tokio::test] + async fn test_vllm_port_is_optional() { + let configs = get_provider_configs(); + let vllm_id = ProviderId::from("vllm".to_string()); + let config = configs.iter().find(|c| c.id == vllm_id).unwrap(); + + let port_param = config + .url_param_vars + .iter() + .find(|v| v.param_name() == "VLLM_PORT") + .unwrap(); + + assert!(port_param.is_optional(), "VLLM_PORT should be optional"); + } + + #[tokio::test] + async fn test_vllm_migration_without_port() { + // vLLM behind a reverse proxy — no port needed + let mut env_vars = HashMap::new(); + env_vars.insert("VLLM_API_KEY".to_string(), "vllm-key".to_string()); + env_vars.insert("VLLM_HOST".to_string(), "my.server.url".to_string()); + env_vars.insert("VLLM_SSL_SCHEME".to_string(), "https".to_string()); + // VLLM_PORT intentionally absent + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let vllm_id = ProviderId::from("vllm".to_string()); + let vllm_cred = creds.iter().find(|c| c.id == vllm_id).unwrap(); + + // Optional absent param should not be stored in the credential at all. + // `render_url_template` handles the absent key by injecting null. + assert!( + !vllm_cred + .url_params + .contains_key(&URLParam::from("VLLM_PORT".to_string())), + "VLLM_PORT should be absent from credential when not provided" + ); + } } diff --git a/crates/forge_services/src/provider_service.rs b/crates/forge_services/src/provider_service.rs index 39748dd3d6..5baac55ebb 100644 --- a/crates/forge_services/src/provider_service.rs +++ b/crates/forge_services/src/provider_service.rs @@ -23,17 +23,37 @@ impl ForgeProviderService { Self { repository } } - /// Renders a URL template with provided parameters + /// Renders a URL template with provided parameters. + /// + /// Params present in the credential are passed through as strings. + /// Optional specs that are absent from the credential entirely are + /// inserted as JSON `null` so that `{{#if PARAM}}` blocks evaluate + /// to false in the Handlebars template (e.g. a port not provided). fn render_url_template( &self, template: &str, params: &HashMap, + specs: &[forge_domain::URLParamSpec], ) -> Result { - let template_data: HashMap<&str, &str> = params + // Start with all stored params as string values. + let mut template_data: HashMap<&str, serde_json::Value> = params .iter() - .map(|(k, v)| (k.as_str(), v.as_str())) + .map(|(k, v)| { + ( + k.as_str(), + serde_json::Value::String(v.as_str().to_string()), + ) + }) .collect(); + // For optional specs that are entirely absent from the credential, + // inject null so {{#if PARAM}} is falsy instead of erroring. + for spec in specs { + if spec.optional && !params.contains_key(&spec.name) { + template_data.insert(spec.name.as_str(), serde_json::Value::Null); + } + } + let handlebars = forge_app::TemplateEngine::handlebar_instance(); let rendered = handlebars.render_template(template, &template_data)?; @@ -48,14 +68,21 @@ impl ForgeProviderService { .ok_or_else(|| anyhow::anyhow!("Provider has no credential"))?; // Render main URL - let url = - self.render_url_template(&template_provider.url.template, &credential.url_params)?; + let url = self.render_url_template( + &template_provider.url.template, + &credential.url_params, + &template_provider.url_params, + )?; // Render model source URLs let models = template_provider.models.as_ref().and_then(|m| match m { ModelSource::Url(template) => { let model_url = self - .render_url_template(&template.template, &credential.url_params) + .render_url_template( + &template.template, + &credential.url_params, + &template_provider.url_params, + ) .ok(); model_url.map(ModelSource::Url) } @@ -299,4 +326,66 @@ mod tests { ); } } + + #[test] + fn test_render_url_template_optional_port_absent() { + // VLLM_PORT is absent from the credential (user left it blank). + // render_url_template must inject null so {{#if VLLM_PORT}} is falsy. + let service = ForgeProviderService::new(Arc::new(MockProviderRepository::new(vec![]))); + let template = "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}{{#if VLLM_PORT}}:{{VLLM_PORT}}{{/if}}/v1/chat/completions"; + + let mut params = HashMap::new(); + params.insert( + forge_domain::URLParam::from("VLLM_SSL_SCHEME".to_string()), + forge_domain::URLParamValue::from("https".to_string()), + ); + params.insert( + forge_domain::URLParam::from("VLLM_HOST".to_string()), + forge_domain::URLParamValue::from("my.server.url".to_string()), + ); + // VLLM_PORT intentionally absent — not in the credential map at all. + + let specs = vec![forge_domain::URLParamSpec::optional( + forge_domain::URLParam::from("VLLM_PORT".to_string()), + )]; + + let actual = service + .render_url_template(template, ¶ms, &specs) + .unwrap(); + let expected = "https://my.server.url/v1/chat/completions"; + + assert_eq!(actual.as_str(), expected); + } + + #[test] + fn test_render_url_template_optional_port_with_value() { + // When VLLM_PORT has a value, it should appear in the URL. + let service = ForgeProviderService::new(Arc::new(MockProviderRepository::new(vec![]))); + let template = "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}{{#if VLLM_PORT}}:{{VLLM_PORT}}{{/if}}/v1/chat/completions"; + + let mut params = HashMap::new(); + params.insert( + forge_domain::URLParam::from("VLLM_SSL_SCHEME".to_string()), + forge_domain::URLParamValue::from("https".to_string()), + ); + params.insert( + forge_domain::URLParam::from("VLLM_HOST".to_string()), + forge_domain::URLParamValue::from("my.server.url".to_string()), + ); + params.insert( + forge_domain::URLParam::from("VLLM_PORT".to_string()), + forge_domain::URLParamValue::from("8000".to_string()), + ); + + let specs = vec![forge_domain::URLParamSpec::optional( + forge_domain::URLParam::from("VLLM_PORT".to_string()), + )]; + + let actual = service + .render_url_template(template, ¶ms, &specs) + .unwrap(); + let expected = "https://my.server.url:8000/v1/chat/completions"; + + assert_eq!(actual.as_str(), expected); + } } diff --git a/forge.schema.json b/forge.schema.json index 6781149768..87dcada42d 100644 --- a/forge.schema.json +++ b/forge.schema.json @@ -850,6 +850,10 @@ "description": "The environment variable name used as the template variable key.", "type": "string" }, + "optional": { + "description": "Whether this parameter is optional. When `true`, the parameter may be\nleft blank without causing an error.", + "type": "boolean" + }, "options": { "description": "Optional preset values for this parameter shown as suggestions in the\nUI.", "type": "array",