From 4f911727d2e9ba7a6bd6eca93fa2ceb65fd4505e Mon Sep 17 00:00:00 2001 From: que3sui Date: Wed, 10 Jun 2026 21:42:54 +0800 Subject: [PATCH] fix: prevent duplicate YAML keys in Hermes config (#3267) * fix: prevent duplicate YAML keys in Hermes config Three changes in hermes_config.rs: 1. deduplicate_top_level_keys() - scan and remove duplicate top-level keys before YAML parsing, preventing "duplicate entry" parse errors 2. remove_all_sections() - helper to strip all occurrences of a given top-level key from raw YAML text 3. replace_yaml_section() now calls remove_all_sections() on the remainder after replacing the primary occurrence, preventing duplicate sections from accumulating on repeated writes Fixes the issue where mcp_servers (or any top-level key) gets duplicated in config.yaml, causing "Failed to parse Hermes config as YAML: duplicate entry with key" errors. Co-Authored-By: que3sui <204201112+que3sui@users.noreply.github.com> * fix: handle CRLF and LF line endings in top-level key deduplication is_top_level_key_line only accepted empty, space, or tab after the colon, but deduplicate_top_level_keys uses split_inclusive('\n'), so lines end with \n (LF) or \r\n (CRLF). Without accepting \r and \n as valid post-colon characters, the dedup safety net never activates. Add \r and \n checks to is_top_level_key_line, and three tests covering LF, CRLF, and first-occurrence preservation. Co-Authored-By: Claude Opus 4.7 * refactor(hermes): keep last occurrence when healing duplicate YAML keys Reworks the healing layers on top of the CRLF root-cause fix: - deduplicate_top_level_keys: keep the LAST occurrence of each duplicated key instead of the first. Duplicates come from section replacement degrading into appends (#3633), so the last block is the newest data -- and Hermes itself reads the config with PyYAML, whose duplicate-key semantics are last-wins. Keeping the first occurrence would silently roll users back to stale config and diverge from what Hermes runs with. Healthy files take a fast path and are returned untouched. - Drop the unused dup_key variable (fails cargo clippy -- -D warnings, which CI enforces). - replace_yaml_section: clean residual duplicate sections from the remainder via remove_all_sections; values come from the keep-last healed read, so dropping all stale on-disk copies loses nothing. - Add regression tests for the actual root cause (find/replace on CRLF input must replace in place, not append), keep-last semantics, identity on healthy files, end-to-end heal-then-parse, and duplicate cleanup on write. Fixes #3633 #2973 #2529 #3310 #3762 --------- Co-authored-by: que3sui <204201112+que3sui@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 Co-authored-by: Jason --- src-tauri/src/hermes_config.rs | 257 ++++++++++++++++++++++++++++++++- 1 file changed, 253 insertions(+), 4 deletions(-) diff --git a/src-tauri/src/hermes_config.rs b/src-tauri/src/hermes_config.rs index cba95617d..c9321886c 100644 --- a/src-tauri/src/hermes_config.rs +++ b/src-tauri/src/hermes_config.rs @@ -116,10 +116,76 @@ pub fn read_hermes_config() -> Result { return Ok(serde_yaml::Value::Mapping(serde_yaml::Mapping::new())); } - serde_yaml::from_str(&content) + // Heal duplicate top-level keys left behind by the pre-CRLF-fix append + // bug (#3633); serde_yaml rejects them outright, which bricked the panel. + let deduped = deduplicate_top_level_keys(&content); + + serde_yaml::from_str(&deduped) .map_err(|e| AppError::Config(format!("Failed to parse Hermes config as YAML: {e}"))) } +/// Remove duplicate top-level YAML sections, keeping the LAST occurrence of +/// each key. +/// +/// Keep-last is deliberate, not arbitrary: the duplicates come from section +/// replacement degrading into appends (#3633), so the last block is the +/// newest data — and Hermes itself reads the file with PyYAML, whose +/// duplicate-key semantics are last-wins. Keeping the first occurrence would +/// silently roll the user back to stale config and diverge from what Hermes +/// actually runs with. +fn deduplicate_top_level_keys(raw: &str) -> String { + use std::collections::HashMap; + + // Pass 1: locate every top-level key line as (key, byte offset). + let mut sections: Vec<(&str, usize)> = Vec::new(); + let mut offset = 0; + for line in raw.split('\n') { + if is_top_level_key_line(line) { + if let Some(colon_pos) = line.find(':') { + sections.push((&line[..colon_pos], offset)); + } + } + offset += line.len() + 1; + } + + let mut remaining: HashMap<&str, usize> = HashMap::new(); + for (key, _) in §ions { + *remaining.entry(key).or_insert(0) += 1; + } + if remaining.values().all(|&count| count <= 1) { + return raw.to_string(); + } + + // Pass 2: re-emit, dropping every section that has a later occurrence of + // the same key. A section spans from its key line to the next top-level + // key line (or EOF), matching find_yaml_section_range. Content before the + // first section (comments, document markers) is always kept. + let mut result = String::with_capacity(raw.len()); + let head_end = sections + .first() + .map(|&(_, start)| start) + .unwrap_or(raw.len()); + result.push_str(&raw[..head_end]); + + for (i, &(key, start)) in sections.iter().enumerate() { + let end = sections + .get(i + 1) + .map(|&(_, next_start)| next_start) + .unwrap_or(raw.len()); + let count = remaining.get_mut(key).expect("key collected in pass 1"); + *count -= 1; + if *count > 0 { + log::warn!( + "Hermes config: dropped duplicate top-level section '{key}' (keeping the last occurrence)" + ); + continue; + } + result.push_str(&raw[start..end]); + } + + result +} + // ============================================================================ // YAML Section-Level Replacement // ============================================================================ @@ -132,6 +198,11 @@ pub fn read_hermes_config() -> Result { /// - Not be a comment (starting with `#`) /// - Not be a sequence item (starting with `-`) /// - Contain `:` followed by space, tab, newline, or end-of-line +/// +/// Lines may carry a trailing `\r` (CRLF files split on `\n`) or `\n` +/// (callers using `split_inclusive`); both count as end-of-line after the +/// colon. Rejecting `\r` here used to make every section lookup miss on +/// CRLF configs, turning section replacement into endless appends (#3633). fn is_top_level_key_line(line: &str) -> bool { if line.is_empty() { return false; @@ -142,7 +213,7 @@ fn is_top_level_key_line(line: &str) -> bool { } if let Some(colon_pos) = line.find(':') { let after_colon = &line[colon_pos + 1..]; - after_colon.is_empty() || after_colon.starts_with(' ') || after_colon.starts_with('\t') + after_colon.is_empty() || after_colon.starts_with([' ', '\t', '\r', '\n']) } else { false } @@ -196,6 +267,21 @@ fn serialize_yaml_section(key: &str, value: &serde_yaml::Value) -> Result String { + let mut result = String::with_capacity(raw.len()); + let mut rest = raw; + while let Some((start, end)) = find_yaml_section_range(rest, section_key) { + result.push_str(&rest[..start]); + rest = &rest[end..]; + } + result.push_str(rest); + result +} + /// Replace a YAML section in raw text, or append it if not found. fn replace_yaml_section( raw: &str, @@ -208,12 +294,14 @@ fn replace_yaml_section( let mut result = String::with_capacity(raw.len()); result.push_str(&raw[..start]); result.push_str(&serialized); + // Drop duplicate sections of this key from the remainder — configs + // written before the CRLF fix may carry several appended copies. + let remainder = remove_all_sections(&raw[end..], section_key); // Ensure proper separation between sections - let remainder = &raw[end..]; if !serialized.ends_with('\n') && !remainder.is_empty() && !remainder.starts_with('\n') { result.push('\n'); } - result.push_str(remainder); + result.push_str(&remainder); Ok(result) } else { // Section not found — append at end @@ -1204,6 +1292,111 @@ model: assert!(!section.starts_with("model_extra:")); } + #[test] + fn find_section_handles_crlf() { + // Regression for #3633: CRLF line endings must not hide sections. + let yaml = "model:\r\n default: gpt-4\r\nagent:\r\n max_turns: 10\r\n"; + let (start, end) = find_yaml_section_range(yaml, "model").unwrap(); + let section = &yaml[start..end]; + assert!(section.starts_with("model:")); + assert!(section.contains("default: gpt-4")); + assert!(!section.contains("agent:")); + } + + // ---- deduplicate_top_level_keys tests ---- + + #[test] + fn dedup_keeps_last_occurrence() { + // Duplicates come from replace-degraded-to-append, so the last block + // is the newest data and must win (PyYAML last-wins, like Hermes). + let yaml = "\ +model: + default: gpt-4 +agent: + max_turns: 10 +model: + default: claude-opus-4-8 +"; + let result = deduplicate_top_level_keys(yaml); + assert_eq!( + result.lines().filter(|l| *l == "model:").count(), + 1, + "duplicate model: section was not removed" + ); + assert!(result.contains("claude-opus-4-8")); + assert!(!result.contains("gpt-4")); + assert!(result.contains("max_turns")); + } + + #[test] + fn dedup_handles_crlf() { + let yaml = "model:\r\n default: gpt-4\r\nagent:\r\n max_turns: 10\r\nmodel:\r\n default: claude\r\n"; + let result = deduplicate_top_level_keys(yaml); + assert_eq!(result.lines().filter(|l| l.trim() == "model:").count(), 1); + assert!(result.contains("default: claude")); + assert!(!result.contains("gpt-4")); + } + + #[test] + fn dedup_is_identity_without_duplicates() { + let yaml = "\ +# Hermes config +model: + default: gpt-4 + +agent: + max_turns: 10 +"; + assert_eq!(deduplicate_top_level_keys(yaml), yaml); + } + + #[test] + fn dedup_result_parses_with_last_value() { + // End-to-end: a config that serde_yaml rejects today must parse after + // healing, and expose the newest (last) value. + let yaml = "\ +custom_providers: + - name: old-provider +model: + default: gpt-4 +custom_providers: + - name: old-provider + - name: new-provider +"; + let healed = deduplicate_top_level_keys(yaml); + let value: serde_yaml::Value = serde_yaml::from_str(&healed).unwrap(); + let providers = value + .get("custom_providers") + .unwrap() + .as_sequence() + .unwrap(); + assert_eq!(providers.len(), 2); + assert_eq!( + providers[1].get("name").unwrap().as_str().unwrap(), + "new-provider" + ); + } + + // ---- remove_all_sections tests ---- + + #[test] + fn remove_all_sections_strips_every_occurrence() { + let yaml = "\ +model: + default: gpt-4 +agent: + max_turns: 10 +model: + default: claude +model: + default: gemini +"; + let result = remove_all_sections(yaml, "model"); + assert!(!result.contains("model:")); + assert!(result.contains("agent:")); + assert!(result.contains("max_turns")); + } + // ---- replace_yaml_section tests ---- #[test] @@ -1239,6 +1432,62 @@ agent: assert!(!result.contains("openai")); } + #[test] + fn replace_section_in_crlf_config_replaces_in_place() { + // Regression for #3633: on CRLF configs every "replace" used to + // degrade into an append, piling up duplicate sections. + let yaml = "model:\r\n default: gpt-4\r\nagent:\r\n max_turns: 10\r\n"; + let new_model = serde_yaml::Value::Mapping({ + let mut m = serde_yaml::Mapping::new(); + m.insert( + serde_yaml::Value::String("default".to_string()), + serde_yaml::Value::String("claude-opus-4-8".to_string()), + ); + m + }); + + let result = replace_yaml_section(yaml, "model", &new_model).unwrap(); + assert_eq!( + result.lines().filter(|l| l.trim() == "model:").count(), + 1, + "model: must be replaced in place, not appended" + ); + assert!(result.contains("claude-opus-4-8")); + assert!(!result.contains("gpt-4")); + assert!(result.contains("max_turns")); + } + + #[test] + fn replace_section_removes_residual_duplicates() { + // A config already broken by the append bug: replacing the section + // must also clean the stale duplicate copies after it. + let yaml = "\ +model: + default: gpt-4 +agent: + max_turns: 10 +model: + default: stale-copy +"; + let new_model = serde_yaml::Value::Mapping({ + let mut m = serde_yaml::Mapping::new(); + m.insert( + serde_yaml::Value::String("default".to_string()), + serde_yaml::Value::String("claude-opus-4-8".to_string()), + ); + m + }); + + let result = replace_yaml_section(yaml, "model", &new_model).unwrap(); + assert_eq!(result.lines().filter(|l| *l == "model:").count(), 1); + assert!(result.contains("claude-opus-4-8")); + assert!(!result.contains("stale-copy")); + assert!(result.contains("agent:")); + // The healed output must be valid YAML again + let parsed: Result = serde_yaml::from_str(&result); + assert!(parsed.is_ok()); + } + #[test] fn append_new_section() { let yaml = "\