Fix: ensure stable YAML serialization by using BTreeMap #5553
Conversation
Replaced HashMap with BTreeMap in config serialization to maintain deterministic key order. Prevents random reordering of YAML files on each save, allowing clean diffs and consistent config outputs. Signed-off-by: tanish111 <tanishdesai37@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces stable, sorted YAML serialization for configuration and secrets storage by using BTreeMap instead of unordered collections. This ensures deterministic output order when writing configuration files.
- Imports
BTreeMapfromstd::collectionsfor ordered map operations - Converts YAML mapping keys to strings with fallback handling for non-string keys before storing in BTreeMap
- Applies similar ordering to secret file storage for consistency
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/goose/src/config/base.rs
Outdated
| .map(|(k, v)| { | ||
| let key = match k { | ||
| serde_yaml::Value::String(s) => s, | ||
| other => serde_yaml::to_string(&other).unwrap_or_else(|_| format!("{:?}", other)), |
There was a problem hiding this comment.
The fallback to format!(\"{:?}\", other) produces debug representations that may not be valid YAML keys and could lead to data loss or corruption. Since serde_yaml::Mapping keys should typically be strings in well-formed configs, consider returning an error instead of silently using a debug representation. This would make key type issues explicit rather than hiding them.
formatting fix Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let key = match k { | ||
| serde_yaml::Value::String(s) => s, | ||
| other => serde_yaml::to_string(&other).unwrap_or_else(|_| format!("{:?}", other)), | ||
| }; |
There was a problem hiding this comment.
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.
| } | ||
| SecretStorage::File { path } => { | ||
| let yaml_value = serde_yaml::to_string(&values)?; | ||
| let ordered: BTreeMap<String, serde_json::Value> = values.into_iter().collect(); |
There was a problem hiding this comment.
[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.
| let ordered: BTreeMap<String, serde_json::Value> = values.into_iter().collect(); | |
| let ordered: BTreeMap<_, _> = values.into_iter().collect(); |
|
Closing this PR as the same change is already covered in #5468 |
1 participant
Summary
Replaced HashMap with BTreeMap in config serialization to maintain deterministic key order. Prevents random reordering of YAML files on each save, allowing clean diffs and consistent config outputs.
base.rs: Collect serde_yaml::Mapping into BTreeMap<String, serde_yaml::Value> by stringifying keys to ensure deterministic, sorted YAML output before writing to disk.
base.rs: Collect HashMap<String, serde_json::Value> into BTreeMap<String, serde_json::Value> before serde_yaml::to_string, preserving sorted order.
Type of Change
Testing
Manual Testing
Related Issues
Relates to #5127