From d081bbb7e1a7e808f2b4bb9682e67fbc46217da9 Mon Sep 17 00:00:00 2001 From: Vlad Gaevsky Date: Fri, 4 Sep 2026 15:28:40 +0000 Subject: [PATCH] settings_json: Locate a key by its own range, not by scanning quotes (#62949) `replace_value_in_json_text` spelled a newly inserted key with hand-written quotes, so a key carrying a `"` or a `\` produced invalid JSON. The removal and rename paths then could not find such a key again: both walked backwards to the nearest raw `"`, which lands inside an escape, and ate the neighbouring entry while leaving JSON that still parses. The parser already hands back the key's byte range, so the three sites use it, and serde for the spelling, the way key lookup and the nested-construction path always did. Worth knowing beyond the escaping: `update_value_in_json_text` pushes every map key into the key path verbatim, so any settings map whose keys are not authored by hand reaches this. Most are typed by the user, but `favorite_config_option_values` is keyed by a `SessionConfigId` that an ACP agent supplies over the protocol. Tests cover insertion into a single-line and a multi-line object, removal and rename, all with a key carrying a quote and a backslash. Release Notes: - Fixed a settings key containing a quote or a backslash corrupting `settings.json`. --- crates/settings_json/src/settings_json.rs | 67 +++++++++++++++++------ 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/crates/settings_json/src/settings_json.rs b/crates/settings_json/src/settings_json.rs index f3458d5333..0c36b56ec1 100644 --- a/crates/settings_json/src/settings_json.rs +++ b/crates/settings_json/src/settings_json.rs @@ -99,6 +99,7 @@ pub fn replace_value_in_json_text>( let mut depth = 0; let mut last_value_range = 0..0; let mut first_key_start = None; + let mut matched_key_start = None; let mut existing_value_range = 0..text.len(); let mut matches = cursor.matches(&PAIR_QUERY, syntax_tree.root_node(), text.as_bytes()); @@ -135,6 +136,7 @@ pub fn replace_value_in_json_text>( .unwrap_or(false); if found_key { + matched_key_start = Some(key_range.start); existing_value_range = value_range; // Reset last value range when increasing in depth last_value_range = existing_value_range.start..existing_value_range.start; @@ -166,12 +168,8 @@ pub fn replace_value_in_json_text>( let new_val = to_pretty_json(new_value, tab_size, tab_size * depth); if let Some(replace_key) = replace_key.and_then(|str| serde_json::to_string(str).ok()) { let new_key = format!("{}: ", replace_key); - if let Some(key_start) = text[..existing_value_range.start].rfind('"') { - if let Some(prev_key_start) = text[..key_start].rfind('"') { - existing_value_range.start = prev_key_start; - } else { - existing_value_range.start = key_start; - } + if let Some(key_start) = matched_key_start { + existing_value_range.start = key_start; } (existing_value_range, new_key + &new_val) } else { @@ -181,14 +179,8 @@ pub fn replace_value_in_json_text>( let mut removal_start = first_key_start.unwrap_or(existing_value_range.start); let mut removal_end = existing_value_range.end; - // Find the actual key position by looking for the key in the pair - // We need to extend the range to include the key, not just the value - if let Some(key_start) = text[..existing_value_range.start].rfind('"') { - if let Some(prev_key_start) = text[..key_start].rfind('"') { - removal_start = prev_key_start; - } else { - removal_start = key_start; - } + if let Some(key_start) = matched_key_start { + removal_start = key_start; } let mut removed_comma = false; @@ -228,7 +220,7 @@ pub fn replace_value_in_json_text>( } else { if let Some(first_key_start) = first_key_start { // We have key paths, construct the sub objects - let new_key = key_path[depth].as_ref(); + let new_key = serde_json::to_string(key_path[depth].as_ref()).unwrap(); // We don't have the key, construct the nested objects let new_value = construct_json_value(&key_path[(depth + 1)..], new_value); @@ -250,11 +242,11 @@ pub fn replace_value_in_json_text>( // depth is 0 based, but division needs to be 1 based. let new_val = to_pretty_json(&new_value, column / (depth + 1), column); let space = ' '; - let content = format!("\"{new_key}\": {new_val},\n{space:width$}", width = column); + let content = format!("{new_key}: {new_val},\n{space:width$}", width = column); (first_key_start..first_key_start, content) } else { let new_val = serde_json::to_string(&new_value).unwrap(); - let mut content = format!(r#""{new_key}": {new_val},"#); + let mut content = format!("{new_key}: {new_val},"); content.push(' '); (first_key_start..first_key_start, content) } @@ -2588,6 +2580,47 @@ mod tests { ) } + #[test] + fn object_replace_escapes_new_key() { + // An object that already has a key: an empty one takes the nested-construction path. + let single_line = r#"{"theme": "One Dark"}"#; + let multi_line = "{\n \"theme\": \"One Dark\"\n}"; + let key = r#"/home/me/say "hi" C:\x"#; + + for input in [single_line, multi_line] { + let mut text = input.to_string(); + let (range, replacement) = + replace_value_in_json_text(&text, &[key], 4, Some(&json!("One Light")), None); + text.replace_range(range, &replacement); + + let parsed: Value = serde_json::from_str(&text) + .expect("a folder name carrying a quote must not break settings.json"); + pretty_assertions::assert_eq!(parsed, json!({ "theme": "One Dark", key: "One Light" })); + } + } + + #[test] + fn object_remove_and_rename_find_an_escaped_key_by_its_own_range() { + let key = "say \"hi\""; + let input = format!( + "{{{}: \"V\", \"theme\": \"One Dark\"}}", + serde_json::to_string(key).unwrap() + ); + + let mut removed = input.clone(); + let (range, replacement) = replace_value_in_json_text(&removed, &[key], 4, None, None); + removed.replace_range(range, &replacement); + let parsed: Value = serde_json::from_str(&removed).expect("removal must leave valid JSON"); + pretty_assertions::assert_eq!(parsed, json!({ "theme": "One Dark" })); + + let mut renamed = input; + let (range, replacement) = + replace_value_in_json_text(&renamed, &[key], 4, Some(&json!("V2")), Some("plain")); + renamed.replace_range(range, &replacement); + let parsed: Value = serde_json::from_str(&renamed).expect("rename must leave valid JSON"); + pretty_assertions::assert_eq!(parsed, json!({ "plain": "V2", "theme": "One Dark" })); + } + #[test] fn test_infer_json_indent_size() { let json_2_spaces = r#"{