From 8826e0fbb9a8fc916013ed4a6e4a271bdcb2b7e9 Mon Sep 17 00:00:00 2001
From: Oscar-Williams <319719771+Oscar-Williams@users.noreply.github.com>
Date: Sun, 25 Oct 2026 15:58:20 +0800
Subject: [PATCH] fix(kits): honor OpenCode config paths and MCP conflicts

---
 crates/hk-core/src/kits/install_plan.rs  | 110 ++++++----
 crates/hk-core/src/kits/tests/service.rs | 259 +++++++++++++++++++++++
 2 files changed, 322 insertions(+), 47 deletions(-)

diff --git a/crates/hk-core/src/kits/install_plan.rs b/crates/hk-core/src/kits/install_plan.rs
index 81774ef9..0bfa9831 100644
--- a/crates/hk-core/src/kits/install_plan.rs
+++ b/crates/hk-core/src/kits/install_plan.rs
@@ -1,16 +1,13 @@
 use crate::adapter::{AgentAdapter, McpFormat};
 use crate::kits::zip_io::read_manifest_from_zip;
-use crate::models::{ConfigCategory, ExtensionKind};
+use crate::models::{ConfigCategory, ConfigScope, ExtensionKind};
 use crate::HkError;
 use std::path::{Path, PathBuf};
 
 /// Returns `true` when `name` is already present as an MCP server entry in the
-/// config file at `config_path`, parsing per the agent's `McpFormat`. Returns
-/// `false` if the file does not exist, fails to parse, or the key is absent.
+/// config file at `config_path` (or OpenCode's sibling config), parsing per
+/// the agent's `McpFormat`. Missing or unparsable files do not add conflicts.
 fn mcp_entry_exists(config_path: &Path, name: &str, format: McpFormat) -> bool {
-    if !config_path.exists() {
-        return false;
-    }
     match format {
         McpFormat::McpServers | McpFormat::Servers => {
             let key = if matches!(format, McpFormat::McpServers) {
@@ -40,17 +37,29 @@ fn mcp_entry_exists(config_path: &Path, name: &str, format: McpFormat) -> bool {
                 .is_some()
         }
         McpFormat::Opencode => {
-            // OpenCode uses JSONC with `mcp` top-level key. For v1, parse as
-            // strict JSON; failure means we conservatively return false (no
-            // conflict detected). OpenCode authors typically don't comment
-            // their config.
-            let Ok(bytes) = std::fs::read(config_path) else {
-                return false;
+            let has_entry = |path: &Path| {
+                let Ok(text) = std::fs::read_to_string(path) else {
+                    return false;
+                };
+                let Ok(value) = jsonc_parser::parse_to_serde_value::<serde_json::Value>(
+                    &text,
+                    &Default::default(),
+                ) else {
+                    return false;
+                };
+                value.get("mcp").and_then(|m| m.get(name)).is_some()
             };
-            let Ok(v) = serde_json::from_slice::<serde_json::Value>(&bytes) else {
-                return false;
-            };
-            v.get("mcp").and_then(|m| m.get(name)).is_some()
+            // OpenCode merges both files. Protect raw keys, including partial
+            // entries that its adapter's server reader does not surface.
+            match (
+                config_path.parent(),
+                config_path.file_name().and_then(|n| n.to_str()),
+            ) {
+                (Some(dir), Some("opencode.json" | "opencode.jsonc")) => {
+                    has_entry(&dir.join("opencode.json")) || has_entry(&dir.join("opencode.jsonc"))
+                }
+                _ => has_entry(config_path),
+            }
         }
         McpFormat::HermesYaml => {
             let Ok(s) = std::fs::read_to_string(config_path) else {
@@ -139,51 +148,58 @@ pub fn compute_kit_install_plan(
         .ok_or_else(|| HkError::NotFound(format!("Agent '{agent_name}' not found")))?;
     let manifest = read_manifest_from_zip(zip_path)?;
     let project_root = PathBuf::from(project_path);
+    let project_scope = ConfigScope::Project {
+        name: project_root
+            .file_name()
+            .map(|n| n.to_string_lossy().into_owned())
+            .unwrap_or_default(),
+        path: project_path.into(),
+    };
     let mut items = Vec::new();
 
     for ext in manifest.extensions.iter() {
-        let target_dir = match ext.kind {
-            ExtensionKind::Skill => adapter
-                .project_skill_dirs()
-                .into_iter()
-                .find(|p| !p.contains('*') && !p.ends_with('/'))
-                .ok_or_else(|| HkError::Internal(format!(
-                    "Agent '{agent_name}' has no project-level skill dir"
-                )))?,
+        let target = match ext.kind {
+            ExtensionKind::Skill => {
+                let target_dir = adapter
+                    .project_skill_dirs()
+                    .into_iter()
+                    .find(|p| !p.contains('*') && !p.ends_with('/'))
+                    .ok_or_else(|| HkError::Internal(format!(
+                        "Agent '{agent_name}' has no project-level skill dir"
+                    )))?;
+                project_root.join(target_dir).join(&ext.name)
+            }
             ExtensionKind::Mcp => adapter
-                .project_mcp_config_relpath()
+                .mcp_config_path_for(&project_scope)
                 .ok_or_else(|| HkError::Validation(format!(
                     "Agent '{agent_name}' doesn't support project-level MCP servers. \
                      Pick a different agent, or remove MCP entries from this Kit."
                 )))?,
-            ExtensionKind::Hook => adapter
-                .project_hook_config_relpath()
-                .ok_or_else(|| HkError::Validation(format!(
-                    "Agent '{agent_name}' doesn't support project-level hooks. \
-                     Pick a different agent, or remove hook entries from this Kit."
-                )))?,
-            ExtensionKind::Cli => adapter
-                .project_skill_dirs()
-                .into_iter()
-                .next()
-                .ok_or_else(|| HkError::Internal(format!(
-                    "Agent '{agent_name}' has no install dir for CLI assets"
-                )))?,
+            ExtensionKind::Hook => {
+                let target_dir = adapter
+                    .project_hook_config_relpath()
+                    .ok_or_else(|| HkError::Validation(format!(
+                        "Agent '{agent_name}' doesn't support project-level hooks. \
+                         Pick a different agent, or remove hook entries from this Kit."
+                    )))?;
+                project_root.join(target_dir)
+            }
+            ExtensionKind::Cli => {
+                let target_dir = adapter
+                    .project_skill_dirs()
+                    .into_iter()
+                    .next()
+                    .ok_or_else(|| HkError::Internal(format!(
+                        "Agent '{agent_name}' has no install dir for CLI assets"
+                    )))?;
+                project_root.join(target_dir).join(&ext.name)
+            }
             ExtensionKind::Plugin => {
                 return Err(HkError::Internal(
                     "plugin kind is not Kit-able in v1".into(),
                 ));
             }
         };
-        // Skills/CLI are folder assets: the target dir is `<project_root>/<target_dir>/<asset_name>/`.
-        // MCP/Hook are single-file assets: the target file is `<project_root>/<target_dir>`.
-        let target = match ext.kind {
-            ExtensionKind::Skill | ExtensionKind::Cli => {
-                project_root.join(&target_dir).join(&ext.name)
-            }
-            ExtensionKind::Mcp | ExtensionKind::Hook => project_root.join(&target_dir),
-            ExtensionKind::Plugin => unreachable!(),
-        };
         // For MCP entries the right conflict question is "does this server name
         // already exist in mcpServers?" not "does the config file exist?".
         // Hook is deferred to v2 (pack time returns an error), so no conflict
diff --git a/crates/hk-core/src/kits/tests/service.rs b/crates/hk-core/src/kits/tests/service.rs
index 41b7e0cb..e2abbd01 100644
--- a/crates/hk-core/src/kits/tests/service.rs
+++ b/crates/hk-core/src/kits/tests/service.rs
@@ -1028,6 +1028,265 @@ fn preview_mcp_conflict_only_when_name_exists() {
     assert_eq!(prev_b.extension_conflicts[0].asset_name, "target-server");
 }
 
+fn assert_opencode_mcp_conflict(json: Option<&str>, jsonc: Option<&str>) {
+    let dir = tempdir().unwrap();
+    let _restore = RestoreHome::new();
+    set_test_home(dir.path());
+    let store = Mutex::new(Store::open(&dir.path().join("hk.db")).unwrap());
+    let adapters = all_adapters();
+    let project = dir.path().join("proj");
+    fs::create_dir_all(&project).unwrap();
+    for (filename, content) in [("opencode.json", json), ("opencode.jsonc", jsonc)] {
+        if let Some(content) = content {
+            fs::write(project.join(filename), content).unwrap();
+        }
+    }
+    let target = project.join(if jsonc.is_some() {
+        "opencode.jsonc"
+    } else {
+        "opencode.json"
+    });
+    let kit_id = make_mcp_kit(
+        &store,
+        dir.path(),
+        "OpenCode conflict",
+        "target-server",
+        serde_json::json!({"command": "node", "args": ["server.js"], "env": {}}),
+    );
+    let preview = preview_kit_project_conflicts(
+        &store,
+        &adapters,
+        PreviewKitConflictsRequest {
+            kit_id: kit_id.clone(),
+            project_path: project.to_string_lossy().into(),
+            agent_name: "opencode".into(),
+        },
+    )
+    .unwrap();
+    let request = SyncKitRequest {
+        kit_id,
+        project_path: project.to_string_lossy().into(),
+        agent_name: "opencode".into(),
+        force_overwrite_extension_ids: vec![],
+        force_overwrite_config_keys: vec![],
+    };
+    let result = sync_kit_to_project(&store, &adapters, request.clone()).unwrap();
+    assert_eq!(
+        result.installed_count, 0,
+        "unexpected writes: {:?}",
+        result.written_paths
+    );
+    assert_eq!(result.skipped_conflict_count, 1);
+    for (filename, content) in [("opencode.json", json), ("opencode.jsonc", jsonc)] {
+        let path = project.join(filename);
+        if let Some(content) = content {
+            assert_eq!(fs::read_to_string(path).unwrap(), content);
+        } else {
+            assert!(!path.exists(), "unforced sync must not create {filename}");
+        }
+    }
+    assert_eq!(preview.extension_conflicts.len(), 1);
+    let conflict = &preview.extension_conflicts[0];
+    assert_eq!(std::path::Path::new(&conflict.target_path), target);
+    let result = sync_kit_to_project(
+        &store,
+        &adapters,
+        SyncKitRequest {
+            force_overwrite_extension_ids: vec![conflict.extension_id.clone()],
+            ..request
+        },
+    )
+    .unwrap();
+    assert_eq!(result.installed_count, 1);
+    assert_eq!(result.skipped_conflict_count, 0);
+    let text = fs::read_to_string(&target).unwrap();
+    let value = jsonc_parser::parse_to_serde_value::<serde_json::Value>(&text, &Default::default())
+        .unwrap();
+    assert_eq!(
+        value["mcp"]["target-server"]["command"],
+        serde_json::json!(["node", "server.js"])
+    );
+    if let Some(original) = jsonc.or(json) {
+        if original.contains("// keep this comment") {
+            assert!(text.contains("// keep this comment"));
+        }
+        let before =
+            jsonc_parser::parse_to_serde_value::<serde_json::Value>(original, &Default::default())
+                .unwrap();
+        assert_eq!(value["mcp"]["other-server"], before["mcp"]["other-server"]);
+    }
+    if jsonc.is_some() {
+        if let Some(json) = json {
+            assert_eq!(
+                fs::read_to_string(project.join("opencode.json")).unwrap(),
+                json
+            );
+        } else {
+            assert!(!project.join("opencode.json").exists());
+        }
+    }
+}
+
+#[test]
+#[serial(kit_env)]
+fn opencode_mcp_json_comments_preserve_conflicts() {
+    assert_opencode_mcp_conflict(
+        Some(
+            r#"{
+        // keep this comment
+        "mcp": {
+            "target-server": {"type": "local", "command": ["old"],},
+            "other-server": {"type": "remote", "url": "https://example.com/mcp"},
+        },
+    }"#,
+        ),
+        None,
+    );
+}
+
+#[test]
+#[serial(kit_env)]
+fn opencode_mcp_jsonc_conflicts_use_existing_file() {
+    assert_opencode_mcp_conflict(
+        None,
+        Some(
+            r#"{
+        // keep this comment
+        "mcp": {
+            "target-server": {"type": "local", "command": ["old"]},
+            "other-server": {"type": "local", "command": ["keep"]},
+        },
+    }"#,
+        ),
+    );
+}
+
+#[test]
+#[serial(kit_env)]
+fn opencode_mcp_conflicts_include_both_config_files_and_partial_entries() {
+    let existing = r#"{"mcp":{"target-server":{"type":"local","command":["old"]}}}"#;
+    let other = r#"{"mcp":{"other-server":{"type":"local","command":["keep"]}}}"#;
+    for (json, jsonc) in [
+        (existing, other),
+        (other, existing),
+        (existing, existing),
+        (r#"{"mcp":{"target-server":{"enabled":false}}}"#, other),
+        (other, r#"{"mcp":{"target-server":{"enabled":false}}}"#),
+    ] {
+        assert_opencode_mcp_conflict(Some(json), Some(jsonc));
+    }
+}
+
+#[test]
+#[serial(kit_env)]
+fn opencode_mcp_new_entries_use_resolved_config_path() {
+    let json = r#"{"mcp":{"json-server":{"type":"local","command":["keep-json"]}}}"#;
+    let jsonc = r#"{
+        // keep this comment
+        "mcp": {"jsonc-server": {"type": "local", "command": ["keep-jsonc"]},},
+    }"#;
+    for (json, jsonc) in [
+        (None, None),
+        (Some(json), None),
+        (None, Some(jsonc)),
+        (Some(json), Some(jsonc)),
+    ] {
+        let dir = tempdir().unwrap();
+        let _restore = RestoreHome::new();
+        set_test_home(dir.path());
+        let store = Mutex::new(Store::open(&dir.path().join("hk.db")).unwrap());
+        let adapters = all_adapters();
+        let project = dir.path().join("proj");
+        fs::create_dir_all(&project).unwrap();
+        for (filename, content) in [("opencode.json", json), ("opencode.jsonc", jsonc)] {
+            if let Some(content) = content {
+                fs::write(project.join(filename), content).unwrap();
+            }
+        }
+        let target = project.join(if jsonc.is_some() {
+            "opencode.jsonc"
+        } else {
+            "opencode.json"
+        });
+        let kit_id = make_mcp_kit(
+            &store,
+            dir.path(),
+            "OpenCode new entry",
+            "new-server",
+            serde_json::json!({"command": "node", "args": [], "env": {}}),
+        );
+        let preview = preview_kit_project_conflicts(
+            &store,
+            &adapters,
+            PreviewKitConflictsRequest {
+                kit_id: kit_id.clone(),
+                project_path: project.to_string_lossy().into(),
+                agent_name: "opencode".into(),
+            },
+        )
+        .unwrap();
+        assert!(preview.extension_conflicts.is_empty());
+        let result = sync_kit_to_project(
+            &store,
+            &adapters,
+            SyncKitRequest {
+                kit_id: kit_id.clone(),
+                project_path: project.to_string_lossy().into(),
+                agent_name: "opencode".into(),
+                force_overwrite_extension_ids: vec![],
+                force_overwrite_config_keys: vec![],
+            },
+        )
+        .unwrap();
+        assert_eq!(result.installed_count, 1);
+        assert_eq!(result.skipped_conflict_count, 0);
+        let text = fs::read_to_string(&target).unwrap();
+        let value =
+            jsonc_parser::parse_to_serde_value::<serde_json::Value>(&text, &Default::default())
+                .unwrap();
+        assert_eq!(
+            value["mcp"]["new-server"]["command"],
+            serde_json::json!(["node"])
+        );
+        if let Some(original) = jsonc.or(json) {
+            let before = jsonc_parser::parse_to_serde_value::<serde_json::Value>(
+                original,
+                &Default::default(),
+            )
+            .unwrap();
+            for (name, entry) in before["mcp"].as_object().unwrap() {
+                assert_eq!(&value["mcp"][name], entry);
+            }
+        }
+        if jsonc.is_some() {
+            assert!(text.contains("// keep this comment"));
+            if let Some(json) = json {
+                assert_eq!(
+                    fs::read_to_string(project.join("opencode.json")).unwrap(),
+                    json
+                );
+            } else {
+                assert!(!project.join("opencode.json").exists());
+            }
+        }
+        unsync_kit_from_project(
+            &store,
+            &adapters,
+            UnsyncKitRequest {
+                kit_id,
+                project_path: project.to_string_lossy().into(),
+                agent_name: "opencode".into(),
+            },
+        )
+        .unwrap();
+        let text = fs::read_to_string(&target).unwrap();
+        let value =
+            jsonc_parser::parse_to_serde_value::<serde_json::Value>(&text, &Default::default())
+                .unwrap();
+        assert!(value["mcp"].get("new-server").is_none());
+    }
+}
+
 /// Attempting to add a Hook extension to a Kit must fail at pack time with a
 /// clear v1-unsupported error message.
 #[test]