[Detail Bug] CLI config set/unset overwrites malformed config file and deletes existing settings

[detail-app]

Detail Bug Report

https://app.detail.dev/org_89d327b3-b883-4365-b6a3-46b6701342a9/bugs/bug_aacd1928-7524-4b66-bf13-30b8048edca1

Summary

• Context: The set_config_value and unset_config_value functions in cli/src/config.rs are responsible for modifying the user's configuration file when running s2 config set and s2 config unset commands.
• Bug: These functions silently destroy all existing configuration when the config file has a parse error (e.g., malformed TOML syntax).
• Actual vs. expected: When the config file cannot be parsed, the functions use .unwrap_or_default() to ignore the error and create an empty config, then overwrite the file; expected behavior is to fail with a clear error message and preserve the existing file.
• Impact: Users lose all their configuration settings (including access tokens and custom endpoints) without any warning, requiring them to reconfigure everything from scratch.

Code with Bug

pub fn set_config_value(key: ConfigKey, value: String) -> Result<PathBuf, CliConfigError> {
    let mut config = load_config_file().unwrap_or_default(); // <-- BUG 🔴 Silently ignores parse errors and overwrites file
    config.set(key, value)?;
    save_cli_config(&config)
}

pub fn unset_config_value(key: ConfigKey) -> Result<PathBuf, CliConfigError> {
    let mut config = load_config_file().unwrap_or_default(); // <-- BUG 🔴 Silently ignores parse errors and overwrites file
    config.unset(key);
    save_cli_config(&config)
}

pub fn load_config_file() -> Result<CliConfig, CliConfigError> {
    let path = config_path()?;
    if !path.exists() {
        return Ok(CliConfig::default());
    }
    let builder = Config::builder().add_source(config::File::new(
        path.to_str().expect("config path is valid utf8"),
        FileFormat::Toml,
    ));
    Ok(builder.build()?.try_deserialize::<CliConfig>()?) // <-- BUG 🔴 Upstream parse error is discarded by unwrap_or_default() callers
}

Explanation

load_config_file() returns an error when the TOML is malformed. set_config_value/unset_config_value discard that error via unwrap_or_default(), proceed with an empty CliConfig, and then call save_cli_config, overwriting the existing (but malformed) file. This results in permanent loss of all previously stored settings, with the CLI reporting success.

Recommended Fix

Propagate parse errors instead of defaulting:

pub fn set_config_value(key: ConfigKey, value: String) -> Result<PathBuf, CliConfigError> {
    let mut config = load_config_file()?;
    config.set(key, value)?;
    save_cli_config(&config)
}

pub fn unset_config_value(key: ConfigKey) -> Result<PathBuf, CliConfigError> {
    let mut config = load_config_file()?;
    config.unset(key);
    save_cli_config(&config)
}

History

This bug was introduced in commit dc3bd4e. The commit integrated the existing s2-cli codebase into the monorepo workspace and added the lite subcommand. The buggy .unwrap_or_default() pattern was present in the original s2-cli code and was carried over during this migration without correction.