From fed4cd194cbb59f2c34c40a93e4952558ae7c634 Mon Sep 17 00:00:00 2001 From: Tushar Date: Sun, 5 Apr 2026 14:32:02 +0530 Subject: [PATCH 1/5] feat(config): read ForgeConfig once at startup with cache --- crates/forge_api/src/forge_api.rs | 19 +- crates/forge_app/src/changed_files.rs | 20 +- crates/forge_app/src/command_generator.rs | 16 +- crates/forge_app/src/git_app.rs | 5 +- crates/forge_app/src/infra.rs | 23 ++- crates/forge_app/src/services.rs | 4 +- crates/forge_config/src/reader.rs | 35 +++- crates/forge_infra/src/env.rs | 64 +++--- crates/forge_infra/src/forge_infra.rs | 38 ++-- crates/forge_main/src/main.rs | 23 ++- crates/forge_main/src/ui.rs | 20 +- crates/forge_repo/src/forge_repo.rs | 39 +++- crates/forge_repo/src/provider/chat.rs | 6 +- .../forge_repo/src/provider/provider_repo.rs | 40 ++-- crates/forge_repo/src/skill.rs | 9 +- crates/forge_services/src/agent_registry.rs | 6 +- crates/forge_services/src/app_config.rs | 16 +- crates/forge_services/src/attachment.rs | 29 +-- crates/forge_services/src/auth.rs | 6 +- crates/forge_services/src/context_engine.rs | 7 +- crates/forge_services/src/discovery.rs | 16 +- crates/forge_services/src/forge_services.rs | 41 +++- .../src/tool_services/fs_read.rs | 6 +- .../src/tool_services/image_read.rs | 4 +- .../forge_services/src/tool_services/shell.rs | 16 +- plans/2026-04-05-config-init-at-startup-v1.md | 193 ++++++++++++++++++ 26 files changed, 510 insertions(+), 191 deletions(-) create mode 100644 plans/2026-04-05-config-init-at-startup-v1.md diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 36df08a4c4..1f033edf80 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -6,9 +6,10 @@ use anyhow::Result; use forge_app::dto::ToolsOverview; use forge_app::{ AgentProviderResolver, AgentRegistry, AppConfigService, AuthService, CommandInfra, - CommandLoaderService, ConversationService, DataGenerationApp, EnvironmentInfra, - FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, McpService, - ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, WorkspaceService, + CommandLoaderService, ConfigReaderInfra, ConversationService, DataGenerationApp, + EnvironmentInfra, FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, + McpService, ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, + WorkspaceService, }; use forge_config::ForgeConfig; use forge_domain::{Agent, ConsoleWriter, *}; @@ -41,8 +42,14 @@ impl ForgeAPI { } impl ForgeAPI>, ForgeRepo> { - pub fn init(cwd: PathBuf) -> Self { - let infra = Arc::new(ForgeInfra::new(cwd)); + /// 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, services_url)); let repo = Arc::new(ForgeRepo::new(infra.clone())); let app = Arc::new(ForgeServices::new(repo.clone())); ForgeAPI::new(app, repo) @@ -55,7 +62,7 @@ impl ForgeAPI>, ForgeRepo> { } #[async_trait::async_trait] -impl API +impl API for ForgeAPI { async fn discover(&self) -> Result> { diff --git a/crates/forge_app/src/changed_files.rs b/crates/forge_app/src/changed_files.rs index 6b52639f93..bbf5309e8e 100644 --- a/crates/forge_app/src/changed_files.rs +++ b/crates/forge_app/src/changed_files.rs @@ -4,7 +4,7 @@ use forge_domain::{Agent, ContextMessage, Conversation, Role, TextMessage}; use forge_template::Element; use crate::utils::format_display_path; -use crate::{EnvironmentInfra, FsReadService}; +use crate::{ConfigReaderInfra, EnvironmentInfra, FsReadService}; /// Service responsible for detecting externally changed files and rendering /// notifications @@ -20,7 +20,7 @@ impl ChangedFiles { } } -impl ChangedFiles { +impl ChangedFiles { /// Detects externally changed files and renders a notification if changes /// are found. Updates file hashes in conversation metrics to prevent /// duplicate notifications. @@ -133,13 +133,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, @@ -156,6 +149,15 @@ mod tests { } } + impl ConfigReaderInfra for TestServices { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + fn fixture( files: HashMap, tracked_files: HashMap>, diff --git a/crates/forge_app/src/command_generator.rs b/crates/forge_app/src/command_generator.rs index 837db4d7c0..9af37166dd 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, @@ -167,6 +160,15 @@ mod tests { } } + impl crate::ConfigReaderInfra for MockServices { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + #[async_trait::async_trait] impl FileDiscoveryService for MockServices { async fn collect_files(&self, _walker: Walker) -> Result> { diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index 4f20cce238..3fcec6389c 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -7,8 +7,8 @@ use schemars::JsonSchema; use serde::Deserialize; use crate::{ - AgentProviderResolver, AgentRegistry, AppConfigService, EnvironmentInfra, ProviderAuthService, - ProviderService, ShellService, TemplateService, + AgentProviderResolver, AgentRegistry, AppConfigService, ConfigReaderInfra, EnvironmentInfra, + ProviderAuthService, ProviderService, ShellService, TemplateService, }; /// Errors specific to GitApp operations @@ -95,6 +95,7 @@ impl GitApp { impl GitApp where S: EnvironmentInfra + + ConfigReaderInfra + ShellService + AgentRegistry + TemplateService diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index 8c1c567772..2fb6ab647b 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -20,8 +20,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 +29,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 @@ -50,6 +42,19 @@ pub trait EnvironmentInfra: Send + Sync { ) -> impl std::future::Future> + Send; } +/// Trait for reading the current [`ForgeConfig`] from an in-memory cache. +/// +/// Separated from [`EnvironmentInfra`] so that consumers that only need to +/// read configuration values do not need access to the full environment +/// infrastructure. The cache is pre-seeded at application startup and is +/// refreshed after every [`EnvironmentInfra::update_environment`] call. +pub trait ConfigReaderInfra: Send + Sync { + /// Returns the full [`ForgeConfig`] for the current session. + /// + /// Reads from the in-memory cache; does not perform disk I/O on every call. + fn get_config(&self) -> ForgeConfig; +} + /// Repository for accessing system environment information /// This uses the EnvironmentService trait from forge_domain /// A service for reading files from the filesystem. diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 9cf7a12c89..3b391896de 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -16,7 +16,7 @@ use reqwest_eventsource::EventSource; use url::Url; use crate::user::{User, UserUsage}; -use crate::{EnvironmentInfra, Walker}; +use crate::{ConfigReaderInfra, EnvironmentInfra, Walker}; #[derive(Debug, Clone)] pub struct ShellOutput { @@ -548,7 +548,7 @@ pub trait ProviderAuthService: Send + Sync { ) -> anyhow::Result>; } -pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { +pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra + ConfigReaderInfra { type ProviderService: ProviderService; type AppConfigService: AppConfigService; type ConversationService: ConversationService; diff --git a/crates/forge_config/src/reader.rs b/crates/forge_config/src/reader.rs index ea74746de6..a411f494a5 100644 --- a/crates/forge_config/src/reader.rs +++ b/crates/forge_config/src/reader.rs @@ -3,6 +3,7 @@ use std::sync::LazyLock; use config::ConfigBuilder; use config::builder::DefaultState; +use tracing::warn; use crate::ForgeConfig; use crate::legacy::LegacyConfig; @@ -101,24 +102,36 @@ impl ConfigReader { Ok(config.try_deserialize::()?) } - /// Adds `~/.forge/.forge.toml` as a config source, silently skipping if - /// absent. + /// Adds `~/.forge/.forge.toml` as a config source. + /// + /// Silently skips if the file does not exist; returns an error if the file + /// exists but cannot be parsed (e.g., malformed TOML, wrong value types). pub fn read_global(mut self) -> Self { let path = Self::config_path(); - self.builder = self - .builder - .add_source(config::File::from(path).required(false)); + // Only add the source if the file exists so that a missing config file + // is silently skipped, while a present-but-malformed file surfaces as + // an error during `build()`. + if path.exists() { + self.builder = self + .builder + .add_source(config::File::from(path).required(true)); + } self } - /// Reads `~/.forge/.config.json` (legacy format) and adds it as a source, - /// silently skipping errors. + /// Reads `~/.forge/.config.json` (legacy format) and adds it as a source. + /// + /// Silently skips if the file does not exist. Emits a `warn!` log if the + /// file exists but cannot be parsed, so the user is informed without + /// aborting startup. pub fn read_legacy(self) -> Self { let content = LegacyConfig::read(&Self::config_legacy_path()); - if let Ok(content) = content { - self.read_toml(&content) - } else { - self + match content { + Ok(content) => self.read_toml(&content), + Err(e) => { + warn!(error = ?e, "Failed to read legacy config file; skipping"); + self + } } } } diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index 902198b447..7f884bd9ca 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -2,10 +2,10 @@ use std::collections::BTreeMap; use std::path::PathBuf; use std::sync::Arc; -use forge_app::EnvironmentInfra; +use forge_app::{ConfigReaderInfra, 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)) } + /// * `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))) } } - /// 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() - } - } - } - - /// 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. + 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,13 +162,20 @@ 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(()) } } +impl ConfigReaderInfra for ForgeEnvironmentInfra { + fn get_config(&self) -> ForgeConfig { + self.cached_config() + .expect("ForgeConfig cache read failed after update_environment") + } +} + #[cfg(test)] mod tests { use std::path::PathBuf; diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index 2ae84ab33f..7cb1e59148 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -5,9 +5,9 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, - FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, McpServerInfra, - StrategyFactory, UserInfra, WalkerInfra, + CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, + FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, + McpServerInfra, StrategyFactory, UserInfra, WalkerInfra, }; use forge_domain::{ AuthMethod, CommandOutput, FileInfo as FileInfoData, McpServerConfig, ProviderId, URLParamSpec, @@ -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,15 @@ 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 +75,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 +101,12 @@ impl ForgeInfra { } } +impl ConfigReaderInfra for ForgeInfra { + fn get_config(&self) -> forge_config::ForgeConfig { + self.config_infra.get_config() + } +} + impl EnvironmentInfra for ForgeInfra { type Config = forge_config::ForgeConfig; @@ -116,10 +122,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..23d3e3c1e6 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. /// @@ -89,6 +91,19 @@ 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. Please check your ~/.forge/.forge.toml file.", + )?; + + // 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 +119,11 @@ 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 3d6b946bac..ff8e3c2ca4 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, @@ -111,7 +112,7 @@ pub struct UI A> { _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<()> { @@ -163,7 +164,8 @@ 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 = self.api.get_config(); + self.api = Arc::new((self.new_api)(config)); self.init_state(false).await?; // Set agent if provided via CLI @@ -208,9 +210,17 @@ 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)); let env = api.environment(); let config = api.get_config(); let command = Arc::new(ForgeCommandManager::default()); diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index ce203f6c4b..f0597c196f 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -4,9 +4,10 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, - FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, - KVStore, McpServerInfra, StrategyFactory, UserInfra, WalkedFile, Walker, WalkerInfra, + AgentRepository, CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, + FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, + GrpcInfra, HttpInfra, KVStore, McpServerInfra, StrategyFactory, UserInfra, WalkedFile, Walker, + WalkerInfra, }; use forge_domain::{ AnyProvider, AuthCredential, ChatCompletionMessage, ChatRepository, CommandOutput, Context, @@ -51,7 +52,14 @@ pub struct ForgeRepo { fuzzy_search_repository: Arc>, } -impl ForgeRepo { +impl< + F: EnvironmentInfra + + ConfigReaderInfra + + FileReaderInfra + + FileWriterInfra + + GrpcInfra + + HttpInfra, +> ForgeRepo { pub fn new(infra: Arc) -> Self { let env = infra.get_environment(); let file_snapshot_service = Arc::new(ForgeFileSnapshotService::new(env.clone())); @@ -140,7 +148,7 @@ impl ConversationRepository for ForgeRepo { } #[async_trait::async_trait] -impl +impl ChatRepository for ForgeRepo { async fn chat( @@ -158,8 +166,15 @@ impl - ProviderRepository for ForgeRepo +impl< + F: EnvironmentInfra + + ConfigReaderInfra + + FileReaderInfra + + FileWriterInfra + + HttpInfra + + Send + + Sync, +> ProviderRepository for ForgeRepo { async fn get_all_providers(&self) -> anyhow::Result> { self.provider_repository.get_all_providers().await @@ -196,10 +211,6 @@ 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, @@ -216,6 +227,12 @@ impl EnvironmentInfra for ForgeRepo { } } +impl ConfigReaderInfra for ForgeRepo { + fn get_config(&self) -> forge_config::ForgeConfig { + self.infra.get_config() + } +} + #[async_trait::async_trait] impl KVStore for ForgeRepo { async fn cache_get(&self, key: &K) -> anyhow::Result> diff --git a/crates/forge_repo/src/provider/chat.rs b/crates/forge_repo/src/provider/chat.rs index 28e208203f..60a79ebc9e 100644 --- a/crates/forge_repo/src/provider/chat.rs +++ b/crates/forge_repo/src/provider/chat.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use forge_app::domain::{ ChatCompletionMessage, Context, Model, ModelId, ProviderResponse, ResultStream, }; -use forge_app::{EnvironmentInfra, HttpInfra}; +use forge_app::{ConfigReaderInfra, EnvironmentInfra, HttpInfra}; use forge_domain::{ChatRepository, Provider, ProviderId}; use forge_infra::CacacheStorage; use tokio::task::AbortHandle; @@ -24,7 +24,7 @@ pub struct ForgeChatRepository { bg_refresh: BgRefresh, } -impl ForgeChatRepository { +impl ForgeChatRepository { /// Creates a new ForgeChatRepository with the given infrastructure. /// /// # Arguments @@ -68,7 +68,7 @@ impl ForgeChatRepository { } #[async_trait::async_trait] -impl ChatRepository for ForgeChatRepository { +impl ChatRepository for ForgeChatRepository { async fn chat( &self, model_id: &ModelId, diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 5ff2a23982..74f9268864 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -2,7 +2,7 @@ use std::sync::{Arc, LazyLock}; use bytes::Bytes; use forge_app::domain::{ProviderId, ProviderResponse}; -use forge_app::{EnvironmentInfra, FileReaderInfra, FileWriterInfra, HttpInfra}; +use forge_app::{ConfigReaderInfra, EnvironmentInfra, FileReaderInfra, FileWriterInfra, HttpInfra}; use forge_domain::{ AnyProvider, ApiKey, AuthCredential, AuthDetails, Error, MigrationResult, Provider, ProviderRepository, ProviderType, URLParam, URLParamSpec, URLParamValue, @@ -212,7 +212,7 @@ impl ForgeProviderRepository { } } -impl +impl ForgeProviderRepository { async fn get_custom_provider_configs(&self) -> anyhow::Result> { @@ -520,8 +520,8 @@ impl } #[async_trait::async_trait] -impl ProviderRepository - for ForgeProviderRepository +impl + ProviderRepository for ForgeProviderRepository { async fn get_all_providers(&self) -> anyhow::Result> { Ok(self.get_providers().await) @@ -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, @@ -824,6 +817,15 @@ mod env_tests { } } + impl forge_app::ConfigReaderInfra for MockInfra { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + #[async_trait::async_trait] impl FileReaderInfra for MockInfra { async fn read_utf8(&self, path: &std::path::Path) -> anyhow::Result { @@ -1297,13 +1299,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, @@ -1323,6 +1318,15 @@ mod env_tests { } } + impl forge_app::ConfigReaderInfra for CustomMockInfra { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + #[async_trait::async_trait] impl FileReaderInfra for CustomMockInfra { async fn read_utf8(&self, path: &std::path::Path) -> anyhow::Result { 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..1b20973468 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use dashmap::DashMap; use forge_app::domain::{AgentId, Error, ModelId, ProviderId}; -use forge_app::{AgentRepository, EnvironmentInfra}; +use forge_app::{AgentRepository, ConfigReaderInfra, EnvironmentInfra}; use forge_domain::Agent; use tokio::sync::RwLock; @@ -33,7 +33,7 @@ impl ForgeAgentRegistryService { } } -impl ForgeAgentRegistryService { +impl ForgeAgentRegistryService { /// Lazily initializes and returns the agents map /// Loads agents from repository on first call, subsequent calls return /// cached value @@ -96,7 +96,7 @@ impl ForgeAgentRegistryService { } #[async_trait::async_trait] -impl forge_app::AgentRegistry +impl forge_app::AgentRegistry for ForgeAgentRegistryService { async fn get_active_agent_id(&self) -> anyhow::Result> { diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index 4dfe26206d..fe6cebcf05 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use forge_app::{AppConfigService, EnvironmentInfra}; +use forge_app::{AppConfigService, ConfigReaderInfra, EnvironmentInfra}; use forge_domain::{ CommitConfig, ConfigOperation, Effort, ModelId, ProviderId, ProviderRepository, SuggestConfig, }; @@ -18,7 +18,7 @@ impl ForgeAppConfigService { } } -impl ForgeAppConfigService { +impl ForgeAppConfigService { /// Helper method to apply a config operation atomically. async fn update(&self, op: ConfigOperation) -> anyhow::Result<()> { debug!(op = ?op, "Updating app config"); @@ -27,7 +27,7 @@ impl ForgeAppConfigService { } #[async_trait::async_trait] -impl AppConfigService +impl AppConfigService for ForgeAppConfigService { async fn get_default_provider(&self) -> anyhow::Result { @@ -246,10 +246,6 @@ mod tests { } } - fn get_config(&self) -> ForgeConfig { - self.config.lock().unwrap().clone() - } - fn update_environment( &self, ops: Vec, @@ -313,6 +309,12 @@ mod tests { } } + impl forge_app::ConfigReaderInfra for MockInfra { + fn get_config(&self) -> ForgeConfig { + self.config.lock().unwrap().clone() + } + } + #[async_trait::async_trait] impl ChatRepository for MockInfra { async fn chat( diff --git a/crates/forge_services/src/attachment.rs b/crates/forge_services/src/attachment.rs index e698d991c9..20bb9ac163 100644 --- a/crates/forge_services/src/attachment.rs +++ b/crates/forge_services/src/attachment.rs @@ -5,7 +5,8 @@ use forge_app::domain::{ }; use forge_app::utils::format_display_path; use forge_app::{ - AttachmentService, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, FileReaderInfra, + AttachmentService, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, + FileReaderInfra, }; use crate::range::resolve_range; @@ -15,7 +16,7 @@ pub struct ForgeChatRequest { infra: Arc, } -impl +impl ForgeChatRequest { pub fn new(infra: Arc) -> Self { @@ -114,8 +115,8 @@ impl AttachmentService - for ForgeChatRequest +impl + AttachmentService for ForgeChatRequest { async fn attachments(&self, url: &str) -> anyhow::Result> { self.prepare_attachments(Attachment::parse_all(url)).await @@ -161,16 +162,18 @@ pub mod tests { fixture.cwd(PathBuf::from("/test")) // Set fixed CWD for predictable tests } + async fn update_environment(&self, _ops: Vec) -> anyhow::Result<()> { + unimplemented!() + } + } + + impl forge_app::ConfigReaderInfra for MockEnvironmentInfra { 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!() - } } impl MockFileService { @@ -489,10 +492,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, @@ -510,6 +509,12 @@ pub mod tests { } } + impl forge_app::ConfigReaderInfra for MockCompositeService { + fn get_config(&self) -> forge_config::ForgeConfig { + self.env_service.get_config() + } + } + #[async_trait::async_trait] impl FileInfoInfra for MockCompositeService { async fn is_binary(&self, path: &Path) -> anyhow::Result { diff --git a/crates/forge_services/src/auth.rs b/crates/forge_services/src/auth.rs index 52ea47f6a9..97bd7a103c 100644 --- a/crates/forge_services/src/auth.rs +++ b/crates/forge_services/src/auth.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use forge_app::{AuthService, EnvironmentInfra, HttpInfra, User, UserUsage}; +use forge_app::{AuthService, ConfigReaderInfra, EnvironmentInfra, HttpInfra, User, UserUsage}; use reqwest::Url; use reqwest::header::{AUTHORIZATION, HeaderMap, HeaderValue}; @@ -12,7 +12,7 @@ pub struct ForgeAuthService { infra: Arc, } -impl ForgeAuthService { +impl ForgeAuthService { pub fn new(infra: Arc) -> Self { Self { infra } } @@ -58,7 +58,7 @@ impl ForgeAuthService { } #[async_trait::async_trait] -impl AuthService for ForgeAuthService { +impl AuthService for ForgeAuthService { async fn user_info(&self, api_key: &str) -> anyhow::Result { self.user_info(api_key).await } diff --git a/crates/forge_services/src/context_engine.rs b/crates/forge_services/src/context_engine.rs index b82a9ad7e1..99bb88e3fd 100644 --- a/crates/forge_services/src/context_engine.rs +++ b/crates/forge_services/src/context_engine.rs @@ -4,7 +4,10 @@ use std::sync::Arc; use anyhow::{Context, Result}; use async_trait::async_trait; -use forge_app::{CommandInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, WorkspaceService}; +use forge_app::{ + CommandInfra, ConfigReaderInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, + WorkspaceService, +}; use forge_domain::{ AuthCredential, AuthDetails, ProviderId, ProviderRepository, SyncProgress, UserId, WorkspaceId, WorkspaceIndexRepository, @@ -48,6 +51,7 @@ impl< + WorkspaceIndexRepository + FileReaderInfra + EnvironmentInfra + + ConfigReaderInfra + CommandInfra + WalkerInfra, D: FileDiscovery + 'static, @@ -220,6 +224,7 @@ impl< + WorkspaceIndexRepository + FileReaderInfra + EnvironmentInfra + + ConfigReaderInfra + CommandInfra + WalkerInfra + 'static, diff --git a/crates/forge_services/src/discovery.rs b/crates/forge_services/src/discovery.rs index 2871b57455..b3a6a1b830 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!() } @@ -117,6 +110,15 @@ mod tests { } } + impl forge_app::ConfigReaderInfra for MockInfra { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + #[async_trait::async_trait] impl WalkerInfra for MockInfra { async fn walk(&self, _config: Walker) -> Result> { diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 5ec09e1191..1d61e27716 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -1,9 +1,9 @@ use std::sync::Arc; use forge_app::{ - AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, - FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, HttpInfra, KVStore, - McpServerInfra, Services, StrategyFactory, UserInfra, WalkerInfra, + AgentRepository, CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, + FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, + HttpInfra, KVStore, McpServerInfra, Services, StrategyFactory, UserInfra, WalkerInfra, }; use forge_domain::{ ChatRepository, ConversationRepository, FuzzySearchRepository, ProviderRepository, @@ -43,11 +43,11 @@ type AuthService = ForgeAuthService; pub struct ForgeServices< F: HttpInfra + EnvironmentInfra + + ConfigReaderInfra + McpServerInfra + WalkerInfra + SnapshotRepository + ConversationRepository - + EnvironmentInfra + KVStore + ChatRepository + ProviderRepository @@ -89,6 +89,7 @@ pub struct ForgeServices< impl< F: McpServerInfra + EnvironmentInfra + + ConfigReaderInfra + FileWriterInfra + FileInfoInfra + FileReaderInfra @@ -99,7 +100,6 @@ impl< + UserInfra + SnapshotRepository + ConversationRepository - + EnvironmentInfra + ChatRepository + ProviderRepository + KVStore @@ -186,13 +186,13 @@ impl< + FileInfoInfra + FileDirectoryInfra + EnvironmentInfra + + ConfigReaderInfra + DirectoryReaderInfra + HttpInfra + WalkerInfra + Clone + SnapshotRepository + ConversationRepository - + EnvironmentInfra + KVStore + ChatRepository + ProviderRepository @@ -344,6 +344,7 @@ impl< impl< F: EnvironmentInfra + + ConfigReaderInfra + HttpInfra + McpServerInfra + WalkerInfra @@ -366,10 +367,6 @@ impl< self.infra.get_environment() } - fn get_config(&self) -> forge_config::ForgeConfig { - self.infra.get_config() - } - fn update_environment( &self, ops: Vec, @@ -385,3 +382,27 @@ impl< self.infra.get_env_vars() } } + +impl< + F: ConfigReaderInfra + + EnvironmentInfra + + HttpInfra + + McpServerInfra + + WalkerInfra + + SnapshotRepository + + ConversationRepository + + KVStore + + ChatRepository + + ProviderRepository + + WorkspaceIndexRepository + + AgentRepository + + SkillRepository + + ValidationRepository + + Send + + Sync, +> ConfigReaderInfra for ForgeServices +{ + fn get_config(&self) -> forge_config::ForgeConfig { + self.infra.get_config() + } +} diff --git a/crates/forge_services/src/tool_services/fs_read.rs b/crates/forge_services/src/tool_services/fs_read.rs index 74719cb7fa..0c760dc9b4 100644 --- a/crates/forge_services/src/tool_services/fs_read.rs +++ b/crates/forge_services/src/tool_services/fs_read.rs @@ -3,8 +3,8 @@ use std::sync::Arc; use anyhow::Context; use forge_app::{ - Content, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, FsReadService, - ReadOutput, compute_hash, + Content, ConfigReaderInfra, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, + FsReadService, ReadOutput, compute_hash, }; use forge_domain::{FileInfo, Image}; @@ -102,7 +102,7 @@ impl ForgeFsRead { } #[async_trait::async_trait] -impl FsReadService for ForgeFsRead { +impl FsReadService for ForgeFsRead { async fn read( &self, path: String, diff --git a/crates/forge_services/src/tool_services/image_read.rs b/crates/forge_services/src/tool_services/image_read.rs index 8442ef37dc..6a2468f8ee 100644 --- a/crates/forge_services/src/tool_services/image_read.rs +++ b/crates/forge_services/src/tool_services/image_read.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use anyhow::Context; use forge_app::domain::Image; -use forge_app::{EnvironmentInfra, FileInfoInfra, ImageReadService}; +use forge_app::{ConfigReaderInfra, EnvironmentInfra, FileInfoInfra, ImageReadService}; use strum_macros::{Display, EnumString}; use crate::utils::assert_absolute_path; @@ -44,7 +44,7 @@ impl ForgeImageRead { } } #[async_trait::async_trait] -impl ImageReadService +impl ImageReadService for ForgeImageRead { async fn read_image(&self, path: String) -> anyhow::Result { diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index 74cbe34405..2993217c5c 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!() } @@ -138,6 +131,15 @@ mod tests { } } + impl forge_app::ConfigReaderInfra for MockCommandInfra { + fn get_config(&self) -> forge_config::ForgeConfig { + forge_config::ConfigReader::default() + .read_defaults() + .build() + .unwrap() + } + } + #[tokio::test] async fn test_shell_service_forwards_env_vars() { let fixture = ForgeShell::new(Arc::new(MockCommandInfra { 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. From 87456923ab6d368c8b66f68165fd9377b26f503c Mon Sep 17 00:00:00 2001 From: Tushar Date: Sun, 5 Apr 2026 15:27:38 +0530 Subject: [PATCH 2/5] refactor(config): pass ForgeConfig through services at startup --- crates/forge_api/src/forge_api.rs | 12 +- crates/forge_app/src/app.rs | 2 +- crates/forge_app/src/changed_files.rs | 30 ++--- crates/forge_app/src/command_generator.rs | 9 -- crates/forge_app/src/git_app.rs | 18 +-- crates/forge_app/src/infra.rs | 14 --- crates/forge_app/src/services.rs | 10 +- crates/forge_config/src/reader.rs | 35 ++---- crates/forge_infra/src/env.rs | 11 +- crates/forge_infra/src/forge_infra.rs | 14 ++- crates/forge_main/src/main.rs | 4 +- crates/forge_repo/src/forge_repo.rs | 25 ++-- crates/forge_repo/src/provider/chat.rs | 15 +-- .../forge_repo/src/provider/provider_repo.rs | 48 +++---- crates/forge_services/src/agent_registry.rs | 16 ++- crates/forge_services/src/app_config.rs | 119 ++++++++---------- crates/forge_services/src/attachment.rs | 76 +++++------ crates/forge_services/src/auth.rs | 18 ++- crates/forge_services/src/context_engine.rs | 14 +-- crates/forge_services/src/discovery.rs | 9 -- crates/forge_services/src/forge_services.rs | 57 ++++----- .../src/tool_services/fs_read.rs | 37 ++++-- .../src/tool_services/image_read.rs | 20 +-- .../forge_services/src/tool_services/shell.rs | 9 -- 24 files changed, 257 insertions(+), 365 deletions(-) diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 1f033edf80..bee7384a81 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -6,7 +6,7 @@ use anyhow::Result; use forge_app::dto::ToolsOverview; use forge_app::{ AgentProviderResolver, AgentRegistry, AppConfigService, AuthService, CommandInfra, - CommandLoaderService, ConfigReaderInfra, ConversationService, DataGenerationApp, + CommandLoaderService, ConversationService, DataGenerationApp, EnvironmentInfra, FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, McpService, ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, WorkspaceService, @@ -49,9 +49,9 @@ impl ForgeAPI>, ForgeRepo> { /// * `config` - Pre-read application configuration (from startup) /// * `services_url` - Pre-validated URL for the gRPC workspace server pub fn init(cwd: PathBuf, config: ForgeConfig, services_url: Url) -> Self { - let infra = Arc::new(ForgeInfra::new(cwd, config, services_url)); - let repo = Arc::new(ForgeRepo::new(infra.clone())); - let app = Arc::new(ForgeServices::new(repo.clone())); + 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)); ForgeAPI::new(app, repo) } @@ -62,7 +62,7 @@ impl ForgeAPI>, ForgeRepo> { } #[async_trait::async_trait] -impl API +impl API for ForgeAPI { async fn discover(&self) -> Result> { @@ -155,7 +155,7 @@ impl ForgeConfig { - self.infra.get_config() + self.services.get_config() } async fn conversation( diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index 169a002f03..3634c36d02 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -132,7 +132,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); diff --git a/crates/forge_app/src/changed_files.rs b/crates/forge_app/src/changed_files.rs index bbf5309e8e..5d7585c740 100644 --- a/crates/forge_app/src/changed_files.rs +++ b/crates/forge_app/src/changed_files.rs @@ -4,7 +4,7 @@ use forge_domain::{Agent, ContextMessage, Conversation, Role, TextMessage}; use forge_template::Element; use crate::utils::format_display_path; -use crate::{ConfigReaderInfra, EnvironmentInfra, FsReadService}; +use crate::{EnvironmentInfra, FsReadService}; /// Service responsible for detecting externally changed files and rendering /// notifications @@ -20,13 +20,16 @@ impl ChangedFiles { } } -impl ChangedFiles { +impl ChangedFiles { /// Detects externally changed files and renders a notification if changes /// are found. Updates file hashes in conversation metrics to prevent /// duplicate notifications. - pub async fn update_file_stats(&self, mut conversation: Conversation) -> 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; @@ -149,15 +152,6 @@ mod tests { } } - impl ConfigReaderInfra for TestServices { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - fn fixture( files: HashMap, tracked_files: HashMap>, @@ -205,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); @@ -221,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); @@ -241,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 @@ -267,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() @@ -290,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 9af37166dd..1f9b74dc32 100644 --- a/crates/forge_app/src/command_generator.rs +++ b/crates/forge_app/src/command_generator.rs @@ -160,15 +160,6 @@ mod tests { } } - impl crate::ConfigReaderInfra for MockServices { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - #[async_trait::async_trait] impl FileDiscoveryService for MockServices { async fn collect_files(&self, _walker: Walker) -> Result> { diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index 3fcec6389c..2de4d38d57 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -6,9 +6,10 @@ use forge_domain::*; use schemars::JsonSchema; use serde::Deserialize; -use crate::{ - AgentProviderResolver, AgentRegistry, AppConfigService, ConfigReaderInfra, EnvironmentInfra, - ProviderAuthService, ProviderService, ShellService, TemplateService, +use crate::{AgentProviderResolver, Services}; +use crate::services::{ + AgentRegistry, AppConfigService, ProviderAuthService, ProviderService, ShellService, + TemplateService, }; /// Errors specific to GitApp operations @@ -92,16 +93,7 @@ impl GitApp { } } -impl GitApp -where - S: EnvironmentInfra - + ConfigReaderInfra - + ShellService - + AgentRegistry - + TemplateService - + ProviderService - + AppConfigService - + ProviderAuthService, +impl GitApp { /// Generates a commit message without committing /// diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index 2fb6ab647b..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, @@ -42,19 +41,6 @@ pub trait EnvironmentInfra: Send + Sync { ) -> impl std::future::Future> + Send; } -/// Trait for reading the current [`ForgeConfig`] from an in-memory cache. -/// -/// Separated from [`EnvironmentInfra`] so that consumers that only need to -/// read configuration values do not need access to the full environment -/// infrastructure. The cache is pre-seeded at application startup and is -/// refreshed after every [`EnvironmentInfra::update_environment`] call. -pub trait ConfigReaderInfra: Send + Sync { - /// Returns the full [`ForgeConfig`] for the current session. - /// - /// Reads from the in-memory cache; does not perform disk I/O on every call. - fn get_config(&self) -> ForgeConfig; -} - /// Repository for accessing system environment information /// This uses the EnvironmentService trait from forge_domain /// A service for reading files from the filesystem. diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 3b391896de..d48f33da13 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -16,7 +16,7 @@ use reqwest_eventsource::EventSource; use url::Url; use crate::user::{User, UserUsage}; -use crate::{ConfigReaderInfra, EnvironmentInfra, Walker}; +use crate::{EnvironmentInfra, Walker}; #[derive(Debug, Clone)] pub struct ShellOutput { @@ -548,7 +548,7 @@ pub trait ProviderAuthService: Send + Sync { ) -> anyhow::Result>; } -pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra + ConfigReaderInfra { +pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { type ProviderService: ProviderService; type AppConfigService: AppConfigService; type ConversationService: ConversationService; @@ -604,6 +604,12 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra + ConfigRea fn provider_auth_service(&self) -> &Self::ProviderAuthService; fn workspace_service(&self) -> &Self::WorkspaceService; fn skill_fetch_service(&self) -> &Self::SkillFetchService; + + /// Returns the current application configuration snapshot. + /// + /// Config is read once at startup and passed via constructor. Values are + /// not re-read from disk dynamically. + fn get_config(&self) -> forge_config::ForgeConfig; } #[async_trait::async_trait] diff --git a/crates/forge_config/src/reader.rs b/crates/forge_config/src/reader.rs index a411f494a5..ea74746de6 100644 --- a/crates/forge_config/src/reader.rs +++ b/crates/forge_config/src/reader.rs @@ -3,7 +3,6 @@ use std::sync::LazyLock; use config::ConfigBuilder; use config::builder::DefaultState; -use tracing::warn; use crate::ForgeConfig; use crate::legacy::LegacyConfig; @@ -102,36 +101,24 @@ impl ConfigReader { Ok(config.try_deserialize::()?) } - /// Adds `~/.forge/.forge.toml` as a config source. - /// - /// Silently skips if the file does not exist; returns an error if the file - /// exists but cannot be parsed (e.g., malformed TOML, wrong value types). + /// Adds `~/.forge/.forge.toml` as a config source, silently skipping if + /// absent. pub fn read_global(mut self) -> Self { let path = Self::config_path(); - // Only add the source if the file exists so that a missing config file - // is silently skipped, while a present-but-malformed file surfaces as - // an error during `build()`. - if path.exists() { - self.builder = self - .builder - .add_source(config::File::from(path).required(true)); - } + self.builder = self + .builder + .add_source(config::File::from(path).required(false)); self } - /// Reads `~/.forge/.config.json` (legacy format) and adds it as a source. - /// - /// Silently skips if the file does not exist. Emits a `warn!` log if the - /// file exists but cannot be parsed, so the user is informed without - /// aborting startup. + /// Reads `~/.forge/.config.json` (legacy format) and adds it as a source, + /// silently skipping errors. pub fn read_legacy(self) -> Self { let content = LegacyConfig::read(&Self::config_legacy_path()); - match content { - Ok(content) => self.read_toml(&content), - Err(e) => { - warn!(error = ?e, "Failed to read legacy config file; skipping"); - self - } + if let Ok(content) = content { + self.read_toml(&content) + } else { + self } } } diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index 7f884bd9ca..18b578093c 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use std::path::PathBuf; use std::sync::Arc; -use forge_app::{ConfigReaderInfra, EnvironmentInfra}; +use forge_app::EnvironmentInfra; use forge_config::{ConfigReader, ForgeConfig, ModelConfig}; use forge_domain::{ConfigOperation, Environment}; use tracing::debug; @@ -115,7 +115,7 @@ impl ForgeEnvironmentInfra { /// # Errors /// /// Returns an error if the cache is empty and the disk read fails. - fn cached_config(&self) -> anyhow::Result { + pub fn cached_config(&self) -> anyhow::Result { let mut cache = self.cache.lock().expect("cache mutex poisoned"); if let Some(ref config) = *cache { Ok(config.clone()) @@ -169,13 +169,6 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { } } -impl ConfigReaderInfra for ForgeEnvironmentInfra { - fn get_config(&self) -> ForgeConfig { - self.cached_config() - .expect("ForgeConfig cache read failed after update_environment") - } -} - #[cfg(test)] mod tests { use std::path::PathBuf; diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index 7cb1e59148..4f1aa4e928 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, + CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, McpServerInfra, StrategyFactory, UserInfra, WalkerInfra, }; @@ -101,9 +101,15 @@ impl ForgeInfra { } } -impl ConfigReaderInfra for ForgeInfra { - fn get_config(&self) -> forge_config::ForgeConfig { - self.config_infra.get_config() +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() } } diff --git a/crates/forge_main/src/main.rs b/crates/forge_main/src/main.rs index 23d3e3c1e6..ac9bcfb3cb 100644 --- a/crates/forge_main/src/main.rs +++ b/crates/forge_main/src/main.rs @@ -93,9 +93,7 @@ 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. Please check your ~/.forge/.forge.toml file.", - )?; + 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. diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index f0597c196f..3c12cfa134 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - AgentRepository, CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, + AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, KVStore, McpServerInfra, StrategyFactory, UserInfra, WalkedFile, Walker, WalkerInfra, @@ -54,13 +54,12 @@ pub struct ForgeRepo { impl< F: EnvironmentInfra - + ConfigReaderInfra + FileReaderInfra + FileWriterInfra + GrpcInfra + HttpInfra, > 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 = @@ -75,8 +74,15 @@ impl< Some(3600), )); // 1 hour TTL - let provider_repository = Arc::new(ForgeProviderRepository::new(infra.clone())); - let chat_repository = Arc::new(ForgeChatRepository::new(infra.clone())); + let provider_repository = Arc::new(ForgeProviderRepository::new( + infra.clone(), + config.providers, + )); + let chat_repository = Arc::new(ForgeChatRepository::new( + infra.clone(), + config.retry.unwrap_or_default(), + config.model_cache_ttl_secs, + )); let codebase_repo = Arc::new(ForgeContextEngineRepository::new(infra.clone())); let agent_repository = Arc::new(ForgeAgentRepository::new(infra.clone())); @@ -148,7 +154,7 @@ impl ConversationRepository for ForgeRepo { } #[async_trait::async_trait] -impl +impl ChatRepository for ForgeRepo { async fn chat( @@ -168,7 +174,6 @@ impl EnvironmentInfra for ForgeRepo { } } -impl ConfigReaderInfra for ForgeRepo { - fn get_config(&self) -> forge_config::ForgeConfig { - self.infra.get_config() - } -} - #[async_trait::async_trait] impl KVStore for ForgeRepo { async fn cache_get(&self, key: &K) -> anyhow::Result> diff --git a/crates/forge_repo/src/provider/chat.rs b/crates/forge_repo/src/provider/chat.rs index 60a79ebc9e..3bc96d7bad 100644 --- a/crates/forge_repo/src/provider/chat.rs +++ b/crates/forge_repo/src/provider/chat.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use forge_app::domain::{ ChatCompletionMessage, Context, Model, ModelId, ProviderResponse, ResultStream, }; -use forge_app::{ConfigReaderInfra, EnvironmentInfra, HttpInfra}; +use forge_app::{EnvironmentInfra, HttpInfra}; use forge_domain::{ChatRepository, Provider, ProviderId}; use forge_infra::CacacheStorage; use tokio::task::AbortHandle; @@ -24,16 +24,17 @@ pub struct ForgeChatRepository { bg_refresh: BgRefresh, } -impl ForgeChatRepository { +impl ForgeChatRepository { /// Creates a new ForgeChatRepository with the given infrastructure. /// /// # 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 +50,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 { @@ -68,7 +69,7 @@ impl ForgeChatRepository } #[async_trait::async_trait] -impl ChatRepository for ForgeChatRepository { +impl ChatRepository for ForgeChatRepository { async fn chat( &self, model_id: &ModelId, diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 74f9268864..8bb864df71 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -2,7 +2,7 @@ use std::sync::{Arc, LazyLock}; use bytes::Bytes; use forge_app::domain::{ProviderId, ProviderResponse}; -use forge_app::{ConfigReaderInfra, EnvironmentInfra, FileReaderInfra, FileWriterInfra, HttpInfra}; +use forge_app::{EnvironmentInfra, FileReaderInfra, FileWriterInfra, HttpInfra}; use forge_domain::{ AnyProvider, ApiKey, AuthCredential, AuthDetails, Error, MigrationResult, Provider, ProviderRepository, ProviderType, URLParam, URLParamSpec, URLParamValue, @@ -204,15 +204,16 @@ 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 } } } -impl +impl ForgeProviderRepository { async fn get_custom_provider_configs(&self) -> anyhow::Result> { @@ -227,10 +228,9 @@ impl Vec { - self.infra - .get_config() - .providers - .into_iter() + self.config_providers + .iter() + .cloned() .map(Into::into) .collect() } @@ -520,7 +520,7 @@ impl +impl ProviderRepository for ForgeProviderRepository { async fn get_all_providers(&self) -> anyhow::Result> { @@ -817,15 +817,6 @@ mod env_tests { } } - impl forge_app::ConfigReaderInfra for MockInfra { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - #[async_trait::async_trait] impl FileReaderInfra for MockInfra { async fn read_utf8(&self, path: &std::path::Path) -> anyhow::Result { @@ -982,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(); @@ -1051,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(); @@ -1098,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(); @@ -1162,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(); @@ -1219,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(); @@ -1318,15 +1309,6 @@ mod env_tests { } } - impl forge_app::ConfigReaderInfra for CustomMockInfra { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - #[async_trait::async_trait] impl FileReaderInfra for CustomMockInfra { async fn read_utf8(&self, path: &std::path::Path) -> anyhow::Result { @@ -1456,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_services/src/agent_registry.rs b/crates/forge_services/src/agent_registry.rs index 1b20973468..f4f445847c 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -2,7 +2,8 @@ use std::sync::Arc; use dashmap::DashMap; use forge_app::domain::{AgentId, Error, ModelId, ProviderId}; -use forge_app::{AgentRepository, ConfigReaderInfra, EnvironmentInfra}; +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,16 +28,17 @@ 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), } } } -impl ForgeAgentRegistryService { +impl ForgeAgentRegistryService { /// Lazily initializes and returns the agents map /// Loads agents from repository on first call, subsequent calls return /// cached value @@ -69,8 +74,7 @@ impl ForgeAgentRegist /// 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() @@ -96,7 +100,7 @@ impl ForgeAgentRegist } #[async_trait::async_trait] -impl forge_app::AgentRegistry +impl forge_app::AgentRegistry for ForgeAgentRegistryService { async fn get_active_agent_id(&self) -> anyhow::Result> { diff --git a/crates/forge_services/src/app_config.rs b/crates/forge_services/src/app_config.rs index fe6cebcf05..1151a38300 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, ConfigReaderInfra, EnvironmentInfra}; +use forge_app::{AppConfigService, EnvironmentInfra}; +use forge_config::ForgeConfig; use forge_domain::{ CommitConfig, ConfigOperation, Effort, ModelId, ProviderId, ProviderRepository, SuggestConfig, }; @@ -9,16 +10,17 @@ 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)) } } } -impl ForgeAppConfigService { +impl ForgeAppConfigService { /// Helper method to apply a config operation atomically. async fn update(&self, op: ConfigOperation) -> anyhow::Result<()> { debug!(op = ?op, "Updating app config"); @@ -27,11 +29,11 @@ impl ForgeAppConfi } #[async_trait::async_trait] -impl AppConfigService +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,18 @@ impl } 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,21 +87,26 @@ impl } 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 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), })) @@ -105,13 +116,12 @@ impl &self, commit_config: forge_domain::CommitConfig, ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetCommitConfig(commit_config)) - .await + self.update(ConfigOperation::SetCommitConfig(commit_config)).await } 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 { @@ -125,13 +135,12 @@ impl &self, suggest_config: forge_domain::SuggestConfig, ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetSuggestConfig(suggest_config)) - .await + self.update(ConfigOperation::SetSuggestConfig(suggest_config)).await } 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 { + 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, @@ -143,8 +152,7 @@ impl } async fn set_reasoning_effort(&self, effort: Effort) -> anyhow::Result<()> { - self.update(ConfigOperation::SetReasoningEffort(effort)) - .await + self.update(ConfigOperation::SetReasoningEffort(effort)).await } } @@ -157,7 +165,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; @@ -309,12 +317,6 @@ mod tests { } } - impl forge_app::ConfigReaderInfra for MockInfra { - fn get_config(&self) -> ForgeConfig { - self.config.lock().unwrap().clone() - } - } - #[async_trait::async_trait] impl ChatRepository for MockInfra { async fn chat( @@ -397,7 +399,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; @@ -408,7 +410,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?; @@ -424,7 +426,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?; @@ -440,17 +442,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(()) @@ -459,7 +456,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; @@ -470,7 +467,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?; @@ -489,7 +486,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?; @@ -497,9 +494,8 @@ 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(()) @@ -508,7 +504,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?; @@ -524,16 +520,11 @@ 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 20bb9ac163..7b70e152b0 100644 --- a/crates/forge_services/src/attachment.rs +++ b/crates/forge_services/src/attachment.rs @@ -5,7 +5,7 @@ use forge_app::domain::{ }; use forge_app::utils::format_display_path; use forge_app::{ - AttachmentService, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, + AttachmentService, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, FileReaderInfra, }; @@ -14,13 +14,14 @@ use crate::range::resolve_range; #[derive(Clone)] pub struct ForgeChatRequest { infra: Arc, + max_read_lines: u64, } -impl +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> { @@ -83,11 +84,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 @@ -115,7 +114,7 @@ impl +impl AttachmentService for ForgeChatRequest { async fn attachments(&self, url: &str) -> anyhow::Result> { @@ -167,15 +166,6 @@ pub mod tests { } } - impl forge_app::ConfigReaderInfra for MockEnvironmentInfra { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - impl MockFileService { pub fn new() -> Self { let mut files = HashMap::new(); @@ -509,12 +499,6 @@ pub mod tests { } } - impl forge_app::ConfigReaderInfra for MockCompositeService { - fn get_config(&self) -> forge_config::ForgeConfig { - self.env_service.get_config() - } - } - #[async_trait::async_trait] impl FileInfoInfra for MockCompositeService { async fn is_binary(&self, path: &Path) -> anyhow::Result { @@ -558,7 +542,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(); @@ -580,7 +564,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(); @@ -607,7 +591,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(); @@ -640,7 +624,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(); @@ -674,7 +658,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(); @@ -691,7 +675,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(); @@ -714,7 +698,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(); @@ -742,7 +726,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 @@ -784,7 +768,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]"; @@ -815,7 +799,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]"; @@ -846,7 +830,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]"; @@ -869,7 +853,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]"; @@ -892,7 +876,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]"; @@ -916,7 +900,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]"; @@ -948,7 +932,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]"; @@ -979,7 +963,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 @@ -1040,7 +1024,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]"; @@ -1081,7 +1065,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]"; @@ -1119,7 +1103,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]"; @@ -1182,7 +1166,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(); @@ -1234,7 +1218,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(); @@ -1266,7 +1250,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(); @@ -1303,7 +1287,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 97bd7a103c..0b503339c8 100644 --- a/crates/forge_services/src/auth.rs +++ b/crates/forge_services/src/auth.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use forge_app::{AuthService, ConfigReaderInfra, EnvironmentInfra, HttpInfra, User, UserUsage}; +use forge_app::{AuthService, EnvironmentInfra, HttpInfra, User, UserUsage}; use reqwest::Url; use reqwest::header::{AUTHORIZATION, HeaderMap, HeaderValue}; @@ -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 } +impl ForgeAuthService { + 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, @@ -58,7 +56,7 @@ impl ForgeAuthService { } #[async_trait::async_trait] -impl AuthService for ForgeAuthService { +impl AuthService for ForgeAuthService { async fn user_info(&self, api_key: &str) -> anyhow::Result { self.user_info(api_key).await } diff --git a/crates/forge_services/src/context_engine.rs b/crates/forge_services/src/context_engine.rs index 99bb88e3fd..83fbb8d5df 100644 --- a/crates/forge_services/src/context_engine.rs +++ b/crates/forge_services/src/context_engine.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use anyhow::{Context, Result}; use async_trait::async_trait; use forge_app::{ - CommandInfra, ConfigReaderInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, + CommandInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, WorkspaceService, }; use forge_domain::{ @@ -26,6 +26,7 @@ use crate::sync::{WorkspaceSyncEngine, canonicalize_path}; pub struct ForgeWorkspaceService { infra: Arc, discovery: Arc, + max_file_read_batch_size: usize, } impl Clone for ForgeWorkspaceService { @@ -33,6 +34,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, } } } @@ -40,8 +42,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 } } } @@ -51,7 +53,6 @@ impl< + WorkspaceIndexRepository + FileReaderInfra + EnvironmentInfra - + ConfigReaderInfra + CommandInfra + WalkerInfra, D: FileDiscovery + 'static, @@ -68,7 +69,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 @@ -224,7 +225,6 @@ impl< + WorkspaceIndexRepository + FileReaderInfra + EnvironmentInfra - + ConfigReaderInfra + CommandInfra + WalkerInfra + 'static, @@ -364,7 +364,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 b3a6a1b830..96b3bb7227 100644 --- a/crates/forge_services/src/discovery.rs +++ b/crates/forge_services/src/discovery.rs @@ -110,15 +110,6 @@ mod tests { } } - impl forge_app::ConfigReaderInfra for MockInfra { - fn get_config(&self) -> forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - #[async_trait::async_trait] impl WalkerInfra for MockInfra { async fn walk(&self, _config: Walker) -> Result> { diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 1d61e27716..2c95793708 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use forge_app::{ - AgentRepository, CommandInfra, ConfigReaderInfra, DirectoryReaderInfra, EnvironmentInfra, + AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, HttpInfra, KVStore, McpServerInfra, Services, StrategyFactory, UserInfra, WalkerInfra, }; @@ -43,7 +43,6 @@ type AuthService = ForgeAuthService; pub struct ForgeServices< F: HttpInfra + EnvironmentInfra - + ConfigReaderInfra + McpServerInfra + WalkerInfra + SnapshotRepository @@ -83,13 +82,13 @@ pub struct ForgeServices< provider_auth_service: ForgeProviderAuthService, workspace_service: Arc>>, skill_service: Arc>, + config: forge_config::ForgeConfig, infra: Arc, } impl< F: McpServerInfra + EnvironmentInfra - + ConfigReaderInfra + FileWriterInfra + FileInfoInfra + FileReaderInfra @@ -109,20 +108,26 @@ 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 +137,7 @@ impl< let followup_service = Arc::new(ForgeFollowup::new(infra.clone())); let custom_instructions_service = Arc::new(ForgeCustomInstructionsService::new(infra.clone())); - let agent_registry_service = Arc::new(ForgeAgentRegistryService::new(infra.clone())); + 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 +145,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())); @@ -171,6 +177,7 @@ impl< workspace_service, skill_service, chat_service, + config, infra, } } @@ -186,7 +193,6 @@ impl< + FileInfoInfra + FileDirectoryInfra + EnvironmentInfra - + ConfigReaderInfra + DirectoryReaderInfra + HttpInfra + WalkerInfra @@ -340,11 +346,14 @@ impl< fn provider_service(&self) -> &Self::ProviderService { &self.chat_service } + + fn get_config(&self) -> forge_config::ForgeConfig { + self.config.clone() + } } impl< F: EnvironmentInfra - + ConfigReaderInfra + HttpInfra + McpServerInfra + WalkerInfra @@ -382,27 +391,3 @@ impl< self.infra.get_env_vars() } } - -impl< - F: ConfigReaderInfra - + EnvironmentInfra - + HttpInfra - + McpServerInfra - + WalkerInfra - + SnapshotRepository - + ConversationRepository - + KVStore - + ChatRepository - + ProviderRepository - + WorkspaceIndexRepository - + AgentRepository - + SkillRepository - + ValidationRepository - + Send - + Sync, -> ConfigReaderInfra for ForgeServices -{ - fn get_config(&self) -> forge_config::ForgeConfig { - self.infra.get_config() - } -} diff --git a/crates/forge_services/src/tool_services/fs_read.rs b/crates/forge_services/src/tool_services/fs_read.rs index 0c760dc9b4..cd9ebc85fc 100644 --- a/crates/forge_services/src/tool_services/fs_read.rs +++ b/crates/forge_services/src/tool_services/fs_read.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use anyhow::Context; use forge_app::{ - Content, ConfigReaderInfra, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, + Content, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, FsReadService, ReadOutput, compute_hash, }; use forge_domain::{FileInfo, Image}; @@ -93,16 +93,28 @@ 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 } } } #[async_trait::async_trait] -impl FsReadService for ForgeFsRead { +impl FsReadService for ForgeFsRead { async fn read( &self, path: String, @@ -111,15 +123,14 @@ impl 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 +142,7 @@ impl>() .join("\n") } else if total_lines == 0 { @@ -188,7 +199,7 @@ impl>() .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 6a2468f8ee..63f80209a6 100644 --- a/crates/forge_services/src/tool_services/image_read.rs +++ b/crates/forge_services/src/tool_services/image_read.rs @@ -3,12 +3,15 @@ use std::sync::Arc; use anyhow::Context; use forge_app::domain::Image; -use forge_app::{ConfigReaderInfra, EnvironmentInfra, FileInfoInfra, ImageReadService}; +use forge_app::{EnvironmentInfra, FileInfoInfra, ImageReadService}; 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,25 +42,24 @@ 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] -impl ImageReadService +impl ImageReadService for ForgeImageRead { 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 forge_config::ForgeConfig { - forge_config::ConfigReader::default() - .read_defaults() - .build() - .unwrap() - } - } - #[tokio::test] async fn test_shell_service_forwards_env_vars() { let fixture = ForgeShell::new(Arc::new(MockCommandInfra { From 0eb1a98eab0aba9aef763951ef7926e06df974e8 Mon Sep 17 00:00:00 2001 From: Tushar Date: Sun, 5 Apr 2026 15:42:55 +0530 Subject: [PATCH 3/5] refactor(config): cache ForgeConfig and pass through stack --- crates/forge_api/src/api.rs | 4 --- crates/forge_api/src/forge_api.rs | 17 +++++------ crates/forge_app/src/agent.rs | 4 ++- crates/forge_app/src/agent_executor.rs | 9 ++++-- crates/forge_app/src/app.rs | 13 +++++---- crates/forge_app/src/git_app.rs | 11 +++---- .../forge_app/src/hooks/title_generation.rs | 1 + crates/forge_app/src/orch.rs | 5 +++- crates/forge_app/src/orch_spec/orch_runner.rs | 3 +- crates/forge_app/src/services.rs | 6 ---- crates/forge_app/src/tool_executor.rs | 16 +++++----- crates/forge_app/src/tool_registry.rs | 14 +++++---- crates/forge_main/src/ui.rs | 29 ++++++++++--------- crates/forge_services/src/forge_services.rs | 6 ---- 14 files changed, 67 insertions(+), 71 deletions(-) diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index b13863b0b2..f9b4f14a93 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 bee7384a81..e6f750c703 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -25,11 +25,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 @@ -37,7 +38,7 @@ impl ForgeAPI { where A: Services, { - ForgeApp::new(self.services.clone()) + ForgeApp::new(self.services.clone(), self.config.clone()) } } @@ -51,8 +52,8 @@ impl ForgeAPI>, ForgeRepo> { 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)); - ForgeAPI::new(app, repo) + let app = Arc::new(ForgeServices::new(repo.clone(), config.clone())); + ForgeAPI::new(app, repo, config) } pub async fn get_skills_internal(&self) -> Result> { @@ -98,7 +99,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?; @@ -154,10 +155,6 @@ impl ForgeConfig { - self.services.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..05bf03fb85 100644 --- a/crates/forge_app/src/agent_executor.rs +++ b/crates/forge_app/src/agent_executor.rs @@ -10,18 +10,21 @@ use forge_template::Element; use futures::StreamExt; use tokio::sync::RwLock; +use forge_config::ForgeConfig; + use crate::error::Error; 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 +66,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 3634c36d02..ac7a5cfae3 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -44,12 +44,13 @@ 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 +70,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?; @@ -157,7 +158,7 @@ impl ForgeApp { let retry_config = forge_config.retry.clone().unwrap_or_default(); - let orch = Orchestrator::new(services.clone(), retry_config, conversation, agent) + 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) @@ -219,7 +220,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/git_app.rs b/crates/forge_app/src/git_app.rs index 2de4d38d57..fa7341f6dc 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -22,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 @@ -66,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 @@ -213,7 +214,7 @@ impl GitApp 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()), @@ -224,7 +225,7 @@ impl GitApp /// Fetches git context (branch name and recent commits) async fn fetch_git_context(&self, cwd: &Path) -> Result<(String, String)> { - let max_commit_count = self.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/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..d98139b66e 100644 --- a/crates/forge_app/src/orch_spec/orch_runner.rs +++ b/crates/forge_app/src/orch_spec/orch_runner.rs @@ -120,7 +120,7 @@ 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) + 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( @@ -171,6 +171,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/services.rs b/crates/forge_app/src/services.rs index d48f33da13..9cf7a12c89 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -604,12 +604,6 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { fn provider_auth_service(&self) -> &Self::ProviderAuthService; fn workspace_service(&self) -> &Self::WorkspaceService; fn skill_fetch_service(&self) -> &Self::SkillFetchService; - - /// Returns the current application configuration snapshot. - /// - /// Config is read once at startup and passed via constructor. Values are - /// not re-read from disk dynamically. - fn get_config(&self) -> forge_config::ForgeConfig; } #[async_trait::async_trait] diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index 35539721a7..47671125c0 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 { 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_main/src/ui.rs b/crates/forge_main/src/ui.rs index ff8e3c2ca4..651a1c5485 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -108,6 +108,7 @@ 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, } @@ -164,7 +165,8 @@ impl A + Send + Sync> UI // Handle creating a new conversation async fn on_new(&mut self) -> Result<()> { - let config = self.api.get_config(); + 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?; @@ -220,20 +222,20 @@ impl A + Send + Sync> UI /// `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(config)); + 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())?, }) } @@ -1562,8 +1564,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(()) } @@ -1758,7 +1760,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)?; @@ -1792,7 +1794,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() { @@ -2985,7 +2987,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 @@ -3144,7 +3146,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 { @@ -3168,7 +3170,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(); } }; @@ -3249,8 +3251,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) @@ -3291,7 +3292,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_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 2c95793708..2982511fd1 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -82,7 +82,6 @@ pub struct ForgeServices< provider_auth_service: ForgeProviderAuthService, workspace_service: Arc>>, skill_service: Arc>, - config: forge_config::ForgeConfig, infra: Arc, } @@ -177,7 +176,6 @@ impl< workspace_service, skill_service, chat_service, - config, infra, } } @@ -346,10 +344,6 @@ impl< fn provider_service(&self) -> &Self::ProviderService { &self.chat_service } - - fn get_config(&self) -> forge_config::ForgeConfig { - self.config.clone() - } } impl< From 585dab072e63d9c2da9707b619ebb271ec98de13 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 10:16:15 +0000 Subject: [PATCH 4/5] [autofix.ci] apply automated fixes --- crates/forge_api/src/forge_api.rs | 7 ++- crates/forge_app/src/agent_executor.rs | 3 +- crates/forge_app/src/app.rs | 22 ++++++--- crates/forge_app/src/git_app.rs | 5 +- crates/forge_app/src/orch_spec/orch_runner.rs | 20 +++++--- crates/forge_app/src/tool_executor.rs | 2 +- crates/forge_infra/src/forge_infra.rs | 15 +++--- crates/forge_main/src/main.rs | 11 ++--- crates/forge_main/src/ui.rs | 15 ++++-- crates/forge_repo/src/forge_repo.rs | 25 +++------- crates/forge_repo/src/provider/chat.rs | 9 +++- .../forge_repo/src/provider/provider_repo.rs | 4 +- crates/forge_services/src/agent_registry.rs | 6 ++- crates/forge_services/src/app_config.rs | 48 ++++++++++++------- crates/forge_services/src/attachment.rs | 7 ++- crates/forge_services/src/context_engine.rs | 5 +- crates/forge_services/src/forge_services.rs | 24 +++++++--- .../src/tool_services/fs_read.rs | 12 +++-- 18 files changed, 142 insertions(+), 98 deletions(-) diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index e6f750c703..55395b7ec5 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -6,10 +6,9 @@ use anyhow::Result; use forge_app::dto::ToolsOverview; use forge_app::{ AgentProviderResolver, AgentRegistry, AppConfigService, AuthService, CommandInfra, - CommandLoaderService, ConversationService, DataGenerationApp, - EnvironmentInfra, FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, - McpService, ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, - WorkspaceService, + CommandLoaderService, ConversationService, DataGenerationApp, EnvironmentInfra, + FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, McpService, + ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, WorkspaceService, }; use forge_config::ForgeConfig; use forge_domain::{Agent, ConsoleWriter, *}; diff --git a/crates/forge_app/src/agent_executor.rs b/crates/forge_app/src/agent_executor.rs index 05bf03fb85..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, @@ -10,8 +11,6 @@ use forge_template::Element; use futures::StreamExt; use tokio::sync::RwLock; -use forge_config::ForgeConfig; - use crate::error::Error; use crate::{AgentRegistry, ConversationService, Services}; diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index ac7a5cfae3..5e762f6e81 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -50,7 +50,11 @@ pub struct ForgeApp { impl ForgeApp { /// Creates a new ForgeApp instance with the provided services and config. pub fn new(services: Arc, config: ForgeConfig) -> Self { - Self { tool_registry: ToolRegistry::new(services.clone(), config.clone()), services, config } + Self { + tool_registry: ToolRegistry::new(services.clone(), config.clone()), + services, + config, + } } /// Executes a chat request and returns a stream of responses. @@ -158,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, forge_config) - .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( diff --git a/crates/forge_app/src/git_app.rs b/crates/forge_app/src/git_app.rs index fa7341f6dc..bf50f44a80 100644 --- a/crates/forge_app/src/git_app.rs +++ b/crates/forge_app/src/git_app.rs @@ -6,11 +6,11 @@ use forge_domain::*; use schemars::JsonSchema; use serde::Deserialize; -use crate::{AgentProviderResolver, Services}; use crate::services::{ AgentRegistry, AppConfigService, ProviderAuthService, ProviderService, ShellService, TemplateService, }; +use crate::{AgentProviderResolver, Services}; /// Errors specific to GitApp operations #[derive(thiserror::Error, Debug)] @@ -94,8 +94,7 @@ impl GitApp { } } -impl GitApp -{ +impl GitApp { /// Generates a commit message without committing /// /// # Arguments diff --git a/crates/forge_app/src/orch_spec/orch_runner.rs b/crates/forge_app/src/orch_spec/orch_runner.rs index d98139b66e..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, setup.config.clone()) - .error_tracker(ToolErrorTracker::new(3)) - .tool_definitions(system_tools) - .hook(Arc::new( - Hook::default().on_request(DoomLoopDetector::default()), - )) - .sender(tx); + let orch = Orchestrator::new( + services.clone(), + 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); diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index 47671125c0..cb3e4d8444 100644 --- a/crates/forge_app/src/tool_executor.rs +++ b/crates/forge_app/src/tool_executor.rs @@ -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_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index 4f1aa4e928..e5188c99b1 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -5,9 +5,9 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, - FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, - McpServerInfra, StrategyFactory, UserInfra, WalkerInfra, + CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, FileInfoInfra, + FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, McpServerInfra, + StrategyFactory, UserInfra, WalkerInfra, }; use forge_domain::{ AuthMethod, CommandOutput, FileInfo as FileInfoData, McpServerConfig, ProviderId, URLParamSpec, @@ -55,11 +55,14 @@ pub struct ForgeInfra { } impl ForgeInfra { - /// Creates a new [`ForgeInfra`] with all infrastructure services initialized. + /// 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 + /// * `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()); diff --git a/crates/forge_main/src/main.rs b/crates/forge_main/src/main.rs index ac9bcfb3cb..624dc696dc 100644 --- a/crates/forge_main/src/main.rs +++ b/crates/forge_main/src/main.rs @@ -93,7 +93,8 @@ 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")?; + 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. @@ -117,11 +118,9 @@ async fn main() -> Result<()> { (_, _) => std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), }; - let mut ui = UI::init( - cli, - config, - move |config| ForgeAPI::init(cwd.clone(), config, services_url.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 651a1c5485..a64822ef27 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -216,10 +216,11 @@ impl A + Send + Sync> UI /// /// # 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 + /// * `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(config.clone())); @@ -230,7 +231,11 @@ impl A + Send + Sync> UI state: Default::default(), api, new_api: Arc::new(f), - console: Console::new(env.clone(), config.custom_history_path.clone(), command.clone()), + console: Console::new( + env.clone(), + config.custom_history_path.clone(), + command.clone(), + ), cli, command, spinner, diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index 3c12cfa134..478aa26979 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -4,10 +4,9 @@ use std::sync::Arc; use bytes::Bytes; use forge_app::{ - AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, - FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, - GrpcInfra, HttpInfra, KVStore, McpServerInfra, StrategyFactory, UserInfra, WalkedFile, Walker, - WalkerInfra, + AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, + FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, GrpcInfra, HttpInfra, + KVStore, McpServerInfra, StrategyFactory, UserInfra, WalkedFile, Walker, WalkerInfra, }; use forge_domain::{ AnyProvider, AuthCredential, ChatCompletionMessage, ChatRepository, CommandOutput, Context, @@ -52,13 +51,7 @@ pub struct ForgeRepo { fuzzy_search_repository: Arc>, } -impl< - F: EnvironmentInfra - + FileReaderInfra - + FileWriterInfra - + GrpcInfra - + HttpInfra, -> ForgeRepo { +impl ForgeRepo { 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())); @@ -172,14 +165,8 @@ impl ProviderRepository for ForgeRepo +impl + ProviderRepository for ForgeRepo { async fn get_all_providers(&self) -> anyhow::Result> { self.provider_repository.get_all_providers().await diff --git a/crates/forge_repo/src/provider/chat.rs b/crates/forge_repo/src/provider/chat.rs index 3bc96d7bad..fccc2505c3 100644 --- a/crates/forge_repo/src/provider/chat.rs +++ b/crates/forge_repo/src/provider/chat.rs @@ -31,8 +31,13 @@ impl ForgeChatRepository { /// /// * `infra` - Infrastructure providing environment and HTTP capabilities /// * `retry_config` - Retry configuration extracted from startup config - /// * `model_cache_ttl_secs` - Model cache TTL in seconds from startup config - pub fn new(infra: Arc, retry_config: forge_config::RetryConfig, model_cache_ttl_secs: u64) -> Self { + /// * `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 retry_config = Arc::new(retry_config); diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 8bb864df71..243aac260b 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -520,8 +520,8 @@ impl } #[async_trait::async_trait] -impl - ProviderRepository for ForgeProviderRepository +impl ProviderRepository + for ForgeProviderRepository { async fn get_all_providers(&self) -> anyhow::Result> { Ok(self.get_providers().await) diff --git a/crates/forge_services/src/agent_registry.rs b/crates/forge_services/src/agent_registry.rs index f4f445847c..dc718e3682 100644 --- a/crates/forge_services/src/agent_registry.rs +++ b/crates/forge_services/src/agent_registry.rs @@ -74,7 +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 session = self.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 1151a38300..98f60bc02f 100644 --- a/crates/forge_services/src/app_config.rs +++ b/crates/forge_services/src/app_config.rs @@ -43,7 +43,8 @@ impl AppConfigService } async fn set_default_provider(&self, provider_id: ProviderId) -> anyhow::Result<()> { - self.update(ConfigOperation::SetProvider(provider_id.clone())).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()); @@ -97,7 +98,11 @@ impl AppConfigService .ok_or(forge_domain::Error::NoDefaultProvider)? }; - self.update(ConfigOperation::SetModel(provider_id.clone(), model.clone())).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()); @@ -116,7 +121,8 @@ impl AppConfigService &self, commit_config: forge_domain::CommitConfig, ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetCommitConfig(commit_config)).await + self.update(ConfigOperation::SetCommitConfig(commit_config)) + .await } async fn get_suggest_config(&self) -> anyhow::Result> { @@ -135,24 +141,30 @@ impl AppConfigService &self, suggest_config: forge_domain::SuggestConfig, ) -> anyhow::Result<()> { - self.update(ConfigOperation::SetSuggestConfig(suggest_config)).await + self.update(ConfigOperation::SetSuggestConfig(suggest_config)) + .await } async fn get_reasoning_effort(&self) -> anyhow::Result> { 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, - })) + 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<()> { - self.update(ConfigOperation::SetReasoningEffort(effort)).await + self.update(ConfigOperation::SetReasoningEffort(effort)) + .await } } @@ -494,7 +506,9 @@ mod tests { .set_default_model("gpt-4".to_string().into()) .await?; - let actual = service.get_provider_model(Some(&ProviderId::OPENAI)).await?; + let actual = service + .get_provider_model(Some(&ProviderId::OPENAI)) + .await?; let expected = "gpt-4".to_string().into(); assert_eq!(actual, expected); @@ -521,7 +535,9 @@ mod tests { // ForgeConfig only tracks a single active session, so the last // provider/model pair wins let actual_provider = service.get_default_provider().await?; - let actual_model = service.get_provider_model(Some(&ProviderId::ANTHROPIC)).await?; + let actual_model = service + .get_provider_model(Some(&ProviderId::ANTHROPIC)) + .await?; assert_eq!(actual_provider, ProviderId::ANTHROPIC); assert_eq!(actual_model, ModelId::new("claude-3")); diff --git a/crates/forge_services/src/attachment.rs b/crates/forge_services/src/attachment.rs index 7b70e152b0..ef6e515430 100644 --- a/crates/forge_services/src/attachment.rs +++ b/crates/forge_services/src/attachment.rs @@ -5,8 +5,7 @@ use forge_app::domain::{ }; use forge_app::utils::format_display_path; use forge_app::{ - AttachmentService, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, - FileReaderInfra, + AttachmentService, DirectoryReaderInfra, EnvironmentInfra, FileInfoInfra, FileReaderInfra, }; use crate::range::resolve_range; @@ -114,8 +113,8 @@ impl - AttachmentService for ForgeChatRequest +impl AttachmentService + for ForgeChatRequest { async fn attachments(&self, url: &str) -> anyhow::Result> { self.prepare_attachments(Attachment::parse_all(url)).await diff --git a/crates/forge_services/src/context_engine.rs b/crates/forge_services/src/context_engine.rs index 83fbb8d5df..5020f3c4cb 100644 --- a/crates/forge_services/src/context_engine.rs +++ b/crates/forge_services/src/context_engine.rs @@ -4,10 +4,7 @@ use std::sync::Arc; use anyhow::{Context, Result}; use async_trait::async_trait; -use forge_app::{ - CommandInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, - WorkspaceService, -}; +use forge_app::{CommandInfra, EnvironmentInfra, FileReaderInfra, WalkerInfra, WorkspaceService}; use forge_domain::{ AuthCredential, AuthDetails, ProviderId, ProviderRepository, SyncProgress, UserId, WorkspaceId, WorkspaceIndexRepository, diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 2982511fd1..9a9afb477a 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -1,9 +1,9 @@ use std::sync::Arc; use forge_app::{ - AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, - FileDirectoryInfra, FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, - HttpInfra, KVStore, McpServerInfra, Services, StrategyFactory, UserInfra, WalkerInfra, + AgentRepository, CommandInfra, DirectoryReaderInfra, EnvironmentInfra, FileDirectoryInfra, + FileInfoInfra, FileReaderInfra, FileRemoverInfra, FileWriterInfra, HttpInfra, KVStore, + McpServerInfra, Services, StrategyFactory, UserInfra, WalkerInfra, }; use forge_domain::{ ChatRepository, ConversationRepository, FuzzySearchRepository, ProviderRepository, @@ -111,10 +111,14 @@ impl< let mcp_manager = Arc::new(ForgeMcpManager::new(infra.clone())); let mcp_service = Arc::new(ForgeMcpService::new(mcp_manager.clone(), infra.clone())); let template_service = Arc::new(ForgeTemplateService::new(infra.clone())); - let attachment_service = Arc::new(ForgeChatRequest::new(infra.clone(), config.max_read_lines)); + let attachment_service = + Arc::new(ForgeChatRequest::new(infra.clone(), 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(), config.services_url.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(), config.clone())); let file_create_service = Arc::new(ForgeFsWrite::new(infra.clone())); @@ -126,7 +130,10 @@ impl< config.max_read_lines, config.max_line_chars, )); - let image_read_service = Arc::new(ForgeImageRead::new(infra.clone(), config.max_image_size_bytes)); + 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())); @@ -136,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(), config.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()); diff --git a/crates/forge_services/src/tool_services/fs_read.rs b/crates/forge_services/src/tool_services/fs_read.rs index cd9ebc85fc..4a133e3e03 100644 --- a/crates/forge_services/src/tool_services/fs_read.rs +++ b/crates/forge_services/src/tool_services/fs_read.rs @@ -3,8 +3,8 @@ use std::sync::Arc; use anyhow::Context; use forge_app::{ - Content, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, - FsReadService, ReadOutput, compute_hash, + Content, EnvironmentInfra, FileInfoInfra, FileReaderInfra as InfraFsReadService, FsReadService, + ReadOutput, compute_hash, }; use forge_domain::{FileInfo, Image}; @@ -109,7 +109,13 @@ impl ForgeFsRead { 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 } + Self { + infra, + max_file_size_bytes, + max_image_size_bytes, + max_read_lines, + max_line_chars, + } } } From 53221d12e9d1496f15ab4a4df423b7a80b9ba842 Mon Sep 17 00:00:00 2001 From: Tushar Date: Mon, 6 Apr 2026 17:42:18 +0530 Subject: [PATCH 5/5] fix(forge_main): handle main errors via run wrapper and exit 1 --- crates/forge_main/src/main.rs | 12 +++++++++++- shell-plugin/forge.theme.zsh | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/forge_main/src/main.rs b/crates/forge_main/src/main.rs index 624dc696dc..b5d4748100 100644 --- a/crates/forge_main/src/main.rs +++ b/crates/forge_main/src/main.rs @@ -46,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 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)