diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index 3f5a2e4aca..fdc36a67d3 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -122,17 +122,13 @@ pub trait API: Sync + Send { /// Retrieves the provider configuration for the default agent async fn get_default_provider(&self) -> anyhow::Result>; - /// Sets the default provider for all the agents - async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()>; - - /// Updates the caller's default provider and model together, ensuring all - /// commands resolve a consistent pair without requiring a follow-up model - /// selection call. - async fn set_default_provider_and_model( - &self, - provider_id: ProviderId, - model: ModelId, - ) -> anyhow::Result<()>; + /// Applies one or more configuration mutations atomically. + /// + /// Each operation in `ops` is applied in order and persisted as a single + /// atomic write. Use [`forge_domain::ConfigOperation`] variants to describe + /// each mutation. Provider and model changes also invalidate the agent + /// cache so the next request picks up the updated configuration. + async fn update_config(&self, ops: Vec) -> anyhow::Result<()>; /// Retrieves information about the currently authenticated user async fn user_info(&self) -> anyhow::Result>; @@ -152,31 +148,17 @@ pub trait API: Sync + Send { /// Gets the default model async fn get_default_model(&self) -> Option; - /// Sets the operating model - async fn set_default_model(&self, model_id: ModelId) -> anyhow::Result<()>; - /// Gets the commit configuration (provider and model for commit message /// generation). - async fn get_commit_config(&self) -> anyhow::Result>; - - /// Sets the commit configuration (provider and model for commit message - /// generation). - async fn set_commit_config(&self, config: forge_domain::CommitConfig) -> anyhow::Result<()>; + async fn get_commit_config(&self) -> anyhow::Result>; /// Gets the suggest configuration (provider and model for command /// suggestion generation). - async fn get_suggest_config(&self) -> anyhow::Result>; - - /// Sets the suggest configuration (provider and model for command - /// suggestion generation). - async fn set_suggest_config(&self, config: forge_domain::SuggestConfig) -> anyhow::Result<()>; + async fn get_suggest_config(&self) -> anyhow::Result>; /// Gets the current reasoning effort setting. async fn get_reasoning_effort(&self) -> anyhow::Result>; - /// Sets the reasoning effort level applied to all agents. - async fn set_reasoning_effort(&self, effort: Effort) -> anyhow::Result<()>; - /// Refresh MCP caches by fetching fresh data async fn reload_mcp(&self) -> Result<()>; diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 2b89dee487..8ed1ab99e2 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -24,20 +24,20 @@ use crate::API; pub struct ForgeAPI { services: Arc, infra: Arc, - config: forge_config::ForgeConfig, } impl ForgeAPI { - pub fn new(services: Arc, infra: Arc, config: forge_config::ForgeConfig) -> Self { - Self { services, infra, config } + pub fn new(services: Arc, infra: Arc) -> Self { + Self { services, infra } } - /// Creates a ForgeApp instance with the current services + /// Creates a ForgeApp instance with the current services and latest config. fn app(&self) -> ForgeApp where - A: Services, + A: Services + EnvironmentInfra, + F: EnvironmentInfra, { - ForgeApp::new(self.services.clone(), self.config.clone()) + ForgeApp::new(self.services.clone()) } } @@ -49,10 +49,10 @@ impl ForgeAPI>, ForgeRepo> { /// * `config` - Pre-read application configuration (from startup) /// * `services_url` - Pre-validated URL for the gRPC workspace server pub fn init(cwd: PathBuf, config: ForgeConfig, services_url: Url) -> Self { - let infra = Arc::new(ForgeInfra::new(cwd, config.clone(), services_url)); - let repo = Arc::new(ForgeRepo::new(infra.clone(), config.clone())); - let app = Arc::new(ForgeServices::new(repo.clone(), config.clone())); - ForgeAPI::new(app, repo, config) + let infra = Arc::new(ForgeInfra::new(cwd, config, services_url)); + let repo = Arc::new(ForgeRepo::new(infra.clone())); + let app = Arc::new(ForgeServices::new(repo.clone())); + ForgeAPI::new(app, repo) } pub async fn get_skills_internal(&self) -> Result> { @@ -62,8 +62,13 @@ impl ForgeAPI>, ForgeRepo> { } #[async_trait::async_trait] -impl API - for ForgeAPI +impl< + A: Services + EnvironmentInfra, + F: CommandInfra + + EnvironmentInfra + + SkillRepository + + GrpcInfra, +> API for ForgeAPI { async fn discover(&self) -> Result> { let environment = self.services.get_environment(); @@ -98,7 +103,7 @@ impl, additional_context: Option, ) -> Result { - let git_app = GitApp::new(self.services.clone(), self.config.clone()); + let git_app = GitApp::new(self.services.clone()); let result = git_app .commit_message(max_diff_size, diff, additional_context) .await?; @@ -225,13 +230,31 @@ impl anyhow::Result<()> { - let result = self.services.set_default_provider(provider_id).await; - // Invalidate cache for agents - let _ = self.services.reload_agents().await; + async fn update_config(&self, ops: Vec) -> anyhow::Result<()> { + // Determine whether any op affects provider/model resolution before writing, + // so we can invalidate the agent cache afterwards. + let needs_agent_reload = ops + .iter() + .any(|op| matches!(op, forge_domain::ConfigOperation::SetSessionConfig(_))); + let result = self.services.update_config(ops).await; + if needs_agent_reload { + let _ = self.services.reload_agents().await; + } result } + async fn get_commit_config(&self) -> anyhow::Result> { + self.services.get_commit_config().await + } + + async fn get_suggest_config(&self) -> anyhow::Result> { + self.services.get_suggest_config().await + } + + async fn get_reasoning_effort(&self) -> anyhow::Result> { + self.services.get_reasoning_effort().await + } + async fn user_info(&self) -> Result> { let provider = self.get_default_provider().await?; if let Some(api_key) = provider.api_key() { @@ -273,50 +296,6 @@ impl Option { self.services.get_provider_model(None).await.ok() } - async fn set_default_model(&self, model_id: ModelId) -> anyhow::Result<()> { - let result = self.services.set_default_model(model_id).await; - // Invalidate cache for agents - let _ = self.services.reload_agents().await; - - result - } - - async fn set_default_provider_and_model( - &self, - provider_id: ProviderId, - model: ModelId, - ) -> anyhow::Result<()> { - let result = self - .services - .set_default_provider_and_model(provider_id, model) - .await; - let _ = self.services.reload_agents().await; - result - } - - async fn get_commit_config(&self) -> anyhow::Result> { - self.services.get_commit_config().await - } - - async fn set_commit_config(&self, config: CommitConfig) -> anyhow::Result<()> { - self.services.set_commit_config(config).await - } - - async fn get_suggest_config(&self) -> anyhow::Result> { - self.services.get_suggest_config().await - } - - async fn set_suggest_config(&self, config: SuggestConfig) -> anyhow::Result<()> { - self.services.set_suggest_config(config).await - } - - async fn get_reasoning_effort(&self) -> anyhow::Result> { - self.services.get_reasoning_effort().await - } - - async fn set_reasoning_effort(&self, effort: Effort) -> anyhow::Result<()> { - self.services.set_reasoning_effort(effort).await - } async fn reload_mcp(&self) -> Result<()> { self.services.mcp_service().reload_mcp().await diff --git a/crates/forge_app/src/agent.rs b/crates/forge_app/src/agent.rs index ec3e403d1d..30562da060 100644 --- a/crates/forge_app/src/agent.rs +++ b/crates/forge_app/src/agent.rs @@ -10,7 +10,7 @@ use merge::Merge; use crate::services::AppConfigService; use crate::tool_registry::ToolRegistry; -use crate::{ConversationService, ProviderService, Services}; +use crate::{ConversationService, EnvironmentInfra, ProviderService, Services}; /// Agent service trait that provides core chat and tool call functionality. /// This trait abstracts the essential operations needed by the Orchestrator. @@ -30,7 +30,6 @@ pub trait AgentService: Send + Sync + 'static { agent: &Agent, context: &ToolCallContext, call: ToolCallFull, - config: &ForgeConfig, ) -> ToolResult; /// Synchronize the on-going conversation @@ -39,7 +38,7 @@ pub trait AgentService: Send + Sync + 'static { /// Blanket implementation of AgentService for any type that implements Services #[async_trait::async_trait] -impl AgentService for T { +impl> AgentService for T { async fn chat_agent( &self, id: &ModelId, @@ -61,9 +60,8 @@ impl AgentService for T { agent: &Agent, context: &ToolCallContext, call: ToolCallFull, - config: &ForgeConfig, ) -> ToolResult { - let registry = ToolRegistry::new(Arc::new(self.clone()), config.clone()); + let registry = ToolRegistry::new(Arc::new(self.clone())); registry.call(agent, context, call).await } diff --git a/crates/forge_app/src/agent_executor.rs b/crates/forge_app/src/agent_executor.rs index 1d533d31c6..fe92b7c7d4 100644 --- a/crates/forge_app/src/agent_executor.rs +++ b/crates/forge_app/src/agent_executor.rs @@ -2,7 +2,6 @@ use std::sync::Arc; use anyhow::Context; use convert_case::{Case, Casing}; -use forge_config::ForgeConfig; use forge_domain::{ AgentId, ChatRequest, ChatResponse, ChatResponseContent, Conversation, ConversationId, Event, TitleFormat, ToolCallContext, ToolDefinition, ToolName, ToolOutput, @@ -12,18 +11,16 @@ use futures::StreamExt; use tokio::sync::RwLock; use crate::error::Error; -use crate::{AgentRegistry, ConversationService, Services}; - +use crate::{AgentRegistry, ConversationService, EnvironmentInfra, Services}; #[derive(Clone)] pub struct AgentExecutor { services: Arc, - config: ForgeConfig, pub tool_agents: Arc>>>, } -impl AgentExecutor { - pub fn new(services: Arc, config: ForgeConfig) -> Self { - Self { services, config, tool_agents: Arc::new(RwLock::new(None)) } +impl> AgentExecutor { + pub fn new(services: Arc) -> Self { + Self { services, tool_agents: Arc::new(RwLock::new(None)) } } /// Returns a list of tool definitions for all available agents. @@ -79,7 +76,7 @@ impl AgentExecutor { conversation }; // Execute the request through the ForgeApp - let app = crate::ForgeApp::new(self.services.clone(), self.config.clone()); + let app = crate::ForgeApp::new(self.services.clone()); let mut response_stream = app .chat( agent_id.clone(), diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index e7f52fe652..13304e911d 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -19,8 +19,8 @@ use crate::tool_registry::ToolRegistry; use crate::tool_resolver::ToolResolver; use crate::user_prompt::UserPromptGenerator; use crate::{ - AgentExt, AgentProviderResolver, ConversationService, FileDiscoveryService, ProviderService, - Services, + AgentExt, AgentProviderResolver, ConversationService, EnvironmentInfra, FileDiscoveryService, + ProviderService, Services, }; /// Builds a [`TemplateConfig`] from a [`ForgeConfig`]. @@ -44,17 +44,12 @@ pub(crate) fn build_template_config(config: &ForgeConfig) -> forge_domain::Templ pub struct ForgeApp { services: Arc, tool_registry: ToolRegistry, - config: ForgeConfig, } -impl ForgeApp { - /// Creates a new ForgeApp instance with the provided services and config. - pub fn new(services: Arc, config: ForgeConfig) -> Self { - Self { - tool_registry: ToolRegistry::new(services.clone(), config.clone()), - services, - config, - } +impl> ForgeApp { + /// Creates a new ForgeApp instance with the provided services. + pub fn new(services: Arc) -> Self { + Self { tool_registry: ToolRegistry::new(services.clone()), services } } /// Executes a chat request and returns a stream of responses. @@ -73,7 +68,7 @@ impl ForgeApp { .ok_or_else(|| forge_domain::Error::ConversationNotFound(chat.conversation_id))?; // Discover files using the discovery service - let forge_config = self.config.clone(); + let forge_config = self.services.get_config()?; let environment = services.get_environment(); let files = services.list_current_directory().await?; @@ -136,7 +131,7 @@ impl ForgeApp { // Detect and render externally changed files notification let conversation = ChangedFiles::new(services.clone(), agent.clone()) - .update_file_stats(conversation, forge_config.max_parallel_file_reads) + .update_file_stats(conversation) .await; let conversation = InitConversationMetrics::new(current_time).apply(conversation); @@ -159,14 +154,11 @@ impl ForgeApp { .on_toolcall_end(tracing_handler.clone()) .on_end(tracing_handler.and(title_handler)); - let retry_config = forge_config.retry.clone().unwrap_or_default(); - let orch = Orchestrator::new( services.clone(), - retry_config, conversation, agent, - forge_config, + self.services.get_config()?, ) .error_tracker(ToolErrorTracker::new(max_tool_failure_per_turn)) .tool_definitions(tool_definitions) @@ -229,7 +221,7 @@ impl ForgeApp { let original_messages = context.messages.len(); let original_token_count = *context.token_count(); - let forge_config = self.config.clone(); + let forge_config = self.services.get_config()?; // Get agent and apply workflow config let agent = self.services.get_agent(&active_agent_id).await?; diff --git a/crates/forge_app/src/changed_files.rs b/crates/forge_app/src/changed_files.rs index 5d7585c740..9eb93334b8 100644 --- a/crates/forge_app/src/changed_files.rs +++ b/crates/forge_app/src/changed_files.rs @@ -20,18 +20,19 @@ impl ChangedFiles { } } -impl ChangedFiles { +impl> ChangedFiles { /// Detects externally changed files and renders a notification if changes /// are found. Updates file hashes in conversation metrics to prevent /// duplicate notifications. - pub async fn update_file_stats( - &self, - mut conversation: Conversation, - parallel_file_reads: usize, - ) -> Conversation { + pub async fn update_file_stats(&self, mut conversation: Conversation) -> Conversation { use crate::file_tracking::FileChangeDetector; - let changes = FileChangeDetector::new(self.services.clone(), parallel_file_reads) - .detect(&conversation.metrics) + let parallel_file_reads = self + .services + .get_config() + .map(|c| c.max_parallel_file_reads) + .unwrap_or(4); + let changes = FileChangeDetector::new(self.services.clone()) + .detect(&conversation.metrics, parallel_file_reads) .await; if changes.is_empty() { @@ -136,6 +137,10 @@ mod tests { env } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig { max_parallel_file_reads: 4, ..Default::default() }) + } + async fn update_environment( &self, _ops: Vec, @@ -199,7 +204,7 @@ mod tests { Some(ModelId::new("test")), ))); - let actual = service.update_file_stats(conversation.clone(), 4).await; + let actual = service.update_file_stats(conversation.clone()).await; assert_eq!(actual.context.clone().unwrap_or_default().messages.len(), 1); assert_eq!(actual.context, conversation.context); @@ -215,7 +220,7 @@ mod tests { [("/test/file.txt".into(), Some(old_hash))].into(), ); - let actual = service.update_file_stats(conversation, 4).await; + let actual = service.update_file_stats(conversation).await; let messages = &actual.context.unwrap().messages; assert_eq!(messages.len(), 1); @@ -235,7 +240,7 @@ mod tests { [("/test/file.txt".into(), Some(old_hash))].into(), ); - let actual = service.update_file_stats(conversation, 4).await; + let actual = service.update_file_stats(conversation).await; let updated_hash = actual .metrics @@ -261,7 +266,7 @@ mod tests { .into(), ); - let actual = service.update_file_stats(conversation, 4).await; + let actual = service.update_file_stats(conversation).await; let message = actual.context.unwrap().messages[0] .content() @@ -284,7 +289,7 @@ mod tests { Some(cwd), ); - let actual = service.update_file_stats(conversation, 4).await; + let actual = service.update_file_stats(conversation).await; let message = actual.context.unwrap().messages[0] .content() diff --git a/crates/forge_app/src/command_generator.rs b/crates/forge_app/src/command_generator.rs index 42ca8b0080..688d3b6f65 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -144,6 +144,10 @@ mod tests { self.environment.clone() } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + async fn update_environment( &self, _ops: Vec, @@ -251,10 +255,6 @@ mod tests { Ok(ProviderId::OPENAI) } - async fn set_default_provider(&self, _provider_id: ProviderId) -> Result<()> { - Ok(()) - } - async fn get_provider_model( &self, _provider_id: Option<&ProviderId>, @@ -262,39 +262,19 @@ mod tests { Ok(ModelId::new("test-model")) } - async fn set_default_model(&self, _model: ModelId) -> Result<()> { - Ok(()) - } - - async fn set_default_provider_and_model( - &self, - _provider_id: ProviderId, - _model: ModelId, - ) -> anyhow::Result<()> { - Ok(()) - } - - async fn get_commit_config(&self) -> Result> { + async fn get_commit_config(&self) -> Result> { Ok(None) } - async fn set_commit_config(&self, _config: forge_domain::CommitConfig) -> Result<()> { - Ok(()) - } - - async fn get_suggest_config(&self) -> Result> { + async fn get_suggest_config(&self) -> Result> { Ok(None) } - async fn set_suggest_config(&self, _config: forge_domain::SuggestConfig) -> Result<()> { - Ok(()) - } - async fn get_reasoning_effort(&self) -> Result> { Ok(None) } - async fn set_reasoning_effort(&self, _effort: forge_domain::Effort) -> Result<()> { + async fn update_config(&self, _ops: Vec) -> Result<()> { Ok(()) } } diff --git a/crates/forge_app/src/file_tracking.rs b/crates/forge_app/src/file_tracking.rs index 71b9c3b7e0..5300f88234 100644 --- a/crates/forge_app/src/file_tracking.rs +++ b/crates/forge_app/src/file_tracking.rs @@ -18,7 +18,6 @@ pub struct FileChange { #[derive(Clone)] pub struct FileChangeDetector { fs_read_service: Arc, - parallel_file_reads: usize, } impl FileChangeDetector { @@ -27,9 +26,8 @@ impl FileChangeDetector { /// # Arguments /// /// * `fs_read_service` - The file system read service implementation - /// * `parallel_file_reads` - Maximum number of files to hash concurrently - pub fn new(fs_read_service: Arc, parallel_file_reads: usize) -> Self { - Self { fs_read_service, parallel_file_reads } + pub fn new(fs_read_service: Arc) -> Self { + Self { fs_read_service } } /// Detects files that have changed since the last notification @@ -41,7 +39,7 @@ impl FileChangeDetector { /// /// * `tracked_files` - Map of file paths to their last known hashes (None /// if unreadable) - pub async fn detect(&self, metrics: &Metrics) -> Vec { + pub async fn detect(&self, metrics: &Metrics, parallel_file_reads: usize) -> Vec { let fs = self.fs_read_service.clone(); // Collect into owned data upfront so the stream futures are 'static-safe let entries: Vec<(std::path::PathBuf, Option)> = metrics @@ -85,7 +83,7 @@ impl FileChangeDetector { } } }) - .buffer_unordered(self.parallel_file_reads) + .buffer_unordered(parallel_file_reads) .filter_map(std::future::ready) .collect() .await; @@ -198,7 +196,7 @@ mod tests { let content_hash = compute_hash(content); let fs = MockFsReadService::new().with_file("/test/file.txt", content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -206,7 +204,7 @@ mod tests { FileOperation::new(ToolKind::Write).content_hash(Some(content_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -219,7 +217,7 @@ mod tests { let new_hash = compute_hash(new_content); let fs = MockFsReadService::new().with_file("/test/file.txt", new_content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -227,7 +225,7 @@ mod tests { FileOperation::new(ToolKind::Write).content_hash(Some(old_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![FileChange { path: std::path::PathBuf::from("/test/file.txt"), content_hash: Some(new_hash), @@ -241,7 +239,7 @@ mod tests { let old_hash = compute_hash("old content"); let fs = MockFsReadService::new().with_not_found("/test/file.txt"); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -249,7 +247,7 @@ mod tests { FileOperation::new(ToolKind::Write).content_hash(Some(old_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![FileChange { path: std::path::PathBuf::from("/test/file.txt"), content_hash: None, @@ -265,7 +263,7 @@ mod tests { let old_hash = "old_hash".to_string(); let fs = MockFsReadService::new().with_file("/test/file.txt", new_content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); // First call: detect change let mut metrics = Metrics::default(); @@ -274,7 +272,7 @@ mod tests { FileOperation::new(ToolKind::Write).content_hash(Some(old_hash)), ); - let first = detector.detect(&metrics).await; + let first = detector.detect(&metrics, 64).await; assert_eq!(first.len(), 1); // Simulate updating content_hash after notification (like app.rs does) @@ -284,7 +282,7 @@ mod tests { ); // Second call: should not detect change - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -296,7 +294,7 @@ mod tests { let content_hash = compute_hash(content); let fs = MockFsReadService::new().with_file("/test/file.txt", content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -305,7 +303,7 @@ mod tests { ); // Hash computed from raw content matches stored hash -- no change - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -324,7 +322,7 @@ mod tests { &raw_content, &displayed_content, ); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -334,7 +332,7 @@ mod tests { // Even though displayed content differs from raw, the hash comparison // uses the raw-based content_hash from ReadOutput, so no false positive. - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -353,7 +351,7 @@ mod tests { &raw_content, &displayed_content, ); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let mut metrics = Metrics::default(); metrics.file_operations.insert( @@ -363,7 +361,7 @@ mod tests { // Hash from ReadOutput.content_hash (raw) matches stored hash -- no // false positive despite displayed content being truncated. - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -378,7 +376,7 @@ mod tests { let written_hash = compute_hash(written); let fs = MockFsReadService::new().with_file("/test/file.txt", written); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); // Step 1: Read the file (insert via Metrics::insert like production) let metrics = Metrics::default().insert( @@ -392,7 +390,7 @@ mod tests { ); // File on disk matches what was written -- no change - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -409,7 +407,7 @@ mod tests { // Disk now has the externally modified content let fs = MockFsReadService::new().with_file("/test/file.txt", external); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); // Step 1: Read, Step 2: Write let metrics = Metrics::default() @@ -423,7 +421,7 @@ mod tests { ); // External modification detected - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![FileChange { path: std::path::PathBuf::from("/test/file.txt"), content_hash: Some(external_hash), @@ -441,7 +439,7 @@ mod tests { let content_hash = compute_hash(content); let fs = MockFsReadService::new().with_file("/test/file.txt", content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); // Step 1: Write, Step 2: Read back (overwrites Write entry) let metrics = Metrics::default() @@ -455,7 +453,7 @@ mod tests { ); // Last entry is Read with matching hash -- no false positive - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -481,7 +479,7 @@ mod tests { .with_file("/test/b.txt", b_external) // user modified B .with_file("/test/c.txt", c_content) .with_file("/test/d.txt", d_content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let metrics = Metrics::default() .insert( @@ -506,7 +504,7 @@ mod tests { FileOperation::new(ToolKind::Read).content_hash(Some(compute_hash(d_content))), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; // Only file B should be detected: externally modified after write. // A and D are unchanged. C is unchanged. @@ -527,14 +525,14 @@ mod tests { let modified = "someone changed this"; let fs = MockFsReadService::new().with_file("/test/file.txt", modified); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let metrics = Metrics::default().insert( "/test/file.txt".to_string(), FileOperation::new(ToolKind::Read).content_hash(Some(compute_hash(original))), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![FileChange { path: std::path::PathBuf::from("/test/file.txt"), content_hash: Some(compute_hash(modified)), @@ -551,7 +549,7 @@ mod tests { let final_hash = compute_hash(final_content); let fs = MockFsReadService::new().with_file("/test/file.txt", final_content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let metrics = Metrics::default() .insert( @@ -571,7 +569,7 @@ mod tests { FileOperation::new(ToolKind::Patch).content_hash(Some(final_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -585,7 +583,7 @@ mod tests { let original_hash = compute_hash(original); let fs = MockFsReadService::new().with_file("/test/file.txt", original); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); let metrics = Metrics::default() .insert( @@ -601,7 +599,7 @@ mod tests { FileOperation::new(ToolKind::Undo).content_hash(Some(original_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); @@ -617,7 +615,7 @@ mod tests { // After write, disk has the written content let fs = MockFsReadService::new().with_file("/test/file.txt", written_content); - let detector = FileChangeDetector::new(Arc::new(fs), 64); + let detector = FileChangeDetector::new(Arc::new(fs)); // Read (truncated display), then Write let metrics = Metrics::default() @@ -630,7 +628,7 @@ mod tests { FileOperation::new(ToolKind::Write).content_hash(Some(written_hash)), ); - let actual = detector.detect(&metrics).await; + let actual = detector.detect(&metrics, 64).await; let expected = vec![]; assert_eq!(actual, expected); diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index bf50f44a80..5c13c7a40e 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -10,7 +10,7 @@ use crate::services::{ AgentRegistry, AppConfigService, ProviderAuthService, ProviderService, ShellService, TemplateService, }; -use crate::{AgentProviderResolver, Services}; +use crate::{AgentProviderResolver, EnvironmentInfra, Services}; /// Errors specific to GitApp operations #[derive(thiserror::Error, Debug)] @@ -22,7 +22,6 @@ pub enum GitAppError { /// GitApp handles git-related operations like commit message generation. pub struct GitApp { services: Arc, - config: forge_config::ForgeConfig, } /// Result of a commit operation @@ -67,9 +66,9 @@ struct DiffContext { } impl GitApp { - /// Creates a new GitApp instance with the provided services and config. - pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { - Self { services, config } + /// Creates a new GitApp instance with the provided services. + pub fn new(services: Arc) -> Self { + Self { services } } /// Truncates diff content if it exceeds the maximum size @@ -94,7 +93,7 @@ impl GitApp { } } -impl GitApp { +impl> GitApp { /// Generates a commit message without committing /// /// # Arguments @@ -213,7 +212,7 @@ impl GitApp { additional_context, }; - let retry_config = self.config.retry.clone().unwrap_or_default(); + let retry_config = self.services.get_config()?.retry.unwrap_or_default(); crate::retry::retry_with_config( &retry_config, || self.generate_message_from_diff(ctx.clone()), @@ -224,7 +223,7 @@ impl GitApp { /// Fetches git context (branch name and recent commits) async fn fetch_git_context(&self, cwd: &Path) -> Result<(String, String)> { - let max_commit_count = self.config.max_commit_count; + let max_commit_count = self.services.get_config()?.max_commit_count; let git_log_cmd = format!("git log --pretty=format:%s --abbrev-commit --max-count={max_commit_count}"); let (recent_commits, branch_name) = tokio::join!( @@ -312,35 +311,28 @@ impl GitApp { // Resolve provider and model: commit config takes priority over agent defaults. // If the configured provider is unavailable (e.g. logged out), fall back to the // agent's provider/model with a warning. - let (provider, model) = match commit_config.and_then(|c| c.provider.zip(c.model)) { - Some((provider_id, commit_model)) => { - match self.services.get_provider(provider_id).await { - Ok(provider) => { - match self.services.refresh_provider_credential(provider).await { - Ok(provider) => (provider, commit_model), - Err(err) => { - tracing::warn!( - error = %err, - "Failed to refresh credentials for configured commit provider. Falling back to the active provider." - ); - self.resolve_agent_provider_and_model( - &agent_provider_resolver, - agent_id, - ) - .await? - } - } - } + let (provider, model) = match commit_config { + Some(mc) => match self.services.get_provider(mc.provider).await { + Ok(provider) => match self.services.refresh_provider_credential(provider).await { + Ok(provider) => (provider, mc.model), Err(err) => { tracing::warn!( error = %err, - "Configured commit provider unavailable. Falling back to the active provider." + "Failed to refresh credentials for configured commit provider. Falling back to the active provider." ); self.resolve_agent_provider_and_model(&agent_provider_resolver, agent_id) .await? } + }, + Err(err) => { + tracing::warn!( + error = %err, + "Configured commit provider unavailable. Falling back to the active provider." + ); + self.resolve_agent_provider_and_model(&agent_provider_resolver, agent_id) + .await? } - } + }, None => { self.resolve_agent_provider_and_model(&agent_provider_resolver, agent_id) .await? diff --git a/crates/forge_app/src/hooks/title_generation.rs b/crates/forge_app/src/hooks/title_generation.rs index edc3057552..debcae12e3 100644 --- a/crates/forge_app/src/hooks/title_generation.rs +++ b/crates/forge_app/src/hooks/title_generation.rs @@ -189,7 +189,6 @@ mod tests { _agent: &Agent, _context: &ToolCallContext, _call: ToolCallFull, - _config: &forge_config::ForgeConfig, ) -> ToolResult { unreachable!("Not used in tests") } diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index 3d8d438af4..371bebcf0a 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -28,6 +28,13 @@ pub trait EnvironmentInfra: Send + Sync { /// Retrieves the current application configuration as an [`Environment`]. fn get_environment(&self) -> Environment; + /// Returns the latest fully-resolved configuration, re-reading from disk + /// if a prior `update_environment` call has invalidated the cache. + /// + /// # Errors + /// Returns an error if the disk read fails. + fn get_config(&self) -> anyhow::Result; + /// Applies a list of configuration operations to the persisted config. /// /// Implementations should load the current config, apply each operation in diff --git a/crates/forge_app/src/orch.rs b/crates/forge_app/src/orch.rs index 78df9f386d..b5435c4afb 100644 --- a/crates/forge_app/src/orch.rs +++ b/crates/forge_app/src/orch.rs @@ -4,15 +4,14 @@ use std::time::Duration; use async_recursion::async_recursion; use derive_setters::Setters; -use forge_config::RetryConfig; use forge_domain::{Agent, *}; use forge_template::Element; use futures::future::join_all; use tokio::sync::Notify; use tracing::warn; -use crate::TemplateEngine; use crate::agent::AgentService; +use crate::{EnvironmentInfra, TemplateEngine}; #[derive(Clone, Setters)] #[setters(into)] @@ -20,7 +19,6 @@ pub struct Orchestrator { services: Arc, sender: Option, conversation: Conversation, - retry_config: RetryConfig, tool_definitions: Vec, models: Vec, agent: Agent, @@ -29,17 +27,15 @@ pub struct Orchestrator { config: forge_config::ForgeConfig, } -impl Orchestrator { +impl> Orchestrator { pub fn new( services: Arc, - retry_config: RetryConfig, conversation: Conversation, agent: Agent, config: forge_config::ForgeConfig, ) -> Self { Self { conversation, - retry_config, services, agent, config, @@ -81,10 +77,11 @@ impl Orchestrator { // Execute task tool calls in parallel — mirrors how direct agent-as-tool calls // work. - let task_results: Vec<(ToolCallFull, ToolResult)> = join_all(task_calls.iter().map(|tc| { - self.services - .call(&self.agent, tool_context, (*tc).clone(), &self.config) - })) + let task_results: Vec<(ToolCallFull, ToolResult)> = join_all( + task_calls + .iter() + .map(|tc| self.services.call(&self.agent, tool_context, (*tc).clone())), + ) .await .into_iter() .zip(task_calls.iter()) @@ -130,12 +127,7 @@ impl Orchestrator { // Execute the tool let tool_result = self .services - .call( - &self.agent, - tool_context, - (*tool_call).clone(), - &self.config, - ) + .call(&self.agent, tool_context, (*tool_call).clone()) .await; // Fire the ToolcallEnd lifecycle event (fires on both success and failure) @@ -277,7 +269,7 @@ impl Orchestrator { .await?; let message = crate::retry::retry_with_config( - &self.retry_config, + &self.config.clone().retry.unwrap_or_default(), || { self.execute_chat_turn( &model_id, diff --git a/crates/forge_app/src/orch_spec/orch_runner.rs b/crates/forge_app/src/orch_spec/orch_runner.rs index 961882db3b..ae02b03b1a 100644 --- a/crates/forge_app/src/orch_spec/orch_runner.rs +++ b/crates/forge_app/src/orch_spec/orch_runner.rs @@ -1,9 +1,9 @@ -use std::collections::VecDeque; +use std::collections::{BTreeMap, VecDeque}; use std::sync::Arc; use forge_domain::{ - Attachment, ChatCompletionMessage, ChatResponse, Conversation, ConversationId, Event, Hook, - ProviderId, ToolCallFull, ToolErrorTracker, ToolResult, + Attachment, ChatCompletionMessage, ChatResponse, Conversation, ConversationId, Environment, + Event, Hook, ProviderId, ToolCallFull, ToolErrorTracker, ToolResult, }; use handlebars::{Handlebars, no_escape}; use include_dir::{Dir, include_dir}; @@ -19,8 +19,8 @@ use crate::set_conversation_id::SetConversationId; use crate::system_prompt::SystemPrompt; use crate::user_prompt::UserPromptGenerator; use crate::{ - AgentExt, AgentService, AttachmentService, ShellOutput, ShellService, SkillFetchService, - TemplateService, + AgentExt, AgentService, AttachmentService, EnvironmentInfra, ShellOutput, ShellService, + SkillFetchService, TemplateService, }; static TEMPLATE_DIR: Dir<'static> = include_dir!("$CARGO_MANIFEST_DIR/../../templates"); @@ -40,6 +40,8 @@ pub struct Runner { test_shell_outputs: Mutex>, attachments: Vec, + config: forge_config::ForgeConfig, + env: Environment, } impl Runner { @@ -57,6 +59,8 @@ impl Runner { Self { hb, attachments: setup.attachments.clone(), + config: setup.config.clone(), + env: setup.env.clone(), conversation_history: Mutex::new(Vec::new()), test_tool_calls: Mutex::new(VecDeque::from(setup.mock_tool_call_responses.clone())), test_completions: Mutex::new(VecDeque::from(setup.mock_assistant_responses.clone())), @@ -119,20 +123,13 @@ impl Runner { ApplyTunableParameters::new(agent.clone(), system_tools.clone()).apply(conversation); let conversation = SetConversationId.apply(conversation); - let retry_config = setup.config.retry.clone().unwrap_or_default(); - let orch = Orchestrator::new( - services.clone(), - retry_config, - conversation, - agent, - setup.config.clone(), - ) - .error_tracker(ToolErrorTracker::new(3)) - .tool_definitions(system_tools) - .hook(Arc::new( - Hook::default().on_request(DoomLoopDetector::default()), - )) - .sender(tx); + let orch = Orchestrator::new(services.clone(), conversation, agent, setup.config.clone()) + .error_tracker(ToolErrorTracker::new(3)) + .tool_definitions(system_tools) + .hook(Arc::new( + Hook::default().on_request(DoomLoopDetector::default()), + )) + .sender(tx); let (mut orch, runner) = (orch, services); @@ -177,7 +174,6 @@ impl AgentService for Runner { _: &forge_domain::Agent, _: &forge_domain::ToolCallContext, test_call: forge_domain::ToolCallFull, - _: &forge_config::ForgeConfig, ) -> forge_domain::ToolResult { let name = test_call.name.clone(); let mut guard = self.test_tool_calls.lock().await; @@ -259,3 +255,30 @@ impl ShellService for Runner { } } } + +impl EnvironmentInfra for Runner { + type Config = forge_config::ForgeConfig; + + fn get_env_var(&self, _key: &str) -> Option { + None + } + + fn get_env_vars(&self) -> BTreeMap { + BTreeMap::new() + } + + fn get_environment(&self) -> forge_domain::Environment { + self.env.clone() + } + + fn get_config(&self) -> anyhow::Result { + Ok(self.config.clone()) + } + + async fn update_environment( + &self, + _ops: Vec, + ) -> anyhow::Result<()> { + Ok(()) + } +} diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index e0983996b2..2f85b0920b 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -178,19 +178,12 @@ pub trait ProviderService: Send + Sync { &self, ) -> anyhow::Result>; } - /// Manages user preferences for default providers and models. #[async_trait::async_trait] pub trait AppConfigService: Send + Sync { /// Gets the user's default provider ID. async fn get_default_provider(&self) -> anyhow::Result; - /// Sets the user's default provider preference. - async fn set_default_provider( - &self, - provider_id: forge_domain::ProviderId, - ) -> anyhow::Result<()>; - /// Gets the user's default model for a specific provider or the currently /// active provider. When provider_id is None, uses the currently active /// provider. @@ -205,41 +198,24 @@ pub trait AppConfigService: Send + Sync { provider_id: Option<&forge_domain::ProviderId>, ) -> anyhow::Result; - /// Sets the user's default model for the currently active provider. - /// - /// # Errors - /// Returns an error if no default provider is configured. - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()>; - - /// Sets the user's default provider and default model in a single atomic - /// update so the persisted configuration never stores a mismatched pair. - async fn set_default_provider_and_model( - &self, - provider_id: ProviderId, - model: ModelId, - ) -> anyhow::Result<()>; - /// Gets the commit configuration (provider and model for commit message /// generation). - async fn get_commit_config(&self) -> anyhow::Result>; - - /// Sets the commit configuration (provider and model for commit message - /// generation). - async fn set_commit_config(&self, config: forge_domain::CommitConfig) -> anyhow::Result<()>; + async fn get_commit_config(&self) -> anyhow::Result>; /// Gets the suggest configuration (provider and model for command /// suggestion generation). - async fn get_suggest_config(&self) -> anyhow::Result>; - - /// Sets the suggest configuration (provider and model for command - /// suggestion generation). - async fn set_suggest_config(&self, config: forge_domain::SuggestConfig) -> anyhow::Result<()>; + async fn get_suggest_config(&self) -> anyhow::Result>; /// Gets the current reasoning effort setting. async fn get_reasoning_effort(&self) -> anyhow::Result>; - /// Sets the reasoning effort level applied to all agents. - async fn set_reasoning_effort(&self, effort: forge_domain::Effort) -> anyhow::Result<()>; + /// Applies one or more configuration mutations atomically. + /// + /// Each operation in `ops` is applied in order, and the result is + /// persisted as a single atomic write. This is the sole write path for + /// all configuration changes; use [`forge_domain::ConfigOperation`] + /// variants to describe each mutation. + async fn update_config(&self, ops: Vec) -> anyhow::Result<()>; } #[async_trait::async_trait] @@ -978,15 +954,6 @@ impl AppConfigService for I { self.config_service().get_default_provider().await } - async fn set_default_provider( - &self, - provider_id: forge_domain::ProviderId, - ) -> anyhow::Result<()> { - self.config_service() - .set_default_provider(provider_id) - .await - } - async fn get_provider_model( &self, provider_id: Option<&forge_domain::ProviderId>, @@ -994,42 +961,20 @@ impl AppConfigService for I { self.config_service().get_provider_model(provider_id).await } - async fn set_default_provider_and_model( - &self, - provider_id: forge_domain::ProviderId, - model: ModelId, - ) -> anyhow::Result<()> { - self.config_service() - .set_default_provider_and_model(provider_id, model) - .await - } - - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()> { - self.config_service().set_default_model(model).await - } - - async fn get_commit_config(&self) -> anyhow::Result> { + async fn get_commit_config(&self) -> anyhow::Result> { self.config_service().get_commit_config().await } - async fn set_commit_config(&self, config: forge_domain::CommitConfig) -> anyhow::Result<()> { - self.config_service().set_commit_config(config).await - } - - async fn get_suggest_config(&self) -> anyhow::Result> { + async fn get_suggest_config(&self) -> anyhow::Result> { self.config_service().get_suggest_config().await } - async fn set_suggest_config(&self, config: forge_domain::SuggestConfig) -> anyhow::Result<()> { - self.config_service().set_suggest_config(config).await - } - async fn get_reasoning_effort(&self) -> anyhow::Result> { self.config_service().get_reasoning_effort().await } - async fn set_reasoning_effort(&self, effort: forge_domain::Effort) -> anyhow::Result<()> { - self.config_service().set_reasoning_effort(effort).await + async fn update_config(&self, ops: Vec) -> anyhow::Result<()> { + self.config_service().update_config(ops).await } } diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index a09fa4958f..fee0c2dcec 100644 --- a/crates/forge_app/src/tool_executor.rs +++ b/crates/forge_app/src/tool_executor.rs @@ -16,7 +16,6 @@ use crate::{ pub struct ToolExecutor { services: Arc, - config: forge_config::ForgeConfig, } impl< @@ -32,7 +31,7 @@ impl< + ShellService + FollowUpService + ConversationService - + EnvironmentInfra + + EnvironmentInfra + PlanCreateService + SkillFetchService + AgentRegistry @@ -40,8 +39,8 @@ impl< + Services, > ToolExecutor { - pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { - Self { services, config } + pub fn new(services: Arc) -> Self { + Self { services } } fn require_prior_read( @@ -69,8 +68,9 @@ impl< async fn dump_operation(&self, operation: &ToolOperation) -> anyhow::Result { match operation { ToolOperation::NetFetch { input: _, output } => { + let config = self.services.get_config()?; let original_length = output.content.len(); - let is_truncated = original_length > self.config.max_fetch_chars; + let is_truncated = original_length > config.max_fetch_chars; let mut files = TempContentFiles::default(); if is_truncated { @@ -83,7 +83,7 @@ impl< Ok(files) } ToolOperation::Shell { output } => { - let config = &self.config; + let config = self.services.get_config()?; let stdout_lines = output.output.stdout.lines().count(); let stderr_lines = output.output.stderr.lines().count(); let stdout_truncated = @@ -185,11 +185,12 @@ impl< (input, output).into() } ToolCatalog::SemSearch(input) => { + let config = self.services.get_config()?; let env = self.services.get_environment(); let services = self.services.clone(); let cwd = env.cwd.clone(); - let limit = self.config.max_sem_search_results; - let top_k = self.config.sem_search_top_k as u32; + let limit = config.max_sem_search_results; + let top_k = config.sem_search_top_k as u32; let params: Vec<_> = input .queries .iter() @@ -337,7 +338,7 @@ impl< ) -> anyhow::Result { let tool_kind = tool_input.kind(); let env = self.services.get_environment(); - let config = &self.config; + let config = self.services.get_config()?; // Enforce read-before-edit for patch operations let file_path = match &tool_input { @@ -373,7 +374,7 @@ impl< let truncation_path = self.dump_operation(&operation).await?; context.with_metrics(|metrics| { - operation.into_tool_output(tool_kind, truncation_path, &env, config, metrics) + operation.into_tool_output(tool_kind, truncation_path, &env, &config, metrics) }) } } diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index 86119d43e1..0db7741760 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -21,28 +21,24 @@ use crate::fmt::content::FormatContent; use crate::mcp_executor::McpExecutor; use crate::tool_executor::ToolExecutor; use crate::{ - AgentRegistry, McpService, PolicyService, ProviderService, Services, ToolResolver, - WorkspaceService, + AgentRegistry, EnvironmentInfra, McpService, PolicyService, ProviderService, Services, + ToolResolver, WorkspaceService, }; pub struct ToolRegistry { tool_executor: ToolExecutor, agent_executor: AgentExecutor, mcp_executor: McpExecutor, - tool_timeout: Duration, services: Arc, - config: forge_config::ForgeConfig, } -impl ToolRegistry { - pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { +impl> ToolRegistry { + pub fn new(services: Arc) -> Self { Self { services: services.clone(), - tool_executor: ToolExecutor::new(services.clone(), config.clone()), - agent_executor: AgentExecutor::new(services.clone(), config.clone()), + tool_executor: ToolExecutor::new(services.clone()), + agent_executor: AgentExecutor::new(services.clone()), mcp_executor: McpExecutor::new(services.clone()), - tool_timeout: Duration::from_secs(config.tool_timeout_secs), - config, } } @@ -55,10 +51,11 @@ impl ToolRegistry { F: FnOnce() -> Fut, Fut: std::future::Future>, { - timeout(self.tool_timeout, future()) + let tool_timeout = Duration::from_secs(self.services.get_config()?.tool_timeout_secs); + timeout(tool_timeout, future()) .await .context(Error::CallTimeout { - timeout: self.tool_timeout.as_secs() / 60, + timeout: tool_timeout.as_secs() / 60, tool_name: tool_name.clone(), })? } @@ -142,7 +139,7 @@ impl ToolRegistry { // Check permissions before executing the tool (only in restricted mode) // This is done BEFORE the timeout to ensure permissions are never timed out - let is_restricted = self.config.restricted; + let is_restricted = self.services.get_config()?.restricted; if is_restricted && self.check_tool_permission(&tool_input, context).await? { // Send formatted output message for policy denial context @@ -260,7 +257,7 @@ impl ToolRegistry { let model = self.get_current_model().await; // Build TemplateConfig from ForgeConfig for tool description templates - let config = &self.config; + let config = self.services.get_config()?; let template_config = TemplateConfig { max_read_size: config.max_read_lines as usize, max_line_length: config.max_line_chars, diff --git a/crates/forge_domain/src/commit_config.rs b/crates/forge_domain/src/commit_config.rs deleted file mode 100644 index f075a0ed2d..0000000000 --- a/crates/forge_domain/src/commit_config.rs +++ /dev/null @@ -1,29 +0,0 @@ -use derive_setters::Setters; -use merge::Merge; -use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; - -use crate::{ModelId, ProviderId}; - -/// Configuration for commit message generation. -/// -/// Allows specifying a dedicated provider and model for commit message -/// generation, instead of using the active agent's provider and model. This is -/// useful when you want to use a cheaper or faster model for simple commit -/// message generation. -#[derive(Default, Debug, Clone, Serialize, Deserialize, Merge, Setters, JsonSchema, PartialEq)] -#[setters(strip_option, into)] -pub struct CommitConfig { - /// Provider ID to use for commit message generation. - /// If not specified, the active agent's provider will be used. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[merge(strategy = crate::merge::option)] - pub provider: Option, - - /// Model ID to use for commit message generation. - /// If not specified, the provider's default model or the active agent's - /// model will be used. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[merge(strategy = crate::merge::option)] - pub model: Option, -} diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 3119ebb6bb..adbadd6ca2 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -5,7 +5,7 @@ use derive_more::Display; use derive_setters::Setters; use serde::{Deserialize, Serialize}; -use crate::{Effort, ModelId, ProviderId}; +use crate::{Effort, ModelConfig}; /// Domain-level session configuration pairing a provider with a model. /// @@ -27,14 +27,19 @@ pub struct SessionConfig { /// each in order, and persist the result atomically. #[derive(Debug, Clone, PartialEq)] pub enum ConfigOperation { - /// Set the active provider. - SetProvider(ProviderId), - /// Set the model for the given provider. - SetModel(ProviderId, ModelId), + /// Set the active session provider and model atomically. + /// + /// When the provider differs from the current session provider the entire + /// session (provider + model) is replaced atomically. When they match only + /// the model field is updated. + SetSessionConfig(ModelConfig), /// Set the commit-message generation configuration. - SetCommitConfig(crate::CommitConfig), + /// + /// `None` clears the commit configuration so the active session + /// provider/model is used for commit message generation. + SetCommitConfig(Option), /// Set the shell-command suggestion configuration. - SetSuggestConfig(crate::SuggestConfig), + SetSuggestConfig(ModelConfig), /// Set the reasoning effort level for all agents. SetReasoningEffort(Effort), } diff --git a/crates/forge_domain/src/lib.rs b/crates/forge_domain/src/lib.rs index 13a6b18135..5db0a8553b 100644 --- a/crates/forge_domain/src/lib.rs +++ b/crates/forge_domain/src/lib.rs @@ -4,7 +4,6 @@ mod auth; mod chat_request; mod chat_response; mod command; -mod commit_config; mod compact; mod console; mod context; @@ -28,6 +27,7 @@ mod message; mod message_pattern; mod migration; mod model; +mod model_config; mod node; mod point; mod policies; @@ -39,7 +39,6 @@ mod session_metrics; mod shell; mod skill; mod snapshot; -mod suggest_config; mod suggestion; mod system_context; mod temperature; @@ -60,7 +59,6 @@ pub use attachment::*; pub use chat_request::*; pub use chat_response::*; pub use command::*; -pub use commit_config::*; pub use compact::*; pub use console::*; pub use context::*; @@ -84,6 +82,7 @@ pub use message::*; pub use message_pattern::*; pub use migration::*; pub use model::*; +pub use model_config::*; pub use node::*; pub use point::*; pub use policies::*; @@ -95,7 +94,6 @@ pub use session_metrics::*; pub use shell::*; pub use skill::*; pub use snapshot::*; -pub use suggest_config::*; pub use suggestion::*; pub use system_context::*; pub use temperature::*; diff --git a/crates/forge_domain/src/model_config.rs b/crates/forge_domain/src/model_config.rs new file mode 100644 index 0000000000..930f514969 --- /dev/null +++ b/crates/forge_domain/src/model_config.rs @@ -0,0 +1,30 @@ +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::{ModelId, ProviderId}; + +/// Domain-level configuration that pairs a provider with a model. +/// +/// Used as the unified payload for [`super::ConfigOperation`] variants that +/// configure a provider/model pair (session, commit, suggest). Both fields are +/// required; use `Option` at the call-site when the configuration +/// is optional. +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] +pub struct ModelConfig { + /// The provider ID (e.g. `"anthropic"`). + pub provider: ProviderId, + + /// The model ID to use with this provider. + pub model: ModelId, +} + +impl ModelConfig { + /// Creates a new [`ModelConfig`] with the given provider and model. + /// + /// # Arguments + /// * `provider` - The provider identifier + /// * `model` - The model identifier + pub fn new(provider: impl Into, model: impl Into) -> Self { + Self { provider: provider.into(), model: model.into() } + } +} diff --git a/crates/forge_domain/src/suggest_config.rs b/crates/forge_domain/src/suggest_config.rs deleted file mode 100644 index 0ac93ee54c..0000000000 --- a/crates/forge_domain/src/suggest_config.rs +++ /dev/null @@ -1,22 +0,0 @@ -use derive_setters::Setters; -use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; - -use crate::{ModelId, ProviderId}; - -/// Configuration for shell command suggestion generation. -/// -/// Allows specifying a dedicated provider and model for shell command -/// suggestion generation, instead of using the active agent's provider and -/// model. This is useful when you want to use a cheaper or faster model for -/// simple command suggestions. Both provider and model must be specified -/// together. -#[derive(Debug, Clone, Serialize, Deserialize, Setters, JsonSchema, PartialEq)] -#[setters(into)] -pub struct SuggestConfig { - /// Provider ID to use for command suggestion generation. - pub provider: ProviderId, - - /// Model ID to use for command suggestion generation. - pub model: ModelId, -} diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index cffa3f5081..d609db1775 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -32,13 +32,9 @@ pub fn to_environment(cwd: PathBuf) -> Environment { /// persisted config without an intermediate `Environment` round-trip. fn apply_config_op(fc: &mut ForgeConfig, op: ConfigOperation) { match op { - ConfigOperation::SetProvider(pid) => { - let session = fc.session.get_or_insert_with(ModelConfig::default); - session.provider_id = Some(pid.as_ref().to_string()); - } - ConfigOperation::SetModel(pid, mid) => { - let pid_str = pid.as_ref().to_string(); - let mid_str = mid.to_string(); + ConfigOperation::SetSessionConfig(mc) => { + let pid_str = mc.provider.as_ref().to_string(); + let mid_str = mc.model.to_string(); let session = fc.session.get_or_insert_with(ModelConfig::default); if session.provider_id.as_deref() == Some(&pid_str) { session.model_id = Some(mid_str); @@ -47,20 +43,16 @@ fn apply_config_op(fc: &mut ForgeConfig, op: ConfigOperation) { Some(ModelConfig { provider_id: Some(pid_str), model_id: Some(mid_str) }); } } - ConfigOperation::SetCommitConfig(commit) => { - fc.commit = commit - .provider - .as_ref() - .zip(commit.model.as_ref()) - .map(|(pid, mid)| ModelConfig { - provider_id: Some(pid.as_ref().to_string()), - model_id: Some(mid.to_string()), - }); + ConfigOperation::SetCommitConfig(mc) => { + fc.commit = mc.map(|m| ModelConfig { + provider_id: Some(m.provider.as_ref().to_string()), + model_id: Some(m.model.to_string()), + }); } - ConfigOperation::SetSuggestConfig(suggest) => { + ConfigOperation::SetSuggestConfig(mc) => { fc.suggest = Some(ModelConfig { - provider_id: Some(suggest.provider.as_ref().to_string()), - model_id: Some(suggest.model.to_string()), + provider_id: Some(mc.provider.as_ref().to_string()), + model_id: Some(mc.model.to_string()), }); } ConfigOperation::SetReasoningEffort(effort) => { @@ -142,6 +134,10 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { to_environment(self.cwd.clone()) } + fn get_config(&self) -> anyhow::Result { + self.cached_config() + } + async fn update_environment(&self, ops: Vec) -> anyhow::Result<()> { // Load the global config (with defaults applied) for the update round-trip let mut fc = ConfigReader::default() @@ -228,30 +224,35 @@ mod tests { } #[test] - fn test_apply_config_op_set_provider() { - use forge_domain::ProviderId; + fn test_apply_config_op_set_model() { + use forge_domain::{ModelConfig as DomainModelConfig, ModelId, ProviderId}; let mut fixture = ForgeConfig::default(); apply_config_op( &mut fixture, - ConfigOperation::SetProvider(ProviderId::ANTHROPIC), + ConfigOperation::SetSessionConfig(DomainModelConfig::new( + ProviderId::ANTHROPIC, + ModelId::new("claude-3-5-sonnet"), + )), ); - let actual = fixture + let actual_provider = fixture .session .as_ref() .and_then(|s| s.provider_id.as_deref()); - let expected = Some("anthropic"); + let actual_model = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); - assert_eq!(actual, expected); + assert_eq!(actual_provider, Some("anthropic")); + assert_eq!(actual_model, Some("claude-3-5-sonnet")); } #[test] fn test_apply_config_op_set_model_matching_provider() { - use forge_domain::{ModelId, ProviderId}; + use forge_config::ModelConfig as ForgeCfgModelConfig; + use forge_domain::{ModelConfig as DomainModelConfig, ModelId, ProviderId}; let mut fixture = ForgeConfig { - session: Some(ModelConfig { + session: Some(ForgeCfgModelConfig { provider_id: Some("anthropic".to_string()), model_id: None, }), @@ -260,10 +261,10 @@ mod tests { apply_config_op( &mut fixture, - ConfigOperation::SetModel( + ConfigOperation::SetSessionConfig(DomainModelConfig::new( ProviderId::ANTHROPIC, ModelId::new("claude-3-5-sonnet-20241022"), - ), + )), ); let actual = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); @@ -274,10 +275,11 @@ mod tests { #[test] fn test_apply_config_op_set_model_different_provider_replaces_session() { - use forge_domain::{ModelId, ProviderId}; + use forge_config::ModelConfig as ForgeCfgModelConfig; + use forge_domain::{ModelConfig as DomainModelConfig, ModelId, ProviderId}; let mut fixture = ForgeConfig { - session: Some(ModelConfig { + session: Some(ForgeCfgModelConfig { provider_id: Some("openai".to_string()), model_id: Some("gpt-4".to_string()), }), @@ -286,10 +288,10 @@ mod tests { apply_config_op( &mut fixture, - ConfigOperation::SetModel( + ConfigOperation::SetSessionConfig(DomainModelConfig::new( ProviderId::ANTHROPIC, ModelId::new("claude-3-5-sonnet-20241022"), - ), + )), ); let actual_provider = fixture diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index 399584fe04..685f0c2e3b 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -61,8 +61,8 @@ impl ForgeInfra { /// # Arguments /// * `cwd` - The working directory for command execution and environment /// resolution - /// * `config` - Pre-read application configuration; passed through to all - /// consumers + /// * `config` - Pre-read application configuration; used only at + /// construction time to initialize infrastructure services /// * `services_url` - Pre-validated URL for the gRPC workspace server pub fn new(cwd: PathBuf, config: forge_config::ForgeConfig, services_url: Url) -> Self { let env = to_environment(cwd.clone()); @@ -70,13 +70,16 @@ impl ForgeInfra { let file_write_service = Arc::new(ForgeFileWriteService::new()); let http_service = Arc::new(ForgeHttpInfra::new( - config.clone(), + config_infra.cached_config().unwrap_or(config), file_write_service.clone(), )); let file_read_service = Arc::new(ForgeFileReadService::new()); let file_meta_service = Arc::new(ForgeFileMetaService); let directory_reader_service = Arc::new(ForgeDirectoryReaderService::new( - config.max_parallel_file_reads, + config_infra + .cached_config() + .map(|c| c.max_parallel_file_reads) + .unwrap_or(4), )); let grpc_client = Arc::new(ForgeGrpcClient::new(services_url)); let output_printer = Arc::new(StdConsoleWriter::default()); @@ -131,6 +134,10 @@ impl EnvironmentInfra for ForgeInfra { self.config_infra.get_environment() } + fn get_config(&self) -> anyhow::Result { + self.config_infra.get_config() + } + async fn update_environment( &self, ops: Vec, diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 36caa17c60..1980ff11c8 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -11,8 +11,8 @@ use console::style; use convert_case::{Case, Casing}; use forge_api::{ API, AgentId, AnyProvider, ApiKeyRequest, AuthContextRequest, AuthContextResponse, ChatRequest, - ChatResponse, CodeRequest, Conversation, ConversationId, DeviceCodeRequest, Event, - InterruptionReason, ModelId, Provider, ProviderId, TextMessage, UserPrompt, + ChatResponse, CodeRequest, ConfigOperation, Conversation, ConversationId, DeviceCodeRequest, + Event, InterruptionReason, ModelId, Provider, ProviderId, TextMessage, UserPrompt, }; use forge_app::utils::{format_display_path, truncate_key}; use forge_app::{CommitResult, ToolResolver}; @@ -1339,13 +1339,11 @@ impl A + Send + Sync> UI let commit_config = self.api.get_commit_config().await.ok().flatten(); let commit_provider = commit_config .as_ref() - .and_then(|c| c.provider.as_ref()) - .map(|p| p.to_string()) + .map(|c| c.provider.to_string()) .unwrap_or_else(|| markers::EMPTY.to_string()); let commit_model = commit_config .as_ref() - .and_then(|c| c.model.as_ref()) - .map(|m| m.as_str().to_string()) + .map(|c| c.model.as_str().to_string()) .unwrap_or_else(|| markers::EMPTY.to_string()); let suggest_config = self.api.get_suggest_config().await.ok().flatten(); @@ -2765,10 +2763,18 @@ impl A + Send + Sync> UI // If we have a provider to activate, write both atomically if let Some(provider_id) = provider_to_activate { self.api - .set_default_provider_and_model(provider_id, model.clone()) + .update_config(vec![ConfigOperation::SetSessionConfig( + forge_domain::ModelConfig::new(provider_id, model.clone()), + )]) .await?; } else { - self.api.set_default_model(model.clone()).await?; + // Resolve the active provider so we can build a SetModel op + let provider_id = self.api.get_default_provider().await?.id; + self.api + .update_config(vec![ConfigOperation::SetSessionConfig( + forge_domain::ModelConfig::new(provider_id, model.clone()), + )]) + .await?; } // Update the UI state with the new model @@ -2844,7 +2850,9 @@ impl A + Send + Sync> UI .validate_model(model.as_str(), Some(&provider.id)) .await?; self.api - .set_default_provider_and_model(provider.id.clone(), model_id.clone()) + .update_config(vec![ConfigOperation::SetSessionConfig( + forge_domain::ModelConfig::new(provider.id.clone(), model_id.clone()), + )]) .await?; self.writeln_title( TitleFormat::action(format!("{}", provider.id)) @@ -2858,8 +2866,8 @@ impl A + Send + Sync> UI // Check if the current model is available for the new provider let current_model = self.api.get_default_model().await; - let needs_model_selection = match current_model { - None => true, + let (needs_model_selection, compatible_model) = match current_model { + None => (true, None), Some(current_model) => { let provider_models = self.api.get_all_provider_models().await?; let model_available = provider_models @@ -2867,7 +2875,11 @@ impl A + Send + Sync> UI .find(|pm| pm.provider_id == provider.id) .map(|pm| pm.models.iter().any(|m| m.id == current_model)) .unwrap_or(false); - !model_available + if model_available { + (false, Some(current_model)) + } else { + (true, None) + } } }; @@ -2881,9 +2893,15 @@ impl A + Send + Sync> UI return Ok(()); } } else { - // Set the provider via API - // Only reaches here if model is confirmed — safe to write provider now - self.api.set_default_provider(provider.id.clone()).await?; + // The current model is compatible with the new provider — write both + // atomically so the session always stores a consistent pair. + let model = + compatible_model.expect("compatible_model is Some when !needs_model_selection"); + self.api + .update_config(vec![ConfigOperation::SetSessionConfig( + forge_domain::ModelConfig::new(provider.id.clone(), model), + )]) + .await?; self.writeln_title( TitleFormat::action(format!("{}", provider.id)) @@ -3536,7 +3554,13 @@ impl A + Send + Sync> UI } ConfigSetField::Model { model } => { let model_id = self.validate_model(model.as_str(), None).await?; - self.api.set_default_model(model_id.clone()).await?; + // Resolve the active provider so we can build a SetModel op + let provider_id = self.api.get_default_provider().await?.id; + self.api + .update_config(vec![ConfigOperation::SetSessionConfig( + forge_domain::ModelConfig::new(provider_id, model_id.clone()), + )]) + .await?; self.writeln_title( TitleFormat::action(model_id.as_str()).sub_title("is now the default model"), )?; @@ -3544,10 +3568,11 @@ impl A + Send + Sync> UI ConfigSetField::Commit { provider, model } => { // Validate provider exists and model belongs to that specific provider let validated_model = self.validate_model(model.as_str(), Some(&provider)).await?; - let commit_config = forge_domain::CommitConfig::default() - .provider(provider.clone()) - .model(validated_model.clone()); - self.api.set_commit_config(commit_config).await?; + let commit_config = + forge_domain::ModelConfig::new(provider.clone(), validated_model.clone()); + self.api + .update_config(vec![ConfigOperation::SetCommitConfig(Some(commit_config))]) + .await?; self.writeln_title( TitleFormat::action(validated_model.as_str()) .sub_title(format!("is now the commit model for provider '{provider}'")), @@ -3556,17 +3581,19 @@ impl A + Send + Sync> UI ConfigSetField::Suggest { provider, model } => { // Validate provider exists and model belongs to that specific provider let validated_model = self.validate_model(model.as_str(), Some(&provider)).await?; - let suggest_config = forge_domain::SuggestConfig { - provider: provider.clone(), - model: validated_model.clone(), - }; - self.api.set_suggest_config(suggest_config).await?; + let suggest_config = + forge_domain::ModelConfig::new(provider.clone(), validated_model.clone()); + self.api + .update_config(vec![ConfigOperation::SetSuggestConfig(suggest_config)]) + .await?; self.writeln_title(TitleFormat::action(validated_model.as_str()).sub_title( format!("is now the suggest model for provider '{provider}'"), ))?; } ConfigSetField::ReasoningEffort { effort } => { - self.api.set_reasoning_effort(effort.clone()).await?; + self.api + .update_config(vec![ConfigOperation::SetReasoningEffort(effort.clone())]) + .await?; self.writeln_title( TitleFormat::action(effort.to_string()) .sub_title("is now the reasoning effort"), @@ -3609,16 +3636,8 @@ impl A + Send + Sync> UI let commit_config = self.api.get_commit_config().await?; match commit_config { Some(config) => { - let provider = config - .provider - .map(|p| p.as_ref().to_string()) - .unwrap_or_else(|| "Not set".to_string()); - let model = config - .model - .map(|m| m.as_str().to_string()) - .unwrap_or_else(|| "Not set".to_string()); - self.writeln(provider)?; - self.writeln(model)?; + self.writeln(config.provider.as_ref())?; + self.writeln(config.model.as_str().to_string())?; } None => self.writeln("Commit: Not set")?, } diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index 4fa8ebedf0..be07f80bf7 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -51,8 +51,15 @@ pub struct ForgeRepo { fuzzy_search_repository: Arc>, } -impl ForgeRepo { - pub fn new(infra: Arc, config: forge_config::ForgeConfig) -> Self { +impl< + F: EnvironmentInfra + + FileReaderInfra + + FileWriterInfra + + GrpcInfra + + HttpInfra, +> ForgeRepo +{ + pub fn new(infra: Arc) -> Self { let env = infra.get_environment(); let file_snapshot_service = Arc::new(ForgeFileSnapshotService::new(env.clone())); let db_pool = @@ -67,15 +74,8 @@ impl ConversationRepository for ForgeRepo { } #[async_trait::async_trait] -impl - ChatRepository for ForgeRepo +impl< + F: EnvironmentInfra + + FileReaderInfra + + FileWriterInfra + + HttpInfra + + Send + + Sync, +> ChatRepository for ForgeRepo { async fn chat( &self, @@ -165,8 +171,14 @@ impl - ProviderRepository for ForgeRepo +impl< + F: EnvironmentInfra + + FileReaderInfra + + FileWriterInfra + + HttpInfra + + Send + + Sync, +> ProviderRepository for ForgeRepo { async fn get_all_providers(&self) -> anyhow::Result> { self.provider_repository.get_all_providers().await @@ -196,13 +208,19 @@ impl EnvironmentInfra for ForgeRepo { +impl + Send + Sync> EnvironmentInfra + for ForgeRepo +{ type Config = forge_config::ForgeConfig; fn get_environment(&self) -> Environment { self.infra.get_environment() } + fn get_config(&self) -> anyhow::Result { + self.infra.get_config() + } + fn update_environment( &self, ops: Vec, diff --git a/crates/forge_repo/src/provider/anthropic.rs b/crates/forge_repo/src/provider/anthropic.rs index c4df9cfc26..3292f5ab9f 100644 --- a/crates/forge_repo/src/provider/anthropic.rs +++ b/crates/forge_repo/src/provider/anthropic.rs @@ -1,9 +1,7 @@ use std::sync::Arc; use anyhow::Context as _; -use derive_setters::Setters; use eventsource_stream::Eventsource; -use forge_app::HttpInfra; use forge_app::domain::{ ChatCompletionMessage, Context, Model, ModelId, ResultStream, Transformer, }; @@ -12,7 +10,7 @@ use forge_app::dto::anthropic::{ EventData, ListModelResponse, ReasoningTransform, RemoveOutputFormat, Request, SanitizeToolIds, SetCache, }; -use forge_config::RetryConfig; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::{ChatRepository, Provider, ProviderId}; use futures::StreamExt; use reqwest::Url; @@ -285,6 +283,84 @@ where } } +/// Repository for Anthropic provider responses +pub struct AnthropicResponseRepository { + infra: Arc, +} + +impl AnthropicResponseRepository { + pub fn new(infra: Arc) -> Self { + Self { infra } + } +} + +impl AnthropicResponseRepository { + /// Creates an Anthropic client from a provider configuration + fn create_client(&self, provider: Provider) -> anyhow::Result> { + // Validate that credentials exist + provider + .credential + .as_ref() + .context("Anthropic provider requires credentials")?; + + // Determine OAuth usage based on auth details + let is_oauth = provider + .credential + .as_ref() + .map(|c| matches!(c.auth_details, forge_domain::AuthDetails::OAuth { .. })) + .unwrap_or(false); + + // Use different API version for Vertex AI + let version = if provider.id == ProviderId::VERTEX_AI_ANTHROPIC { + "vertex-2023-10-16".to_string() + } else { + "2023-06-01".to_string() + }; + + Ok(Anthropic::new( + self.infra.clone(), + provider, + version, + is_oauth, + )) + } +} + +#[async_trait::async_trait] +impl + 'static> ChatRepository + for AnthropicResponseRepository +{ + async fn chat( + &self, + model_id: &ModelId, + context: Context, + provider: Provider, + ) -> ResultStream { + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); + let provider_client = self.create_client(provider)?; + + let stream = provider_client + .chat(model_id, context) + .await + .map_err(|e| into_retry(e, &retry_config))?; + + Ok(Box::pin(stream.map(move |item| { + item.map_err(|e| into_retry(e, &retry_config)) + }))) + } + + async fn models(&self, provider: Provider) -> anyhow::Result> { + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); + let provider_client = self.create_client(provider)?; + + provider_client + .models() + .await + .map_err(|e| into_retry(e, &retry_config)) + .context("Failed to fetch models from Anthropic provider") + } +} + #[cfg(test)] mod tests { @@ -780,82 +856,3 @@ mod tests { ); } } - -/// Repository for Anthropic provider responses -#[derive(Setters)] -#[setters(strip_option, into)] -pub struct AnthropicResponseRepository { - infra: Arc, - retry_config: Arc, -} - -impl AnthropicResponseRepository { - pub fn new(infra: Arc) -> Self { - Self { infra, retry_config: Arc::new(RetryConfig::default()) } - } -} - -impl AnthropicResponseRepository { - /// Creates an Anthropic client from a provider configuration - fn create_client(&self, provider: Provider) -> anyhow::Result> { - // Validate that credentials exist - provider - .credential - .as_ref() - .context("Anthropic provider requires credentials")?; - - // Determine OAuth usage based on auth details - let is_oauth = provider - .credential - .as_ref() - .map(|c| matches!(c.auth_details, forge_domain::AuthDetails::OAuth { .. })) - .unwrap_or(false); - - // Use different API version for Vertex AI - let version = if provider.id == ProviderId::VERTEX_AI_ANTHROPIC { - "vertex-2023-10-16".to_string() - } else { - "2023-06-01".to_string() - }; - - Ok(Anthropic::new( - self.infra.clone(), - provider, - version, - is_oauth, - )) - } -} - -#[async_trait::async_trait] -impl ChatRepository for AnthropicResponseRepository { - async fn chat( - &self, - model_id: &ModelId, - context: Context, - provider: Provider, - ) -> ResultStream { - let retry_config = self.retry_config.clone(); - let provider_client = self.create_client(provider)?; - - let stream = provider_client - .chat(model_id, context) - .await - .map_err(|e| into_retry(e, &retry_config))?; - - Ok(Box::pin(stream.map(move |item| { - item.map_err(|e| into_retry(e, &retry_config)) - }))) - } - - async fn models(&self, provider: Provider) -> anyhow::Result> { - let retry_config = self.retry_config.clone(); - let provider_client = self.create_client(provider)?; - - provider_client - .models() - .await - .map_err(|e| into_retry(e, &retry_config)) - .context("Failed to fetch models from Anthropic provider") - } -} diff --git a/crates/forge_repo/src/provider/chat.rs b/crates/forge_repo/src/provider/chat.rs index fccc2505c3..b57690eccc 100644 --- a/crates/forge_repo/src/provider/chat.rs +++ b/crates/forge_repo/src/provider/chat.rs @@ -24,34 +24,24 @@ pub struct ForgeChatRepository { bg_refresh: BgRefresh, } -impl ForgeChatRepository { +impl + HttpInfra> ForgeChatRepository { /// Creates a new ForgeChatRepository with the given infrastructure. /// /// # Arguments /// /// * `infra` - Infrastructure providing environment and HTTP capabilities - /// * `retry_config` - Retry configuration extracted from startup config - /// * `model_cache_ttl_secs` - Model cache TTL in seconds from startup - /// config - pub fn new( - infra: Arc, - retry_config: forge_config::RetryConfig, - model_cache_ttl_secs: u64, - ) -> Self { + pub fn new(infra: Arc) -> Self { let env = infra.get_environment(); - let retry_config = Arc::new(retry_config); - - let openai_repo = - OpenAIResponseRepository::new(infra.clone()).retry_config(retry_config.clone()); - let codex_repo = OpenAIResponsesResponseRepository::new(infra.clone()) - .retry_config(retry_config.clone()); - let anthropic_repo = - AnthropicResponseRepository::new(infra.clone()).retry_config(retry_config.clone()); - let bedrock_repo = BedrockResponseRepository::new(retry_config.clone()); - let google_repo = - GoogleResponseRepository::new(infra.clone()).retry_config(retry_config.clone()); - let opencode_zen_repo = - OpenCodeZenResponseRepository::new(infra.clone()).retry_config(retry_config.clone()); + let config = infra.get_config().unwrap_or_default(); + let model_cache_ttl_secs = config.model_cache_ttl_secs; + + let openai_repo = OpenAIResponseRepository::new(infra.clone()); + let codex_repo = OpenAIResponsesResponseRepository::new(infra.clone()); + let anthropic_repo = AnthropicResponseRepository::new(infra.clone()); + let bedrock_repo = + BedrockResponseRepository::new(Arc::new(config.retry.unwrap_or_default())); + let google_repo = GoogleResponseRepository::new(infra.clone()); + let opencode_zen_repo = OpenCodeZenResponseRepository::new(infra.clone()); let model_cache = Arc::new(CacacheStorage::new( env.cache_dir().join("model_cache"), @@ -74,7 +64,9 @@ impl ForgeChatRepository { } #[async_trait::async_trait] -impl ChatRepository for ForgeChatRepository { +impl + HttpInfra + Sync> ChatRepository + for ForgeChatRepository +{ async fn chat( &self, model_id: &ModelId, @@ -138,7 +130,7 @@ struct ProviderRouter { opencode_zen_repo: OpenCodeZenResponseRepository, } -impl ProviderRouter { +impl + Sync> ProviderRouter { async fn chat( &self, model_id: &ModelId, diff --git a/crates/forge_repo/src/provider/google.rs b/crates/forge_repo/src/provider/google.rs index e8af9a533a..390f1cd7f6 100644 --- a/crates/forge_repo/src/provider/google.rs +++ b/crates/forge_repo/src/provider/google.rs @@ -1,11 +1,9 @@ use std::sync::Arc; use anyhow::Context as _; -use derive_setters::Setters; -use forge_app::HttpInfra; use forge_app::domain::{ChatCompletionMessage, Context, Model, ModelId, ResultStream}; use forge_app::dto::google::{EventData, Request}; -use forge_config::RetryConfig; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::{ChatRepository, Provider}; use reqwest::Url; use tokio_stream::StreamExt; @@ -139,16 +137,13 @@ impl Google { } /// Repository for Google provider responses -#[derive(Setters)] -#[setters(strip_option, into)] pub struct GoogleResponseRepository { infra: Arc, - retry_config: Arc, } impl GoogleResponseRepository { pub fn new(infra: Arc) -> Self { - Self { infra, retry_config: Arc::new(RetryConfig::default()) } + Self { infra } } } @@ -191,14 +186,16 @@ impl GoogleResponseRepository { } } #[async_trait::async_trait] -impl ChatRepository for GoogleResponseRepository { +impl + 'static> ChatRepository + for GoogleResponseRepository +{ async fn chat( &self, model_id: &ModelId, context: Context, provider: Provider, ) -> ResultStream { - let retry_config = self.retry_config.clone(); + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); let provider_client = self.create_client(&provider)?; let stream = provider_client @@ -212,7 +209,7 @@ impl ChatRepository for GoogleResponseRepository { } async fn models(&self, provider: Provider) -> anyhow::Result> { - let retry_config = self.retry_config.clone(); + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); let provider_client = self.create_client(&provider)?; provider_client diff --git a/crates/forge_repo/src/provider/openai.rs b/crates/forge_repo/src/provider/openai.rs index f3dc680a2e..31eccd8592 100644 --- a/crates/forge_repo/src/provider/openai.rs +++ b/crates/forge_repo/src/provider/openai.rs @@ -1,14 +1,12 @@ use std::sync::{Arc, LazyLock}; use anyhow::{Context as _, Result}; -use derive_setters::Setters; -use forge_app::HttpInfra; use forge_app::domain::{ ChatCompletionMessage, Context as ChatContext, Model, ModelId, ProviderId, ResultStream, Transformer, }; use forge_app::dto::openai::{ListModelResponse, ProviderPipeline, Request, Response}; -use forge_config::RetryConfig; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::{ChatRepository, Provider}; use forge_infra::sanitize_headers; use reqwest::header::AUTHORIZATION; @@ -311,6 +309,59 @@ impl OpenAIProvider { } } +/// Repository for OpenAI-compatible provider responses +/// +/// Handles providers that use OpenAI's API format including: +/// - OpenAI +/// - Azure OpenAI +/// - Vertex AI +/// - OpenRouter +/// - DeepSeek +/// - Groq +pub struct OpenAIResponseRepository { + infra: Arc, +} + +impl OpenAIResponseRepository { + pub fn new(infra: Arc) -> Self { + Self { infra } + } +} + +#[async_trait::async_trait] +impl + 'static> ChatRepository + for OpenAIResponseRepository +{ + async fn chat( + &self, + model_id: &ModelId, + context: ChatContext, + provider: Provider, + ) -> ResultStream { + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); + let provider_id = provider.id.clone(); + let provider_client = OpenAIProvider::new(provider, self.infra.clone()); + let stream = provider_client + .chat(model_id, context) + .await + .map_err(|e| into_retry(e, &retry_config))?; + + Ok(Box::pin(stream.map(move |item| { + item.map_err(|e| enhance_error(into_retry(e, &retry_config), &provider_id)) + }))) + } + + async fn models(&self, provider: Provider) -> anyhow::Result> { + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); + let provider_client = OpenAIProvider::new(provider, self.infra.clone()); + provider_client + .models() + .await + .map_err(|e| into_retry(e, &retry_config)) + .context("Failed to fetch models from OpenAI-compatible provider") + } +} + #[cfg(test)] mod tests { @@ -1119,57 +1170,3 @@ mod tests { Ok(()) } } - -/// Repository for OpenAI-compatible provider responses -/// -/// Handles providers that use OpenAI's API format including: -/// - OpenAI -/// - Azure OpenAI -/// - Vertex AI -/// - OpenRouter -/// - DeepSeek -/// - Groq -#[derive(Setters)] -#[setters(strip_option, into)] -pub struct OpenAIResponseRepository { - infra: Arc, - retry_config: Arc, -} - -impl OpenAIResponseRepository { - pub fn new(infra: Arc) -> Self { - Self { infra, retry_config: Arc::new(RetryConfig::default()) } - } -} - -#[async_trait::async_trait] -impl ChatRepository for OpenAIResponseRepository { - async fn chat( - &self, - model_id: &ModelId, - context: ChatContext, - provider: Provider, - ) -> ResultStream { - let retry_config = self.retry_config.clone(); - let provider_id = provider.id.clone(); - let provider_client = OpenAIProvider::new(provider, self.infra.clone()); - let stream = provider_client - .chat(model_id, context) - .await - .map_err(|e| into_retry(e, &retry_config))?; - - Ok(Box::pin(stream.map(move |item| { - item.map_err(|e| enhance_error(into_retry(e, &retry_config), &provider_id)) - }))) - } - - async fn models(&self, provider: Provider) -> anyhow::Result> { - let retry_config = self.retry_config.clone(); - let provider_client = OpenAIProvider::new(provider, self.infra.clone()); - provider_client - .models() - .await - .map_err(|e| into_retry(e, &retry_config)) - .context("Failed to fetch models from OpenAI-compatible provider") - } -} diff --git a/crates/forge_repo/src/provider/openai_responses/repository.rs b/crates/forge_repo/src/provider/openai_responses/repository.rs index 35b90d1126..e311d13c20 100644 --- a/crates/forge_repo/src/provider/openai_responses/repository.rs +++ b/crates/forge_repo/src/provider/openai_responses/repository.rs @@ -2,13 +2,11 @@ use std::sync::Arc; use anyhow::Context as _; use async_openai::types::responses as oai; -use derive_setters::Setters; use eventsource_stream::Eventsource; -use forge_app::HttpInfra; use forge_app::domain::{ ChatCompletionMessage, Context as ChatContext, Model, ModelId, ResultStream, }; -use forge_config::RetryConfig; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::{BoxStream, ChatRepository, Provider}; use forge_infra::sanitize_headers; use futures::StreamExt; @@ -356,28 +354,27 @@ fn request_message_count(request: &oai::CreateResponse) -> usize { /// /// Handles OpenAI's Codex models (e.g., gpt-5.1-codex, codex-mini-latest) /// which use the Responses API instead of the standard Chat Completions API. -#[derive(Setters)] -#[setters(strip_option, into)] pub struct OpenAIResponsesResponseRepository { infra: Arc, - retry_config: Arc, } impl OpenAIResponsesResponseRepository { pub fn new(infra: Arc) -> Self { - Self { infra, retry_config: Arc::new(RetryConfig::default()) } + Self { infra } } } #[async_trait::async_trait] -impl ChatRepository for OpenAIResponsesResponseRepository { +impl + 'static> ChatRepository + for OpenAIResponsesResponseRepository +{ async fn chat( &self, model_id: &ModelId, context: ChatContext, provider: Provider, ) -> ResultStream { - let retry_config = self.retry_config.clone(); + let retry_config = self.infra.get_config()?.retry.unwrap_or_default(); let provider_client: OpenAIResponsesProvider = OpenAIResponsesProvider::new(provider, self.infra.clone()); let stream = provider_client @@ -523,6 +520,34 @@ mod tests { } } + impl forge_app::EnvironmentInfra for MockHttpClient { + type Config = forge_config::ForgeConfig; + + fn get_env_var(&self, _key: &str) -> Option { + None + } + + fn get_env_vars(&self) -> std::collections::BTreeMap { + std::collections::BTreeMap::new() + } + + fn get_environment(&self) -> forge_domain::Environment { + use fake::{Fake, Faker}; + Faker.fake() + } + + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + + async fn update_environment( + &self, + _ops: Vec, + ) -> anyhow::Result<()> { + Ok(()) + } + } + /// Test fixture for creating a sample OpenAI Responses API response. fn openai_response_fixture() -> serde_json::Value { serde_json::json!({ diff --git a/crates/forge_repo/src/provider/opencode_zen.rs b/crates/forge_repo/src/provider/opencode_zen.rs index fa3827b5b0..792586622e 100644 --- a/crates/forge_repo/src/provider/opencode_zen.rs +++ b/crates/forge_repo/src/provider/opencode_zen.rs @@ -1,13 +1,11 @@ use std::sync::Arc; use anyhow::Result; -use derive_setters::Setters; -use forge_app::HttpInfra; use forge_app::domain::{ ChatCompletionMessage, Context as ChatContext, Model, ModelId, Provider, ProviderResponse, ResultStream, }; -use forge_config::RetryConfig; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::ChatRepository; use url::Url; @@ -21,24 +19,22 @@ use crate::provider::openai_responses::OpenAIResponsesResponseRepository; /// - GPT-5 models (gpt-5*) -> OpenAIResponses endpoint /// - Gemini models (gemini-*) -> Google endpoint /// - Others (GLM, MiniMax, Kimi, etc.) -> OpenAI endpoint -#[derive(Setters)] -#[setters(strip_option, into)] pub struct OpenCodeZenResponseRepository { openai_repo: OpenAIResponseRepository, codex_repo: OpenAIResponsesResponseRepository, anthropic_repo: AnthropicResponseRepository, google_repo: GoogleResponseRepository, - retry_config: Arc, } -impl OpenCodeZenResponseRepository { +impl + Sync> + OpenCodeZenResponseRepository +{ pub fn new(infra: Arc) -> Self { Self { openai_repo: OpenAIResponseRepository::new(infra.clone()), codex_repo: OpenAIResponsesResponseRepository::new(infra.clone()), anthropic_repo: AnthropicResponseRepository::new(infra.clone()), google_repo: GoogleResponseRepository::new(infra.clone()), - retry_config: Arc::new(RetryConfig::default()), } } diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 2806ede74d..74c71e9c08 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -204,17 +204,22 @@ fn get_provider_configs() -> &'static Vec { pub struct ForgeProviderRepository { infra: Arc, - config_providers: Vec, } -impl ForgeProviderRepository { - pub fn new(infra: Arc, config_providers: Vec) -> Self { - Self { infra, config_providers } +impl + HttpInfra> + ForgeProviderRepository +{ + pub fn new(infra: Arc) -> Self { + Self { infra } } } -impl - ForgeProviderRepository +impl< + F: EnvironmentInfra + + FileReaderInfra + + FileWriterInfra + + HttpInfra, +> ForgeProviderRepository { async fn get_custom_provider_configs(&self) -> anyhow::Result> { let environment = self.infra.get_environment(); @@ -228,9 +233,11 @@ impl /// Converts provider entries from `ForgeConfig` into `ProviderConfig` /// instances that can be merged into the provider list. fn get_config_provider_configs(&self) -> Vec { - self.config_providers - .iter() - .cloned() + self.infra + .get_config() + .unwrap_or_default() + .providers + .into_iter() .map(Into::into) .collect() } @@ -520,8 +527,13 @@ impl } #[async_trait::async_trait] -impl ProviderRepository - for ForgeProviderRepository +impl< + F: EnvironmentInfra + + FileReaderInfra + + FileWriterInfra + + HttpInfra + + Sync, +> ProviderRepository for ForgeProviderRepository { async fn get_all_providers(&self) -> anyhow::Result> { Ok(self.get_providers().await) @@ -805,6 +817,10 @@ mod env_tests { Ok(()) } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + fn get_env_var(&self, key: &str) -> Option { self.env_vars.get(key).cloned() } @@ -977,7 +993,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone(), vec![]); + let registry = ForgeProviderRepository::new(infra.clone()); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1046,7 +1062,7 @@ mod env_tests { env_vars.insert("OPENAI_API_KEY".to_string(), "test-key".to_string()); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone(), vec![]); + let registry = ForgeProviderRepository::new(infra.clone()); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1093,7 +1109,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone(), vec![]); + let registry = ForgeProviderRepository::new(infra.clone()); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1157,7 +1173,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra, vec![]); + let registry = ForgeProviderRepository::new(infra); // Trigger migration to populate credentials file registry.migrate_env_to_file().await.unwrap(); @@ -1214,7 +1230,7 @@ mod env_tests { env_vars.insert("ANTHROPIC_API_KEY".to_string(), "test-key".to_string()); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra, vec![]); + let registry = ForgeProviderRepository::new(infra); // Migrate environment variables to credentials file registry.migrate_env_to_file().await.unwrap(); @@ -1301,6 +1317,10 @@ mod env_tests { Ok(()) } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + fn get_env_var(&self, key: &str) -> Option { self.env_vars.get(key).cloned() } @@ -1446,7 +1466,7 @@ mod env_tests { } let infra = Arc::new(CustomMockInfra { env_vars, base_path }); - let registry = ForgeProviderRepository::new(infra, vec![]); + let registry = ForgeProviderRepository::new(infra); // Get merged configs let merged_configs = registry.get_merged_configs().await; diff --git a/crates/forge_services/src/agent_registry.rs b/crates/forge_services/src/agent_registry.rs index dc718e3682..37a801c09f 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use dashmap::DashMap; use forge_app::domain::{AgentId, Error, ModelId, ProviderId}; use forge_app::{AgentRepository, EnvironmentInfra}; -use forge_config::ForgeConfig; use forge_domain::Agent; use tokio::sync::RwLock; @@ -14,9 +13,6 @@ pub struct ForgeAgentRegistryService { // Infrastructure dependency for loading agents repository: Arc, - // Startup configuration snapshot used to resolve default provider/model - config: ForgeConfig, - // In-memory storage for agents keyed by AgentId string // Lazily initialized on first access // Wrapped in RwLock to allow invalidation @@ -28,17 +24,18 @@ pub struct ForgeAgentRegistryService { impl ForgeAgentRegistryService { /// Creates a new AgentRegistryService with the given repository - pub fn new(repository: Arc, config: ForgeConfig) -> Self { + pub fn new(repository: Arc) -> Self { Self { repository, - config, agents: RwLock::new(None), active_agent_id: RwLock::new(None), } } } -impl ForgeAgentRegistryService { +impl> + ForgeAgentRegistryService +{ /// Lazily initializes and returns the agents map /// Loads agents from repository on first call, subsequent calls return /// cached value @@ -70,15 +67,13 @@ impl ForgeAgentRegistryService { /// Load agents from repository and populate the in-memory map. /// - /// Reads the default provider and model from [`ForgeConfig`] and passes - /// them to the repository so agents that do not specify their own - /// provider/model receive the session-level defaults. + /// Reads the default provider and model from the latest [`ForgeConfig`] + /// (via `get_config()`) and passes them to the repository so agents that + /// do not specify their own provider/model receive the session-level + /// defaults. async fn load_agents(&self) -> anyhow::Result> { - let session = self - .config - .session - .as_ref() - .ok_or(Error::NoDefaultProvider)?; + let config = self.repository.get_config()?; + let session = config.session.as_ref().ok_or(Error::NoDefaultProvider)?; let provider_id = session .provider_id .as_ref() @@ -104,8 +99,8 @@ impl ForgeAgentRegistryService { } #[async_trait::async_trait] -impl forge_app::AgentRegistry - for ForgeAgentRegistryService +impl + Send + Sync> + forge_app::AgentRegistry for ForgeAgentRegistryService { async fn get_active_agent_id(&self) -> anyhow::Result> { let agent_id = self.active_agent_id.read().await; diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index eaefedd83f..2e908d0f45 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -1,39 +1,30 @@ -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use forge_app::{AppConfigService, EnvironmentInfra}; -use forge_config::ForgeConfig; -use forge_domain::{ - CommitConfig, ConfigOperation, Effort, ModelId, ProviderId, ProviderRepository, SuggestConfig, -}; +use forge_domain::{ConfigOperation, Effort, ModelConfig, ModelId, ProviderId, ProviderRepository}; use tracing::debug; /// Service for managing user preferences for default providers and models. +/// +/// All reads go through `infra.get_config()` so they always reflect the latest +/// on-disk state after any `update_environment` call. pub struct ForgeAppConfigService { infra: Arc, - config: Arc>, } impl ForgeAppConfigService { /// Creates a new provider preferences service. - pub fn new(infra: Arc, config: ForgeConfig) -> Self { - Self { infra, config: Arc::new(Mutex::new(config)) } - } -} - -impl ForgeAppConfigService { - /// Helper method to apply a config operation atomically. - async fn update(&self, op: ConfigOperation) -> anyhow::Result<()> { - debug!(op = ?op, "Updating app config"); - self.infra.update_environment(vec![op]).await + pub fn new(infra: Arc) -> Self { + Self { infra } } } #[async_trait::async_trait] -impl AppConfigService - for ForgeAppConfigService +impl + Send + Sync> + AppConfigService for ForgeAppConfigService { async fn get_default_provider(&self) -> anyhow::Result { - let config = self.config.lock().unwrap(); + let config = self.infra.get_config()?; config .session .as_ref() @@ -42,20 +33,11 @@ impl AppConfigService .ok_or_else(|| forge_domain::Error::NoDefaultProvider.into()) } - async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()> { - self.update(ConfigOperation::SetProvider(provider_id.clone())) - .await?; - let mut config = self.config.lock().unwrap(); - let session = config.session.get_or_insert_with(Default::default); - session.provider_id = Some(provider_id.as_ref().to_string()); - Ok(()) - } - async fn get_provider_model( &self, provider_id: Option<&ProviderId>, ) -> anyhow::Result { - let config = self.config.lock().unwrap(); + let config = self.infra.get_config()?; let session = config .session @@ -87,75 +69,32 @@ impl AppConfigService } } - async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()> { - let provider_id = { - let config = self.config.lock().unwrap(); - config - .session - .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())) - .ok_or(forge_domain::Error::NoDefaultProvider)? - }; - - self.update(ConfigOperation::SetModel( - provider_id.clone(), - model.clone(), - )) - .await?; - let mut config = self.config.lock().unwrap(); - let session = config.session.get_or_insert_with(Default::default); - session.model_id = Some(model.to_string()); - Ok(()) - } - - async fn set_default_provider_and_model( - &self, - provider_id: ProviderId, - model: ModelId, - ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetModel(provider_id, model)) - .await - } - - async fn get_commit_config(&self) -> anyhow::Result> { - let config = self.config.lock().unwrap(); - Ok(config.commit.clone().map(|mc| CommitConfig { - provider: mc.provider_id.map(ProviderId::from), - model: mc.model_id.map(ModelId::new), + async fn get_commit_config(&self) -> anyhow::Result> { + let config = self.infra.get_config()?; + Ok(config.commit.clone().and_then(|mc| { + mc.provider_id + .zip(mc.model_id) + .map(|(pid, mid)| ModelConfig { + provider: ProviderId::from(pid), + model: ModelId::new(mid), + }) })) } - async fn set_commit_config( - &self, - commit_config: forge_domain::CommitConfig, - ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetCommitConfig(commit_config)) - .await - } - - async fn get_suggest_config(&self) -> anyhow::Result> { - let config = self.config.lock().unwrap(); + async fn get_suggest_config(&self) -> anyhow::Result> { + let config = self.infra.get_config()?; Ok(config.suggest.clone().and_then(|mc| { mc.provider_id .zip(mc.model_id) - .map(|(pid, mid)| SuggestConfig { + .map(|(pid, mid)| ModelConfig { provider: ProviderId::from(pid), model: ModelId::new(mid), }) })) } - async fn set_suggest_config( - &self, - suggest_config: forge_domain::SuggestConfig, - ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetSuggestConfig(suggest_config)) - .await - } - async fn get_reasoning_effort(&self) -> anyhow::Result> { - let config = self.config.lock().unwrap(); + let config = self.infra.get_config()?; Ok(config .reasoning .clone() @@ -171,9 +110,9 @@ impl AppConfigService })) } - async fn set_reasoning_effort(&self, effort: Effort) -> anyhow::Result<()> { - self.update(ConfigOperation::SetReasoningEffort(effort)) - .await + async fn update_config(&self, ops: Vec) -> anyhow::Result<()> { + debug!(ops = ?ops, "Updating app config"); + self.infra.update_environment(ops).await } } @@ -184,6 +123,8 @@ mod tests { use std::sync::Mutex; use forge_config::{ForgeConfig, ModelConfig}; + // Alias to avoid collision with forge_config::ModelConfig used in test fixtures + use forge_domain::ModelConfig as DomainModelConfig; use forge_domain::{ AnyProvider, ChatRepository, ConfigOperation, Environment, InputModality, MigrationResult, Model, ModelId, ModelSource, Provider, ProviderId, ProviderResponse, ProviderTemplate, @@ -283,40 +224,32 @@ mod tests { let mut config = config.lock().unwrap(); for op in ops { match op { - ConfigOperation::SetProvider(pid) => { - let pid_str = pid.as_ref().to_string(); - config.session = Some(match config.session.take() { - Some(mc) => mc.provider_id(pid_str), - None => ModelConfig::default().provider_id(pid_str), - }); - } - ConfigOperation::SetModel(pid, mid) => { - let pid_str = pid.as_ref().to_string(); - let mid_str = mid.to_string(); + ConfigOperation::SetSessionConfig(mc) => { + let pid_str = mc.provider.as_ref().to_string(); + let mid_str = mc.model.to_string(); config.session = Some(match config.session.take() { - Some(mc) if mc.provider_id.as_deref() == Some(&pid_str) => { - mc.model_id(mid_str) + Some(existing) + if existing.provider_id.as_deref() == Some(&pid_str) => + { + existing.model_id(mid_str) } _ => ModelConfig::default() .provider_id(pid_str) .model_id(mid_str), }); } - ConfigOperation::SetCommitConfig(commit) => { - config.commit = - commit.provider.as_ref().zip(commit.model.as_ref()).map( - |(pid, mid)| { - ModelConfig::default() - .provider_id(pid.as_ref().to_string()) - .model_id(mid.to_string()) - }, - ); + ConfigOperation::SetCommitConfig(mc) => { + config.commit = mc.map(|m| { + ModelConfig::default() + .provider_id(m.provider.as_ref().to_string()) + .model_id(m.model.to_string()) + }); } - ConfigOperation::SetSuggestConfig(suggest) => { + ConfigOperation::SetSuggestConfig(mc) => { config.suggest = Some( ModelConfig::default() - .provider_id(suggest.provider.as_ref().to_string()) - .model_id(suggest.model.to_string()), + .provider_id(mc.provider.as_ref().to_string()) + .model_id(mc.model.to_string()), ); } ConfigOperation::SetReasoningEffort(_) => { @@ -328,6 +261,10 @@ mod tests { } } + fn get_config(&self) -> anyhow::Result { + Ok(self.config.lock().unwrap().clone()) + } + fn get_env_var(&self, _key: &str) -> Option { None } @@ -419,7 +356,7 @@ mod tests { #[tokio::test] async fn test_get_default_provider_when_none_set() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture)); let result = service.get_default_provider().await; @@ -430,9 +367,13 @@ mod tests { #[tokio::test] async fn test_get_default_provider_when_set() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - service.set_default_provider(ProviderId::ANTHROPIC).await?; + service + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::ANTHROPIC, ModelId::new("claude-3")), + )]) + .await?; let actual = service.get_default_provider().await?; let expected = ProviderId::ANTHROPIC; @@ -446,10 +387,14 @@ mod tests { let mut fixture = MockInfra::new(); // Remove OpenAI from available providers but keep it in config fixture.providers.retain(|p| p.id != ProviderId::OPENAI); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - // Set OpenAI as the default provider in config - service.set_default_provider(ProviderId::OPENAI).await?; + // Set OpenAI as the default provider in config (with a model) + service + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::OPENAI, ModelId::new("gpt-4")), + )]) + .await?; // Should return the provider ID even if provider is not available // Validation happens when getting the actual provider via ProviderService @@ -462,9 +407,13 @@ mod tests { #[tokio::test] async fn test_set_default_provider() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - service.set_default_provider(ProviderId::ANTHROPIC).await?; + service + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::ANTHROPIC, ModelId::new("claude-3")), + )]) + .await?; let actual = service.get_default_provider().await?; let expected = ProviderId::ANTHROPIC; @@ -476,7 +425,7 @@ mod tests { #[tokio::test] async fn test_get_default_model_when_none_set() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture)); let result = service.get_provider_model(Some(&ProviderId::OPENAI)).await; @@ -487,12 +436,13 @@ mod tests { #[tokio::test] async fn test_get_default_model_when_set() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; + // Set OpenAI as the default provider first, then set model atomically service - .set_default_model("gpt-4".to_string().into()) + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::OPENAI, ModelId::new("gpt-4")), + )]) .await?; let actual = service .get_provider_model(Some(&ProviderId::OPENAI)) @@ -506,12 +456,13 @@ mod tests { #[tokio::test] async fn test_set_default_model() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); - // Set OpenAI as the default provider first - service.set_default_provider(ProviderId::OPENAI).await?; + // Set provider and model atomically service - .set_default_model("gpt-4".to_string().into()) + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::OPENAI, ModelId::from("gpt-4".to_string())), + )]) .await?; let actual = service @@ -526,18 +477,23 @@ mod tests { #[tokio::test] async fn test_set_multiple_default_models() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); // Set model for OpenAI first - service.set_default_provider(ProviderId::OPENAI).await?; service - .set_default_model("gpt-4".to_string().into()) + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new(ProviderId::OPENAI, ModelId::from("gpt-4".to_string())), + )]) .await?; - // Then switch to Anthropic and set its model - service.set_default_provider(ProviderId::ANTHROPIC).await?; + // Then switch to Anthropic with its model service - .set_default_model("claude-3".to_string().into()) + .update_config(vec![ConfigOperation::SetSessionConfig( + DomainModelConfig::new( + ProviderId::ANTHROPIC, + ModelId::from("claude-3".to_string()), + ), + )]) .await?; // ForgeConfig only tracks a single active session, so the last diff --git a/crates/forge_services/src/attachment.rs b/crates/forge_services/src/attachment.rs index 1ec1db7e7b..5ebfe6bf29 100644 --- a/crates/forge_services/src/attachment.rs +++ b/crates/forge_services/src/attachment.rs @@ -13,14 +13,17 @@ use crate::range::resolve_range; #[derive(Clone)] pub struct ForgeChatRequest { infra: Arc, - max_read_lines: u64, } -impl - ForgeChatRequest +impl< + F: FileReaderInfra + + EnvironmentInfra + + FileInfoInfra + + DirectoryReaderInfra, +> ForgeChatRequest { - pub fn new(infra: Arc, max_read_lines: u64) -> Self { - Self { infra, max_read_lines } + pub fn new(infra: Arc) -> Self { + Self { infra } } async fn prepare_attachments(&self, paths: Vec) -> anyhow::Result> { @@ -85,7 +88,8 @@ impl { let start = tag.loc.as_ref().and_then(|loc| loc.start); let end = tag.loc.as_ref().and_then(|loc| loc.end); - let (start_line, end_line) = resolve_range(start, end, self.max_read_lines); + let max_read_lines = self.infra.get_config()?.max_read_lines; + let (start_line, end_line) = resolve_range(start, end, max_read_lines); // range_read_utf8 returns the range content and FileInfo which // carries a content_hash of the **full** file. Using the @@ -113,8 +117,12 @@ impl AttachmentService - for ForgeChatRequest +impl< + F: FileReaderInfra + + EnvironmentInfra + + FileInfoInfra + + DirectoryReaderInfra, +> AttachmentService for ForgeChatRequest { async fn attachments(&self, url: &str) -> anyhow::Result> { self.prepare_attachments(Attachment::parse_all(url)).await @@ -160,6 +168,10 @@ pub mod tests { fixture.cwd(PathBuf::from("/test")) // Set fixed CWD for predictable tests } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig { max_read_lines: 2000, ..Default::default() }) + } + async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { unimplemented!() } @@ -496,6 +508,10 @@ pub mod tests { self.env_service.get_environment() } + fn get_config(&self) -> anyhow::Result { + self.env_service.get_config() + } + fn update_environment( &self, ops: Vec, @@ -556,7 +572,7 @@ pub mod tests { async fn test_add_url_with_text_file() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with a text file path in chat message let url = "@[/test/file1.txt]".to_string(); @@ -578,7 +594,7 @@ pub mod tests { async fn test_add_url_with_image() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with an image file let url = "@[/test/image.png]".to_string(); @@ -605,7 +621,7 @@ pub mod tests { async fn test_add_url_with_jpg_image_with_spaces() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with an image file that has spaces in the path let url = "@[/test/image with spaces.jpg]".to_string(); @@ -638,7 +654,7 @@ pub mod tests { "This is another text file".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with multiple files mentioned let url = "@[/test/file1.txt] @[/test/file2.txt] @[/test/image.png]".to_string(); @@ -672,7 +688,7 @@ pub mod tests { async fn test_add_url_with_nonexistent_file() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with a file that doesn't exist let url = "@[/test/nonexistent.txt]".to_string(); @@ -689,7 +705,7 @@ pub mod tests { async fn test_add_url_empty() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with an empty message let url = "".to_string(); @@ -712,7 +728,7 @@ pub mod tests { "Some content".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with the file let url = "@[/test/unknown.xyz]".to_string(); @@ -740,7 +756,7 @@ pub mod tests { "Line 1\nLine 2\nLine 3\nLine 4\nLine 5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); let url = "@[/test/multiline.txt]".to_string(); // Execute @@ -782,7 +798,7 @@ pub mod tests { "Line 1\nLine 2\nLine 3\nLine 4\nLine 5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test reading line 2 only let url = "@[/test/multiline.txt:2:2]"; @@ -813,7 +829,7 @@ pub mod tests { "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test reading lines 2-4 let url = "@[/test/range_test.txt:2:4]"; @@ -844,7 +860,7 @@ pub mod tests { "First\nSecond\nThird\nFourth".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test reading from start to line 2 let url = "@[/test/start_range.txt:1:2]"; @@ -867,7 +883,7 @@ pub mod tests { "Alpha\nBeta\nGamma\nDelta\nEpsilon".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test reading from line 3 to end let url = "@[/test/end_range.txt:3:5]"; @@ -890,7 +906,7 @@ pub mod tests { "Only line".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test reading beyond file length let url = "@[/test/edge_case.txt:1:10]"; @@ -914,7 +930,7 @@ pub mod tests { "B1\nB2\nB3\nB4".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test multiple files with different ranges let url = "Check @[/test/file_a.txt:1:2] and @[/test/file_b.txt:3:4]"; @@ -946,7 +962,7 @@ pub mod tests { "Meta1\nMeta2\nMeta3\nMeta4\nMeta5\nMeta6\nMeta7".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test that metadata is preserved correctly with ranges let url = "@[/test/metadata_test.txt:3:5]"; @@ -977,7 +993,7 @@ pub mod tests { "Full1\nFull2\nFull3\nFull4\nFull5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // All reads of the same file (full or ranged) should produce the same // content_hash, so the external-change detector can correctly identify @@ -1038,7 +1054,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/mydir/subdir")); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with directory path let url = "@[/test/mydir]"; @@ -1079,7 +1095,7 @@ pub mod tests { // Add empty directory infra.file_service.add_dir(PathBuf::from("/test/emptydir")); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with empty directory path let url = "@[/test/emptydir]"; @@ -1117,7 +1133,7 @@ pub mod tests { "Standalone file".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); // Test with both file and directory let url = "@[/test/mixdir] @[/test/standalone.txt]"; @@ -1180,7 +1196,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/sortdir/berry_dir")); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); let url = "@[/test/sortdir]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1232,7 +1248,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/onlydirs/middle_dir")); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); let url = "@[/test/onlydirs]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1264,7 +1280,7 @@ pub mod tests { infra.add_file(PathBuf::from("/test/onlyfiles/alpha.txt"), "A".to_string()); infra.add_file(PathBuf::from("/test/onlyfiles/middle.txt"), "M".to_string()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); let url = "@[/test/onlyfiles]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1301,7 +1317,7 @@ pub mod tests { infra.add_file(PathBuf::from("/test/casetest/Zebra.txt"), "Z".to_string()); infra.add_file(PathBuf::from("/test/casetest/apple.txt"), "A".to_string()); - let chat_request = ForgeChatRequest::new(infra.clone(), 2000); + let chat_request = ForgeChatRequest::new(infra.clone()); let url = "@[/test/casetest]"; let attachments = chat_request.attachments(url).await.unwrap(); diff --git a/crates/forge_services/src/auth.rs b/crates/forge_services/src/auth.rs index 0b503339c8..8a917e4fc6 100644 --- a/crates/forge_services/src/auth.rs +++ b/crates/forge_services/src/auth.rs @@ -10,16 +10,16 @@ const USER_USAGE_ROUTE: &str = "auth/usage"; #[derive(Default, Clone)] pub struct ForgeAuthService { infra: Arc, - services_url: String, } -impl ForgeAuthService { - pub fn new(infra: Arc, services_url: String) -> Self { - Self { infra, services_url } +impl> ForgeAuthService { + pub fn new(infra: Arc) -> Self { + Self { infra } } async fn user_info(&self, api_key: &str) -> anyhow::Result { - let url = format!("{}{USER_INFO_ROUTE}", self.services_url); + let services_url = self.infra.get_config()?.services_url; + let url = format!("{services_url}{USER_INFO_ROUTE}"); let url = Url::parse(&url)?; let mut headers = HeaderMap::new(); @@ -38,7 +38,8 @@ impl ForgeAuthService { } async fn user_usage(&self, api_key: &str) -> anyhow::Result { - let url = Url::parse(&format!("{}{USER_USAGE_ROUTE}", self.services_url))?; + let services_url = self.infra.get_config()?.services_url; + let url = Url::parse(&format!("{services_url}{USER_USAGE_ROUTE}"))?; let mut headers = HeaderMap::new(); headers.insert( AUTHORIZATION, @@ -56,7 +57,9 @@ impl ForgeAuthService { } #[async_trait::async_trait] -impl AuthService for ForgeAuthService { +impl> AuthService + for ForgeAuthService +{ async fn user_info(&self, api_key: &str) -> anyhow::Result { self.user_info(api_key).await } diff --git a/crates/forge_services/src/context_engine.rs b/crates/forge_services/src/context_engine.rs index 5020f3c4cb..713ca56bb4 100644 --- a/crates/forge_services/src/context_engine.rs +++ b/crates/forge_services/src/context_engine.rs @@ -23,7 +23,6 @@ use crate::sync::{WorkspaceSyncEngine, canonicalize_path}; pub struct ForgeWorkspaceService { infra: Arc, discovery: Arc, - max_file_read_batch_size: usize, } impl Clone for ForgeWorkspaceService { @@ -31,7 +30,6 @@ impl Clone for ForgeWorkspaceService { Self { infra: Arc::clone(&self.infra), discovery: Arc::clone(&self.discovery), - max_file_read_batch_size: self.max_file_read_batch_size, } } } @@ -39,8 +37,8 @@ impl Clone for ForgeWorkspaceService { impl ForgeWorkspaceService { /// Creates a new workspace service with the provided infrastructure and /// file-discovery strategy. - pub fn new(infra: Arc, discovery: Arc, max_file_read_batch_size: usize) -> Self { - Self { infra, discovery, max_file_read_batch_size } + pub fn new(infra: Arc, discovery: Arc) -> Self { + Self { infra, discovery } } } @@ -49,7 +47,7 @@ impl< + ProviderRepository + WorkspaceIndexRepository + FileReaderInfra - + EnvironmentInfra + + EnvironmentInfra + CommandInfra + WalkerInfra, D: FileDiscovery + 'static, @@ -66,7 +64,7 @@ impl< emit(SyncProgress::Starting).await; let (token, user_id) = self.get_workspace_credentials().await?; - let batch_size = self.max_file_read_batch_size; + let batch_size = self.infra.get_config()?.max_file_read_batch_size; let path = canonicalize_path(path)?; // Find existing workspace - do NOT auto-create @@ -221,7 +219,7 @@ impl< F: ProviderRepository + WorkspaceIndexRepository + FileReaderInfra - + EnvironmentInfra + + EnvironmentInfra + CommandInfra + WalkerInfra + 'static, @@ -361,7 +359,7 @@ impl< // sync), avoiding a redundant canonicalize() IO call. let canonical_path = PathBuf::from(&workspace.working_dir); - let batch_size = self.max_file_read_batch_size; + let batch_size = self.infra.get_config()?.max_file_read_batch_size; WorkspaceSyncEngine::new( Arc::clone(&self.infra), diff --git a/crates/forge_services/src/discovery.rs b/crates/forge_services/src/discovery.rs index 96b3bb7227..db83e45b47 100644 --- a/crates/forge_services/src/discovery.rs +++ b/crates/forge_services/src/discovery.rs @@ -97,6 +97,10 @@ mod tests { env } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { unimplemented!() } diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 9a9afb477a..7ff1d1a2fb 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -87,7 +87,7 @@ pub struct ForgeServices< impl< F: McpServerInfra - + EnvironmentInfra + + EnvironmentInfra + FileWriterInfra + FileInfoInfra + FileReaderInfra @@ -107,33 +107,20 @@ impl< + ValidationRepository, > ForgeServices { - pub fn new(infra: Arc, config: forge_config::ForgeConfig) -> Self { + pub fn new(infra: Arc) -> Self { let mcp_manager = Arc::new(ForgeMcpManager::new(infra.clone())); let mcp_service = Arc::new(ForgeMcpService::new(mcp_manager.clone(), infra.clone())); let template_service = Arc::new(ForgeTemplateService::new(infra.clone())); - let attachment_service = - Arc::new(ForgeChatRequest::new(infra.clone(), config.max_read_lines)); + let attachment_service = Arc::new(ForgeChatRequest::new(infra.clone())); let suggestion_service = Arc::new(ForgeDiscoveryService::new(infra.clone())); let conversation_service = Arc::new(ForgeConversationService::new(infra.clone())); - let auth_service = Arc::new(ForgeAuthService::new( - infra.clone(), - config.services_url.clone(), - )); + let auth_service = Arc::new(ForgeAuthService::new(infra.clone())); let chat_service = Arc::new(ForgeProviderService::new(infra.clone())); - let config_service = Arc::new(ForgeAppConfigService::new(infra.clone(), config.clone())); + let config_service = Arc::new(ForgeAppConfigService::new(infra.clone())); let file_create_service = Arc::new(ForgeFsWrite::new(infra.clone())); let plan_create_service = Arc::new(ForgePlanCreate::new(infra.clone())); - let file_read_service = Arc::new(ForgeFsRead::new( - infra.clone(), - config.max_file_size_bytes, - config.max_image_size_bytes, - config.max_read_lines, - config.max_line_chars, - )); - let image_read_service = Arc::new(ForgeImageRead::new( - infra.clone(), - config.max_image_size_bytes, - )); + let file_read_service = Arc::new(ForgeFsRead::new(infra.clone())); + let image_read_service = Arc::new(ForgeImageRead::new(infra.clone())); let file_search_service = Arc::new(ForgeFsSearch::new(infra.clone())); let file_remove_service = Arc::new(ForgeFsRemove::new(infra.clone())); let file_patch_service = Arc::new(ForgeFsPatch::new(infra.clone())); @@ -143,10 +130,7 @@ impl< let followup_service = Arc::new(ForgeFollowup::new(infra.clone())); let custom_instructions_service = Arc::new(ForgeCustomInstructionsService::new(infra.clone())); - let agent_registry_service = Arc::new(ForgeAgentRegistryService::new( - infra.clone(), - config.clone(), - )); + let agent_registry_service = Arc::new(ForgeAgentRegistryService::new(infra.clone())); let command_loader_service = Arc::new(ForgeCommandLoaderService::new(infra.clone())); let policy_service = ForgePolicyService::new(infra.clone()); let provider_auth_service = ForgeProviderAuthService::new(infra.clone()); @@ -154,7 +138,6 @@ impl< let workspace_service = Arc::new(crate::context_engine::ForgeWorkspaceService::new( infra.clone(), discovery, - config.max_file_read_batch_size, )); let skill_service = Arc::new(ForgeSkillFetch::new(infra.clone())); @@ -200,7 +183,7 @@ impl< + FileRemoverInfra + FileInfoInfra + FileDirectoryInfra - + EnvironmentInfra + + EnvironmentInfra + DirectoryReaderInfra + HttpInfra + WalkerInfra @@ -357,7 +340,7 @@ impl< } impl< - F: EnvironmentInfra + F: EnvironmentInfra + HttpInfra + McpServerInfra + WalkerInfra @@ -380,6 +363,10 @@ impl< self.infra.get_environment() } + fn get_config(&self) -> anyhow::Result { + self.infra.get_config() + } + fn update_environment( &self, ops: Vec, diff --git a/crates/forge_services/src/tool_services/fs_read.rs b/crates/forge_services/src/tool_services/fs_read.rs index 4a133e3e03..50ff952ca6 100644 --- a/crates/forge_services/src/tool_services/fs_read.rs +++ b/crates/forge_services/src/tool_services/fs_read.rs @@ -95,32 +95,18 @@ pub async fn assert_file_size( /// files are automatically detected and rejected. pub struct ForgeFsRead { infra: Arc, - max_file_size_bytes: u64, - max_image_size_bytes: u64, - max_read_lines: u64, - max_line_chars: usize, } impl ForgeFsRead { - pub fn new( - infra: Arc, - max_file_size_bytes: u64, - max_image_size_bytes: u64, - max_read_lines: u64, - max_line_chars: usize, - ) -> Self { - Self { - infra, - max_file_size_bytes, - max_image_size_bytes, - max_read_lines, - max_line_chars, - } + pub fn new(infra: Arc) -> Self { + Self { infra } } } #[async_trait::async_trait] -impl FsReadService for ForgeFsRead { +impl + InfraFsReadService> + FsReadService for ForgeFsRead +{ async fn read( &self, path: String, @@ -130,8 +116,10 @@ impl FsReadService for let path = Path::new(&path); assert_absolute_path(path)?; + let config = self.infra.get_config()?; + // Validate with the larger limit initially since we don't know file type yet - let initial_size_limit = self.max_file_size_bytes.max(self.max_image_size_bytes); + let initial_size_limit = config.max_file_size_bytes.max(config.max_image_size_bytes); assert_file_size(&*self.infra, path, initial_size_limit).await?; // Read file content to detect MIME type @@ -148,7 +136,7 @@ impl FsReadService for if is_visual_content(&mime_type) { // Validate against image-specific size limit (may be different from // max_file_size) - assert_file_size(&*self.infra, path, self.max_image_size_bytes) + assert_file_size(&*self.infra, path, config.max_image_size_bytes) .await .with_context(|| { if mime_type == "application/pdf" { @@ -171,7 +159,7 @@ impl FsReadService for // Handle text content (including Jupyter notebooks) // File size already validated above - let (start_line, end_line) = resolve_range(start_line, end_line, self.max_read_lines); + let (start_line, end_line) = resolve_range(start_line, end_line, config.max_read_lines); // Convert bytes to UTF-8 string let full_content = String::from_utf8(raw_content) @@ -196,7 +184,7 @@ impl FsReadService for // Return full content with line truncation lines .iter() - .map(|line| truncate_line(line, self.max_line_chars)) + .map(|line| truncate_line(line, config.max_line_chars)) .collect::>() .join("\n") } else if total_lines == 0 { @@ -205,7 +193,7 @@ impl FsReadService for // Return range with line truncation lines[start_pos as usize..=end_pos as usize] .iter() - .map(|line| truncate_line(line, self.max_line_chars)) + .map(|line| truncate_line(line, config.max_line_chars)) .collect::>() .join("\n") }; diff --git a/crates/forge_services/src/tool_services/image_read.rs b/crates/forge_services/src/tool_services/image_read.rs index 63f80209a6..dce227789b 100644 --- a/crates/forge_services/src/tool_services/image_read.rs +++ b/crates/forge_services/src/tool_services/image_read.rs @@ -10,7 +10,6 @@ use crate::utils::assert_absolute_path; pub struct ForgeImageRead { infra: Arc, - max_image_size_bytes: u64, } /// Supported image formats for binary file reading @@ -42,29 +41,31 @@ impl ImageFormat { } impl ForgeImageRead { - pub fn new(infra: Arc, max_image_size_bytes: u64) -> Self { - Self { infra, max_image_size_bytes } + pub fn new(infra: Arc) -> Self { + Self { infra } } } + #[async_trait::async_trait] -impl ImageReadService - for ForgeImageRead +impl< + F: FileInfoInfra + + EnvironmentInfra + + forge_app::FileReaderInfra, +> ImageReadService for ForgeImageRead { async fn read_image(&self, path: String) -> anyhow::Result { let path = Path::new(&path); assert_absolute_path(path)?; + let max_image_size_bytes = self.infra.get_config()?.max_image_size_bytes; + // Validate file size before reading content using image-specific file size // limit - crate::tool_services::fs_read::assert_file_size( - &*self.infra, - path, - self.max_image_size_bytes, - ) - .await - .with_context( - || "Image exceeds size limit. Compress the image or increase FORGE_MAX_IMAGE_SIZE.", - )?; + crate::tool_services::fs_read::assert_file_size(&*self.infra, path, max_image_size_bytes) + .await + .with_context( + || "Image exceeds size limit. Compress the image or increase FORGE_MAX_IMAGE_SIZE.", + )?; // Determine image format from file extension let extension = path diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index 713c7bf694..05a671de71 100644 --- a/crates/forge_services/src/tool_services/shell.rs +++ b/crates/forge_services/src/tool_services/shell.rs @@ -118,6 +118,10 @@ mod tests { Faker.fake() } + fn get_config(&self) -> anyhow::Result { + Ok(forge_config::ForgeConfig::default()) + } + async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { unimplemented!() }