fix: restore mcp flexibility and improve cli tooling
This commit is contained in:
@@ -109,6 +109,8 @@ impl Config {
|
||||
let mut config: Config =
|
||||
toml::from_str(&content).map_err(|e| crate::Error::Config(e.to_string()))?;
|
||||
config.ensure_defaults();
|
||||
config.mcp.apply_backward_compat();
|
||||
config.validate()?;
|
||||
Ok(config)
|
||||
} else {
|
||||
Ok(Config::default())
|
||||
@@ -117,6 +119,8 @@ impl Config {
|
||||
|
||||
/// Persist configuration to disk
|
||||
pub fn save(&self, path: Option<&Path>) -> Result<()> {
|
||||
self.validate()?;
|
||||
|
||||
let path = match path {
|
||||
Some(path) => path.to_path_buf(),
|
||||
None => default_config_path(),
|
||||
@@ -168,6 +172,87 @@ impl Config {
|
||||
ensure_provider_config(self, "ollama");
|
||||
ensure_provider_config(self, "ollama-cloud");
|
||||
}
|
||||
|
||||
/// Validate configuration invariants and surface actionable error messages.
|
||||
pub fn validate(&self) -> Result<()> {
|
||||
self.validate_default_provider()?;
|
||||
self.validate_mcp_settings()?;
|
||||
self.validate_mcp_servers()?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn validate_default_provider(&self) -> Result<()> {
|
||||
if self.general.default_provider.trim().is_empty() {
|
||||
return Err(crate::Error::Config(
|
||||
"general.default_provider must reference a configured provider".to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
if self.provider(&self.general.default_provider).is_none() {
|
||||
return Err(crate::Error::Config(format!(
|
||||
"Default provider '{}' is not defined under [providers]",
|
||||
self.general.default_provider
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn validate_mcp_settings(&self) -> Result<()> {
|
||||
match self.mcp.mode {
|
||||
McpMode::RemoteOnly => {
|
||||
if self.mcp_servers.is_empty() {
|
||||
return Err(crate::Error::Config(
|
||||
"[mcp].mode = 'remote_only' requires at least one [[mcp_servers]] entry"
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
}
|
||||
McpMode::RemotePreferred => {
|
||||
if !self.mcp.allow_fallback && self.mcp_servers.is_empty() {
|
||||
return Err(crate::Error::Config(
|
||||
"[mcp].allow_fallback = false requires at least one [[mcp_servers]] entry"
|
||||
.to_string(),
|
||||
));
|
||||
}
|
||||
}
|
||||
McpMode::Disabled => {
|
||||
return Err(crate::Error::Config(
|
||||
"[mcp].mode = 'disabled' is not supported by this build of Owlen".to_string(),
|
||||
));
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn validate_mcp_servers(&self) -> Result<()> {
|
||||
for server in &self.mcp_servers {
|
||||
if server.name.trim().is_empty() {
|
||||
return Err(crate::Error::Config(
|
||||
"Each [[mcp_servers]] entry must include a non-empty name".to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
if server.command.trim().is_empty() {
|
||||
return Err(crate::Error::Config(format!(
|
||||
"MCP server '{}' must define a command or endpoint",
|
||||
server.name
|
||||
)));
|
||||
}
|
||||
|
||||
let transport = server.transport.to_lowercase();
|
||||
if !matches!(transport.as_str(), "stdio" | "http" | "websocket") {
|
||||
return Err(crate::Error::Config(format!(
|
||||
"Unknown MCP transport '{}' for server '{}'",
|
||||
server.transport, server.name
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
fn default_ollama_provider_config() -> ProviderConfig {
|
||||
@@ -190,6 +275,10 @@ fn default_ollama_cloud_provider_config() -> ProviderConfig {
|
||||
|
||||
/// Default configuration path with user home expansion
|
||||
pub fn default_config_path() -> PathBuf {
|
||||
if let Some(config_dir) = dirs::config_dir() {
|
||||
return config_dir.join("owlen").join("config.toml");
|
||||
}
|
||||
|
||||
PathBuf::from(shellexpand::tilde(DEFAULT_CONFIG_PATH).as_ref())
|
||||
}
|
||||
|
||||
@@ -239,11 +328,90 @@ impl Default for GeneralSettings {
|
||||
}
|
||||
}
|
||||
|
||||
/// Operating modes for the MCP subsystem.
|
||||
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum McpMode {
|
||||
/// Prefer remote MCP servers when configured, but allow local fallback.
|
||||
#[serde(alias = "enabled", alias = "auto")]
|
||||
RemotePreferred,
|
||||
/// Require a configured remote MCP server; fail if none are available.
|
||||
RemoteOnly,
|
||||
/// Always use the in-process MCP server for tooling.
|
||||
#[serde(alias = "local")]
|
||||
LocalOnly,
|
||||
/// Compatibility shim for pre-v1.0 behaviour; treated as `local_only`.
|
||||
Legacy,
|
||||
/// Disable MCP entirely (not recommended).
|
||||
Disabled,
|
||||
}
|
||||
|
||||
impl Default for McpMode {
|
||||
fn default() -> Self {
|
||||
Self::RemotePreferred
|
||||
}
|
||||
}
|
||||
|
||||
impl McpMode {
|
||||
/// Whether this mode requires a remote MCP server.
|
||||
pub const fn requires_remote(self) -> bool {
|
||||
matches!(self, Self::RemoteOnly)
|
||||
}
|
||||
|
||||
/// Whether this mode prefers to use a remote MCP server when available.
|
||||
pub const fn prefers_remote(self) -> bool {
|
||||
matches!(self, Self::RemotePreferred | Self::RemoteOnly)
|
||||
}
|
||||
|
||||
/// Whether this mode should operate purely locally.
|
||||
pub const fn is_local(self) -> bool {
|
||||
matches!(self, Self::LocalOnly | Self::Legacy)
|
||||
}
|
||||
}
|
||||
|
||||
/// MCP (Multi-Client-Provider) settings
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, Default)]
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct McpSettings {
|
||||
// MCP is now always enabled in v1.0+
|
||||
// Kept as a struct for future configuration options
|
||||
/// Operating mode for MCP integration.
|
||||
#[serde(default)]
|
||||
pub mode: McpMode,
|
||||
/// Allow falling back to the local MCP client when remote startup fails.
|
||||
#[serde(default = "McpSettings::default_allow_fallback")]
|
||||
pub allow_fallback: bool,
|
||||
/// Emit a warning when the deprecated `legacy` mode is used.
|
||||
#[serde(default = "McpSettings::default_warn_on_legacy")]
|
||||
pub warn_on_legacy: bool,
|
||||
}
|
||||
|
||||
impl McpSettings {
|
||||
const fn default_allow_fallback() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
const fn default_warn_on_legacy() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
fn apply_backward_compat(&mut self) {
|
||||
if self.mode == McpMode::Legacy && self.warn_on_legacy {
|
||||
log::warn!(
|
||||
"MCP legacy mode detected. This mode will be removed in a future release; \
|
||||
switch to 'local_only' or 'remote_preferred' after verifying your setup."
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Default for McpSettings {
|
||||
fn default() -> Self {
|
||||
let mut settings = Self {
|
||||
mode: McpMode::default(),
|
||||
allow_fallback: Self::default_allow_fallback(),
|
||||
warn_on_legacy: Self::default_warn_on_legacy(),
|
||||
};
|
||||
settings.apply_backward_compat();
|
||||
settings
|
||||
}
|
||||
}
|
||||
|
||||
/// Privacy controls governing network access and storage
|
||||
@@ -653,4 +821,48 @@ mod tests {
|
||||
assert_eq!(cloud.provider_type, "ollama-cloud");
|
||||
assert_eq!(cloud.base_url.as_deref(), Some("https://ollama.com"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_rejects_missing_default_provider() {
|
||||
let mut config = Config::default();
|
||||
config.general.default_provider = "does-not-exist".to_string();
|
||||
let result = config.validate();
|
||||
assert!(
|
||||
matches!(result, Err(crate::Error::Config(message)) if message.contains("Default provider"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_rejects_remote_only_without_servers() {
|
||||
let mut config = Config::default();
|
||||
config.mcp.mode = McpMode::RemoteOnly;
|
||||
config.mcp_servers.clear();
|
||||
let result = config.validate();
|
||||
assert!(
|
||||
matches!(result, Err(crate::Error::Config(message)) if message.contains("remote_only"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_rejects_unknown_transport() {
|
||||
let mut config = Config::default();
|
||||
config.mcp_servers = vec![McpServerConfig {
|
||||
name: "bad".into(),
|
||||
command: "binary".into(),
|
||||
transport: "udp".into(),
|
||||
args: Vec::new(),
|
||||
env: std::collections::HashMap::new(),
|
||||
}];
|
||||
let result = config.validate();
|
||||
assert!(
|
||||
matches!(result, Err(crate::Error::Config(message)) if message.contains("transport"))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn validate_accepts_local_only_configuration() {
|
||||
let mut config = Config::default();
|
||||
config.mcp.mode = McpMode::LocalOnly;
|
||||
assert!(config.validate().is_ok());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user