From 7599be8e6b39d1f2faf23cc77c40116f94a894d1 Mon Sep 17 00:00:00 2001 From: Dom Porada Date: Sat, 29 Aug 2026 22:33:35 +0000 Subject: [PATCH] Stop prompting to migrate settings that contain no deprecations (#62818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Objective Zed can report that a settings file uses deprecated settings even when it contains no deprecations. A context server configured with only a `url` is enough to trigger the prompt, although the backup Zed offers to write is identical to the current file. Fixes #24967. ## Solution Two text-edit migrations cancel each other on `context_servers` entries: one adds a `source` key and a later one removes it. The round trip reindents the entry, and the migrator mistakes the leftover whitespace for a migration. Remove `m_2025_06_16` rather than comparing around that whitespace. It wrote `source` and `settings` into every entry, but `ContextServerSettingsContent` contains neither field today. `settings` is worse than inert: an empty entry such as `{ "settings": {} }` matches neither the `Stdio` nor `Http` variant of the untagged enum. Update `m_2025_06_27`, the only remaining consumer of the generated `"source": "custom"`, to detect legacy entries from the shape of `command` itself. This continues to flatten settings written before `source` existed. It also deliberately flattens an object `command` beside `"source": "extension"`, which the old migration skipped. The command shape is invalid regardless of the declared source, and a test pins the new behavior. `m_2025_11_25` remains unchanged because it still removes `source` from settings that genuinely carry it. This deletes one migration and edits another despite the warning at the top of `migrator.rs`. That warning protects later migrations that depend on earlier output, but this output has no remaining consumer. I left the policy unchanged rather than adding an exception in the same PR that relies on it, but I can document the exception if preferred. ## Testing - `cargo test -p migrator` passes, 86 tests. The suite loses the three tests that only exercised the deleted migration and gains `test_flatten_context_server_command_alongside_source`, covering a legacy entry that carries both `source` and an object `command`. - Restoring the `source` requirement on `m_2025_06_27` with the deletion in place turns `test_flatten_context_server_command` red. Its input is a pre-`source` entry, which is what makes the relaxation load-bearing rather than incidental. - To check by hand: add `"context_servers": { "grep": { "url": "https://mcp.grep.app" } }` to user settings and confirm no migration prompt appears. Add `"show_inline_completions": true` alongside it and confirm the prompt returns and leaves the context server block untouched. - I have not run those steps in a built editor. The tests call `migrate_settings`, which is all `should_migrate_settings` checks before raising the prompt, so that call is the whole untested gap. It’s all platform-independent text processing, so I did not test Linux or Windows. ## Self-Review Checklist: - [x] I’ve reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed’s UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - Fixed the migration prompt appearing for settings that contain no deprecations. Context servers configured with only a `url` no longer trigger it. --------- Co-authored-by: MrSubidubi --- Cargo.lock | 1 + crates/migrator/Cargo.toml | 1 + crates/migrator/src/migrations.rs | 6 - .../src/migrations/m_2025_06_16/settings.rs | 92 ----- .../src/migrations/m_2025_06_27/settings.rs | 6 - crates/migrator/src/migrator.rs | 354 +++++------------- crates/settings_content/src/project.rs | 1 + 7 files changed, 107 insertions(+), 354 deletions(-) delete mode 100644 crates/migrator/src/migrations/m_2025_06_16/settings.rs diff --git a/Cargo.lock b/Cargo.lock index 053c989970..38d73288a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10924,6 +10924,7 @@ dependencies = [ "anyhow", "collections", "convert_case 0.11.0", + "indoc", "log", "pretty_assertions", "serde_json", diff --git a/crates/migrator/Cargo.toml b/crates/migrator/Cargo.toml index 1ba6f704d0..a531873d3d 100644 --- a/crates/migrator/Cargo.toml +++ b/crates/migrator/Cargo.toml @@ -26,5 +26,6 @@ settings_content.workspace = true settings_json = { workspace = true, features = ["editing"] } [dev-dependencies] +indoc.workspace = true pretty_assertions.workspace = true unindent.workspace = true diff --git a/crates/migrator/src/migrations.rs b/crates/migrator/src/migrations.rs index 97240d5592..5592449c87 100644 --- a/crates/migrator/src/migrations.rs +++ b/crates/migrator/src/migrations.rs @@ -197,12 +197,6 @@ pub(crate) mod m_2025_05_08 { pub(crate) use settings::SETTINGS_PATTERNS; } -pub(crate) mod m_2025_06_16 { - mod settings; - - pub(crate) use settings::SETTINGS_PATTERNS; -} - pub(crate) mod m_2025_06_25 { mod settings; diff --git a/crates/migrator/src/migrations/m_2025_06_16/settings.rs b/crates/migrator/src/migrations/m_2025_06_16/settings.rs deleted file mode 100644 index 1fe490287e..0000000000 --- a/crates/migrator/src/migrations/m_2025_06_16/settings.rs +++ /dev/null @@ -1,92 +0,0 @@ -use std::ops::Range; - -use tree_sitter::{Query, QueryMatch}; - -use crate::MigrationPatterns; - -pub const SETTINGS_PATTERNS: MigrationPatterns = &[( - SETTINGS_CONTEXT_SERVER_PATTERN, - migrate_context_server_settings, -)]; - -const SETTINGS_CONTEXT_SERVER_PATTERN: &str = r#"(document - (object - (pair - key: (string (string_content) @context-servers) - value: (object - (pair - key: (string (string_content) @server-name) - value: (object) @server-settings - ) - ) - ) - ) - (#eq? @context-servers "context_servers") -)"#; - -fn migrate_context_server_settings( - contents: &str, - mat: &QueryMatch, - query: &Query, -) -> Option<(Range, String)> { - let server_settings_index = query.capture_index_for_name("server-settings")?; - let server_settings = mat.nodes_for_capture_index(server_settings_index).next()?; - - let mut has_command = false; - let mut has_settings = false; - let mut has_url = false; - let mut other_keys = 0; - let mut column = None; - - // Parse the server settings to check what keys it contains - let mut cursor = server_settings.walk(); - for child in server_settings.children(&mut cursor) { - if child.kind() == "pair" - && let Some(key_node) = child.child_by_field_name("key") - { - if let (None, Some(quote_content)) = (column, key_node.child(0)) { - column = Some(quote_content.start_position().column); - } - if let Some(string_content) = key_node.child(1) { - let key = &contents[string_content.byte_range()]; - match key { - // If it already has a source key, don't modify it - "source" => return None, - "command" => has_command = true, - "settings" => has_settings = true, - "url" => has_url = true, - _ => other_keys += 1, - } - } - } - } - - let source_type = if has_command { "custom" } else { "extension" }; - - // Insert the source key at the beginning of the object - let start = server_settings.start_byte() + 1; - let indent = " ".repeat(column.unwrap_or(12)); - - if !has_command && !has_settings && !has_url { - return Some(( - start..start, - format!( - r#" -{indent}"source": "{}", -{indent}"settings": {{}}{} - "#, - source_type, - if other_keys > 0 { "," } else { "" } - ), - )); - } - - Some(( - start..start, - format!( - r#" -{indent}"source": "{}","#, - source_type - ), - )) -} diff --git a/crates/migrator/src/migrations/m_2025_06_27/settings.rs b/crates/migrator/src/migrations/m_2025_06_27/settings.rs index e3e951b1a6..d675e7b2e9 100644 --- a/crates/migrator/src/migrations/m_2025_06_27/settings.rs +++ b/crates/migrator/src/migrations/m_2025_06_27/settings.rs @@ -16,10 +16,6 @@ const SETTINGS_CONTEXT_SERVER_PATTERN: &str = r#"(document (pair key: (string (string_content) @server-name) value: (object - (pair - key: (string (string_content) @source-key) - value: (string (string_content) @source-value) - ) (pair key: (string (string_content) @command-key) value: (object) @command-object @@ -30,8 +26,6 @@ const SETTINGS_CONTEXT_SERVER_PATTERN: &str = r#"(document ) ) (#eq? @context-servers "context_servers") - (#eq? @source-key "source") - (#eq? @source-value "custom") (#eq? @command-key "command") )"#; diff --git a/crates/migrator/src/migrator.rs b/crates/migrator/src/migrator.rs index a5cf349e9e..acf5289e0c 100644 --- a/crates/migrator/src/migrator.rs +++ b/crates/migrator/src/migrator.rs @@ -194,10 +194,6 @@ pub fn migrate_settings(text: &str) -> Result> { migrations::m_2025_05_08::SETTINGS_PATTERNS, &SETTINGS_QUERY_2025_05_08, ), - MigrationType::TreeSitter( - migrations::m_2025_06_16::SETTINGS_PATTERNS, - &SETTINGS_QUERY_2025_06_16, - ), MigrationType::TreeSitter( migrations::m_2025_06_25::SETTINGS_PATTERNS, &SETTINGS_QUERY_2025_06_25, @@ -353,10 +349,6 @@ define_query!( SETTINGS_QUERY_2025_05_08, migrations::m_2025_05_08::SETTINGS_PATTERNS ); -define_query!( - SETTINGS_QUERY_2025_06_16, - migrations::m_2025_06_16::SETTINGS_PATTERNS -); define_query!( SETTINGS_QUERY_2025_06_25, migrations::m_2025_06_25::SETTINGS_PATTERNS @@ -418,6 +410,7 @@ static EDIT_PREDICTION_SETTINGS_MIGRATION_QUERY: LazyLock = LazyLock::new #[cfg(test)] mod tests { use super::*; + use indoc::indoc; use unindent::Unindent as _; #[track_caller] @@ -997,207 +990,6 @@ mod tests { ); } - #[test] - fn test_mcp_settings_migration() { - assert_migrate_with_migrations( - &[MigrationType::TreeSitter( - migrations::m_2025_06_16::SETTINGS_PATTERNS, - &SETTINGS_QUERY_2025_06_16, - )], - r#"{ - "context_servers": { - "empty_server": {}, - "extension_server": { - "settings": { - "foo": "bar" - } - }, - "custom_server": { - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - } - }, - "invalid_server": { - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "settings": { - "foo": "bar" - } - }, - "empty_server2": {}, - "extension_server2": { - "foo": "bar", - "settings": { - "foo": "bar" - }, - "bar": "foo" - }, - "custom_server2": { - "foo": "bar", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "bar": "foo" - }, - "invalid_server2": { - "foo": "bar", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "bar": "foo", - "settings": { - "foo": "bar" - } - } - } -}"#, - Some( - r#"{ - "context_servers": { - "empty_server": { - "source": "extension", - "settings": {} - }, - "extension_server": { - "source": "extension", - "settings": { - "foo": "bar" - } - }, - "custom_server": { - "source": "custom", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - } - }, - "invalid_server": { - "source": "custom", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "settings": { - "foo": "bar" - } - }, - "empty_server2": { - "source": "extension", - "settings": {} - }, - "extension_server2": { - "source": "extension", - "foo": "bar", - "settings": { - "foo": "bar" - }, - "bar": "foo" - }, - "custom_server2": { - "source": "custom", - "foo": "bar", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "bar": "foo" - }, - "invalid_server2": { - "source": "custom", - "foo": "bar", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "bar": "foo", - "settings": { - "foo": "bar" - } - } - } -}"#, - ), - ); - } - - #[test] - fn test_mcp_settings_migration_doesnt_change_valid_settings() { - let settings = r#"{ - "context_servers": { - "empty_server": { - "source": "extension", - "settings": {} - }, - "extension_server": { - "source": "extension", - "settings": { - "foo": "bar" - } - }, - "custom_server": { - "source": "custom", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - } - }, - "invalid_server": { - "source": "custom", - "command": { - "path": "foo", - "args": ["bar"], - "env": { - "FOO": "BAR" - } - }, - "settings": { - "foo": "bar" - } - } - } -}"#; - assert_migrate_with_migrations( - &[MigrationType::TreeSitter( - migrations::m_2025_06_16::SETTINGS_PATTERNS, - &SETTINGS_QUERY_2025_06_16, - )], - settings, - None, - ); - } - #[test] fn test_custom_agent_server_settings_migration() { assert_migrate_with_migrations( @@ -1324,6 +1116,44 @@ mod tests { ); } + #[test] + fn test_flatten_context_server_command_alongside_source() { + assert_migrate_settings( + indoc! {r#" + { + "context_servers": { + "custom_server": { + "source": "custom", + "command": { + "path": "npx", + "args": ["-y", "some-mcp-server"] + } + }, + "hand_edited_server": { + "source": "extension", + "command": { + "path": "other-server" + } + } + } + } + "#}, + Some(indoc! {r#" + { + "context_servers": { + "custom_server": { + "command": "npx", + "args": ["-y", "some-mcp-server"] + }, + "hand_edited_server": { + "command": "other-server" + } + } + } + "#}), + ); + } + #[test] fn test_flatten_context_server_command() { assert_migrate_settings( @@ -5096,48 +4926,32 @@ mod tests { } #[test] - fn test_mcp_settings_migration_adds_settings_to_extension_servers() { + fn test_context_server_types_report_no_migration() { assert_migrate_settings( - r#"{ - "context_servers": { - "extension_server": {}, - "stdio_server": { - "command": "npx", - "args": ["-y", "some-server"] - }, - "http_server": { - "url": "https://example.com/mcp" - }, - "http_server_with_headers": { - "url": "https://example.com/mcp", - "headers": { - "Authorization": "Bearer token" - } - } - } -}"#, - Some( - r#"{ - "context_servers": { - "extension_server": { - "settings": {} - }, - "stdio_server": { - "command": "npx", - "args": ["-y", "some-server"] - }, - "http_server": { - "url": "https://example.com/mcp" - }, - "http_server_with_headers": { - "url": "https://example.com/mcp", - "headers": { - "Authorization": "Bearer token" - } - } - } -}"#, - ), + indoc! {r#" + { + "context_servers": { + "extension_server": {}, + "disabled_extension_server": { + "enabled": false + }, + "stdio_server": { + "command": "npx", + "args": ["-y", "some-server"] + }, + "http_server": { + "url": "https://example.com/mcp" + }, + "http_server_with_headers": { + "url": "https://example.com/mcp", + "headers": { + "Authorization": "Bearer token" + } + } + } + } + "#}, + None, ); } @@ -5483,6 +5297,46 @@ mod tests { assert_migrate_settings(&r#"{ "theme": "One Dark" }"#.unindent(), None); } + #[test] + fn test_url_only_context_servers_are_left_alone() { + assert_migrate_settings( + indoc! {r#" + { + "context_servers": { "grep": { "url": "https://mcp.grep.app" } } + } + "#}, + None, + ); + + assert_migrate_settings( + indoc! {r#" + { + "context_servers": { + "grep": { "url": "https://mcp.grep.app" }, + "local": { + "source": "custom", + "command": { + "path": "npx", + "args": ["-y", "some-mcp-server"] + } + } + } + } + "#}, + Some(indoc! {r#" + { + "context_servers": { + "grep": { "url": "https://mcp.grep.app" }, + "local": { + "command": "npx", + "args": ["-y", "some-mcp-server"] + } + } + } + "#}), + ) + } + #[test] fn test_rename_folder_icons_to_folder_indicator_in_all_panels() { assert_migrate_with_migrations( diff --git a/crates/settings_content/src/project.rs b/crates/settings_content/src/project.rs index fc5eb11c2d..0335a26499 100644 --- a/crates/settings_content/src/project.rs +++ b/crates/settings_content/src/project.rs @@ -470,6 +470,7 @@ pub enum ContextServerSettingsContent { /// /// Consult the documentation for the context server to see what settings /// are supported. + #[serde(default)] settings: serde_json::Value, }, }