Skip to content

config read once#2850

Open
tusharmath wants to merge 5 commits intomainfrom
config-read-once
Open

config read once#2850
tusharmath wants to merge 5 commits intomainfrom
config-read-once

Conversation

@tusharmath
Copy link
Copy Markdown
Collaborator

Comment on lines +51 to +57
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.

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.

// 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.",
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.

Keep context, but don't provide file path. It can change we don't want to hardcode it here. Just say .forge.toml

Comment on lines +172 to +177
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.

// Handle creating a new conversation
async fn on_new(&mut self) -> Result<()> {
self.api = Arc::new((self.new_api)());
let config = forge_config::ForgeConfig::read().unwrap_or_default();
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.

Defeats the purpose of the PR by silently falling back to defaults on config read errors.

The line reads:

let config = forge_config::ForgeConfig::read().unwrap_or_default();

This silently swallows configuration errors using unwrap_or_default(), which is exactly the behavior this PR is supposed to fix. If the config file is corrupt when the user runs /new, it will silently use default values (all zeros) instead of surfacing the error to the user.

Fix:

let config = forge_config::ForgeConfig::read()
    .context("Failed to read configuration for new conversation")?;

This propagates the error so the user sees a clear message instead of silently broken behavior with default config values.

Suggested change
let config = forge_config::ForgeConfig::read().unwrap_or_default();
let config = forge_config::ForgeConfig::read().context("Failed to read configuration for new conversation")?;

Spotted by Graphite

Fix in Graphite


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant