diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index 0099be47b8..3f5a2e4aca 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -3,7 +3,6 @@ use std::path::PathBuf; use anyhow::Result; use forge_app::dto::ToolsOverview; use forge_app::{User, UserUsage}; -use forge_config::ForgeConfig; use forge_domain::{AgentId, Effort, ModelId, ProviderModels}; use forge_stream::MpscStream; use futures::stream::BoxStream; @@ -51,9 +50,6 @@ pub trait API: Sync + Send { /// Returns the current environment fn environment(&self) -> Environment; - /// Returns the full application configuration. - fn get_config(&self) -> ForgeConfig; - /// Adds a new conversation to the conversation store async fn upsert_conversation(&self, conversation: Conversation) -> Result<()>; diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index c5a59d60c2..2b89dee487 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -24,11 +24,12 @@ use crate::API; pub struct ForgeAPI { services: Arc, infra: Arc, + config: forge_config::ForgeConfig, } impl ForgeAPI { - pub fn new(services: Arc, infra: Arc) -> Self { - Self { services, infra } + pub fn new(services: Arc, infra: Arc, config: forge_config::ForgeConfig) -> Self { + Self { services, infra, config } } /// Creates a ForgeApp instance with the current services @@ -36,16 +37,22 @@ impl ForgeAPI { where A: Services, { - ForgeApp::new(self.services.clone()) + ForgeApp::new(self.services.clone(), self.config.clone()) } } impl ForgeAPI>, ForgeRepo> { - pub fn init(cwd: PathBuf) -> Self { - let infra = Arc::new(ForgeInfra::new(cwd)); - let repo = Arc::new(ForgeRepo::new(infra.clone())); - let app = Arc::new(ForgeServices::new(repo.clone())); - ForgeAPI::new(app, repo) + /// Creates a fully-initialized [`ForgeAPI`] from a pre-read configuration. + /// + /// # Arguments + /// * `cwd` - The working directory path for environment and file resolution + /// * `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) } pub async fn get_skills_internal(&self) -> Result> { @@ -91,7 +98,7 @@ impl, additional_context: Option, ) -> Result { - let git_app = GitApp::new(self.services.clone()); + let git_app = GitApp::new(self.services.clone(), self.config.clone()); let result = git_app .commit_message(max_diff_size, diff, additional_context) .await?; @@ -147,10 +154,6 @@ impl ForgeConfig { - self.infra.get_config() - } - async fn conversation( &self, conversation_id: &ConversationId, diff --git a/crates/forge_app/src/agent.rs b/crates/forge_app/src/agent.rs index 30ce87198a..ec3e403d1d 100644 --- a/crates/forge_app/src/agent.rs +++ b/crates/forge_app/src/agent.rs @@ -30,6 +30,7 @@ pub trait AgentService: Send + Sync + 'static { agent: &Agent, context: &ToolCallContext, call: ToolCallFull, + config: &ForgeConfig, ) -> ToolResult; /// Synchronize the on-going conversation @@ -60,8 +61,9 @@ impl AgentService for T { agent: &Agent, context: &ToolCallContext, call: ToolCallFull, + config: &ForgeConfig, ) -> ToolResult { - let registry = ToolRegistry::new(Arc::new(self.clone())); + let registry = ToolRegistry::new(Arc::new(self.clone()), config.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 77325db6ef..b5afcb3c57 100644 --- a/crates/forge_app/src/agent_executor.rs +++ b/crates/forge_app/src/agent_executor.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use anyhow::Context; use convert_case::{Case, Casing}; +use forge_config::ForgeConfig; use forge_domain::{ AgentId, ChatRequest, ChatResponse, ChatResponseContent, Conversation, Event, TitleFormat, ToolCallContext, ToolDefinition, ToolName, ToolOutput, @@ -16,12 +17,13 @@ use crate::{AgentRegistry, ConversationService, Services}; #[derive(Clone)] pub struct AgentExecutor { services: Arc, + config: ForgeConfig, pub tool_agents: Arc>>>, } impl AgentExecutor { - pub fn new(services: Arc) -> Self { - Self { services, tool_agents: Arc::new(RwLock::new(None)) } + pub fn new(services: Arc, config: ForgeConfig) -> Self { + Self { services, config, tool_agents: Arc::new(RwLock::new(None)) } } /// Returns a list of tool definitions for all available agents. @@ -63,7 +65,7 @@ impl AgentExecutor { .upsert_conversation(conversation.clone()) .await?; // Execute the request through the ForgeApp - let app = crate::ForgeApp::new(self.services.clone()); + let app = crate::ForgeApp::new(self.services.clone(), self.config.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 169a002f03..5e762f6e81 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -44,12 +44,17 @@ 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. - pub fn new(services: Arc) -> Self { - Self { tool_registry: ToolRegistry::new(services.clone()), services } + /// 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, + } } /// Executes a chat request and returns a stream of responses. @@ -69,7 +74,7 @@ impl ForgeApp { .expect("conversation for the request should've been created at this point."); // Discover files using the discovery service - let forge_config = services.get_config(); + let forge_config = self.config.clone(); let environment = services.get_environment(); let files = services.list_current_directory().await?; @@ -132,7 +137,7 @@ impl ForgeApp { // Detect and render externally changed files notification let conversation = ChangedFiles::new(services.clone(), agent.clone()) - .update_file_stats(conversation) + .update_file_stats(conversation, forge_config.max_parallel_file_reads) .await; let conversation = InitConversationMetrics::new(current_time).apply(conversation); @@ -157,11 +162,17 @@ impl ForgeApp { let retry_config = forge_config.retry.clone().unwrap_or_default(); - let orch = Orchestrator::new(services.clone(), retry_config, conversation, agent) - .error_tracker(ToolErrorTracker::new(max_tool_failure_per_turn)) - .tool_definitions(tool_definitions) - .models(models) - .hook(Arc::new(hook)); + let orch = Orchestrator::new( + services.clone(), + retry_config, + conversation, + agent, + forge_config, + ) + .error_tracker(ToolErrorTracker::new(max_tool_failure_per_turn)) + .tool_definitions(tool_definitions) + .models(models) + .hook(Arc::new(hook)); // Create and return the stream let stream = MpscStream::spawn( @@ -219,7 +230,7 @@ impl ForgeApp { let original_messages = context.messages.len(); let original_token_count = *context.token_count(); - let forge_config = self.services.get_config(); + let forge_config = self.config.clone(); // 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 6b52639f93..5d7585c740 100644 --- a/crates/forge_app/src/changed_files.rs +++ b/crates/forge_app/src/changed_files.rs @@ -24,9 +24,12 @@ 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) -> Conversation { + pub async fn update_file_stats( + &self, + mut conversation: Conversation, + parallel_file_reads: usize, + ) -> Conversation { use crate::file_tracking::FileChangeDetector; - let parallel_file_reads = self.services.get_config().max_parallel_file_reads; let changes = FileChangeDetector::new(self.services.clone(), parallel_file_reads) .detect(&conversation.metrics) .await; @@ -133,13 +136,6 @@ mod tests { env } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment( &self, _ops: Vec, @@ -203,7 +199,7 @@ mod tests { Some(ModelId::new("test")), ))); - let actual = service.update_file_stats(conversation.clone()).await; + let actual = service.update_file_stats(conversation.clone(), 4).await; assert_eq!(actual.context.clone().unwrap_or_default().messages.len(), 1); assert_eq!(actual.context, conversation.context); @@ -219,7 +215,7 @@ mod tests { [("/test/file.txt".into(), Some(old_hash))].into(), ); - let actual = service.update_file_stats(conversation).await; + let actual = service.update_file_stats(conversation, 4).await; let messages = &actual.context.unwrap().messages; assert_eq!(messages.len(), 1); @@ -239,7 +235,7 @@ mod tests { [("/test/file.txt".into(), Some(old_hash))].into(), ); - let actual = service.update_file_stats(conversation).await; + let actual = service.update_file_stats(conversation, 4).await; let updated_hash = actual .metrics @@ -265,7 +261,7 @@ mod tests { .into(), ); - let actual = service.update_file_stats(conversation).await; + let actual = service.update_file_stats(conversation, 4).await; let message = actual.context.unwrap().messages[0] .content() @@ -288,7 +284,7 @@ mod tests { Some(cwd), ); - let actual = service.update_file_stats(conversation).await; + let actual = service.update_file_stats(conversation, 4).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 ac3912aa7d..42ca8b0080 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -144,13 +144,6 @@ mod tests { self.environment.clone() } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment( &self, _ops: Vec, diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index 4f20cce238..bf50f44a80 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -6,10 +6,11 @@ use forge_domain::*; use schemars::JsonSchema; use serde::Deserialize; -use crate::{ - AgentProviderResolver, AgentRegistry, AppConfigService, EnvironmentInfra, ProviderAuthService, - ProviderService, ShellService, TemplateService, +use crate::services::{ + AgentRegistry, AppConfigService, ProviderAuthService, ProviderService, ShellService, + TemplateService, }; +use crate::{AgentProviderResolver, Services}; /// Errors specific to GitApp operations #[derive(thiserror::Error, Debug)] @@ -21,6 +22,7 @@ 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 @@ -65,9 +67,9 @@ struct DiffContext { } impl GitApp { - /// Creates a new GitApp instance with the provided services. - pub fn new(services: Arc) -> Self { - Self { services } + /// Creates a new GitApp instance with the provided services and config. + pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { + Self { services, config } } /// Truncates diff content if it exceeds the maximum size @@ -92,16 +94,7 @@ impl GitApp { } } -impl GitApp -where - S: EnvironmentInfra - + ShellService - + AgentRegistry - + TemplateService - + ProviderService - + AppConfigService - + ProviderAuthService, -{ +impl GitApp { /// Generates a commit message without committing /// /// # Arguments @@ -220,7 +213,7 @@ where additional_context, }; - let retry_config = self.services.get_config().retry.unwrap_or_default(); + let retry_config = self.config.retry.clone().unwrap_or_default(); crate::retry::retry_with_config( &retry_config, || self.generate_message_from_diff(ctx.clone()), @@ -231,7 +224,7 @@ where /// Fetches git context (branch name and recent commits) async fn fetch_git_context(&self, cwd: &Path) -> Result<(String, String)> { - let max_commit_count = self.services.get_config().max_commit_count; + let max_commit_count = self.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!( diff --git a/crates/forge_app/src/hooks/title_generation.rs b/crates/forge_app/src/hooks/title_generation.rs index debcae12e3..edc3057552 100644 --- a/crates/forge_app/src/hooks/title_generation.rs +++ b/crates/forge_app/src/hooks/title_generation.rs @@ -189,6 +189,7 @@ 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 8c1c567772..bc57aa38dc 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -4,7 +4,6 @@ use std::path::{Path, PathBuf}; use anyhow::Result; use bytes::Bytes; -use forge_config::ForgeConfig; use forge_domain::{ AuthCodeParams, CommandOutput, ConfigOperation, Environment, FileInfo, McpServerConfig, OAuthConfig, OAuthTokenResponse, ToolDefinition, ToolName, ToolOutput, @@ -20,8 +19,7 @@ use crate::{WalkedFile, Walker}; /// Infrastructure trait for accessing environment configuration, system /// variables, and persisted application configuration. pub trait EnvironmentInfra: Send + Sync { - /// The fully-resolved configuration type returned by - /// [`EnvironmentInfra::get_config`]. + /// The fully-resolved configuration type stored by the implementation. type Config: Clone + Send + Sync; fn get_env_var(&self, key: &str) -> Option; @@ -30,13 +28,6 @@ pub trait EnvironmentInfra: Send + Sync { /// Retrieves the current application configuration as an [`Environment`]. fn get_environment(&self) -> Environment; - /// Returns the full [`ForgeConfig`] for the current session. - /// - /// Callers that need configuration values previously carried on - /// [`Environment`] (e.g. `retry_config`, `tool_timeout_secs`, - /// `session`, etc.) must call this method instead. - fn get_config(&self) -> ForgeConfig; - /// 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 e744705afb..bfdc7e771d 100644 --- a/crates/forge_app/src/orch.rs +++ b/crates/forge_app/src/orch.rs @@ -25,6 +25,7 @@ pub struct Orchestrator { agent: Agent, error_tracker: ToolErrorTracker, hook: Arc, + config: forge_config::ForgeConfig, } impl Orchestrator { @@ -33,12 +34,14 @@ impl Orchestrator { retry_config: RetryConfig, conversation: Conversation, agent: Agent, + config: forge_config::ForgeConfig, ) -> Self { Self { conversation, retry_config, services, agent, + config, sender: Default::default(), tool_definitions: Default::default(), models: Default::default(), @@ -97,7 +100,7 @@ impl Orchestrator { // Execute the tool let tool_result = self .services - .call(&self.agent, tool_context, tool_call.clone()) + .call(&self.agent, tool_context, tool_call.clone(), &self.config) .await; // Fire the ToolcallEnd lifecycle event (fires on both success and failure) diff --git a/crates/forge_app/src/orch_spec/orch_runner.rs b/crates/forge_app/src/orch_spec/orch_runner.rs index 9e42bddc24..961882db3b 100644 --- a/crates/forge_app/src/orch_spec/orch_runner.rs +++ b/crates/forge_app/src/orch_spec/orch_runner.rs @@ -120,13 +120,19 @@ impl Runner { 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) - .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(), + 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 (mut orch, runner) = (orch, services); @@ -171,6 +177,7 @@ 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; diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index 35539721a7..cb3e4d8444 100644 --- a/crates/forge_app/src/tool_executor.rs +++ b/crates/forge_app/src/tool_executor.rs @@ -16,6 +16,7 @@ use crate::{ pub struct ToolExecutor { services: Arc, + config: forge_config::ForgeConfig, } impl< @@ -39,8 +40,8 @@ impl< + Services, > ToolExecutor { - pub fn new(services: Arc) -> Self { - Self { services } + pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { + Self { services, config } } fn require_prior_read( @@ -69,7 +70,7 @@ impl< match operation { ToolOperation::NetFetch { input: _, output } => { let original_length = output.content.len(); - let is_truncated = original_length > self.services.get_config().max_fetch_chars; + let is_truncated = original_length > self.config.max_fetch_chars; let mut files = TempContentFiles::default(); if is_truncated { @@ -82,7 +83,7 @@ impl< Ok(files) } ToolOperation::Shell { output } => { - let config = self.services.get_config(); + let config = &self.config; let stdout_lines = output.output.stdout.lines().count(); let stderr_lines = output.output.stderr.lines().count(); let stdout_truncated = @@ -185,11 +186,10 @@ impl< } ToolCatalog::SemSearch(input) => { let env = self.services.get_environment(); - let config = self.services.get_config(); let services = self.services.clone(); let cwd = env.cwd.clone(); - let limit = config.max_sem_search_results; - let top_k = config.sem_search_top_k as u32; + let limit = self.config.max_sem_search_results; + let top_k = self.config.sem_search_top_k as u32; let params: Vec<_> = input .queries .iter() @@ -325,7 +325,7 @@ impl< ) -> anyhow::Result { let tool_kind = tool_input.kind(); let env = self.services.get_environment(); - let config = self.services.get_config(); + let config = &self.config; // Enforce read-before-edit for patch if let ToolCatalog::Patch(input) = &tool_input { @@ -355,7 +355,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 9c84fa6254..c68107535d 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -31,16 +31,18 @@ pub struct ToolRegistry { mcp_executor: McpExecutor, tool_timeout: Duration, services: Arc, + config: forge_config::ForgeConfig, } impl ToolRegistry { - pub fn new(services: Arc) -> Self { + pub fn new(services: Arc, config: forge_config::ForgeConfig) -> Self { Self { services: services.clone(), - tool_executor: ToolExecutor::new(services.clone()), - agent_executor: AgentExecutor::new(services.clone()), + tool_executor: ToolExecutor::new(services.clone(), config.clone()), + agent_executor: AgentExecutor::new(services.clone(), config.clone()), mcp_executor: McpExecutor::new(services.clone()), - tool_timeout: Duration::from_secs(services.get_config().tool_timeout_secs), + tool_timeout: Duration::from_secs(config.tool_timeout_secs), + config, } } @@ -112,7 +114,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.services.get_config().restricted; + let is_restricted = self.config.restricted; if is_restricted && self.check_tool_permission(&tool_input, context).await? { // Send formatted output message for policy denial context @@ -221,7 +223,7 @@ impl ToolRegistry { let model = self.get_current_model().await; // Build TemplateConfig from ForgeConfig for tool description templates - let config = self.services.get_config(); + let config = &self.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_infra/src/env.rs b/crates/forge_infra/src/env.rs index 902198b447..18b578093c 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use forge_app::EnvironmentInfra; use forge_config::{ConfigReader, ForgeConfig, ModelConfig}; use forge_domain::{ConfigOperation, Environment}; -use tracing::{debug, error}; +use tracing::debug; /// Builds a [`forge_domain::Environment`] from runtime context only. /// @@ -13,7 +13,7 @@ use tracing::{debug, error}; /// here: `os`, `pid`, `cwd`, `home`, `shell`, and `base_path`. All /// configuration values are now accessed through /// `EnvironmentInfra::get_config()`. -fn to_environment(cwd: PathBuf) -> Environment { +pub fn to_environment(cwd: PathBuf) -> Environment { Environment { os: std::env::consts::OS.to_string(), pid: std::process::id(), @@ -97,39 +97,36 @@ pub struct ForgeEnvironmentInfra { } impl ForgeEnvironmentInfra { - /// Creates a new [`ForgeEnvironmentInfra`]. + /// Creates a new [`ForgeEnvironmentInfra`] with the given pre-read config. + /// + /// The cache is pre-seeded with `config` so no disk I/O occurs on the + /// first [`EnvironmentInfra::get_config`] call. /// /// # Arguments /// * `cwd` - The working directory path; used to resolve `.env` files - pub fn new(cwd: PathBuf) -> Self { - Self { cwd, cache: Arc::new(std::sync::Mutex::new(None)) } - } - - /// Reads [`ForgeConfig`] from disk via [`ForgeConfig::read`]. - fn read_from_disk() -> ForgeConfig { - match ForgeConfig::read() { - Ok(config) => { - debug!(config = ?config, "read .forge.toml"); - config - } - Err(e) => { - // NOTE: This should never-happen - error!(error = ?e, "Failed to read config file. Using default config."); - Default::default() - } - } + /// * `config` - The pre-read [`ForgeConfig`] to seed the in-memory cache + pub fn new(cwd: PathBuf, config: ForgeConfig) -> Self { + Self { cwd, cache: Arc::new(std::sync::Mutex::new(Some(config))) } } - /// Returns the cached [`ForgeConfig`], reading from disk if the cache is - /// empty. - fn cached_config(&self) -> ForgeConfig { + /// Returns the cached [`ForgeConfig`], re-reading from disk if the cache + /// has been invalidated by [`Self::update_environment`]. + /// + /// # Errors + /// + /// Returns an error if the cache is empty and the disk read fails. + pub fn cached_config(&self) -> anyhow::Result { let mut cache = self.cache.lock().expect("cache mutex poisoned"); if let Some(ref config) = *cache { - config.clone() + Ok(config.clone()) } else { - let config = Self::read_from_disk(); + let config = ConfigReader::default() + .read_defaults() + .read_global() + .read_env() + .build()?; *cache = Some(config.clone()); - config + Ok(config) } } } @@ -149,10 +146,6 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { to_environment(self.cwd.clone()) } - fn get_config(&self) -> ForgeConfig { - 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() @@ -169,7 +162,7 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { fc.write()?; debug!(config = ?fc, "written .forge.toml"); - // Reset cache + // Reset cache so next get_config() re-reads the updated values from disk *self.cache.lock().expect("cache mutex poisoned") = None; Ok(()) diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index 2ae84ab33f..e5188c99b1 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -18,7 +18,7 @@ use reqwest_eventsource::EventSource; use crate::auth::{AnyAuthStrategy, ForgeAuthStrategyFactory}; use crate::console::StdConsoleWriter; -use crate::env::ForgeEnvironmentInfra; +use crate::env::{ForgeEnvironmentInfra, to_environment}; use crate::executor::ForgeCommandExecutorService; use crate::fs_create_dirs::ForgeCreateDirsService; use crate::fs_meta::ForgeFileMetaService; @@ -55,10 +55,18 @@ pub struct ForgeInfra { } impl ForgeInfra { - pub fn new(cwd: PathBuf) -> Self { - let config_infra = Arc::new(ForgeEnvironmentInfra::new(cwd)); - let env = config_infra.get_environment(); - let config = config_infra.get_config(); + /// Creates a new [`ForgeInfra`] with all infrastructure services + /// initialized. + /// + /// # Arguments + /// * `cwd` - The working directory for command execution and environment + /// resolution + /// * `config` - Pre-read application configuration; passed through to all + /// consumers + /// * `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()); + let config_infra = Arc::new(ForgeEnvironmentInfra::new(cwd, config.clone())); let file_write_service = Arc::new(ForgeFileWriteService::new()); let http_service = Arc::new(ForgeHttpInfra::new( @@ -70,12 +78,7 @@ impl ForgeInfra { let directory_reader_service = Arc::new(ForgeDirectoryReaderService::new( config.max_parallel_file_reads, )); - let grpc_client = Arc::new(ForgeGrpcClient::new( - config - .services_url - .parse() - .expect("services_url must be a valid URL"), - )); + let grpc_client = Arc::new(ForgeGrpcClient::new(services_url)); let output_printer = Arc::new(StdConsoleWriter::default()); Self { @@ -101,6 +104,18 @@ impl ForgeInfra { } } +impl ForgeInfra { + /// Returns the current application configuration, re-reading from disk if + /// the cache was invalidated by a prior `update_environment` call. + /// + /// # Errors + /// + /// Returns an error if the disk read fails. + pub fn config(&self) -> anyhow::Result { + self.config_infra.cached_config() + } +} + impl EnvironmentInfra for ForgeInfra { type Config = forge_config::ForgeConfig; @@ -116,10 +131,6 @@ impl EnvironmentInfra for ForgeInfra { self.config_infra.get_environment() } - fn get_config(&self) -> forge_config::ForgeConfig { - self.config_infra.get_config() - } - async fn update_environment( &self, ops: Vec, diff --git a/crates/forge_main/src/main.rs b/crates/forge_main/src/main.rs index 2802cfa452..b5d4748100 100644 --- a/crates/forge_main/src/main.rs +++ b/crates/forge_main/src/main.rs @@ -2,11 +2,13 @@ use std::io::Read; use std::panic; use std::path::PathBuf; -use anyhow::Result; +use anyhow::{Context, Result}; use clap::Parser; use forge_api::ForgeAPI; +use forge_config::ForgeConfig; use forge_domain::TitleFormat; use forge_main::{Cli, Sandbox, TitleDisplayExt, UI, tracker}; +use url::Url; /// Enables ENABLE_VIRTUAL_TERMINAL_PROCESSING on the stdout console handle. /// @@ -44,7 +46,17 @@ fn enable_stdout_vt_processing() { } #[tokio::main] -async fn main() -> Result<()> { +async fn main() { + if let Err(err) = run().await { + eprintln!("{}", TitleFormat::error(format!("{err}")).display()); + if let Some(cause) = err.chain().nth(1) { + eprintln!("{cause}"); + } + std::process::exit(1); + } +} + +async fn run() -> Result<()> { // Enable ANSI escape code support on Windows console. // `enable_ansi_support` sets VT processing on the `CONOUT$` screen buffer // handle. We additionally set it on `STD_OUTPUT_HANDLE` directly, since @@ -89,6 +101,18 @@ async fn main() -> Result<()> { } } + // Read and validate configuration at startup so any errors are surfaced + // immediately rather than silently falling back to defaults at runtime. + let config = + ForgeConfig::read().context("Failed to read Forge configuration from .forge.toml")?; + + // Pre-validate services_url so a malformed URL produces a clear error + // message at startup instead of panicking inside the constructor. + let services_url: Url = config + .services_url + .parse() + .context("services_url in configuration must be a valid URL")?; + // Handle worktree creation if specified let cwd: PathBuf = match (&cli.sandbox, &cli.directory) { (Some(sandbox), Some(cli)) => { @@ -104,7 +128,9 @@ async fn main() -> Result<()> { (_, _) => std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), }; - let mut ui = UI::init(cli, move || ForgeAPI::init(cwd.clone()))?; + let mut ui = UI::init(cli, config, move |config| { + ForgeAPI::init(cwd.clone(), config, services_url.clone()) + })?; ui.run().await; Ok(()) diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 6deedbbb8b..3b0ab93991 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -16,6 +16,7 @@ use forge_api::{ }; use forge_app::utils::{format_display_path, truncate_key}; use forge_app::{CommitResult, ToolResolver}; +use forge_config::ForgeConfig; use forge_display::MarkdownFormat; use forge_domain::{ AuthMethod, ChatResponseContent, ConsoleWriter, ContextMessage, Role, TitleFormat, UserCommand, @@ -98,7 +99,7 @@ fn format_mcp_headers(server: &forge_domain::McpServerConfig) -> Option } } -pub struct UI A> { +pub struct UI A> { markdown: MarkdownFormat, state: UIState, api: Arc, @@ -107,11 +108,12 @@ pub struct UI A> { command: Arc, cli: Cli, spinner: SharedSpinner, + config: ForgeConfig, #[allow(dead_code)] // The guard is kept alive by being held in the struct _guard: forge_tracker::Guard, } -impl A + Send + Sync> UI { +impl A + Send + Sync> UI { /// Writes a line to the console output /// Takes anything that implements ToString trait fn writeln(&mut self, content: T) -> anyhow::Result<()> { @@ -155,7 +157,9 @@ impl A + Send + Sync> UI { // Handle creating a new conversation async fn on_new(&mut self) -> Result<()> { - self.api = Arc::new((self.new_api)()); + let config = forge_config::ForgeConfig::read().unwrap_or_default(); + self.config = config.clone(); + self.api = Arc::new((self.new_api)(config)); self.init_state(false).await?; // Set agent if provided via CLI @@ -200,22 +204,35 @@ impl A + Send + Sync> UI { Ok(()) } - pub fn init(cli: Cli, f: F) -> Result { + /// Initialises the UI with the provided CLI arguments and API factory. + /// + /// # Arguments + /// * `cli` - Parsed command-line arguments + /// * `config` - Pre-read application configuration for the initial API + /// instance + /// * `f` - Factory closure invoked once at startup and again on each `/new` + /// command; receives the latest [`ForgeConfig`] so that config changes + /// from `forge config set` are reflected in new conversations + pub fn init(cli: Cli, config: ForgeConfig, f: F) -> Result { // Parse CLI arguments first to get flags - let api = Arc::new(f()); + let api = Arc::new(f(config.clone())); let env = api.environment(); - let config = api.get_config(); let command = Arc::new(ForgeCommandManager::default()); let spinner = SharedSpinner::new(SpinnerManager::new(api.clone())); Ok(Self { state: Default::default(), api, new_api: Arc::new(f), - console: Console::new(env.clone(), config.custom_history_path, command.clone()), + console: Console::new( + env.clone(), + config.custom_history_path.clone(), + command.clone(), + ), cli, command, spinner, markdown: MarkdownFormat::new(), + config, _guard: forge_tracker::init_tracing(env.log_path(), TRACKER.clone())?, }) } @@ -1545,8 +1562,8 @@ impl A + Send + Sync> UI { async fn on_env(&mut self) -> anyhow::Result<()> { let env = self.api.environment(); - let config = self.api.get_config(); - let info = Info::from(&env).extend(Info::from(&config)); + let config = &self.config; + let info = Info::from(&env).extend(Info::from(config)); self.writeln(info)?; Ok(()) } @@ -1741,7 +1758,7 @@ impl A + Send + Sync> UI { async fn list_conversations(&mut self) -> anyhow::Result<()> { self.spinner.start(Some("Loading Conversations"))?; - let max_conversations = self.api.get_config().max_conversations; + let max_conversations = self.config.max_conversations; let conversations = self.api.get_conversations(Some(max_conversations)).await?; self.spinner.stop(None)?; @@ -1775,7 +1792,7 @@ impl A + Send + Sync> UI { } async fn on_show_conversations(&mut self, porcelain: bool) -> anyhow::Result<()> { - let max_conversations = self.api.get_config().max_conversations; + let max_conversations = self.config.max_conversations; let conversations = self.api.get_conversations(Some(max_conversations)).await?; if conversations.is_empty() { @@ -2977,7 +2994,7 @@ impl A + Send + Sync> UI { .set_active_agent(active_agent.clone().unwrap_or_default()) .await?; // only call on_update if this is the first initialization - on_update(self.api.clone(), self.api.get_config().updates.as_ref()).await; + on_update(self.api.clone(), self.config.updates.as_ref()).await; } // Execute independent operations in parallel to improve performance @@ -3136,7 +3153,7 @@ impl A + Send + Sync> UI { .sub_title(subtitle), )?; - if self.api.get_config().auto_open_dump { + if self.config.auto_open_dump { open::that(path.as_str()).ok(); } } else { @@ -3160,7 +3177,7 @@ impl A + Send + Sync> UI { .sub_title(subtitle), )?; - if self.api.get_config().auto_open_dump { + if self.config.auto_open_dump { open::that(path.as_str()).ok(); } }; @@ -3241,8 +3258,7 @@ impl A + Send + Sync> UI { } ChatResponse::RetryAttempt { cause, duration: _ } => { if !self - .api - .get_config() + .config .retry .as_ref() .is_some_and(|r| r.suppress_errors) @@ -3283,7 +3299,7 @@ impl A + Send + Sync> UI { TitleFormat::debug("Finished").sub_title(conversation_id.into_string()), )?; } - if let Some(format) = self.api.get_config().auto_dump { + if let Some(format) = self.config.auto_dump.clone() { let html = matches!(format, forge_config::AutoDumpFormat::Html); self.on_dump(html).await?; } diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index ce203f6c4b..478aa26979 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -52,7 +52,7 @@ pub struct ForgeRepo { } impl ForgeRepo { - pub fn new(infra: Arc) -> Self { + pub fn new(infra: Arc, config: forge_config::ForgeConfig) -> Self { let env = infra.get_environment(); let file_snapshot_service = Arc::new(ForgeFileSnapshotService::new(env.clone())); let db_pool = @@ -67,8 +67,15 @@ impl EnvironmentInfra for ForgeRepo { self.infra.get_environment() } - fn get_config(&self) -> forge_config::ForgeConfig { - self.infra.get_config() - } - fn update_environment( &self, ops: Vec, diff --git a/crates/forge_repo/src/provider/chat.rs b/crates/forge_repo/src/provider/chat.rs index 28e208203f..fccc2505c3 100644 --- a/crates/forge_repo/src/provider/chat.rs +++ b/crates/forge_repo/src/provider/chat.rs @@ -30,10 +30,16 @@ impl ForgeChatRepository { /// # Arguments /// /// * `infra` - Infrastructure providing environment and HTTP capabilities - pub fn new(infra: Arc) -> Self { + /// * `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 { let env = infra.get_environment(); - let config = infra.get_config(); - let retry_config = Arc::new(config.retry.clone().unwrap_or_default()); + let retry_config = Arc::new(retry_config); let openai_repo = OpenAIResponseRepository::new(infra.clone()).retry_config(retry_config.clone()); @@ -49,7 +55,7 @@ impl ForgeChatRepository { let model_cache = Arc::new(CacacheStorage::new( env.cache_dir().join("model_cache"), - Some(config.model_cache_ttl_secs as u128), + Some(model_cache_ttl_secs as u128), )); Self { diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 5ff2a23982..243aac260b 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -204,11 +204,12 @@ fn get_provider_configs() -> &'static Vec { pub struct ForgeProviderRepository { infra: Arc, + config_providers: Vec, } impl ForgeProviderRepository { - pub fn new(infra: Arc) -> Self { - Self { infra } + pub fn new(infra: Arc, config_providers: Vec) -> Self { + Self { infra, config_providers } } } @@ -227,10 +228,9 @@ 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.infra - .get_config() - .providers - .into_iter() + self.config_providers + .iter() + .cloned() .map(Into::into) .collect() } @@ -798,13 +798,6 @@ mod env_tests { env } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment( &self, _ops: Vec, @@ -980,7 +973,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone()); + let registry = ForgeProviderRepository::new(infra.clone(), vec![]); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1049,7 +1042,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()); + let registry = ForgeProviderRepository::new(infra.clone(), vec![]); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1096,7 +1089,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra.clone()); + let registry = ForgeProviderRepository::new(infra.clone(), vec![]); // Trigger migration registry.migrate_env_to_file().await.unwrap(); @@ -1160,7 +1153,7 @@ mod env_tests { ); let infra = Arc::new(MockInfra::new(env_vars)); - let registry = ForgeProviderRepository::new(infra); + let registry = ForgeProviderRepository::new(infra, vec![]); // Trigger migration to populate credentials file registry.migrate_env_to_file().await.unwrap(); @@ -1217,7 +1210,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); + let registry = ForgeProviderRepository::new(infra, vec![]); // Migrate environment variables to credentials file registry.migrate_env_to_file().await.unwrap(); @@ -1297,13 +1290,6 @@ mod env_tests { env } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment( &self, _ops: Vec, @@ -1452,7 +1438,7 @@ mod env_tests { } let infra = Arc::new(CustomMockInfra { env_vars, base_path }); - let registry = ForgeProviderRepository::new(infra); + let registry = ForgeProviderRepository::new(infra, vec![]); // Get merged configs let merged_configs = registry.get_merged_configs().await; diff --git a/crates/forge_repo/src/skill.rs b/crates/forge_repo/src/skill.rs index fcbef3bc79..efe80ab25f 100644 --- a/crates/forge_repo/src/skill.rs +++ b/crates/forge_repo/src/skill.rs @@ -288,6 +288,7 @@ fn resolve_skill_conflicts(skills: Vec) -> Vec { #[cfg(test)] mod tests { + use forge_config::ForgeConfig; use forge_infra::ForgeInfra; use pretty_assertions::assert_eq; @@ -296,7 +297,13 @@ mod tests { fn fixture_skill_repo() -> (ForgeSkillRepository, std::path::PathBuf) { let skill_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("src/fixtures/skills_with_resources"); - let infra = Arc::new(ForgeInfra::new(std::env::current_dir().unwrap())); + let config = ForgeConfig::read().unwrap_or_default(); + let services_url = config.services_url.parse().unwrap(); + let infra = Arc::new(ForgeInfra::new( + std::env::current_dir().unwrap(), + config, + services_url, + )); let repo = ForgeSkillRepository::new(infra); (repo, skill_dir) } diff --git a/crates/forge_services/src/agent_registry.rs b/crates/forge_services/src/agent_registry.rs index 7afd62f8f4..dc718e3682 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -3,6 +3,7 @@ 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; @@ -13,6 +14,9 @@ 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 @@ -24,9 +28,10 @@ pub struct ForgeAgentRegistryService { impl ForgeAgentRegistryService { /// Creates a new AgentRegistryService with the given repository - pub fn new(repository: Arc) -> Self { + pub fn new(repository: Arc, config: ForgeConfig) -> Self { Self { repository, + config, agents: RwLock::new(None), active_agent_id: RwLock::new(None), } @@ -69,8 +74,11 @@ impl ForgeAgentRegistryService { /// 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 config = self.repository.get_config(); - let session = config.session.as_ref().ok_or(Error::NoDefaultProvider)?; + let session = self + .config + .session + .as_ref() + .ok_or(Error::NoDefaultProvider)?; let provider_id = session .provider_id .as_ref() diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index 2c54ccfc3d..8dbbe6ca85 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -1,6 +1,7 @@ -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use forge_app::{AppConfigService, EnvironmentInfra}; +use forge_config::ForgeConfig; use forge_domain::{ CommitConfig, ConfigOperation, Effort, ModelId, ProviderId, ProviderRepository, SuggestConfig, }; @@ -9,12 +10,13 @@ use tracing::debug; /// Service for managing user preferences for default providers and models. pub struct ForgeAppConfigService { infra: Arc, + config: Arc>, } impl ForgeAppConfigService { /// Creates a new provider preferences service. - pub fn new(infra: Arc) -> Self { - Self { infra } + pub fn new(infra: Arc, config: ForgeConfig) -> Self { + Self { infra, config: Arc::new(Mutex::new(config)) } } } @@ -31,7 +33,7 @@ impl AppConfigService for ForgeAppConfigService { async fn get_default_provider(&self) -> anyhow::Result { - let config = self.infra.get_config(); + let config = self.config.lock().unwrap(); config .session .as_ref() @@ -41,14 +43,19 @@ impl AppConfigService } async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()> { - self.update(ConfigOperation::SetProvider(provider_id)).await + 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.infra.get_config(); + let config = self.config.lock().unwrap(); let session = config .session @@ -81,16 +88,25 @@ impl AppConfigService } async fn set_default_model(&self, model: ModelId) -> anyhow::Result<()> { - let config = self.infra.get_config(); - let provider_id = config - .session - .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())) - .ok_or(forge_domain::Error::NoDefaultProvider)?; + 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, model)) - .await + 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( @@ -103,8 +119,8 @@ impl AppConfigService } async fn get_commit_config(&self) -> anyhow::Result> { - let config = self.infra.get_config(); - Ok(config.commit.map(|mc| CommitConfig { + 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), })) @@ -119,8 +135,8 @@ impl AppConfigService } async fn get_suggest_config(&self) -> anyhow::Result> { - let config = self.infra.get_config(); - Ok(config.suggest.and_then(|mc| { + let config = self.config.lock().unwrap(); + Ok(config.suggest.clone().and_then(|mc| { mc.provider_id .zip(mc.model_id) .map(|(pid, mid)| SuggestConfig { @@ -139,16 +155,20 @@ impl AppConfigService } async fn get_reasoning_effort(&self) -> anyhow::Result> { - let config = self.infra.get_config(); - Ok(config.reasoning.and_then(|r| r.effort).map(|e| match e { - forge_config::Effort::None => Effort::None, - forge_config::Effort::Minimal => Effort::Minimal, - forge_config::Effort::Low => Effort::Low, - forge_config::Effort::Medium => Effort::Medium, - forge_config::Effort::High => Effort::High, - forge_config::Effort::XHigh => Effort::XHigh, - forge_config::Effort::Max => Effort::Max, - })) + let config = self.config.lock().unwrap(); + Ok(config + .reasoning + .clone() + .and_then(|r| r.effort) + .map(|e| match e { + forge_config::Effort::None => Effort::None, + forge_config::Effort::Minimal => Effort::Minimal, + forge_config::Effort::Low => Effort::Low, + forge_config::Effort::Medium => Effort::Medium, + forge_config::Effort::High => Effort::High, + forge_config::Effort::XHigh => Effort::XHigh, + forge_config::Effort::Max => Effort::Max, + })) } async fn set_reasoning_effort(&self, effort: Effort) -> anyhow::Result<()> { @@ -166,7 +186,7 @@ mod tests { use forge_config::{ForgeConfig, ModelConfig}; use forge_domain::{ AnyProvider, ChatRepository, ConfigOperation, Environment, InputModality, MigrationResult, - Model, ModelSource, Provider, ProviderId, ProviderResponse, ProviderTemplate, + Model, ModelId, ModelSource, Provider, ProviderId, ProviderResponse, ProviderTemplate, }; use pretty_assertions::assert_eq; use url::Url; @@ -255,10 +275,6 @@ mod tests { } } - fn get_config(&self) -> ForgeConfig { - self.config.lock().unwrap().clone() - } - fn update_environment( &self, ops: Vec, @@ -404,7 +420,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)); + let service = ForgeAppConfigService::new(Arc::new(fixture), ForgeConfig::default()); let result = service.get_default_provider().await; @@ -415,7 +431,7 @@ 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())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); service.set_default_provider(ProviderId::ANTHROPIC).await?; let actual = service.get_default_provider().await?; @@ -431,7 +447,7 @@ 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())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set OpenAI as the default provider in config service.set_default_provider(ProviderId::OPENAI).await?; @@ -447,17 +463,12 @@ mod tests { #[tokio::test] async fn test_set_default_provider() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); service.set_default_provider(ProviderId::ANTHROPIC).await?; - let config = fixture.get_config(); - let actual = config - .session - .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())); - let expected = Some(ProviderId::ANTHROPIC); + let actual = service.get_default_provider().await?; + let expected = ProviderId::ANTHROPIC; assert_eq!(actual, expected); Ok(()) @@ -466,7 +477,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)); + let service = ForgeAppConfigService::new(Arc::new(fixture), ForgeConfig::default()); let result = service.get_provider_model(Some(&ProviderId::OPENAI)).await; @@ -477,7 +488,7 @@ 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())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set OpenAI as the default provider first service.set_default_provider(ProviderId::OPENAI).await?; @@ -496,7 +507,7 @@ mod tests { #[tokio::test] async fn test_set_default_model() -> anyhow::Result<()> { let fixture = MockInfra::new(); - let service = ForgeAppConfigService::new(Arc::new(fixture.clone())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set OpenAI as the default provider first service.set_default_provider(ProviderId::OPENAI).await?; @@ -504,9 +515,10 @@ mod tests { .set_default_model("gpt-4".to_string().into()) .await?; - let config = fixture.get_config(); - let actual = config.session.as_ref().and_then(|s| s.model_id.as_deref()); - let expected = Some("gpt-4"); + let actual = service + .get_provider_model(Some(&ProviderId::OPENAI)) + .await?; + let expected = "gpt-4".to_string().into(); assert_eq!(actual, expected); Ok(()) @@ -515,7 +527,7 @@ 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())); + let service = ForgeAppConfigService::new(Arc::new(fixture.clone()), ForgeConfig::default()); // Set model for OpenAI first service.set_default_provider(ProviderId::OPENAI).await?; @@ -531,16 +543,13 @@ mod tests { // ForgeConfig only tracks a single active session, so the last // provider/model pair wins - let config = fixture.get_config(); - let actual_provider = config - .session - .as_ref() - .and_then(|s| s.provider_id.as_ref()) - .map(|id| ProviderId::from(id.clone())); - let actual_model = config.session.as_ref().and_then(|s| s.model_id.as_deref()); + let actual_provider = service.get_default_provider().await?; + let actual_model = service + .get_provider_model(Some(&ProviderId::ANTHROPIC)) + .await?; - assert_eq!(actual_provider, Some(ProviderId::ANTHROPIC)); - assert_eq!(actual_model, Some("claude-3")); + assert_eq!(actual_provider, ProviderId::ANTHROPIC); + assert_eq!(actual_model, ModelId::new("claude-3")); Ok(()) } } diff --git a/crates/forge_services/src/attachment.rs b/crates/forge_services/src/attachment.rs index e698d991c9..ef6e515430 100644 --- a/crates/forge_services/src/attachment.rs +++ b/crates/forge_services/src/attachment.rs @@ -13,13 +13,14 @@ use crate::range::resolve_range; #[derive(Clone)] pub struct ForgeChatRequest { infra: Arc, + max_read_lines: u64, } impl ForgeChatRequest { - pub fn new(infra: Arc) -> Self { - Self { infra } + pub fn new(infra: Arc, max_read_lines: u64) -> Self { + Self { infra, max_read_lines } } async fn prepare_attachments(&self, paths: Vec) -> anyhow::Result> { @@ -82,11 +83,9 @@ impl { - let config = self.infra.get_config(); - 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, config.max_read_lines); + let (start_line, end_line) = resolve_range(start, end, self.max_read_lines); // range_read_utf8 returns the range content and FileInfo which // carries a content_hash of the **full** file. Using the @@ -161,13 +160,6 @@ pub mod tests { fixture.cwd(PathBuf::from("/test")) // Set fixed CWD for predictable tests } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { unimplemented!() } @@ -489,10 +481,6 @@ pub mod tests { self.env_service.get_environment() } - fn get_config(&self) -> forge_config::ForgeConfig { - self.env_service.get_config() - } - fn update_environment( &self, ops: Vec, @@ -553,7 +541,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with a text file path in chat message let url = "@[/test/file1.txt]".to_string(); @@ -575,7 +563,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with an image file let url = "@[/test/image.png]".to_string(); @@ -602,7 +590,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with an image file that has spaces in the path let url = "@[/test/image with spaces.jpg]".to_string(); @@ -635,7 +623,7 @@ pub mod tests { "This is another text file".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with multiple files mentioned let url = "@[/test/file1.txt] @[/test/file2.txt] @[/test/image.png]".to_string(); @@ -669,7 +657,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with a file that doesn't exist let url = "@[/test/nonexistent.txt]".to_string(); @@ -686,7 +674,7 @@ pub mod tests { async fn test_add_url_empty() { // Setup let infra = Arc::new(MockCompositeService::new()); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with an empty message let url = "".to_string(); @@ -709,7 +697,7 @@ pub mod tests { "Some content".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with the file let url = "@[/test/unknown.xyz]".to_string(); @@ -737,7 +725,7 @@ pub mod tests { "Line 1\nLine 2\nLine 3\nLine 4\nLine 5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); let url = "@[/test/multiline.txt]".to_string(); // Execute @@ -779,7 +767,7 @@ pub mod tests { "Line 1\nLine 2\nLine 3\nLine 4\nLine 5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test reading line 2 only let url = "@[/test/multiline.txt:2:2]"; @@ -810,7 +798,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test reading lines 2-4 let url = "@[/test/range_test.txt:2:4]"; @@ -841,7 +829,7 @@ pub mod tests { "First\nSecond\nThird\nFourth".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test reading from start to line 2 let url = "@[/test/start_range.txt:1:2]"; @@ -864,7 +852,7 @@ pub mod tests { "Alpha\nBeta\nGamma\nDelta\nEpsilon".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test reading from line 3 to end let url = "@[/test/end_range.txt:3:5]"; @@ -887,7 +875,7 @@ pub mod tests { "Only line".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test reading beyond file length let url = "@[/test/edge_case.txt:1:10]"; @@ -911,7 +899,7 @@ pub mod tests { "B1\nB2\nB3\nB4".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test multiple files with different ranges let url = "Check @[/test/file_a.txt:1:2] and @[/test/file_b.txt:3:4]"; @@ -943,7 +931,7 @@ pub mod tests { "Meta1\nMeta2\nMeta3\nMeta4\nMeta5\nMeta6\nMeta7".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test that metadata is preserved correctly with ranges let url = "@[/test/metadata_test.txt:3:5]"; @@ -974,7 +962,7 @@ pub mod tests { "Full1\nFull2\nFull3\nFull4\nFull5".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // All reads of the same file (full or ranged) should produce the same // content_hash, so the external-change detector can correctly identify @@ -1035,7 +1023,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/mydir/subdir")); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with directory path let url = "@[/test/mydir]"; @@ -1076,7 +1064,7 @@ pub mod tests { // Add empty directory infra.file_service.add_dir(PathBuf::from("/test/emptydir")); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with empty directory path let url = "@[/test/emptydir]"; @@ -1114,7 +1102,7 @@ pub mod tests { "Standalone file".to_string(), ); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); // Test with both file and directory let url = "@[/test/mixdir] @[/test/standalone.txt]"; @@ -1177,7 +1165,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/sortdir/berry_dir")); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); let url = "@[/test/sortdir]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1229,7 +1217,7 @@ pub mod tests { .file_service .add_dir(PathBuf::from("/test/onlydirs/middle_dir")); - let chat_request = ForgeChatRequest::new(infra.clone()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); let url = "@[/test/onlydirs]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1261,7 +1249,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); let url = "@[/test/onlyfiles]"; let attachments = chat_request.attachments(url).await.unwrap(); @@ -1298,7 +1286,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()); + let chat_request = ForgeChatRequest::new(infra.clone(), 2000); 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 52ea47f6a9..0b503339c8 100644 --- a/crates/forge_services/src/auth.rs +++ b/crates/forge_services/src/auth.rs @@ -10,15 +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) -> Self { - Self { infra } + pub fn new(infra: Arc, services_url: String) -> Self { + Self { infra, services_url } } async fn user_info(&self, api_key: &str) -> anyhow::Result { - let url = format!("{}{USER_INFO_ROUTE}", self.infra.get_config().services_url); + let url = format!("{}{USER_INFO_ROUTE}", self.services_url); let url = Url::parse(&url)?; let mut headers = HeaderMap::new(); @@ -37,10 +38,7 @@ impl ForgeAuthService { } async fn user_usage(&self, api_key: &str) -> anyhow::Result { - let url = Url::parse(&format!( - "{}{USER_USAGE_ROUTE}", - self.infra.get_config().services_url - ))?; + let url = Url::parse(&format!("{}{USER_USAGE_ROUTE}", self.services_url))?; let mut headers = HeaderMap::new(); headers.insert( AUTHORIZATION, diff --git a/crates/forge_services/src/context_engine.rs b/crates/forge_services/src/context_engine.rs index b82a9ad7e1..5020f3c4cb 100644 --- a/crates/forge_services/src/context_engine.rs +++ b/crates/forge_services/src/context_engine.rs @@ -23,6 +23,7 @@ use crate::sync::{WorkspaceSyncEngine, canonicalize_path}; pub struct ForgeWorkspaceService { infra: Arc, discovery: Arc, + max_file_read_batch_size: usize, } impl Clone for ForgeWorkspaceService { @@ -30,6 +31,7 @@ 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, } } } @@ -37,8 +39,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) -> Self { - Self { infra, discovery } + pub fn new(infra: Arc, discovery: Arc, max_file_read_batch_size: usize) -> Self { + Self { infra, discovery, max_file_read_batch_size } } } @@ -64,7 +66,7 @@ impl< emit(SyncProgress::Starting).await; let (token, user_id) = self.get_workspace_credentials().await?; - let batch_size = self.infra.get_config().max_file_read_batch_size; + let batch_size = self.max_file_read_batch_size; let path = canonicalize_path(path)?; // Find existing workspace - do NOT auto-create @@ -359,7 +361,7 @@ impl< // sync), avoiding a redundant canonicalize() IO call. let canonical_path = PathBuf::from(&workspace.working_dir); - let batch_size = self.infra.get_config().max_file_read_batch_size; + let batch_size = self.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 2871b57455..96b3bb7227 100644 --- a/crates/forge_services/src/discovery.rs +++ b/crates/forge_services/src/discovery.rs @@ -97,13 +97,6 @@ mod tests { env } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - 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 5ec09e1191..9a9afb477a 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -47,7 +47,6 @@ pub struct ForgeServices< + WalkerInfra + SnapshotRepository + ConversationRepository - + EnvironmentInfra + KVStore + ChatRepository + ProviderRepository @@ -99,7 +98,6 @@ impl< + UserInfra + SnapshotRepository + ConversationRepository - + EnvironmentInfra + ChatRepository + ProviderRepository + KVStore @@ -109,20 +107,33 @@ impl< + ValidationRepository, > ForgeServices { - pub fn new(infra: Arc) -> Self { + pub fn new(infra: Arc, config: forge_config::ForgeConfig) -> 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())); + let attachment_service = + Arc::new(ForgeChatRequest::new(infra.clone(), config.max_read_lines)); 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())); + let auth_service = Arc::new(ForgeAuthService::new( + infra.clone(), + config.services_url.clone(), + )); let chat_service = Arc::new(ForgeProviderService::new(infra.clone())); - let config_service = Arc::new(ForgeAppConfigService::new(infra.clone())); + let config_service = Arc::new(ForgeAppConfigService::new(infra.clone(), config.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())); - let image_read_service = Arc::new(ForgeImageRead::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_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())); @@ -132,7 +143,10 @@ 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())); + let agent_registry_service = Arc::new(ForgeAgentRegistryService::new( + infra.clone(), + config.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()); @@ -140,6 +154,7 @@ 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())); @@ -192,7 +207,6 @@ impl< + Clone + SnapshotRepository + ConversationRepository - + EnvironmentInfra + KVStore + ChatRepository + ProviderRepository @@ -366,10 +380,6 @@ impl< self.infra.get_environment() } - fn get_config(&self) -> forge_config::ForgeConfig { - 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 74719cb7fa..4a133e3e03 100644 --- a/crates/forge_services/src/tool_services/fs_read.rs +++ b/crates/forge_services/src/tool_services/fs_read.rs @@ -93,11 +93,29 @@ pub async fn assert_file_size( /// end_line parameters, ensuring the total range does not exceed 2,000 lines. /// Specifying a range exceeding this limit will result in an error. Binary /// files are automatically detected and rejected. -pub struct ForgeFsRead(Arc); +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) -> Self { - Self(infra) + 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, + } } } @@ -111,15 +129,14 @@ impl FsReadService for ) -> anyhow::Result { let path = Path::new(&path); assert_absolute_path(path)?; - let config = self.0.get_config(); // Validate with the larger limit initially since we don't know file type yet - let initial_size_limit = config.max_file_size_bytes.max(config.max_image_size_bytes); - assert_file_size(&*self.0, path, initial_size_limit).await?; + let initial_size_limit = self.max_file_size_bytes.max(self.max_image_size_bytes); + assert_file_size(&*self.infra, path, initial_size_limit).await?; // Read file content to detect MIME type let raw_content = self - .0 + .infra .read(path) .await .with_context(|| format!("Failed to read file from {}", path.display()))?; @@ -131,7 +148,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.0, path, config.max_image_size_bytes) + assert_file_size(&*self.infra, path, self.max_image_size_bytes) .await .with_context(|| { if mime_type == "application/pdf" { @@ -154,7 +171,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, config.max_read_lines); + let (start_line, end_line) = resolve_range(start_line, end_line, self.max_read_lines); // Convert bytes to UTF-8 string let full_content = String::from_utf8(raw_content) @@ -179,7 +196,7 @@ impl FsReadService for // Return full content with line truncation lines .iter() - .map(|line| truncate_line(line, config.max_line_chars)) + .map(|line| truncate_line(line, self.max_line_chars)) .collect::>() .join("\n") } else if total_lines == 0 { @@ -188,7 +205,7 @@ impl FsReadService for // Return range with line truncation lines[start_pos as usize..=end_pos as usize] .iter() - .map(|line| truncate_line(line, config.max_line_chars)) + .map(|line| truncate_line(line, self.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 8442ef37dc..63f80209a6 100644 --- a/crates/forge_services/src/tool_services/image_read.rs +++ b/crates/forge_services/src/tool_services/image_read.rs @@ -8,7 +8,10 @@ use strum_macros::{Display, EnumString}; use crate::utils::assert_absolute_path; -pub struct ForgeImageRead(Arc); +pub struct ForgeImageRead { + infra: Arc, + max_image_size_bytes: u64, +} /// Supported image formats for binary file reading #[derive(Debug, Clone, Copy, EnumString, Display)] @@ -39,8 +42,8 @@ impl ImageFormat { } impl ForgeImageRead { - pub fn new(infra: Arc) -> Self { - Self(infra) + pub fn new(infra: Arc, max_image_size_bytes: u64) -> Self { + Self { infra, max_image_size_bytes } } } #[async_trait::async_trait] @@ -50,14 +53,13 @@ impl ImageRead async fn read_image(&self, path: String) -> anyhow::Result { let path = Path::new(&path); assert_absolute_path(path)?; - let config = self.0.get_config(); // Validate file size before reading content using image-specific file size // limit crate::tool_services::fs_read::assert_file_size( - &*self.0, + &*self.infra, path, - config.max_image_size_bytes, + self.max_image_size_bytes, ) .await .with_context( @@ -82,7 +84,7 @@ impl ImageRead // Read the binary content let content = self - .0 + .infra .read(path) .await .with_context(|| format!("Failed to read binary file from {}", path.display()))?; diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index 74cbe34405..713c7bf694 100644 --- a/crates/forge_services/src/tool_services/shell.rs +++ b/crates/forge_services/src/tool_services/shell.rs @@ -118,13 +118,6 @@ mod tests { Faker.fake() } - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { unimplemented!() } diff --git a/plans/2026-04-05-config-init-at-startup-v1.md b/plans/2026-04-05-config-init-at-startup-v1.md new file mode 100644 index 0000000000..7cb20af2b9 --- /dev/null +++ b/plans/2026-04-05-config-init-at-startup-v1.md @@ -0,0 +1,193 @@ +# Config Read at Application Init — Surface Errors, Remove Silent Defaults + +## Objective + +Currently, `ForgeConfig` is read lazily from disk inside `ForgeEnvironmentInfra` and any parse/deserialization errors are silently swallowed — returning `ForgeConfig::default()` (all-zero values) with only a tracing log that the user never sees. This causes silent breakage of all tool limits and agent parameters when the user's config file is corrupt or invalid. + +The goal is to: +1. Read `ForgeConfig` **once at application startup** in `main.rs`, surfacing any parse error directly to the user before the app proceeds. +2. Pass the pre-read config through the construction chain to every consumer. +3. **Remove `get_config()` from the `EnvironmentInfra` trait** entirely — it is no longer needed since config is injected at construction time. +4. Preserve the `update_environment` write path so that `forge config set` commands continue to work correctly. + +--- + +## Architecture Overview + +### Current Flow (Lazy, Silent-Error) + +``` +main.rs + └─ ForgeAPI::init(cwd) + └─ ForgeInfra::new(cwd) + └─ ForgeEnvironmentInfra::new(cwd) ← cache = None + └─ (first get_config() call) + └─ ForgeConfig::read() ← disk I/O + └─ Err(_) → Default::default() ← SILENT! +``` + +### Target Flow (Eager, Error-Surfaced) + +``` +main.rs + └─ ForgeConfig::read()? ← fails loudly here + └─ ForgeAPI::init(cwd, config) + └─ ForgeInfra::new(cwd, config) + └─ ForgeEnvironmentInfra::new(cwd, config) ← cache = Some(config) + └─ ForgeHttpInfra::new(config, ...) + └─ ForgeDirectoryReaderService::new(config.max_parallel_file_reads) + └─ ForgeGrpcClient::new(config.services_url.parse()?) +``` + +### What `update_environment` Does (Preserved) + +`ForgeEnvironmentInfra::update_environment` performs a **read-from-disk → mutate → write-to-disk → cache-invalidate** cycle whenever the user changes provider/model/reasoning settings. After invalidation, the next `get_config()` call re-reads from disk. Since `get_config()` is being removed from the trait, the internal cache mechanism in `ForgeEnvironmentInfra` must be adapted so the fresh post-write value is propagated back to all consumers that hold a stored config. + +The cleanest resolution: `update_environment` returns the updated `ForgeConfig` in its result, and callers store the new value. However, since the trait is used in many places, the simplest compatible approach is to keep the cache internal to `ForgeEnvironmentInfra` but make `update_environment` callable from the services layer to update its stored config — or to have the application re-read config after an update via a narrower dedicated trait method (not `get_config()` on the general infra trait). + +--- + +## Key Invariants to Preserve + +1. **`update_environment` must still work** — `forge config set model ...` must update the TOML and the in-memory state visible to subsequent calls. +2. **`/new` conversation re-creation** — `on_new()` calls `(self.new_api)()`, rebuilding `ForgeAPI`. The captured `ForgeConfig` in the closure must be the **latest** config (post any `update_environment` calls), not the startup snapshot. +3. **`get_config()` removal scope** — removing it from `EnvironmentInfra` does not mean removing it from every consumer; it means the consumers receive the value as a constructor argument or method parameter rather than calling `infra.get_config()` at runtime. +4. **`services_url` panic elimination** — with config surfaced in `main.rs`, the `.expect()` on `services_url.parse()` can be converted to `?`-propagation, making `ForgeInfra::new()` fallible. + +--- + +## Implementation Plan + +### Phase 1 — Surface Config Errors in `ForgeConfig::read()` + +- [~] Task 1.1. **Fix silent error in `ConfigReader::read_global()`** (`crates/forge_config/src/reader.rs`): The `.required(false)` flag on `config::File::from(path)` silently swallows parse errors for malformed TOML files. Change this so that if the file *exists* but is invalid (e.g., malformed TOML, wrong types), an error is returned. Only missing files should be silently skipped. This may require checking file existence before adding the `config` source, or using a custom file reader that returns `Err` on parse failure but `Ok` on file-not-found. + +- [ ] Task 1.2. **Fix silent skip in `ConfigReader::read_legacy()`** (`crates/forge_config/src/reader.rs`): Currently uses `if let Ok(content) = content { ... } else { self }` — silently ignores errors. Change to at minimum emit a `warn!` log message, or propagate the error. Since legacy JSON is a migration concern, a `warn!` is appropriate rather than a hard error. + +- [ ] Task 1.3. **Verify `ForgeConfig::read()` return type** — it already returns `anyhow::Result`. No signature change needed here. The fix is upstream in the reader chain ensuring errors actually reach the `Result::Err` variant. + +### Phase 2 — Read Config Once in `main.rs` + +- [ ] Task 2.1. **Call `ForgeConfig::read()` at the top of `main()` in `crates/forge_main/src/main.rs`** before the `UI::init` call. Use `?` propagation so any error is printed to stderr and exits with a non-zero code. The error message from `anyhow` will include the cause (e.g., "invalid TOML at line 12: expected string, found integer"), which is exactly what the user needs to see. + +- [ ] Task 2.2. **Thread the `config: ForgeConfig` into the `UI::init` factory closure** in `main.rs`. The closure currently captures `cwd: PathBuf`; it must now also capture `config: ForgeConfig`. Since `ForgeConfig` derives `Clone`, the closure can clone it on each invocation (once at startup, once per `/new` command). + + > **Note on `/new` and config freshness**: The closure will capture the **startup config**. After a `forge config set` command, `update_environment` writes to disk and invalidates the `ForgeEnvironmentInfra` internal cache. However, since `get_config()` is being removed from the trait, the stale startup config captured in the closure would be used on the next `/new`. This must be addressed in Phase 4. + +### Phase 3 — Propagate Config Through the Construction Chain + +- [ ] Task 3.1. **Change `ForgeAPI::init` signature** (`crates/forge_api/src/forge_api.rs`): Add `config: ForgeConfig` as a parameter. Forward it to `ForgeInfra::new(cwd, config)`. This is the only `impl ForgeAPI<...>` concrete method that constructs the infra stack. + +- [ ] Task 3.2. **Change `ForgeInfra::new` signature** (`crates/forge_infra/src/forge_infra.rs`): Add `config: ForgeConfig` as a parameter. Remove the `config_infra.get_config()` call that currently triggers the lazy disk read. Pass `config` directly to: + - `ForgeEnvironmentInfra::new(cwd, config)` — seeds the internal cache + - `ForgeHttpInfra::new(config.clone(), ...)` — already accepts `ForgeConfig` + - `ForgeDirectoryReaderService::new(config.max_parallel_file_reads)` — already accepts the field + - `ForgeGrpcClient::new(...)` — use `?` propagation instead of `.expect()` (see Task 3.3) + + Make `ForgeInfra::new` return `anyhow::Result` to allow `?`-propagation from within. + +- [ ] Task 3.3. **Replace `.expect()` with `?` for `services_url` parsing** (`crates/forge_infra/src/forge_infra.rs:73-78`): Now that `ForgeInfra::new` is fallible, convert `config.services_url.parse().expect(...)` to `config.services_url.parse().context("services_url must be a valid URL")?`. This turns a panic into a clean error message at startup. + +- [ ] Task 3.4. **Change `ForgeEnvironmentInfra::new` signature** (`crates/forge_infra/src/env.rs`): Add `config: ForgeConfig` parameter. Initialize `cache` as `Arc::new(Mutex::new(Some(config)))` instead of `None`. The `cached_config()` method (`env.rs:125-134`) already handles the `Some` case by returning the cached value — it will simply never need to perform a disk read for the initial config. The read-from-disk path in `read_from_disk()` becomes dead code after this change and can be removed. + +- [ ] Task 3.5. **Update `ForgeAPI::init` call site in `main.rs`** to handle the new `Result` return from `ForgeInfra::new` (if propagated up through `ForgeAPI::init`). The closure passed to `UI::init` currently produces `A` (not `Result`); if `ForgeAPI::init` becomes fallible, either the closure's return type changes to `Result` and `on_new` handles the error, or the URL validation is done eagerly before the closure is constructed in `main.rs`. + + > **Recommended resolution**: Validate `services_url` in `main.rs` by parsing it there using `config.services_url.parse::().context(...)?` before creating the closure. Pass the parsed `Url` into `ForgeInfra::new` instead of the raw string. This keeps the factory closure infallible (consistent with current `UI` design where `F: Fn() -> A`). + +### Phase 4 — Handle Config Freshness After `update_environment` + +The `/new` closure captures `ForgeConfig` at startup. After `update_environment` writes a new config and invalidates the `ForgeEnvironmentInfra` cache, the next `/new` would reconstruct `ForgeAPI` with the stale startup config. This must be addressed. + +- [ ] Task 4.1. **Expose a config-accessor on the existing `API` trait** that the `UI` can use to retrieve the latest config when constructing a new API instance on `/new`. Since `ForgeEnvironmentInfra` still holds the authoritative in-memory cache (updated after `update_environment`), calling `api.get_config()` after an `update_environment` will return the fresh value. The `UI` can call `self.api.get_config()` inside `on_new` to get the latest config, then pass it to the new `ForgeAPI` factory. + + Concretely: Change the factory closure stored in `new_api` from `Fn() -> A` to `Fn(ForgeConfig) -> A`. The `UI::on_new` method calls `(self.new_api)(self.api.get_config())` to forward the live config into the new API instance. + + Adjust `UI` struct and `UI::init` accordingly: + - Change the `F` bound from `Fn() -> A` to `Fn(ForgeConfig) -> A` + - Update `main.rs` closure from `move || ForgeAPI::init(cwd.clone())` to `move |config| ForgeAPI::init(cwd.clone(), config)` + - Update `on_new` to call `(self.new_api)(self.api.get_config())` + + > This preserves `get_config()` on the `API` trait (not `EnvironmentInfra`) for this specific use case. The `API` trait's `get_config()` can delegate to the stored infra's cache (same as today), but the `EnvironmentInfra` trait no longer exposes it. + +### Phase 5 — Remove `get_config()` from `EnvironmentInfra` Trait + +- [ ] Task 5.1. **Remove `get_config()` from the `EnvironmentInfra` trait** in `crates/forge_app/src/infra.rs`. This is the core structural change. All code that calls `infra.get_config()` through the trait must be updated to receive `ForgeConfig` through another mechanism (constructor parameter, method parameter). + +- [ ] Task 5.2. **Remove `get_config()` from `ForgeInfra`** (`crates/forge_infra/src/forge_infra.rs`): The delegation to `config_infra.get_config()` is no longer needed. + +- [ ] Task 5.3. **Remove `get_config()` from `ForgeRepo`** (`crates/forge_repo/src/forge_repo.rs`): Same delegation removal. + +- [ ] Task 5.4. **Remove `get_config()` from `ForgeServices`** (`crates/forge_services/src/forge_services.rs`): Same delegation removal. The `AppConfigService` methods currently call `self.infra.get_config()` to read existing config fields before mutating — these should instead use the config passed through method parameters or read from the updated state returned by `update_environment`. + +- [ ] Task 5.5. **Audit all call sites of `get_config()` through the `EnvironmentInfra` trait** — specifically inside `crates/forge_app/src/app.rs`, `crates/forge_app/src/agent.rs`, `crates/forge_app/src/tool_executor.rs`, `crates/forge_app/src/operation.rs`, and `crates/forge_services/src/app_config.rs`. For each call site: + - If the caller is a service method, consider passing `ForgeConfig` as a method parameter from the call site one level up. + - If the caller is a long-lived struct that currently reads config on every operation, consider storing `ForgeConfig` as a field injected at construction time. + +- [ ] Task 5.6. **Update `ForgeAppConfigService::get_default_provider`, `get_provider_model`, etc.** (`crates/forge_services/src/app_config.rs`): These currently call `self.infra.get_config()` to read the current provider/model. After removing `get_config()` from `EnvironmentInfra`, the service needs another way to get the current config. Options: + - Store `ForgeConfig` in `ForgeAppConfigService` at construction time (simplest, but may go stale after `update_environment`). + - Have `update_environment` return the updated `ForgeConfig` value, letting callers store the fresh value. + - Keep a **separate, narrow trait** like `ConfigReader` with only `get_config()` on it, distinct from `EnvironmentInfra`, used only where runtime config re-reads are genuinely needed (i.e., after `update_environment` writes). + + > **Recommended**: The cleanest approach given the existing architecture is to have `update_environment` return `ForgeConfig` (the new config after applying ops). Services that call `update_environment` can then update their stored config reference. This requires changing `update_environment`'s return type from `anyhow::Result<()>` to `anyhow::Result` in the `EnvironmentInfra` trait. + +### Phase 6 — Remove `read_from_disk()` Dead Code + +- [ ] Task 6.1. **Remove `read_from_disk()` from `ForgeEnvironmentInfra`** (`crates/forge_infra/src/env.rs`): With the cache always pre-seeded from the constructor, the `read_from_disk()` method and its `error!("Failed to read config file. Using default config.")` fallback are dead code. Remove both the method and the `// NOTE: This should never-happen` comment that has been lying about the real risk. + +- [ ] Task 6.2. **Simplify `cached_config()`** (`crates/forge_infra/src/env.rs`): The `Mutex>` can be simplified. If the cache is always initialized via the constructor and only set to `None` by `update_environment` (which then repopulates from disk), the `Option` wrapper is still needed for the update cycle. Retain the structure but remove the `read_from_disk()` call in the `None` branch — instead call `ConfigReader::default().read_defaults().read_global().build()?` directly and propagate the error rather than swallowing it. + + > **Note**: `cached_config()` currently returns `ForgeConfig` (not `Result`). If `update_environment` invalidates the cache and the subsequent re-read can fail, `cached_config()` must return `Result`. Propagate this change upward through `get_config()` on `ForgeEnvironmentInfra` (which may remain internal/non-trait after trait removal). + +### Phase 7 — Update Tests and Snapshots + +- [ ] Task 7.1. **Update mock implementations of `EnvironmentInfra`** in test code: Any `#[cfg(test)]` or mock structs that implement `EnvironmentInfra` will need `get_config()` removed from their impl blocks after Phase 5. Search for all `impl EnvironmentInfra` in the codebase and remove the `get_config()` method bodies. + +- [ ] Task 7.2. **Update tests that exercise `ForgeInfra::new` or `ForgeAPI::init`**: These now require a `ForgeConfig` argument. Construct test configs using `ForgeConfig::default()` (which provides Rust-defaulted zeroes) or `ForgeConfig::read()` where a realistic config is needed. + +- [ ] Task 7.3. **Run `cargo insta test --accept`** to update any snapshot tests affected by structural changes. + +- [ ] Task 7.4. **Run `cargo check`** across the workspace to surface any remaining compilation errors from the trait removal cascade. + +--- + +## Verification Criteria + +- Starting forge with a corrupt `~/.forge.toml` file (e.g., `forge = {invalid`) must print a clear, human-readable error message to stderr and exit with a non-zero code — not silently start with default config. +- Starting forge with a `FORGE_SERVICES_URL=not-a-url` environment variable must print a clear error and exit rather than panicking. +- `forge config set model anthropic claude-3-opus` must still work correctly — the model is updated in `~/.forge.toml` and subsequent operations use the new model. +- Starting a new conversation with `/new` after `forge config set ...` must reflect the updated config (not the startup snapshot). +- `forge env` must still display the current config. +- All existing tests pass under `cargo insta test --accept`. +- `cargo check` produces no errors across the workspace. + +--- + +## Potential Risks and Mitigations + +1. **`/new` gets stale config after `update_environment`** + Mitigation: Change the factory closure type from `Fn() -> A` to `Fn(ForgeConfig) -> A` (Task 4.1). `on_new` reads the latest config from the live API before constructing the new one. + +2. **`EnvironmentInfra` implementors in external or test code break on trait change** + Mitigation: Search all `impl EnvironmentInfra` blocks across the workspace (Task 7.1) and update them. Since the trait is internal (not `pub` across crate boundaries to user code), the scope is bounded to this repository. + +3. **`update_environment` re-read from disk can fail post-startup** + Mitigation: Change `update_environment` return type to `anyhow::Result` and propagate the error up through the service layer to the UI, which displays it as a user-visible error message rather than silently ignoring it. + +4. **`ConfigReader::read_global()` change breaks behavior for missing config file** + Mitigation: The fix in Task 1.1 must distinguish between "file does not exist" (skip silently, as today) and "file exists but is malformed" (return error). Use `path.exists()` check before deciding whether to add the source as required or not. + +5. **`ForgeInfra::new` becoming `Result`-returning cascades through `ForgeAPI::init` and the closure** + Mitigation: Pre-validate `services_url` in `main.rs` before the closure (Task 3.5, recommended approach). This keeps the factory closure return type as `A` (not `Result`), preserving the `UI` generic constraint without requiring a `UI` redesign. + +6. **`AppConfigService::get_default_provider` and similar read-config methods need config access** + Mitigation: Implement the `update_environment` → returns `ForgeConfig` approach (Task 5.6). Alternatively, store `ForgeConfig` at service construction time and update it in-place after each `update_environment` call returns. The former is cleaner. + +--- + +## Alternative Approaches + +1. **Keep `get_config()` on `EnvironmentInfra` but add startup validation**: Instead of full removal, keep the lazy read but call `ForgeConfig::read()?` in `main.rs` purely for validation (discard the result), then let the infra re-read it lazily. Simpler change, but duplicates disk reads and doesn't achieve the "config piped through everywhere" goal. Does not eliminate the dead error path in `read_from_disk()`. + +2. **Wrap `ForgeConfig` in `Arc>`**: Rather than threading config through constructors, store a shared `Arc>` that all consumers read from. `update_environment` acquires the write lock and updates in place. This is a valid reactive pattern but adds lock complexity and requires write-lock acquisition on every config read, even in hot paths like `tool_executor`. + +3. **Narrow the `EnvironmentInfra` trait instead of removing `get_config()`**: Split into `EnvironmentInfra` (env vars, environment) and a separate `ConfigInfra` trait (just `get_config()` and `update_environment()`). Services that need only config use `ConfigInfra`; services that need only env use `EnvironmentInfra`. This reduces the trait surface each service depends on and is a valid design improvement, but is a larger refactor scope than strictly required. diff --git a/shell-plugin/forge.theme.zsh b/shell-plugin/forge.theme.zsh index 8081396174..fab0e5de6c 100644 --- a/shell-plugin/forge.theme.zsh +++ b/shell-plugin/forge.theme.zsh @@ -18,7 +18,7 @@ function _forge_prompt_info() { [[ -n "$_FORGE_SESSION_MODEL" ]] && local -x FORGE_SESSION__MODEL_ID="$_FORGE_SESSION_MODEL" [[ -n "$_FORGE_SESSION_PROVIDER" ]] && local -x FORGE_SESSION__PROVIDER_ID="$_FORGE_SESSION_PROVIDER" [[ -n "$_FORGE_SESSION_REASONING_EFFORT" ]] && local -x FORGE_REASONING__EFFORT="$_FORGE_SESSION_REASONING_EFFORT" - _FORGE_CONVERSATION_ID=$_FORGE_CONVERSATION_ID _FORGE_ACTIVE_AGENT=$_FORGE_ACTIVE_AGENT "${forge_cmd[@]}" + _FORGE_CONVERSATION_ID=$_FORGE_CONVERSATION_ID _FORGE_ACTIVE_AGENT=$_FORGE_ACTIVE_AGENT "${forge_cmd[@]}" 2>/dev/null } # Right prompt: agent and model with token count (uses single forge prompt command)