Skip to content

Fix: ensure stable YAML serialization by using BTreeMap #5553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tanish111 wants to merge 2 commits into from
Closed

Fix: ensure stable YAML serialization by using BTreeMap #5553

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1f3fb2
Fix: ensure stable YAML serialization by using BTreeMap
tanish111 Nov 3, 2025
1dbb5af
Update crates/goose/src/config/base.rs
tanish111 Nov 3, 2025
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: 15 additions & 4 deletions crates/goose/src/config/base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use once_cell::sync::OnceCell;
use serde::{Deserialize, Serialize};
use serde_json::Value;
use serde_yaml::Mapping;
use std::collections::HashMap;
use std::collections::{HashMap, BTreeMap};
use std::env;
use std::fs::OpenOptions;
use std::io::Write;
Expand Down Expand Up @@ -369,8 +369,18 @@ impl Config {
// Create backup before writing new config
self.create_backup_if_needed()?;

// Convert to YAML for storage
let yaml_value = serde_yaml::to_string(&values)?;
// Convert to YAML for storage (sorted by key for stable serialization)
let ordered: BTreeMap<String, serde_yaml::Value> = values
.into_iter()
.map(|(k, v)| {
let key = match k {
serde_yaml::Value::String(s) => s,
other => serde_yaml::to_string(&other).unwrap_or_else(|_| format!("{:?}", other)),
};
Comment on lines +376 to +379
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The serde_yaml::to_string call can fail and produce unpredictable fallback keys using format!(\"{:?}\", other). This fallback produces Debug output rather than a proper string representation, which could lead to confusing keys in the config file. Consider handling non-string keys more explicitly, or document why non-string keys are expected in a Mapping that should only have string keys.

Copilot uses AI. Check for mistakes.
(key, v)
})
.collect();
let yaml_value = serde_yaml::to_string(&ordered)?;

// Ensure the directory exists
if let Some(parent) = self.config_path.parent() {
Expand Down Expand Up @@ -701,7 +711,8 @@ impl Config {
entry.set_password(&json_value)?;
}
SecretStorage::File { path } => {
let yaml_value = serde_yaml::to_string(&values)?;
let ordered: BTreeMap<String, serde_json::Value> = values.into_iter().collect();
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

[nitpick] The values variable is a HashMap<String, Value> (where Value is serde_json::Value from the function return type), so the explicit type annotation is redundant here. Unlike the first change at line 373 which handles serde_yaml::Value keys, this is already a HashMap<String, serde_json::Value>, so the transformation is straightforward. Consider simplifying to let ordered: BTreeMap<_, _> = values.into_iter().collect(); for clearer code.

Suggested change
let ordered: BTreeMap<String, serde_json::Value> = values.into_iter().collect();
let ordered: BTreeMap<_, _> = values.into_iter().collect();

Copilot uses AI. Check for mistakes.
let yaml_value = serde_yaml::to_string(&ordered)?;
std::fs::write(path, yaml_value)?;
}
};
Expand Down