refactor: make agent provider and model fields required

Cargo.toml

@@ -116,7 +116,7 @@ nucleo = "0.5.0"
 gray_matter = "0.3.2"
 num-format = "0.4"
 humantime = "2.1.0"
-
+dashmap = "7.0.0-rc2"
 
 # Internal crates
 forge_api = { path = "crates/forge_api" }

crates/forge_api/src/api.rs

@@ -123,11 +123,9 @@ pub trait API: Sync + Send {
     /// Logs out the current user and clears authentication data
     async fn logout(&self) -> anyhow::Result<()>;
 
-    /// Retrieves the provider configuration for the specified agent
-    async fn get_agent_provider(&self, agent_id: AgentId) -> anyhow::Result<Provider<Url>>;
-
-    /// Retrieves the provider configuration for the default agent
-    async fn get_default_provider(&self) -> anyhow::Result<Provider<Url>>;
+    /// Retrieves the provider configuration for the specified agent.
+    /// If agent_id is None, returns the default provider.
+    async fn get_agent_provider(&self, agent_id: Option<AgentId>) -> anyhow::Result<Provider<Url>>;
 
     /// Sets the default provider for all the agents
     async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()>;
@@ -145,17 +143,13 @@ pub trait API: Sync + Send {
     async fn set_active_agent(&self, agent_id: AgentId) -> anyhow::Result<()>;
 
     /// Gets the model for the specified agent
-    async fn get_agent_model(&self, agent_id: AgentId) -> Option<ModelId>;
+    async fn get_agent_model(&self, agent_id: Option<AgentId>) -> Option<ModelId>;
 
     /// Gets the default model
     async fn get_default_model(&self) -> Option<ModelId>;
 
     /// Sets the operating model
-    async fn set_default_model(
-        &self,
-        agent_id: Option<AgentId>,
-        model_id: ModelId,
-    ) -> anyhow::Result<()>;
+    async fn set_model(&self, agent_id: Option<AgentId>, model_id: ModelId) -> anyhow::Result<()>;
 
     /// Refresh MCP caches by fetching fresh data
     async fn reload_mcp(&self) -> Result<()>;

crates/forge_api/src/forge_api.rs

@@ -63,14 +63,14 @@ impl<A: Services, F: CommandInfra + EnvironmentInfra> API for ForgeAPI<A, F> {
         Ok(self
             .services
             .models(
-                self.get_default_provider()
+                self.get_agent_provider(None)
                     .await
                     .context("Failed to fetch models")?,
             )
             .await?)
     }
     async fn get_agents(&self) -> Result<Vec<Agent>> {
-        Ok(self.services.get_agents().await?)
+        self.app().get_agents().await
     }
 
     async fn get_providers(&self) -> Result<Vec<AnyProvider>> {
@@ -217,22 +217,18 @@ impl<A: Services, F: CommandInfra + EnvironmentInfra> API for ForgeAPI<A, F> {
     async fn logout(&self) -> Result<()> {
         self.app().logout().await
     }
-    async fn get_agent_provider(&self, agent_id: AgentId) -> anyhow::Result<Provider<Url>> {
-        let agent_provider_resolver = AgentProviderResolver::new(self.services.clone());
-        agent_provider_resolver.get_provider(Some(agent_id)).await
-    }
 
-    async fn get_default_provider(&self) -> anyhow::Result<Provider<Url>> {
+    async fn get_agent_provider(&self, agent_id: Option<AgentId>) -> anyhow::Result<Provider<Url>> {
         let agent_provider_resolver = AgentProviderResolver::new(self.services.clone());
-        agent_provider_resolver.get_provider(None).await
+        agent_provider_resolver.get_provider(agent_id).await
     }
 
     async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()> {
         self.services.set_default_provider(provider_id).await
     }
 
     async fn user_info(&self) -> Result<Option<User>> {
-        let provider = self.get_default_provider().await?;
+        let provider = self.get_agent_provider(None).await?;
         if let Some(api_key) = provider.api_key() {
             let user_info = self.services.user_info(api_key.as_str()).await?;
             return Ok(Some(user_info));
@@ -241,7 +237,7 @@ impl<A: Services, F: CommandInfra + EnvironmentInfra> API for ForgeAPI<A, F> {
     }
 
     async fn user_usage(&self) -> Result<Option<UserUsage>> {
-        let provider = self.get_default_provider().await?;
+        let provider = self.get_agent_provider(None).await?;
         if let Some(api_key) = provider
             .credential
             .as_ref()
@@ -264,21 +260,17 @@ impl<A: Services, F: CommandInfra + EnvironmentInfra> API for ForgeAPI<A, F> {
         self.services.set_active_agent_id(agent_id).await
     }
 
-    async fn get_agent_model(&self, agent_id: AgentId) -> Option<ModelId> {
+    async fn get_agent_model(&self, agent_id: Option<AgentId>) -> Option<ModelId> {
         let agent_provider_resolver = AgentProviderResolver::new(self.services.clone());
-        agent_provider_resolver.get_model(Some(agent_id)).await.ok()
+        agent_provider_resolver.get_model(agent_id).await.ok()
     }
 
     async fn get_default_model(&self) -> Option<ModelId> {
         let agent_provider_resolver = AgentProviderResolver::new(self.services.clone());
         agent_provider_resolver.get_model(None).await.ok()
     }
-    async fn set_default_model(
-        &self,
-        agent_id: Option<AgentId>,
-        model_id: ModelId,
-    ) -> anyhow::Result<()> {
-        self.app().set_default_model(agent_id, model_id).await
+    async fn set_model(&self, agent_id: Option<AgentId>, model_id: ModelId) -> anyhow::Result<()> {
+        self.app().set_model(agent_id, model_id).await
     }
 
     async fn get_login_info(&self) -> Result<Option<LoginInfo>> {

crates/forge_app/src/agent_executor.rs

@@ -10,7 +10,7 @@ use futures::StreamExt;
 use tokio::sync::RwLock;
 
 use crate::error::Error;
-use crate::{AgentRegistry, ConversationService, Services};
+use crate::{ConversationService, Services};
 
 #[derive(Clone)]
 pub struct AgentExecutor<S> {
@@ -28,7 +28,8 @@ impl<S: Services> AgentExecutor<S> {
         if let Some(tool_agents) = self.tool_agents.read().await.clone() {
             return Ok(tool_agents);
         }
-        let agents = self.services.get_agents().await?;
+        let app = crate::ForgeApp::new(self.services.clone());
+        let agents = app.get_agents().await?;
         let tools: Vec<ToolDefinition> = agents.into_iter().map(Into::into).collect();
         *self.tool_agents.write().await = Some(tools.clone());
         Ok(tools)

crates/forge_app/src/agent_orchestrator.rs

@@ -0,0 +1,64 @@
+use std::sync::Arc;
+
+use anyhow::Result;
+use forge_domain::{Agent, AgentId};
+
+use crate::Services;
+use crate::services::{AgentLoader, AgentRegistry, AppConfigService};
+
+/// Orchestrates agent operations including loading definitions, converting to
+/// runtime agents, and managing the agent registry.
+pub struct AgentOrchestrator<S> {
+    services: Arc<S>,
+}
+
+impl<S: Services> AgentOrchestrator<S> {
+    /// Creates a new AgentOrchestrator with the provided services
+    pub fn new(services: Arc<S>) -> Self {
+        Self { services }
+    }
+
+    /// Gets all agents, converting AgentDefinitions to Agents with default
+    /// provider and model. Uses cached agents from registry if available,
+    /// otherwise loads definitions and populates the registry.
+    pub async fn get_agents(&self) -> Result<Vec<Agent>> {
+        // try to get from registry
+        let cached_agents = AgentRegistry::get_agents(&*self.services).await?;
+        if !cached_agents.is_empty() {
+            return Ok(cached_agents);
+        }
+
+        // load definitions from AgentLoader, convert, and cache
+        let agent_defs = AgentLoader::get_agents(&*self.services).await?;
+        let default_provider = self.services.get_default_provider().await?;
+        let default_model = self
+            .services
+            .get_default_model(&default_provider.id)
+            .await?;
+
+        let agents: Vec<Agent> = agent_defs
+            .into_iter()
+            .map(|def| def.into_agent(default_provider.id, default_model.clone()))
+            .collect();
+
+        // Populate the runtime registry so other parts of the app can query
+        // agents by id efficiently.
+        self.services.set_agents(agents.clone()).await?;
+
+        Ok(agents)
+    }
+
+    /// Gets a specific agent by ID, converting AgentDefinition to Agent with
+    /// default provider and model
+    pub async fn get_agent(&self, agent_id: &AgentId) -> Result<Option<Agent>> {
+        // First try to get from registry (fast path)
+        if let Some(agent) = AgentRegistry::get_agent(&*self.services, agent_id).await? {
+            return Ok(Some(agent));
+        }
+
+        // Fallback: load all agents and find the one we need
+        // This will also populate the cache for future requests
+        let agents = self.get_agents().await?;
+        Ok(agents.into_iter().find(|a| &a.id == agent_id))
+    }
+}

crates/forge_app/src/agent_provider_resolver.rs

@@ -24,11 +24,17 @@ where
     /// agent is provided. Automatically refreshes OAuth credentials if they're
     /// about to expire.
     pub async fn get_provider(&self, agent_id: Option<AgentId>) -> Result<Provider<url::Url>> {
-        let provider = if let Some(agent) = agent_id
-            && let Some(agent) = self.0.get_agent(&agent).await?
-            && let Some(provider_id) = agent.provider
-        {
-            self.0.get_provider(provider_id).await?
+        let provider = if let Some(agent_id) = agent_id {
+            // Load all agent definitions and find the one we need
+
+            if let Some(agent) = self.0.get_agent(&agent_id).await? {
+                // If the agent definition has a provider, use it; otherwise use default
+                self.0.get_provider(agent.provider).await?
+            } else {
+                // TODO: Needs review, should we throw an err here?
+                // we can throw crate::Error::AgentNotFound
+                self.0.get_default_provider().await?
+            }
         } else {
             self.0.get_default_provider().await?
         };
@@ -69,13 +75,39 @@ where
     /// Gets the model for the specified agent, or the default model if no agent
     /// is provided
     pub async fn get_model(&self, agent_id: Option<AgentId>) -> Result<ModelId> {
-        let provider_id = self.get_provider(agent_id).await?.id;
-        self.0.get_default_model(&provider_id).await
+        if let Some(agent_id) = agent_id {
+            if let Some(agent) = self.0.get_agent(&agent_id).await? {
+                Ok(agent.model)
+            } else {
+                // TODO: Needs review, should we throw an err here?
+                // we can throw crate::Error::AgentNotFound
+                let provider_id = self.get_provider(Some(agent_id)).await?.id;
+                self.0.get_default_model(&provider_id).await
+            }
+        } else {
+            let provider_id = self.get_provider(None).await?.id;
+            self.0.get_default_model(&provider_id).await
+        }
     }
 
-    /// Sets the default model for the agent's provider
-    pub async fn set_default_model(&self, agent_id: Option<AgentId>, model: ModelId) -> Result<()> {
-        let provider_id = self.get_provider(agent_id).await?.id;
-        self.0.set_default_model(model, provider_id).await
+    /// Sets the model for the agent's provider
+    pub async fn set_model(&self, agent_id: Option<AgentId>, model: ModelId) -> Result<()> {
+        if let Some(agent_id) = agent_id {
+            if let Some(agent) = self.0.get_agent(&agent_id).await? {
+                let agent = agent.model(model.clone());
+                self.0.insert_agent(agent).await?;
+
+                let provider_id = self.get_provider(Some(agent_id)).await?.id;
+                self.0.set_default_model(model, provider_id).await
+            } else {
+                // TODO: Needs review, should we throw an err here?
+                // we can throw crate::Error::AgentNotFound
+                let provider_id = self.get_provider(None).await?.id;
+                self.0.set_default_model(model, provider_id).await
+            }
+        } else {
+            let provider_id = self.get_provider(None).await?.id;
+            self.0.set_default_model(model, provider_id).await
+        }
     }
 }