Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions crates/forge_api/src/forge_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, *};
Expand Down Expand Up @@ -41,8 +42,14 @@ impl<A, F> ForgeAPI<A, F> {
}

impl ForgeAPI<ForgeServices<ForgeRepo<ForgeInfra>>, ForgeRepo<ForgeInfra>> {
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)
Expand All @@ -55,7 +62,7 @@ impl ForgeAPI<ForgeServices<ForgeRepo<ForgeInfra>>, ForgeRepo<ForgeInfra>> {
}

#[async_trait::async_trait]
impl<A: Services, F: CommandInfra + EnvironmentInfra + SkillRepository + GrpcInfra> API
impl<A: Services, F: CommandInfra + ConfigReaderInfra + EnvironmentInfra + SkillRepository + GrpcInfra> API
for ForgeAPI<A, F>
{
async fn discover(&self) -> Result<Vec<File>> {
Expand Down
20 changes: 11 additions & 9 deletions crates/forge_app/src/changed_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,7 +20,7 @@ impl<S> ChangedFiles<S> {
}
}

impl<S: FsReadService + EnvironmentInfra> ChangedFiles<S> {
impl<S: FsReadService + EnvironmentInfra + ConfigReaderInfra> ChangedFiles<S> {
/// Detects externally changed files and renders a notification if changes
/// are found. Updates file hashes in conversation metrics to prevent
/// duplicate notifications.
Expand Down Expand Up @@ -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<forge_domain::ConfigOperation>,
Expand All @@ -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<String, String>,
tracked_files: HashMap<String, Option<String>>,
Expand Down
16 changes: 9 additions & 7 deletions crates/forge_app/src/command_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<forge_domain::ConfigOperation>,
Expand All @@ -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<Vec<File>> {
Expand Down
5 changes: 3 additions & 2 deletions crates/forge_app/src/git_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -95,6 +95,7 @@ impl<S> GitApp<S> {
impl<S> GitApp<S>
where
S: EnvironmentInfra
+ ConfigReaderInfra
+ ShellService
+ AgentRegistry
+ TemplateService
Expand Down
23 changes: 14 additions & 9 deletions crates/forge_app/src/infra.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>;
Expand All @@ -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
Expand All @@ -50,6 +42,19 @@ pub trait EnvironmentInfra: Send + Sync {
) -> impl std::future::Future<Output = anyhow::Result<()>> + 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;
}

Copy link
Copy Markdown
Collaborator Author

@tusharmath tusharmath Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely drop ConfigInfra. We will never read config infra dynamically ever. Make sure to pass an instance of ForgeConfig in <TypeName>::new() constructor wherever required.

/// Repository for accessing system environment information
/// This uses the EnvironmentService trait from forge_domain
/// A service for reading files from the filesystem.
Expand Down
4 changes: 2 additions & 2 deletions crates/forge_app/src/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -548,7 +548,7 @@ pub trait ProviderAuthService: Send + Sync {
) -> anyhow::Result<Provider<Url>>;
}

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;
Expand Down
35 changes: 24 additions & 11 deletions crates/forge_config/src/reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -101,24 +102,36 @@ impl ConfigReader {
Ok(config.try_deserialize::<ForgeConfig>()?)
}

/// 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
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert all changes in forge_config.

}
}
}
Expand Down
64 changes: 32 additions & 32 deletions crates/forge_infra/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,18 @@ 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.
///
/// Only the six fields that cannot be sourced from [`ForgeConfig`] are set
/// 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(),
Expand Down Expand Up @@ -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<ForgeConfig> {
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)
}
}
}
Expand All @@ -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<ConfigOperation>) -> anyhow::Result<()> {
// Load the global config (with defaults applied) for the update round-trip
let mut fc = ConfigReader::default()
Expand All @@ -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")
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: .expect() will panic if config re-read fails after update_environment

The get_config() implementation uses .expect() on a Result<ForgeConfig> that can fail. After update_environment() invalidates the cache (line 165), the next get_config() call triggers a disk re-read via cached_config(). If that re-read fails (e.g., corrupted TOML file, permissions issue), the application will panic instead of returning an error.

Fix: Change the trait signature to return Result<ForgeConfig> and propagate the error, or handle the error gracefully:

impl ConfigReaderInfra for ForgeEnvironmentInfra {
    fn get_config(&self) -> ForgeConfig {
        self.cached_config()
            .unwrap_or_else(|e| {
                tracing::error!(error = ?e, "Failed to read config after update; using defaults");
                ForgeConfig::default()
            })
    }
}

Alternatively, make the trait method fallible:

fn get_config(&self) -> anyhow::Result<ForgeConfig>
Suggested change
impl ConfigReaderInfra for ForgeEnvironmentInfra {
fn get_config(&self) -> ForgeConfig {
self.cached_config()
.expect("ForgeConfig cache read failed after update_environment")
}
}
impl ConfigReaderInfra for ForgeEnvironmentInfra {
fn get_config(&self) -> ForgeConfig {
self.cached_config()
.unwrap_or_else(|e| {
tracing::error!(error = ?e, "Failed to read config after update; using defaults");
ForgeConfig::default()
})
}
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.


#[cfg(test)]
mod tests {
use std::path::PathBuf;
Expand Down
Loading
Loading