Conversation
tusharmath
commented
Mar 24, 2026
tusharmath
commented
- feat(forge_config): add forge configuration crate and loader
- refactor(forge_config): switch config serde to snake_case
- refactor(forge_config): load global config with oncecell panic-on-fail
- feat(forge_config): load user config from home directory
- refactor(forge_config): rename retry and parallel read config fields
- refactor(forge_config): add provider and model selection fields
- feat(forge_config): add FORGE env var overrides for config
- refactor(forge_config): switch config loading from json to toml
- chore(forge_config): add default toml config for tool limits
- refactor(forge_config): rename forge_config module to config
- refactor(forge_config): switch to async config reader/writer
- feat(forge_config): add merge strategies to config structs
- feat(forge_config): implement multi-source config loading order
- refactor(forge_config): remove merge derive and use toml_edit serialization
- refactor(forge_config): load config from resolved path
- feat(forge_config): add temperature, sampling, update, compact types
- feat(forge_config): add tool limits and compact/update config
- refactor(forge_config): make config path optional in reader
- refactor(forge_repo): decouple disk appconfig from domain type
- refactor(domain): rename AppConfig to ForgeConfig everywhere
tusharmath
force-pushed
the
config-refactor-mar-24
branch
from
41a0729 to
5ce8133
Compare
tusharmath
force-pushed
the
config-refactor-mar-24
branch
from
5ce8133 to
f12fe62
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| impl From<f32> for Temperature { | ||
| fn from(value: f32) -> Self { | ||
| Temperature::new_unchecked(value) | ||
| } |
There was a problem hiding this comment.
The From<f32> implementation for Temperature bypasses validation by calling new_unchecked(). This allows creating invalid Temperature values (outside the 0.0-2.0 range) in production builds where debug assertions are disabled.
Impact: Code like Temperature::from(5.0) will create an invalid state that violates the type's invariants, potentially causing incorrect behavior downstream.
Fix: Remove this implementation or change to TryFrom:
impl TryFrom<f32> for Temperature {
type Error = String;
fn try_from(value: f32) -> Result<Self, Self::Error> {
Temperature::new(value)
}
}| impl From<f32> for Temperature { | |
| fn from(value: f32) -> Self { | |
| Temperature::new_unchecked(value) | |
| } | |
| impl TryFrom<f32> for Temperature { | |
| type Error = String; | |
| fn try_from(value: f32) -> Result<Self, Self::Error> { | |
| Temperature::new(value) | |
| } | |
| } |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| let home_dir = dirs::home_dir().ok_or_else(|| { | ||
| std::io::Error::new(std::io::ErrorKind::NotFound, "home directory not found") | ||
| })?; | ||
| Ok(home_dir.join("forge").join(".forge.toml")) |
There was a problem hiding this comment.
Critical Path Mismatch Bug
The config path uses home_dir.join("forge") which creates ~/forge/.forge.toml, but the test fixtures in app_config.rs expect ~/.forge/.forge.toml (using .forge directory).
This will cause production to read/write config to the wrong location, breaking all config persistence.
// Fix: Change line 202 to:
Ok(home_dir.join(".forge").join(".forge.toml"))| Ok(home_dir.join("forge").join(".forge.toml")) | |
| Ok(home_dir.join(".forge").join(".forge.toml")) |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
1 participant
