Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,7 @@ System-level environment variables (usually set automatically):

```bash
# .env
FORGE_CONFIG=/custom/config/dir # Base directory for all Forge config files (default: ~/forge)
FORGE_MAX_SEARCH_RESULT_BYTES=10240 # Maximum bytes for search results (default: 10240 - 10 KB)
FORGE_HISTORY_FILE=/path/to/history # Custom path for Forge history file (default: uses system default location)
FORGE_BANNER="Your custom banner text" # Custom banner text to display on startup (default: Forge ASCII art)
Expand Down
1 change: 0 additions & 1 deletion crates/forge_app/src/orch_spec/orch_setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ impl Default for TestContext {
attachments: Default::default(),
env: Environment {
os: "MacOS".to_string(),
pid: 1234,
cwd: PathBuf::from("/Users/tushar"),
home: Some(PathBuf::from("/Users/tushar")),
shell: "bash".to_string(),
Expand Down
23 changes: 22 additions & 1 deletion crates/forge_config/src/reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,14 @@ impl ConfigReader {
Self::base_path().join(".forge.toml")
}

/// Returns the base directory for all Forge config files (`~/forge`).
/// Returns the base directory for all Forge config files.
///
/// If the `FORGE_CONFIG` environment variable is set, its value is used
/// directly as the base path. Otherwise defaults to `~/forge`.
pub fn base_path() -> PathBuf {
if let Ok(path) = std::env::var("FORGE_CONFIG") {
return PathBuf::from(path);
}
dirs::home_dir().unwrap_or(PathBuf::from(".")).join("forge")
}

Expand Down Expand Up @@ -164,6 +170,21 @@ mod tests {
}
}

#[test]
fn test_base_path_uses_forge_config_env_var() {
let _guard = EnvGuard::set(&[("FORGE_CONFIG", "/custom/forge/dir")]);
let actual = ConfigReader::base_path();
let expected = PathBuf::from("/custom/forge/dir");
assert_eq!(actual, expected);
}

#[test]
fn test_base_path_falls_back_to_home_dir_when_env_var_absent() {
let actual = ConfigReader::base_path();
// Without FORGE_CONFIG set the path must end with "forge"
assert_eq!(actual.file_name().unwrap(), "forge");
}

#[test]
fn test_read_parses_without_error() {
let actual = ConfigReader::default().read_defaults().build();
Expand Down
6 changes: 2 additions & 4 deletions crates/forge_domain/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ const VERSION: &str = match option_env!("APP_VERSION") {
/// Represents the minimal runtime environment in which the application is
/// running.
///
/// Contains only the six fields that cannot be sourced from [`ForgeConfig`]:
/// `os`, `pid`, `cwd`, `home`, `shell`, and `base_path`. All configuration
/// Contains only the five fields that cannot be sourced from [`ForgeConfig`]:
/// `os`, `cwd`, `home`, `shell`, and `base_path`. All configuration
/// values previously carried here are now accessed through
/// `EnvironmentInfra::get_config()`.
#[derive(Debug, Setters, Clone, PartialEq, Serialize, Deserialize, fake::Dummy)]
Expand All @@ -57,8 +57,6 @@ const VERSION: &str = match option_env!("APP_VERSION") {
pub struct Environment {
/// The operating system of the environment.
pub os: String,
/// The process ID of the current process.
pub pid: u32,
/// The current working directory.
pub cwd: PathBuf,
/// The home directory.
Expand Down
58 changes: 50 additions & 8 deletions crates/forge_infra/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,20 @@ 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()`.
/// Only the five fields that cannot be sourced from [`ForgeConfig`] are set
/// here: `os`, `cwd`, `home`, `shell`, and `base_path`. All configuration
/// values are now accessed through `EnvironmentInfra::get_config()`.
pub fn to_environment(cwd: PathBuf) -> Environment {
Environment {
os: std::env::consts::OS.to_string(),
pid: std::process::id(),
cwd,
home: dirs::home_dir(),
shell: if cfg!(target_os = "windows") {
std::env::var("COMSPEC").unwrap_or_else(|_| "cmd.exe".to_string())
} else {
std::env::var("SHELL").unwrap_or_else(|_| "/bin/sh".to_string())
},
base_path: dirs::home_dir()
.map(|h| h.join("forge"))
.unwrap_or_else(|| PathBuf::from(".").join("forge")),
base_path: ConfigReader::base_path(),
}
}

Expand Down Expand Up @@ -172,19 +168,65 @@ impl EnvironmentInfra for ForgeEnvironmentInfra {
#[cfg(test)]
mod tests {
use std::path::PathBuf;
use std::sync::{Mutex, MutexGuard};

use forge_config::ForgeConfig;
use pretty_assertions::assert_eq;

use super::*;

/// Serializes tests that mutate environment variables to prevent races.
static ENV_MUTEX: Mutex<()> = Mutex::new(());

/// Holds env vars set for a test's duration and removes them on drop,
/// while holding [`ENV_MUTEX`].
struct EnvGuard {
keys: Vec<&'static str>,
_lock: MutexGuard<'static, ()>,
}

impl EnvGuard {
#[must_use]
fn set(pairs: &[(&'static str, &str)]) -> Self {
let lock = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner());
let keys = pairs.iter().map(|(k, _)| *k).collect();
for (key, value) in pairs {
unsafe { std::env::set_var(key, value) };
}
Self { keys, _lock: lock }
}
}

impl Drop for EnvGuard {
fn drop(&mut self) {
for key in &self.keys {
unsafe { std::env::remove_var(key) };
}
}
}

#[test]
fn test_to_environment_sets_cwd() {
let fixture_cwd = PathBuf::from("/test/cwd");
let actual = to_environment(fixture_cwd.clone());
assert_eq!(actual.cwd, fixture_cwd);
}

#[test]
fn test_to_environment_uses_forge_config_env_var() {
let _guard = EnvGuard::set(&[("FORGE_CONFIG", "/custom/config/dir")]);
let actual = to_environment(PathBuf::from("/any/cwd"));
let expected = PathBuf::from("/custom/config/dir");
assert_eq!(actual.base_path, expected);
}

#[test]
fn test_to_environment_falls_back_to_home_dir_when_env_var_absent() {
let actual = to_environment(PathBuf::from("/any/cwd"));
// Without FORGE_CONFIG the base_path must end with "forge"
assert_eq!(actual.base_path.file_name().unwrap(), "forge");
}
Comment on lines +224 to +228
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.

Test assumes FORGE_CONFIG is not set but doesn't guarantee it. If FORGE_CONFIG is set in the test environment, the test will fail because to_environment() will return the env var value instead of a path ending in "forge".

Fix: Use EnvGuard to acquire the mutex and explicitly unset the variable:

#[test]
fn test_to_environment_falls_back_to_home_dir_when_env_var_absent() {
    let _guard = EnvGuard::set(&[]); // Ensures mutex is held
    unsafe { std::env::remove_var("FORGE_CONFIG") };
    let actual = to_environment(PathBuf::from("/any/cwd"));
    assert_eq!(actual.base_path.file_name().unwrap(), "forge");
}
Suggested change
fn test_to_environment_falls_back_to_home_dir_when_env_var_absent() {
let actual = to_environment(PathBuf::from("/any/cwd"));
// Without FORGE_CONFIG the base_path must end with "forge"
assert_eq!(actual.base_path.file_name().unwrap(), "forge");
}
#[test]
fn test_to_environment_falls_back_to_home_dir_when_env_var_absent() {
let _guard = EnvGuard::set(&[]); // Ensures mutex is held
unsafe { std::env::remove_var("FORGE_CONFIG") };
let actual = to_environment(PathBuf::from("/any/cwd"));
// Without FORGE_CONFIG the base_path must end with "forge"
assert_eq!(actual.base_path.file_name().unwrap(), "forge");
}

Spotted by Graphite

Fix in Graphite


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


#[test]
fn test_apply_config_op_set_provider() {
use forge_domain::ProviderId;
Expand Down
1 change: 0 additions & 1 deletion crates/forge_services/src/app_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ mod tests {
fn get_environment(&self) -> Environment {
Environment {
os: "test".to_string(),
pid: 0,
cwd: PathBuf::new(),
home: None,
shell: "bash".to_string(),
Expand Down
Loading