diff --git a/README.md b/README.md index 113a9168..438ae3f8 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ HarnessKit manages **all five extension types** from a unified interface — **S | **Hermes** | ✓ | ✓ | ✓ | ✓ | ✓ | | **Kiro** | ✓ | ✓ | — | ✓ | ✓ | | **Oh My Pi** | ✓ | ✓ | ✓ | — | ✓ | -| **DeepSeek Harness** | ✓ | ✓ | — | — | ✓ | +| **DeepSeek Harness** | ✓ | ✓ | ✓ | — | ✓ | * "—" indicates the agent currently does not support this extension type. Devin Desktop support keeps legacy Windsurf paths compatible. diff --git a/crates/hk-core/src/adapter/claude.rs b/crates/hk-core/src/adapter/claude.rs index e2b49e66..5866750e 100644 --- a/crates/hk-core/src/adapter/claude.rs +++ b/crates/hk-core/src/adapter/claude.rs @@ -373,6 +373,8 @@ impl AgentAdapter for ClaudeAdapter { uri: None, installed_at, updated_at, + base_layers: vec![], + pack: None, }); } entries diff --git a/crates/hk-core/src/adapter/codex.rs b/crates/hk-core/src/adapter/codex.rs index 7211f985..86f36fc3 100644 --- a/crates/hk-core/src/adapter/codex.rs +++ b/crates/hk-core/src/adapter/codex.rs @@ -435,6 +435,8 @@ impl AgentAdapter for CodexAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); break; // Take the latest version after sorting } diff --git a/crates/hk-core/src/adapter/copilot.rs b/crates/hk-core/src/adapter/copilot.rs index eebdabac..c0b6be72 100644 --- a/crates/hk-core/src/adapter/copilot.rs +++ b/crates/hk-core/src/adapter/copilot.rs @@ -265,6 +265,8 @@ impl AgentAdapter for CopilotAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } @@ -316,6 +318,8 @@ impl AgentAdapter for CopilotAdapter { uri: Some(plugin_uri.to_string()), installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } diff --git a/crates/hk-core/src/adapter/cursor.rs b/crates/hk-core/src/adapter/cursor.rs index 8e40ce20..d0bbabc5 100644 --- a/crates/hk-core/src/adapter/cursor.rs +++ b/crates/hk-core/src/adapter/cursor.rs @@ -241,6 +241,8 @@ impl AgentAdapter for CursorAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } @@ -279,6 +281,8 @@ impl AgentAdapter for CursorAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } diff --git a/crates/hk-core/src/adapter/dsh.rs b/crates/hk-core/src/adapter/dsh.rs index 76f9a96b..5366e7ae 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -82,25 +82,113 @@ impl DshAdapter { /// Existing per-profile patch files (settings listing only — MCP reading /// is home-layer-only by design; see module header). fn profile_patch_files(&self) -> Vec { - let profiles = self.dsh_home.join("profiles"); + Self::profile_dirs_in(&self.dsh_home) + .into_iter() + .map(|d| d.join("cordis.patch.yml")) + .filter(|p| p.is_file()) + .collect() + } + + /// Sorted profile directories under `/profiles/`. Skips the + /// `node_modules` entry — that is dsh's in-box-bundle symlink farm + /// (healed on every launch), not a profile. + fn profile_dirs_in(dsh_home: &Path) -> Vec { + let profiles = dsh_home.join("profiles"); let mut dirs: Vec = std::fs::read_dir(&profiles) .ok() .into_iter() .flatten() .flatten() .map(|e| e.path()) - .filter(|p| p.is_dir()) + .filter(|p| p.is_dir() && p.file_name().is_some_and(|n| n != "node_modules")) .collect(); dirs.sort(); - dirs.into_iter() - .map(|d| d.join("cordis.patch.yml")) - .filter(|p| p.is_file()) + dirs + } + + fn profile_dirs(&self) -> Vec { + Self::profile_dirs_in(&self.dsh_home) + } + + /// Patch texts of every profile layer under `dsh_home`, in sorted-dir + /// order — the layers dsh applies BEFORE the home patch. Associated fn + /// (not `&self`) so `deployer::set_dsh_plugin_enabled` can call it for + /// the exact home it is editing. + pub fn profile_patch_texts(dsh_home: &Path) -> Vec { + Self::profile_dirs_in(dsh_home) + .into_iter() + .filter_map(|d| std::fs::read_to_string(d.join("cordis.patch.yml")).ok()) .collect() } + + /// On-disk directory of an npm package visible to `profile_dir`. + /// + /// dsh keeps a maintained symlink farm at + /// `/profiles/node_modules/` (healed on every launch) + /// holding the in-box bundles AND their transitive deps — which is where + /// the packages named by BUNDLE rows live. A profile's own + /// `node_modules` holds what the user installed into that profile. The + /// likelier location for this package is tried first and the other used + /// as a fallback; `None` when it is in neither (fresh install, dsh never + /// booted) — never an error, just an unknown path. + fn package_dir(&self, profile_dir: &Path, pkg: &str) -> Option { + let farm = self.dsh_home.join("profiles/node_modules").join(pkg); + let local = profile_dir.join("node_modules").join(pkg); + let (first, second) = if IN_BOX_BUNDLES.contains(&pkg) { + (farm, local) + } else { + (local, farm) + }; + if first.is_dir() { + Some(first) + } else { + second.is_dir().then_some(second) + } + } + + /// A mounted bundle's OWN patch layer — an ordinary cordis patch file at + /// the package-relative path its `package.json` declares under + /// `dsh.bundle.patch` (verified against the installed + /// `@deepseek-ai/dsh-base` / `dsh-web-app` 0.1.0-rc.6: + /// `"patch": "./cordis.patch.yml"`). This is the file that actually + /// carries a bundle's plugin ROWS; the bundle itself is a layer, not a + /// plugin. `None` when the package is not on disk, declares no patch, or + /// the declared file is missing. + fn bundle_patch_path(&self, profile_dir: &Path, bundle: &str) -> Option { + let dir = self.package_dir(profile_dir, bundle)?; + let manifest_text = std::fs::read_to_string(dir.join("package.json")).ok()?; + let manifest: BundleManifest = serde_json::from_str(&manifest_text).ok()?; + let rel = manifest.dsh.bundle.patch?; + let path = dir.join(rel.trim_start_matches("./")); + path.is_file().then_some(path) + } } /// dsh's `@deepseek-ai/dsh-mcp-client` plugin name — the marker for MCP rows. -const MCP_CLIENT_PLUGIN: &str = "@deepseek-ai/dsh-mcp-client"; +pub(crate) const MCP_CLIENT_PLUGIN: &str = "@deepseek-ai/dsh-mcp-client"; + +/// Config key carrying the ORIGINAL, unsanitized MCP server name on rows the +/// HK install writer created. mcp-client requires `serverName` to match +/// `/^[A-Za-z0-9_-]{1,32}$/`, so a name like `microsoft/markitdown` is stored +/// as `microsoft-markitdown`; without a record of the original the scanner +/// would read the row back under a different name and HK would model it as a +/// SECOND extension (ghost duplicate row, install button never turning ✓). +/// Same round-trip contract as Codex's `_hk_name` TOML key — shared +/// producer (`deployer::build_dsh_insert_row`) / consumer +/// (`mcp_entries_in_text`) constant, like `ANON_ROW_SOURCE_SUFFIX`. +/// +/// Safe to carry inside `config`: dsh's schema library +/// (`@deepseek-ai/schemastery`) accepts and preserves unknown keys, and +/// `dsh --dump-config` composes such a row without error (verified against +/// the installed rc.6). +pub(crate) const HK_NAME_CONFIG_KEY: &str = "_hk_name"; + +/// Suffix of a plugin entry's `source` string for insert rows that carry no +/// `id:`. A shared producer/consumer contract: the adapter renders it and +/// `manager` matches on it. +/// Rendered source strings feed `scanner::stable_id_for`, so this value is +/// part of extension identity and must never change. +pub(crate) const ANON_ROW_SOURCE_SUFFIX: &str = "anonymous row"; /// One entry parsed from a patch file. `from_insert` distinguishes row /// DEFINITIONS (inside `insert:`) from id-targeted overrides — upstream, an @@ -122,6 +210,12 @@ struct CordisRow { /// is out of scope and skipped. A parse failure returns empty WITH a stderr /// diagnostic — silence here would read as "dsh has no MCP". fn parse_patch_rows(text: &str, origin: &Path) -> Vec { + // Absent/empty file is normal, not malformed — callers feed "" for a + // missing patch file. (dsh's empty-file-must-be-`[]` boot rule applies + // only to files that EXIST; "" here means the file was absent or empty.) + if text.trim().is_empty() { + return vec![]; + } let doc: serde_yaml::Value = match serde_yaml::from_str(text) { Ok(doc) => doc, Err(err) => { @@ -184,8 +278,214 @@ fn yaml_config_str(config: &serde_yaml::Value, key: &str) -> Option { config.get(key).and_then(|v| v.as_str()).map(String::from) } -/// Folded final state of one MCP row within one patch file (single ordered -/// apply, later entries win — mirrors upstream applyEntryPatches). +/// In-box bundles resolve from dsh's maintained symlink farm at +/// `/profiles/node_modules/` (healed on every dsh launch), +/// NOT from any profile's own node_modules. +const IN_BOX_BUNDLES: [&str; 3] = [ + "@deepseek-ai/dsh-base", + "@deepseek-ai/dsh-web-app", + "@deepseek-ai/dsh-headless", +]; + +/// The plugin-relevant slice of a profile `package.json`: +/// `{ dsh: { profile: { bundles: [...] } } }` +/// (upstream: packages/boot/app-boot/src/profile.ts). `dependencies` is +/// deliberately NOT modeled: a dependency no layer mounts is never loaded by +/// dsh and never shown in its UI, so HK does not list it either. +#[derive(serde::Deserialize, Default)] +struct ProfileManifest { + #[serde(default)] + dsh: ProfileDshSection, +} + +#[derive(serde::Deserialize, Default)] +struct ProfileDshSection { + #[serde(default)] + profile: ProfileSection, +} + +#[derive(serde::Deserialize, Default)] +struct ProfileSection { + #[serde(default)] + bundles: Vec, +} + +/// The patch-relevant slice of a BUNDLE package's `package.json`: +/// `{ dsh: { bundle: { patch: "./cordis.patch.yml" } } }`. Verified against +/// the installed `@deepseek-ai/dsh-base` / `@deepseek-ai/dsh-web-app` +/// 0.1.0-rc.6 — `patch` is a package-relative path string, and its presence +/// is also what makes `dsh plugin add` auto-mount a package as a bundle. +#[derive(serde::Deserialize, Default)] +struct BundleManifest { + #[serde(default)] + dsh: BundleDshSection, +} + +#[derive(serde::Deserialize, Default)] +struct BundleDshSection { + #[serde(default)] + bundle: BundleSection, +} + +#[derive(serde::Deserialize, Default)] +struct BundleSection { + #[serde(default)] + patch: Option, +} + +/// One patch text folded by dsh's apply rule: a single ordered pass in which +/// an `insert:` row DEFINES an entry and a later id-targeted row OVERRIDES it +/// (mirrors upstream applyEntryPatches — an override can never create a row). +struct FoldedText { + /// Rows DEFINED in this text, in definition order (anonymous ones last), + /// each carrying the merged effect of every later override in the SAME + /// text. Only definitions the caller's `is_def` predicate selected. + defined: Vec, + /// Last literal `disabled:` value each row id received in this text, + /// including ids this text only OVERRIDES — their definition lives in + /// another layer, and dsh applies layers in order, so such an override is + /// still live. Definitions contribute their own value (absent ≡ `false`, so + /// a definition whose `disabled` is an unevaluable `!!js` expression reads + /// as enabled — the P0 "show the base state" rule); an OVERRIDE carrying + /// `!!js` contributes nothing, since HK cannot evaluate it. + disabled_by_id: std::collections::HashMap, +} + +/// The one fold used by every dsh patch reader. `is_def` selects which +/// definitions this caller cares about — mcp-client rows for the MCP reader, +/// every other named row for the plugin reader, any insert row for the +/// per-id state lookup. Overrides are merged uniformly (`disabled` AND +/// `config`); callers that model no config simply drop it. +fn fold_rows_in_text( + text: &str, + origin: &Path, + is_def: impl Fn(&CordisRow) -> bool, +) -> FoldedText { + let mut order: Vec = Vec::new(); + let mut by_id: std::collections::HashMap = + std::collections::HashMap::new(); + let mut anon: Vec = Vec::new(); + let mut disabled_by_id: std::collections::HashMap = + std::collections::HashMap::new(); + + for row in parse_patch_rows(text, origin) { + let is_definition = is_def(&row); + let Some(id) = row.id.clone() else { + if is_definition { + anon.push(row); + } + continue; + }; + if is_definition { + disabled_by_id.insert(id.clone(), row.disabled.unwrap_or(false)); + order.push(id.clone()); + by_id.insert(id, row); + } else if !row.from_insert { + // Override: mutates an existing row (upstream: an unknown id is + // warn+skip, never a definition) — but its literal state still + // counts for `disabled_by_id`, since the definition may live in + // an earlier layer that this text never sees. + if let Some(d) = row.disabled { + disabled_by_id.insert(id.clone(), d); + } + if let Some(existing) = by_id.get_mut(&id) { + if let Some(d) = row.disabled { + existing.disabled = Some(d); + } + if !row.config.is_null() { + existing.config = row.config; + } + } + } + // else: from-insert definition of a kind this caller ignores — never + // an override; skip (even on a malformed id collision). + } + let mut defined: Vec = + order.into_iter().filter_map(|id| by_id.remove(&id)).collect(); + defined.extend(anon); + FoldedText { defined, disabled_by_id } +} + +/// Folded final state of one third-party plugin row within one patch file. +/// Excludes mcp-client rows (modeled as MCP servers) and override-only rows +/// (an override can never create a row upstream). +struct PluginRowState { + id: Option, + name: String, + disabled: bool, +} + +/// Returns the rows this text DEFINES plus the `disabled` state it +/// establishes for every id it touches (definitions and overrides alike). +/// Callers compose the second value across an ordered layer chain — later +/// layers win — which is why it is returned instead of recomputed per row: +/// a per-row lookup would re-parse each ~450-line bundle patch once for +/// every one of its ~80 rows. +fn fold_plugin_rows_in_text( + text: &str, + origin: &Path, +) -> (Vec, std::collections::HashMap) { + let folded = fold_rows_in_text(text, origin, |row| { + row.from_insert && row.name.as_deref().is_some_and(|n| n != MCP_CLIENT_PLUGIN) + }); + let rows = folded + .defined + .into_iter() + .map(|row| PluginRowState { + id: row.id, + name: row.name.expect("a plugin definition implies a name"), + disabled: row.disabled.unwrap_or(false), + }) + .collect(); + (rows, folded.disabled_by_id) +} + +/// Display name, identity-bearing `source`, and toggle `uri` of one composed +/// plugin row that the layer `where_` owns ("profile web, bundle ", +/// "profile web", "home layer"). +/// +/// **The display name is the cordis patch ROW ID, not the npm package name.** +/// dsh's own Settings → Plugins list labels a row by its id (`hmr`, `timer`, +/// `llm`, `api-gateway`, …) — verified against dsh rc.6's UI — and HK shows +/// what dsh shows. +/// +/// The package name (`CordisRow.name`, what the row instantiates) is real, +/// useful information, so it moves into the `source` string, which +/// `scanner::scan_plugins` renders verbatim as the extension's description +/// ("Plugin from ") in the detail panel. It is the only field that +/// suits it: `path` is absent for home-layer rows (they apply to whichever +/// profile is booted, so no single `/node_modules/` exists) and +/// `source_url` means "upstream URL from the agent's own manifest", which a +/// package name is not. The package slot REPLACES the old `row ` suffix, +/// which the name now carries. +/// +/// Identity: `scanner::plugin_extension_id` hashes `":"`, so +/// `(id, where_)` must be unique. It is — a row id is defined at most once per +/// layer chain (`fold_rows_in_text` keeps the first definition per file, +/// `read_plugins` the first per profile via `seen_ids`), and `where_` names +/// the profile (or the home layer), which is what keeps two profiles' +/// instances of the same row apart: they compose different layer chains and +/// can disagree on the enabled state. +/// +/// An ANONYMOUS row (no `id:`) has no id to be named by, so it keeps the +/// package name and the `ANON_ROW_SOURCE_SUFFIX` marker — unchanged, and never +/// equal to an id-bearing row's source, which always ends in `package `. +fn plugin_row_identity(row: &PluginRowState, where_: &str) -> (String, String, Option) { + match &row.id { + Some(id) => ( + id.clone(), + format!("{where_}, package {}", row.name), + Some(id.clone()), + ), + None => ( + row.name.clone(), + format!("{where_}, {ANON_ROW_SOURCE_SUFFIX}"), + None, + ), + } +} + +/// Folded final state of one MCP row within one patch file. struct McpRowState { id: Option, disabled: bool, @@ -194,47 +494,17 @@ struct McpRowState { impl DshAdapter { fn fold_mcp_rows_in_text(text: &str, origin: &Path) -> Vec { - let mut order: Vec = Vec::new(); - let mut by_id: std::collections::HashMap = - std::collections::HashMap::new(); - let mut anon: Vec = Vec::new(); - - for row in parse_patch_rows(text, origin) { - let CordisRow { id, name, disabled, config, from_insert } = row; - let is_mcp_def = from_insert && name.as_deref() == Some(MCP_CLIENT_PLUGIN); - match id { - Some(id) if is_mcp_def => { - order.push(id.clone()); - by_id.insert( - id.clone(), - McpRowState { id: Some(id), disabled: disabled.unwrap_or(false), config }, - ); - } - Some(id) if !from_insert => { - // Override: only mutates an existing row (upstream: - // unknown id is warn+skip, never a definition). - if let Some(existing) = by_id.get_mut(&id) { - if let Some(d) = disabled { - existing.disabled = d; - } - if !config.is_null() { - existing.config = config; - } - } - } - // From-insert definition of some other plugin — never an - // override; skip (even on a malformed id collision). - Some(_) => {} - None if is_mcp_def => { - anon.push(McpRowState { id: None, disabled: disabled.unwrap_or(false), config }) - } - None => {} - } - } - let mut out: Vec = - order.into_iter().filter_map(|id| by_id.remove(&id)).collect(); - out.extend(anon); - out + fold_rows_in_text(text, origin, |row| { + row.from_insert && row.name.as_deref() == Some(MCP_CLIENT_PLUGIN) + }) + .defined + .into_iter() + .map(|row| McpRowState { + id: row.id, + disabled: row.disabled.unwrap_or(false), + config: row.config, + }) + .collect() } fn mcp_entries_in_text(text: &str, origin: &Path) -> Vec { @@ -242,7 +512,26 @@ impl DshAdapter { .into_iter() .filter_map(|row| { let config = &row.config; - let server_name = yaml_config_str(config, "serverName")?; + // `serverName` is what dsh itself keys the server by, and its + // presence is what makes this a readable MCP row. `_hk_name`, + // when present, is the ORIGINAL name HK sanitized to produce + // it — prefer it so the scanned extension name matches the + // other agents' and the row GROUPS with them instead of + // appearing as a second, sanitized extension. Absent = the + // name never needed sanitizing, so the two are identical. + // + // Deliberately reader-only: every deployer/kits lookup + // (`mcp_enabled_in_text`, `mcp_row_id_in_text`, + // `DshManagedBlock::insert_server_name`) keeps keying on the + // STORED `serverName` and normalizes its input through + // `normalize_dsh_server_name`, exactly as Codex's writers key + // on the sanitized TOML key. Both directions therefore keep + // working: a lookup by the original name sanitizes to the + // stored one, and a lookup by an already-valid name is + // unchanged. + let stored_name = yaml_config_str(config, "serverName")?; + let server_name = + yaml_config_str(config, HK_NAME_CONFIG_KEY).unwrap_or(stored_name); // Remote MCP: {url, headers?} — stdio MCP: {command, args, env}. // `url` decides remote-vs-stdio FIRST (as in hermes.rs): dsh // ships only stdio and streamable-http, so a url-bearing row is @@ -290,6 +579,35 @@ impl DshAdapter { .and_then(|r| r.id) } + /// Per-text state of one plugin row id: `(defined, disabled)` where + /// `defined` is true when the text contains an insert DEFINITION of the + /// id, and `disabled` is the last value the text establishes for it — + /// definition default `false`, later literal overrides win, `!!js`/absent + /// overrides change nothing. Callers fold across the layer texts dsh + /// composes for ONE profile (that profile's patch, then home) — never + /// across sibling profiles, which are never loaded together. + /// + /// A plain lookup over the shared fold: `is_def` is "any insert row", + /// because this per-id question is name-agnostic (an id-targeted + /// override does not know what kind of plugin it targets). + pub fn plugin_row_state_in_text(text: &str, row_id: &str) -> (bool, Option) { + let folded = fold_rows_in_text(text, Path::new("cordis.patch.yml"), |row| row.from_insert); + let defined = folded + .defined + .iter() + .any(|row| row.id.as_deref() == Some(row_id)); + (defined, folded.disabled_by_id.get(row_id).copied()) + } + + /// Every row id appearing in a patch text (definitions AND overrides) — + /// the collision domain for HK-generated insert-row ids. + pub fn row_ids_in_text(text: &str) -> std::collections::HashSet { + parse_patch_rows(text, Path::new("cordis.patch.yml")) + .into_iter() + .filter_map(|r| r.id) + .collect() + } + /// serverName → enabled for the given home-layer text (deployer uses this /// to compute base state with HK's managed block stripped). pub fn mcp_enabled_in_text(text: &str) -> std::collections::HashMap { @@ -327,6 +645,56 @@ impl AgentAdapter for DshAdapter { self.dsh_home.join("cordis.patch.yml") } + /// dsh models a plugin as a ROW naming a package, and the package is + /// reached through TWO manifest records — the profile's `dependencies` and + /// its `dsh.profile.bundles` layer list. Deleting the package directory + /// satisfies neither: both records keep naming it, and a configured-but- + /// absent package is a hard mount failure at the next boot. `dsh plugin + /// remove` runs pnpm and then reconciles `bundles` against what is still + /// installed (upstream `reconcilePlugins`), which is the only step that + /// leaves the profile consistent. + /// + /// In-box bundles are not dependencies, so that reconcile deliberately + /// never touches them — dsh itself cannot uninstall its own baseline, and + /// neither may we. + fn plugin_removal(&self, plugin: &super::PluginEntry) -> super::PluginRemoval { + let Some(pack) = plugin.pack.as_deref() else { + // No bundle: a row the user wrote into a patch layer by hand. + // Removing it means editing bytes outside HarnessKit's managed + // block, which the writer never does. + return super::PluginRemoval::Shipped; + }; + if IN_BOX_BUNDLES.contains(&pack) { + return super::PluginRemoval::Shipped; + } + // The profile owning this row is the one whose patch closes its layer + // chain (`read_plugins` builds the chain bundles-first, profile last). + let Some(profile) = plugin + .base_layers + .last() + .and_then(|p| p.parent()) + .and_then(|d| d.file_name()) + .map(|n| n.to_string_lossy().to_string()) + else { + return super::PluginRemoval::Shipped; + }; + let args = vec![ + "plugin".into(), + "--profile".into(), + profile, + "remove".into(), + pack.to_string(), + ]; + super::PluginRemoval::Command { + program: "dsh".into(), + args, + } + } + + fn vendor_baseline_packs(&self) -> Vec { + IN_BOX_BUNDLES.iter().map(|b| (*b).to_string()).collect() + } + fn hook_config_path(&self) -> PathBuf { // dsh has no own hook config; return the settings doc so the default // plugin_config_path() has a sane anchor. Never read for hooks @@ -338,6 +706,185 @@ impl AgentAdapter for DshAdapter { vec![] } + /// dsh plugin discovery. dsh's own vocabulary is the composed ROW: its + /// Settings → Plugins list shows one Enabled/Disabled entry per row + /// (`timer`, `hmr`, `llm`, …), and a BUNDLE never appears there at all — + /// a bundle is a patch LAYER that inserts rows, not a plugin. HK mirrors + /// that, so two sources per profile: + /// + /// 1. Rows DEFINED by each mounted bundle's own patch file + /// (`dsh.bundle.patch` in the bundle's package.json — an ordinary + /// patch file, parsed by the same `parse_patch_rows`). This is the + /// bulk of the list (~130 rows on a stock install) and the only place + /// most toggleable rows live. + /// 2. Rows DEFINED by the user's own patch files (home layer first, then + /// each profile's `cordis.patch.yml`). + /// + /// A package that is a profile `dependency` but which no layer mounts is + /// deliberately NOT listed: dsh never loads it and its own UI never shows + /// it, so neither does HK ("if dsh doesn't show it, HK doesn't show it"). + /// + /// mcp-client rows are excluded throughout (modeled as MCP servers), and + /// bundles themselves are NOT emitted as entries. + /// + /// Each entry is named by its patch ROW ID, exactly as dsh's own + /// Settings → Plugins list labels it; the package the row instantiates + /// lives in the `source` string. See `plugin_row_identity`. + /// + /// Ordering and identity within a profile follow dsh's own composition: + /// bundle patches in `bundles` order, then the profile patch, then the + /// home patch. The EARLIEST layer defining a row id owns the entry (a + /// later layer can only override it — upstream, an override never + /// creates a row), and the `disabled` state folds across the whole chain, + /// which is why `hmr` reads as disabled: `@deepseek-ai/dsh-base` defines + /// it and `@deepseek-ai/dsh-web-app` disables it two layers later. + /// + /// Known parser limitation (accepted): `{id, insert}` group-appends are + /// skipped, so plugins inserted into a group are invisible. + fn read_plugins(&self) -> Vec { + use super::PluginEntry; + let mut entries: Vec = Vec::new(); + + // --- Home layer rows (source 2, home) --- + let home_patch = self.mcp_config_path(); + let home_text = std::fs::read_to_string(&home_patch).unwrap_or_default(); + let (home_rows, home_disabled) = fold_plugin_rows_in_text(&home_text, &home_patch); + for row in home_rows { + let (name, source, uri) = plugin_row_identity(&row, "home layer"); + entries.push(PluginEntry { + name, + source, + // The home layer is applied LAST, so nothing overrides a + // home-defined row but the home text itself (already folded). + enabled: !row.disabled, + // No path: a home row applies to EVERY profile, so its + // package resolves under whichever profile is booted — there + // is no single `/node_modules/` to probe. + path: None, + // No bundle: a home row is one the USER wrote, which is + // exactly what the source filter separates from the vendor + // baseline. + pack: None, + source_url: None, + uri, + installed_at: None, + updated_at: None, + // Its own layer; the writer recognises the home patch and + // folds the (block-stripped) user text instead of re-reading. + base_layers: vec![home_patch.clone()], + }); + } + + // --- Per profile: bundle rows (1), profile rows (2) --- + for profile_dir in self.profile_dirs() { + let profile = profile_dir + .file_name() + .map(|n| n.to_string_lossy().to_string()) + .unwrap_or_default(); + let manifest_path = profile_dir.join("package.json"); + let Ok(manifest_text) = std::fs::read_to_string(&manifest_path) else { + continue; // no package.json — not a plugin-bearing profile + }; + let manifest: ProfileManifest = match serde_json::from_str(&manifest_text) { + Ok(m) => m, + Err(err) => { + // Skip THIS profile with a diagnostic; never abort the scan. + eprintln!( + "[hk] warning: cannot parse {}: {err}", + manifest_path.display() + ); + continue; + } + }; + let bundles = manifest.dsh.profile.bundles; + + // The layers dsh composes for this profile BELOW the home patch, + // in application order: each mounted bundle's own patch file, + // then the profile's own. A bundle whose patch cannot be + // resolved (package absent, no `dsh.bundle.patch`, file missing) + // simply contributes no layer — it is not an error here; dsh + // itself fails loud on a bundle it cannot load. + let profile_patch = profile_dir.join("cordis.patch.yml"); + let mut layers: Vec<(Option, PathBuf)> = bundles + .iter() + .filter_map(|b| { + self.bundle_patch_path(&profile_dir, b) + .map(|p| (Some(b.clone()), p)) + }) + .collect(); + layers.push((None, profile_patch)); + let base_layers: Vec = layers.iter().map(|(_, p)| p.clone()).collect(); + + // One ordered pass: collect each layer's row DEFINITIONS + // (earliest layer wins per id) while folding the `disabled` state + // every layer establishes. Computed once per profile — asking + // per row would re-parse each ~450-line bundle patch ~80 times. + let mut defined: Vec<(Option, PluginRowState)> = Vec::new(); + let mut seen_ids: std::collections::HashSet = + std::collections::HashSet::new(); + let mut composed: std::collections::HashMap = + std::collections::HashMap::new(); + for (bundle, path) in &layers { + let text = std::fs::read_to_string(path).unwrap_or_default(); + let (rows, disabled_by_id) = fold_plugin_rows_in_text(&text, path); + composed.extend(disabled_by_id); + for row in rows { + // `insert` returns false when the id was already seen — + // an earlier layer defines it, and upstream a later + // restatement can only override, never redefine. + if row + .id + .as_ref() + .is_some_and(|id| !seen_ids.insert(id.clone())) + { + continue; + } + defined.push((bundle.clone(), row)); + } + } + // The home patch applies after every profile layer, so its + // overrides win — user text and HK's managed block alike (the + // block is plain YAML within the file). + composed.extend(home_disabled.clone()); + + // (1) + (2) one entry per composed row. + for (bundle, row) in defined { + let enabled = match &row.id { + Some(id) => !composed.get(id).copied().unwrap_or(row.disabled), + None => !row.disabled, + }; + // Identity is id-load-bearing (scanner::stable_id over + // ":"), so the source names the profile and the + // bundle that provided the row when one did. The profile + // prefix is what keeps two profiles' instances of the same + // bundle row apart — they can differ in enabled state and + // compose different layer chains. + let where_ = match &bundle { + Some(pkg) => format!("profile {profile}, bundle {pkg}"), + None => format!("profile {profile}"), + }; + let (name, source, uri) = plugin_row_identity(&row, &where_); + entries.push(PluginEntry { + name, + source, + enabled, + path: self.package_dir(&profile_dir, &row.name), + source_url: None, + uri, + installed_at: None, + updated_at: None, + base_layers: base_layers.clone(), + // Bundle-provided rows carry their bundle as the source + // filter's pack; rows the user added in a patch layer + // have no bundle and stay unattributed, which is what + // separates the vendor baseline from the user's own. + pack: bundle, + }); + } + } + entries + } + fn hook_format(&self) -> HookFormat { HookFormat::None } @@ -351,6 +898,12 @@ impl AgentAdapter for DshAdapter { true } + fn remote_mcp_schema(&self) -> super::RemoteMcpSchema { + // Registered LAST in the P1 sequence, after the insert writer works — + // flipping this first would advertise an install path that errors. + super::RemoteMcpSchema::DshTransport + } + fn read_mcp_servers(&self) -> Vec { self.read_mcp_servers_from(&self.mcp_config_path()) } @@ -403,7 +956,7 @@ impl AgentAdapter for DshAdapter { #[cfg(test)] mod tests { - use super::super::AgentAdapter; + use super::super::{AgentAdapter, PluginRemoval}; use super::*; #[test] @@ -590,6 +1143,52 @@ mod tests { assert!(adapter.read_mcp_servers().is_empty()); } + #[test] + fn absent_patch_file_text_parses_to_no_rows_without_warning() { + // Callers feed "" for a MISSING cordis.patch.yml (read_to_string + // .unwrap_or_default()); empty/whitespace text is the absent-file + // case, not a malformed list — no rows, and no stderr warning. + assert!(parse_patch_rows("", Path::new("cordis.patch.yml")).is_empty()); + assert!(parse_patch_rows(" \n\t\n", Path::new("cordis.patch.yml")).is_empty()); + } + + #[test] + fn hk_name_recovers_the_original_server_name_without_moving_the_lookup_key() { + // Written by deployer::build_dsh_insert_row when mcp-client's + // /^[A-Za-z0-9_-]{1,32}$/ forced sanitization. + let text = r#"- insert: + - id: mcp-microsoft-markitdown + name: '@deepseek-ai/dsh-mcp-client' + config: + transport: stdio + serverName: microsoft-markitdown + _hk_name: microsoft/markitdown + command: uvx +"#; + let tmp = tempfile::tempdir().unwrap(); + write_home_patch(tmp.path(), text); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let servers = adapter.read_mcp_servers(); + assert_eq!(servers.len(), 1); + assert_eq!( + servers[0].name, "microsoft/markitdown", + "reader reports the ORIGINAL name so the extension groups across agents" + ); + + // Lookups stay keyed on the STORED serverName — the deployer and + // kits normalize their input to it, so both directions resolve. + assert!(DshAdapter::mcp_enabled_in_text(text).contains_key("microsoft-markitdown")); + assert_eq!( + DshAdapter::mcp_row_id_in_text(text, "microsoft-markitdown").as_deref(), + Some("mcp-microsoft-markitdown") + ); + assert_eq!( + crate::deployer::normalize_dsh_server_name("microsoft/markitdown"), + "microsoft-markitdown", + "the lookup normalizer and the writer's sanitizer agree" + ); + } + #[test] fn mcp_row_id_lookup_by_server_name() { assert_eq!( @@ -645,6 +1244,549 @@ mod tests { } } + fn write_profile(home: &Path, profile: &str, package_json: &str, patch: Option<&str>) { + let dir = home.join(".dsh/profiles").join(profile); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("package.json"), package_json).unwrap(); + if let Some(p) = patch { + std::fs::write(dir.join("cordis.patch.yml"), p).unwrap(); + } + } + + /// Install a mounted BUNDLE package into dsh's symlink farm the way a + /// real one ships: a `package.json` declaring `dsh.bundle.patch` plus the + /// patch file it points at (verified shape: `"patch": "./cordis.patch.yml"`). + fn write_bundle(home: &Path, pkg: &str, patch: &str) { + let dir = home.join(".dsh/profiles/node_modules").join(pkg); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("package.json"), + format!( + r#"{{"name": "{pkg}", "dsh": {{"bundle": {{"patch": "./cordis.patch.yml"}}}}}}"# + ), + ) + .unwrap(); + std::fs::write(dir.join("cordis.patch.yml"), patch).unwrap(); + } + + /// Shape copied from the installed `@deepseek-ai/dsh-base` rc.6: ONE + /// `insert:` group holding every base row. + const BASE_BUNDLE_PATCH: &str = "\ +- insert: + - id: timer + name: '@deepseek-ai/cordis-plugin-timer' + - id: hmr + name: '@deepseek-ai/cordis-plugin-hmr' + config: + root: ['.'] + - id: llm + name: '@deepseek-ai/dsh-llm' +"; + + /// Shape copied from the installed `@deepseek-ai/dsh-web-app` rc.6: id + /// overrides of base rows (including the real `hmr` disable) plus its own + /// insert group. + const WEB_APP_BUNDLE_PATCH: &str = "\ +- id: hmr + disabled: true +- insert: + - id: web-server + name: '@deepseek-ai/dsh-host-webserver' +"; + + const WEB_MANIFEST: &str = r#"{ + "name": "dsh-profile-web", + "dependencies": { + "@deepseek-ai/dsh-base": "0.1.0", + "@deepseek-ai/dsh-mcp-client": "0.1.0", + "dsh-plugin-tool": "1.0.0", + "left-pad": "1.3.0" + }, + "dsh": { "profile": { "bundles": ["@deepseek-ai/dsh-base"] } } +}"#; + + const WEB_PATCH: &str = "- insert:\n - id: tool-policy\n name: dsh-plugin-tool\n config:\n mode: strict\n"; + + /// `web` mounting both in-box bundles, with a user row — the real + /// machine's shape in miniature. + fn two_bundle_profile(tmp: &Path) { + write_bundle(tmp, "@deepseek-ai/dsh-base", BASE_BUNDLE_PATCH); + write_bundle(tmp, "@deepseek-ai/dsh-web-app", WEB_APP_BUNDLE_PATCH); + write_profile( + tmp, + "web", + r#"{ + "dependencies": {"dsh-plugin-tool": "1.0.0", "left-pad": "1.3.0"}, + "dsh": { "profile": { "bundles": ["@deepseek-ai/dsh-base", "@deepseek-ai/dsh-web-app"] } } +}"#, + Some(WEB_PATCH), + ); + } + + #[test] + fn read_plugins_lists_bundle_rows_and_user_rows() { + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + + // Source 1: a row from a mounted bundle's own patch file — listed + // under its ROW ID (what dsh's own plugin list shows), with the + // bundle and the package it instantiates in the identity-bearing + // source string. + let timer = plugins.iter().find(|p| p.name == "timer").unwrap(); + assert_eq!( + timer.source, + "profile web, bundle @deepseek-ai/dsh-base, package @deepseek-ai/cordis-plugin-timer" + ); + assert_eq!(timer.uri.as_deref(), Some("timer")); + assert!(timer.enabled); + // Toggle input: the profile's whole chain, bundles first. + assert_eq!( + timer.base_layers, + vec![ + tmp.path().join(".dsh/profiles/node_modules/@deepseek-ai/dsh-base/cordis.patch.yml"), + tmp.path() + .join(".dsh/profiles/node_modules/@deepseek-ai/dsh-web-app/cordis.patch.yml"), + tmp.path().join(".dsh/profiles/web/cordis.patch.yml"), + ] + ); + + // A row a LATER bundle inserts is owned by that bundle. + let web_server = plugins.iter().find(|p| p.name == "web-server").unwrap(); + assert_eq!( + web_server.source, + "profile web, bundle @deepseek-ai/dsh-web-app, package @deepseek-ai/dsh-host-webserver" + ); + + // Source 2: the user's own profile patch row. + let row = plugins.iter().find(|p| p.name == "tool-policy").unwrap(); + assert_eq!(row.source, "profile web, package dsh-plugin-tool"); + assert_eq!(row.uri.as_deref(), Some("tool-policy")); + assert!(row.enabled); + + // A dependency no layer mounts is NOT listed: dsh never loads it and + // never shows it, so neither does HK. `left-pad` is such a dep of the + // `web` profile below. + assert!(plugins + .iter() + .all(|p| p.name != "left-pad" && !p.source.ends_with("package left-pad"))); + + // The profiles/node_modules symlink farm is not a profile. + assert!(plugins.iter().all(|p| !p.source.starts_with("profile node_modules"))); + } + + #[test] + fn removal_delegates_third_party_bundles_and_refuses_the_in_box_baseline() { + // Verified against a real install: `dsh plugin add dshmarket` puts the + // package in the profile's `dependencies` AND appends it to + // `dsh.profile.bundles`. Deleting its directory would leave both + // records naming a missing package, so removal has to go through the + // CLI that reconciles them. In-box bundles are not dependencies, so + // that same reconcile can never remove them — nor may we. + let tmp = tempfile::tempdir().unwrap(); + write_bundle(tmp.path(), "@deepseek-ai/dsh-base", BASE_BUNDLE_PATCH); + write_bundle( + tmp.path(), + "dshmarket", + "- insert:\n - id: dsh-market\n name: 'dshmarket'\n", + ); + write_profile( + tmp.path(), + "web", + r#"{ + "dependencies": {"dshmarket": "1.11.2"}, + "dsh": { "profile": { "bundles": ["@deepseek-ai/dsh-base", "dshmarket"] } } +}"#, + Some("[]\n"), + ); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + let find = |name: &str| plugins.iter().find(|p| p.name == name).unwrap(); + + assert_eq!( + adapter.plugin_removal(find("timer")), + PluginRemoval::Shipped, + "an in-box bundle's row is not the user's to delete" + ); + + let PluginRemoval::Command { program, args, .. } = + adapter.plugin_removal(find("dsh-market")) + else { + panic!("a third-party bundle's row must delegate to dsh's own CLI"); + }; + assert_eq!(program, "dsh"); + // The profile has to be named: the same package can be installed into + // several profiles, and `remove` only touches the one it is given. + assert_eq!(args, ["plugin", "--profile", "web", "remove", "dshmarket"]); + } + + #[test] + fn only_bundle_provided_rows_are_attributed_to_a_source_pack() { + // The Extensions "source" filter derives `pack` from a git URL, which + // no dsh row has — the whole vendor baseline would report no source. + // The bundle IS that source, and a row the user wrote has none, which + // is what separates the two in the filter. + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + + let pack_of = |name: &str| { + plugins.iter().find(|p| p.name == name).unwrap().pack.clone() + }; + assert_eq!(pack_of("timer").as_deref(), Some("@deepseek-ai/dsh-base")); + assert_eq!( + pack_of("web-server").as_deref(), + Some("@deepseek-ai/dsh-web-app"), + "a row belongs to the bundle that inserted it, not the first bundle" + ); + // The user's own profile-patch row: no bundle, so no source. + assert_eq!(pack_of("tool-policy"), None); + } + + #[test] + fn bundles_themselves_are_not_plugin_entries() { + // A bundle is a LAYER. dsh's own Settings → Plugins list has no such + // entry — searching it for "@deepseek-ai/dsh-base" finds nothing — + // so neither does HK; only the rows the layer inserts are listed. + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + for bundle in ["@deepseek-ai/dsh-base", "@deepseek-ai/dsh-web-app"] { + assert!( + plugins + .iter() + .all(|p| p.name != bundle && !p.source.ends_with(&format!("package {bundle}"))), + "{bundle} must not be listed as a plugin" + ); + } + // ...not even when a profile also lists the bundle as a dependency. + write_profile( + tmp.path(), + "web2", + r#"{"dependencies": {"@deepseek-ai/dsh-base": "0.1.0"}, "dsh": {"profile": {"bundles": ["@deepseek-ai/dsh-base"]}}}"#, + None, + ); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + assert!(plugins + .iter() + .all(|p| !p.source.ends_with("package @deepseek-ai/dsh-base"))); + } + + #[test] + fn a_later_bundle_layer_disables_an_earlier_bundles_row() { + // dsh's real `hmr` case: defined by dsh-base, disabled by dsh-web-app + // two layers on. The entry belongs to the DEFINING bundle and reads + // as disabled — the composed state, not the definition's. + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + let hmr = plugins.iter().find(|p| p.name == "hmr").unwrap(); + assert_eq!( + hmr.source, + "profile web, bundle @deepseek-ai/dsh-base, package @deepseek-ai/cordis-plugin-hmr", + "the earliest layer defining the id owns the entry" + ); + assert!(!hmr.enabled, "a later bundle layer's disable wins"); + } + + #[test] + fn home_layer_override_disables_a_bundle_row() { + // The home patch applies after every profile layer, so an HK managed + // block disable (plain YAML in that file) turns a bundle row off — + // exactly the mechanism the plugin toggle writes. + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + write_home_patch(tmp.path(), "- id: timer\n disabled: true\n"); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + let timer = plugins.iter().find(|p| p.name == "timer").unwrap(); + assert!(!timer.enabled, "home-layer disable of a bundle row wins"); + + // ...and re-enabling a bundle-disabled row works the same way. + write_home_patch(tmp.path(), "- id: hmr\n disabled: false\n"); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + let hmr = plugins.iter().find(|p| p.name == "hmr").unwrap(); + assert!(hmr.enabled, "home layer wins over the web-app bundle disable"); + } + + #[test] + fn mcp_client_rows_in_a_bundle_patch_are_not_plugins() { + // mcp-client rows are modeled as MCP servers wherever they appear. + let tmp = tempfile::tempdir().unwrap(); + write_bundle( + tmp.path(), + "dsh-mcp-bundle", + "- insert:\n - id: mcp-x\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: x\n - id: plain\n name: dsh-plugin-plain\n", + ); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": ["dsh-mcp-bundle"]}}}"#, + None, + ); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + assert!(plugins + .iter() + .all(|p| p.uri.as_deref() != Some("mcp-x") + && !p.source.ends_with(&format!("package {MCP_CLIENT_PLUGIN}")))); + assert!(plugins.iter().any(|p| p.name == "plain")); + } + + #[test] + fn earliest_layer_defining_a_row_id_owns_the_entry() { + // Upstream, only the first `insert` of an id creates the row; a later + // layer restating it can merely override. One entry, owned by the + // bundle — not two. + let tmp = tempfile::tempdir().unwrap(); + write_bundle( + tmp.path(), + "dsh-extra-bundle", + "- insert:\n - id: extra-row\n name: dsh-plugin-extra\n", + ); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": ["dsh-extra-bundle"]}}}"#, + Some("- insert:\n - id: extra-row\n name: dsh-plugin-extra\n"), + ); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + let entries: Vec<_> = plugins.iter().filter(|p| p.uri.as_deref() == Some("extra-row")).collect(); + assert_eq!(entries.len(), 1); + assert_eq!( + entries[0].source, + "profile web, bundle dsh-extra-bundle, package dsh-plugin-extra" + ); + } + + #[test] + fn a_bundle_without_a_resolvable_patch_contributes_no_rows_and_no_error() { + // Fresh install / never-booted dsh: the symlink farm may be absent. + // The scan must degrade to "no rows from that layer", not blow up. + let tmp = tempfile::tempdir().unwrap(); + write_profile(tmp.path(), "web", WEB_MANIFEST, Some(WEB_PATCH)); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + // Only the user's own row survives. + assert!(plugins.iter().any(|p| p.name == "tool-policy")); + assert!(plugins.iter().all(|p| !p.source.contains("bundle "))); + // mcp-client is mounted (its rows are MCP servers), never a plugin. + assert!(plugins + .iter() + .all(|p| !p.source.ends_with(&format!("package {MCP_CLIENT_PLUGIN}")))); + } + + #[test] + fn bundle_rows_resolve_their_package_from_the_symlink_farm() { + // A bundle row's package is a transitive dep of the bundle, hoisted + // into `/profiles/node_modules` — not into the profile's + // own node_modules. + let tmp = tempfile::tempdir().unwrap(); + two_bundle_profile(tmp.path()); + let farm = tmp + .path() + .join(".dsh/profiles/node_modules/@deepseek-ai/dsh-llm"); + std::fs::create_dir_all(&farm).unwrap(); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + let llm = plugins.iter().find(|p| p.name == "llm").unwrap(); + assert_eq!( + llm.source, + "profile web, bundle @deepseek-ai/dsh-base, package @deepseek-ai/dsh-llm" + ); + assert_eq!(llm.path.as_deref(), Some(farm.as_path())); + } + + #[test] + fn home_layer_rows_and_home_overrides_of_profile_rows() { + let tmp = tempfile::tempdir().unwrap(); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {"dsh-plugin-tool": "1.0.0"}, "dsh": {"profile": {"bundles": []}}}"#, + Some(WEB_PATCH), + ); + std::fs::write( + tmp.path().join(".dsh/cordis.patch.yml"), + "- insert:\n - id: theme-row\n name: dsh-plugin-theme\n- id: tool-policy\n disabled: true\n", + ) + .unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + + let theme = plugins.iter().find(|p| p.name == "theme-row").unwrap(); + assert_eq!(theme.source, "home layer, package dsh-plugin-theme"); + assert_eq!(theme.uri.as_deref(), Some("theme-row")); + assert!(theme.enabled); + + // The home layer applies after every profile layer, so a home + // `disabled: true` override (user- OR HK-block-authored — the block + // is plain YAML within the file) wins over the profile definition. + let tool = plugins.iter().find(|p| p.name == "tool-policy").unwrap(); + assert!(!tool.enabled, "home-layer disable override wins"); + } + + #[test] + fn unparseable_profile_manifest_skips_that_profile_only() { + let tmp = tempfile::tempdir().unwrap(); + write_profile(tmp.path(), "bad", "{ not json", None); + write_profile( + tmp.path(), + "good", + r#"{"dependencies": {"dsh-plugin-tool": "1.0.0"}, "dsh": {"profile": {"bundles": []}}}"#, + Some(WEB_PATCH), + ); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + assert_eq!(plugins.len(), 1, "bad profile skipped, scan not aborted"); + assert_eq!(plugins[0].name, "tool-policy"); + assert_eq!(plugins[0].source, "profile good, package dsh-plugin-tool"); + } + + #[test] + fn in_box_bundle_patch_resolves_from_the_symlink_farm() { + // The three in-box bundles resolve from dsh's maintained farm, NOT + // from a profile's own node_modules — a stale copy there must not + // shadow the farm's rows. + let tmp = tempfile::tempdir().unwrap(); + write_bundle(tmp.path(), "@deepseek-ai/dsh-base", BASE_BUNDLE_PATCH); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": ["@deepseek-ai/dsh-base"]}}}"#, + None, + ); + let stale = tmp + .path() + .join(".dsh/profiles/web/node_modules/@deepseek-ai/dsh-base"); + std::fs::create_dir_all(&stale).unwrap(); + std::fs::write( + stale.join("package.json"), + r#"{"dsh": {"bundle": {"patch": "./cordis.patch.yml"}}}"#, + ) + .unwrap(); + std::fs::write( + stale.join("cordis.patch.yml"), + "- insert:\n - id: stale\n name: dsh-plugin-stale\n", + ) + .unwrap(); + + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + assert!( + plugins.iter().any(|p| p.uri.as_deref() == Some("timer")), + "farm patch is the one that is read" + ); + assert!(plugins.iter().all(|p| p.uri.as_deref() != Some("stale"))); + } + + #[test] + fn two_rows_of_same_package_have_distinct_identities() { + // Cordis is instance-based: one package can carry two insert rows + // with distinct ids (the mcp-client pattern). Each row is NAMED by + // its id, so the two are distinct extensions — and the package they + // share is still visible, in the source string. + let tmp = tempfile::tempdir().unwrap(); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": []}}}"#, + Some("- insert:\n - id: a-row\n name: dsh-plugin-multi\n - id: b-row\n name: dsh-plugin-multi\n"), + ); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let plugins = adapter.read_plugins(); + let rows: Vec<_> = plugins + .iter() + .filter(|p| p.source == "profile web, package dsh-plugin-multi") + .collect(); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].name, "a-row"); + assert_eq!(rows[1].name, "b-row"); + let ids: Vec = rows + .iter() + .map(|p| crate::scanner::plugin_extension_id(&p.name, &p.source, "dsh")) + .collect(); + assert_ne!(ids[0], ids[1]); + } + + #[test] + fn one_row_id_in_several_layers_keeps_distinct_identities() { + // Naming a row by its id makes the SOURCE the only discriminator + // left, so it has to carry the owning layer: the same id can be + // defined once per profile and once in the home layer, and those are + // different rows with different composed states. An anonymous row + // (no id) keeps the package name and the anon marker, which can never + // equal an id row's `…, package ` source. + let tmp = tempfile::tempdir().unwrap(); + write_profile( + tmp.path(), + "web", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": []}}}"#, + Some("- insert:\n - id: shared\n name: dsh-plugin-a\n - name: dsh-plugin-anon\n"), + ); + write_profile( + tmp.path(), + "cli", + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": []}}}"#, + Some("- insert:\n - id: shared\n name: dsh-plugin-b\n"), + ); + write_home_patch( + tmp.path(), + "- insert:\n - id: shared\n name: dsh-plugin-c\n", + ); + let plugins = DshAdapter::with_home(tmp.path().to_path_buf()).read_plugins(); + + let mut sources: Vec<&str> = plugins + .iter() + .filter(|p| p.name == "shared") + .map(|p| p.source.as_str()) + .collect(); + sources.sort(); + assert_eq!( + sources, + vec![ + "home layer, package dsh-plugin-c", + "profile cli, package dsh-plugin-b", + "profile web, package dsh-plugin-a", + ] + ); + + // The anonymous row is named by its package and is untoggleable. + let anon = plugins + .iter() + .find(|p| p.name == "dsh-plugin-anon") + .unwrap(); + assert_eq!(anon.source, "profile web, anonymous row"); + assert!(anon.uri.is_none()); + + // Every entry in the scan is a distinct extension. + let mut ids: Vec = plugins + .iter() + .map(|p| crate::scanner::plugin_extension_id(&p.name, &p.source, "dsh")) + .collect(); + let total = ids.len(); + ids.sort(); + ids.dedup(); + assert_eq!(ids.len(), total, "no two rows may collide on (name, source)"); + } + + #[test] + fn scan_plugins_carries_identity_to_extensions() { + let tmp = tempfile::tempdir().unwrap(); + write_profile(tmp.path(), "web", WEB_MANIFEST, Some(WEB_PATCH)); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let exts = crate::scanner::scan_plugins(&adapter); + let row = exts.iter().find(|e| e.name == "tool-policy").unwrap(); + // The package the row instantiates rides in the description — this is + // where the detail panel surfaces it now that the name is the row id. + assert_eq!( + row.description, + "Plugin from profile web, package dsh-plugin-tool" + ); + assert!(row.enabled); + // Global scope only (existing scan_plugins behavior; dsh has no + // project-level plugins). ConfigScope has no PartialEq — use matches!. + assert!(matches!(row.scope, crate::models::ConfigScope::Global)); + } + #[test] fn read_mcp_servers_from_reads_the_given_file() { // The service delete path locates entries via read_mcp_servers_from; @@ -656,4 +1798,17 @@ mod tests { let servers = adapter.read_mcp_servers_from(&tmp.path().join(".dsh/cordis.patch.yml")); assert_eq!(servers.len(), 2); } + + #[test] + fn dsh_remote_capabilities_are_http_only() { + let tmp = tempfile::tempdir().unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + assert_eq!( + adapter.remote_mcp_schema(), + super::super::RemoteMcpSchema::DshTransport + ); + let caps = crate::models::AgentCapabilities::from_adapter(&adapter); + assert!(caps.mcp_remote.http, "streamable-http installable"); + assert!(!caps.mcp_remote.sse, "dsh ships no SSE transport"); + } } diff --git a/crates/hk-core/src/adapter/gemini.rs b/crates/hk-core/src/adapter/gemini.rs index 9fe6ee35..2807f54d 100644 --- a/crates/hk-core/src/adapter/gemini.rs +++ b/crates/hk-core/src/adapter/gemini.rs @@ -253,6 +253,8 @@ impl AgentAdapter for GeminiAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } entries diff --git a/crates/hk-core/src/adapter/hermes.rs b/crates/hk-core/src/adapter/hermes.rs index ddb168dc..3355ebd9 100644 --- a/crates/hk-core/src/adapter/hermes.rs +++ b/crates/hk-core/src/adapter/hermes.rs @@ -113,6 +113,8 @@ impl HermesAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }) } } diff --git a/crates/hk-core/src/adapter/mod.rs b/crates/hk-core/src/adapter/mod.rs index 0aeaba70..780531f4 100644 --- a/crates/hk-core/src/adapter/mod.rs +++ b/crates/hk-core/src/adapter/mod.rs @@ -243,6 +243,25 @@ pub struct HookEntry { pub enabled: bool, } +/// How a plugin has to be removed, which is a property of the agent's install +/// model rather than of HarnessKit. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PluginRemoval { + /// Delete `PluginEntry::path` from disk. The default, and correct whenever + /// the plugin IS its directory. + Files, + /// Run the agent's own uninstaller. Required when removing the files would + /// leave the agent's manifests naming a package that is no longer there — + /// dsh keeps a plugin in a profile's `dependencies` AND in its + /// `dsh.profile.bundles` layer list, and `dsh plugin remove` is the one + /// step that reconciles both. + Command { program: String, args: Vec }, + /// The agent ships this plugin; it is not the user's to delete. dsh's own + /// CLI cannot remove an in-box bundle either — those are not dependencies, + /// so its reconcile step never touches them. + Shipped, +} + /// Represents a plugin entry parsed from an agent's config #[derive(Debug, Clone)] pub struct PluginEntry { @@ -255,13 +274,44 @@ pub struct PluginEntry { /// `.git`-walk source detection, which mis-attributes plugins cached inside /// a dotfiles repo. `None` for agents without such a manifest. pub source_url: Option, - /// Agent-specific URI for the plugin (e.g. VS Code pluginUri "file:///..."). - /// Used by toggle to identify the plugin in the agent's state store. + /// Agent-specific toggle identifier: VS Code pluginUri ("file:///...") + /// for Copilot, the cordis patch row id for dsh row-plugins. Used by + /// toggle to address the plugin in the agent's own state store/config. pub uri: Option, /// Precise install timestamp (e.g. from a registry file). Overrides file-system heuristic. pub installed_at: Option>, /// Precise last-updated timestamp. Overrides file-system heuristic. pub updated_at: Option>, + /// Ordered config layers, BELOW the agent's own write target, whose + /// composition produces this entry's base enabled-state — for agents that + /// model plugins as rows in a LAYERED config rather than as directories. + /// + /// dsh boots ONE profile at a time and composes, in order, each mounted + /// bundle's own patch file, then that profile's `cordis.patch.yml`, then + /// the home patch. A row's base state therefore depends on the WHOLE + /// chain, not just the layer that defines it: `hmr` is defined by + /// `@deepseek-ai/dsh-base` and disabled by `@deepseek-ai/dsh-web-app` two + /// layers later. `read_plugins` emits one entry per (profile, row) with + /// the profile's full chain here, and the toggle writer folds exactly + /// these layers plus the home patch — a sibling profile's patch is never + /// loaded alongside it and must never be folded in. + /// + /// Structured counterpart of the human-readable `source` string, like + /// `uri` is for the row id. Empty for directory-based plugins and for + /// entries with no row to target. + pub base_layers: Vec, + /// Package/repo this plugin was provided by, for the Extensions "source" + /// filter. `scan_plugins` normally derives this from a detected git URL, + /// which only works for plugins that ARE git checkouts; an adapter sets + /// this when it knows the provider by other means. + /// + /// dsh is the case that needs it: every row it reports comes from a bundle + /// package (`@deepseek-ai/dsh-base`, `@deepseek-ai/dsh-web-app`) named in + /// the profile manifest, and none of them is a git checkout, so the whole + /// vendor baseline would otherwise report no source at all. `None` for + /// adapters that have nothing to add, which leaves the git-URL derivation + /// untouched. + pub pack: Option, } /// Format used by an agent for hook configuration files. @@ -333,7 +383,8 @@ pub enum McpFormat { /// This is the single source of truth for "which transports can this agent /// receive": the deployer's JSON writer dispatches on the four JSON-family /// variants, and `AgentCapabilities::from_adapter` derives UI install-gating -/// from it (`Toml` is the only HTTP-only variant — Codex has no SSE support; +/// from it (`Toml` and `DshTransport` are the HTTP-only variants — Codex +/// and dsh have no SSE support; /// `Unsupported` receives no remote entries at all). #[derive(Debug, Clone, Copy, PartialEq)] pub enum RemoteMcpSchema { @@ -351,6 +402,11 @@ pub enum RemoteMcpSchema { OpencodeRemote, /// YAML `url:` + `headers:` + optional `transport: sse` — Hermes. HermesUrl, + /// dsh mcp-client YAML config: explicit `transport: streamable-http` + /// discriminant + `serverName` + `url` + `headers` inside a cordis + /// insert row. Streamable HTTP only — dsh ships no SSE transport + /// (source-verified: packages/mcp/mcp-client/src/index.ts). + DshTransport, /// Agent has no remote MCP concept; deploying a remote entry is an error. Unsupported, } @@ -368,6 +424,20 @@ pub trait AgentAdapter: Send + Sync { fn plugin_config_path(&self) -> PathBuf { self.hook_config_path() } + /// How `service::delete_extension` must remove this plugin. Answering it + /// here keeps the agent's install model out of the generic delete path, + /// which otherwise assumes every plugin is a directory HarnessKit owns. + fn plugin_removal(&self, _plugin: &PluginEntry) -> PluginRemoval { + PluginRemoval::Files + } + /// Packs whose plugins ship WITH the agent and therefore can never be + /// removed — the `PluginRemoval::Shipped` set, exposed through + /// `AgentCapabilities` so the UI disables delete on exactly the rows the + /// backend refuses. Empty for agents whose baseline is compiled in and so + /// never appears as an extension at all, which is every agent but dsh. + fn vendor_baseline_packs(&self) -> Vec { + vec![] + } fn read_mcp_servers(&self) -> Vec; fn read_hooks(&self) -> Vec; /// Parse MCP servers from a specific config file (e.g. a project's `.mcp.json`). @@ -664,13 +734,16 @@ impl crate::models::AgentCapabilities { }, hooks_supported: a.hook_format() != HookFormat::None, global_hook_install: a.supports_global_hook_install(), - // Codex's TOML schema (the only `Toml` agent) speaks Streamable - // HTTP but not SSE; every other non-Unsupported schema takes both. + vendor_baseline_packs: a.vendor_baseline_packs(), + // Codex (`Toml`) and dsh (`DshTransport`) speak Streamable HTTP + // but not SSE; every other non-Unsupported schema takes both. mcp_remote: crate::models::RemoteTransportFlags { http: remote_schema != RemoteMcpSchema::Unsupported, sse: !matches!( remote_schema, - RemoteMcpSchema::Unsupported | RemoteMcpSchema::Toml + RemoteMcpSchema::Unsupported + | RemoteMcpSchema::Toml + | RemoteMcpSchema::DshTransport ), }, } @@ -744,25 +817,17 @@ mod tests { #[test] fn mcp_remote_capability_derivation() { - // Codex (TOML) is HTTP-only and dsh supports neither (remote entries - // are never deployed into cordis rows; see arm below); every other + // Codex (TOML) and dsh (DshTransport) are HTTP-only; every other // adapter's remote schema supports both transports. Pinned so a // future agent with partial support must consciously extend the // derivation. for a in all_adapters() { let caps = crate::models::AgentCapabilities::from_adapter(a.as_ref()); match a.name() { - "codex" => { + "codex" | "dsh" => { assert!(caps.mcp_remote.http); assert!(!caps.mcp_remote.sse); } - "dsh" => { - // Remote schema Unsupported: HK never deploys remote - // entries into cordis patch rows (home-layer read is - // display-only for remote transports). - assert!(!caps.mcp_remote.http); - assert!(!caps.mcp_remote.sse); - } _ => { assert!(caps.mcp_remote.http, "{} should accept http", a.name()); assert!(caps.mcp_remote.sse, "{} should accept sse", a.name()); @@ -891,6 +956,15 @@ mod tests { assert_eq!(caps.project_install.cli, skill, "{name} project cli follows skill"); assert_eq!(caps.hooks_supported, hooks_supported, "{name} hooks_supported"); assert_eq!(caps.global_hook_install, global_hook, "{name} global_hook_install"); + // Only dsh surfaces its own baseline as extensions; everyone + // else compiles theirs in, so nothing is greyed out or hidden. + // A new non-empty list here changes the Extensions list for that + // agent, which should be a deliberate edit, not a surprise. + assert_eq!( + caps.vendor_baseline_packs.is_empty(), + name != "dsh", + "{name} vendor_baseline_packs" + ); } } diff --git a/crates/hk-core/src/adapter/omp.rs b/crates/hk-core/src/adapter/omp.rs index 8b1bb39f..8f9e4a65 100644 --- a/crates/hk-core/src/adapter/omp.rs +++ b/crates/hk-core/src/adapter/omp.rs @@ -246,6 +246,8 @@ impl AgentAdapter for OmpAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } else if path.is_dir() { // Directory-form extension: /index.{ts,js}, TypeScript @@ -280,6 +282,8 @@ impl AgentAdapter for OmpAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } diff --git a/crates/hk-core/src/adapter/opencode.rs b/crates/hk-core/src/adapter/opencode.rs index d30edbbe..106d5922 100644 --- a/crates/hk-core/src/adapter/opencode.rs +++ b/crates/hk-core/src/adapter/opencode.rs @@ -231,6 +231,8 @@ impl AgentAdapter for OpencodeAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], + pack: None, }); } } diff --git a/crates/hk-core/src/auditor/mod.rs b/crates/hk-core/src/auditor/mod.rs index 6492b332..5e1888d6 100644 --- a/crates/hk-core/src/auditor/mod.rs +++ b/crates/hk-core/src/auditor/mod.rs @@ -10,6 +10,14 @@ pub struct AuditInput { pub kind: crate::models::ExtensionKind, pub name: String, pub content: String, + /// Raw text of the agent config file this extension was read from, for + /// the rare rule that must see the file BEFORE parsing (dsh's `!!js` + /// tags, which YAML parsing strips). Separate from `content` on purpose: + /// a config file is shared by every entry inside it, so feeding it as + /// `content` would make every content-scanning rule (plaintext-secrets + /// and friends) report a neighbour's text on this extension. Only + /// `DshJsEnvNoFallback` reads it; empty for everything else. + pub raw_config: String, pub source: crate::models::Source, pub file_path: String, pub mcp_command: Option, @@ -72,9 +80,13 @@ impl Auditor { } pub fn audit(&self, input: &AuditInput) -> AuditResult { - // Deobfuscate content to detect hidden malicious instructions + // Deobfuscate content to detect hidden malicious instructions. + // Raw config text gets the same treatment (it is scanned by a rule + // too); deobfuscate only drops invisible characters, never newlines, + // so reported line numbers stay accurate. let clean_input = AuditInput { content: deobfuscate(&input.content), + raw_config: deobfuscate(&input.raw_config), ..input.clone() }; let mut findings = Vec::new(); @@ -173,7 +185,7 @@ mod tests { #[test] fn test_auditor_runs_all_enabled_rules() { let auditor = Auditor::new(); - assert_eq!(auditor.rules.len(), 19); + assert_eq!(auditor.rules.len(), 20); } #[test] diff --git a/crates/hk-core/src/auditor/rules.rs b/crates/hk-core/src/auditor/rules.rs index 46e34cc0..c7f9533b 100644 --- a/crates/hk-core/src/auditor/rules.rs +++ b/crates/hk-core/src/auditor/rules.rs @@ -13,9 +13,13 @@ pub use cli::{ CliAggregateRisk, CliBinarySource, CliCredentialStorage, CliNetworkAccess, CliPermissionScope, }; pub use content::{ - CredentialTheft, DangerousCommands, PlaintextSecrets, PromptInjection, RemoteCodeExecution, - SafetyBypass, SkillInvocationKeyCase, + CredentialTheft, DangerousCommands, DshJsEnvNoFallback, PlaintextSecrets, PromptInjection, + RemoteCodeExecution, SafetyBypass, SkillInvocationKeyCase, }; +/// Scanner-only: dsh drops camelCase-invocation-key skills wholesale, so the +/// scanner must not emit them for dsh. Same key vocabulary as the +/// `skill-invocation-key-case` rule, asked as a yes/no question. +pub(crate) use content::dsh_drops_skill_for_invocation_key; pub use mcp::McpCommandInjection; pub use permissions::{ BroadPermissions, PermissionCombinationRisk, SupplyChainRisk, UnknownSource, @@ -31,6 +35,7 @@ pub fn all_rules() -> Vec> { Box::new(SafetyBypass), Box::new(DangerousCommands), Box::new(SkillInvocationKeyCase), + Box::new(DshJsEnvNoFallback), Box::new(BroadPermissions), Box::new(SupplyChainRisk), Box::new(UnknownSource), diff --git a/crates/hk-core/src/auditor/rules/content.rs b/crates/hk-core/src/auditor/rules/content.rs index c0b58a10..5a59e0e5 100644 --- a/crates/hk-core/src/auditor/rules/content.rs +++ b/crates/hk-core/src/auditor/rules/content.rs @@ -400,6 +400,27 @@ const CAMELCASE_INVOCATION_KEYS: [(&str, &str); 3] = [ ("userInvocable", "user-invocable"), ]; +/// Whether dsh will DROP this skill wholesale: its frontmatter carries one of +/// the camelCase invocation-key aliases dsh rejects (it logs a warning and +/// loads nothing). Lives beside `CAMELCASE_INVOCATION_KEYS` so the rejected- +/// key vocabulary has one home — the scanner needs only this yes/no answer, +/// while the rule below needs per-key suggestions and line numbers, so they +/// deliberately ask different questions of the same list. +pub(crate) fn dsh_drops_skill_for_invocation_key(content: &str) -> bool { + // Only inspect the frontmatter block: first line `---` … next `---`. + let mut lines = content.lines(); + if lines.next().map(str::trim) != Some("---") { + return false; + } + lines + .take_while(|line| line.trim() != "---") + .any(|line| { + CAMELCASE_INVOCATION_KEYS + .iter() + .any(|(key, _)| line.trim_start().starts_with(&format!("{key}:"))) + }) +} + impl AuditRule for SkillInvocationKeyCase { fn id(&self) -> &str { "skill-invocation-key-case" @@ -442,6 +463,83 @@ impl AuditRule for SkillInvocationKeyCase { } } +/// dsh `!!js` config expressions are evaluated at plugin MOUNT time; an +/// expression that reads `process.env.X` with no fallback yields `undefined` +/// when the variable is unset, which fails mcp-client config validation and +/// kills the WHOLE dsh boot (real-machine verified on rc.6). Warn with real +/// file:line. Reads `AuditInput::raw_config`, the dedicated pre-parse field +/// that only the dsh MCP path fills; no other agent's MCP config contains +/// `!!js`, so the tag itself is the gate — no agent field needed. +/// +/// Heuristic scope (accepted): the fallback must appear within the same +/// `!!js` expression — single-line, or the deeper-indented continuation +/// lines of a block scalar (`>-` / `|`). A fallback further away is a miss. +/// Only `??`/`||` count as fallbacks — ternaries are still flagged. A `||` +/// ANYWHERE in the expression suppresses, including inside string literals +/// or YAML comments, and the check is expression-global: one fallback +/// suppresses all env reads in the same expression. Blank lines do not end +/// a block scalar here, so an expression followed by a blank line and then +/// any deeper-indented line — including a SIBLING key that YAML no longer +/// considers part of the scalar — absorbs that line's text, and a `||` +/// there suppresses a real finding. Bracket reads (`process.env["X"]`) are +/// not detected. +/// +/// Attribution: the cordis patch file is shared by every dsh MCP row, and +/// service::run_audit attaches its raw text to the FIRST dsh MCP row only — +/// the reported file:line may point into another server's block. +pub struct DshJsEnvNoFallback; + +impl AuditRule for DshJsEnvNoFallback { + fn id(&self) -> &str { + "dsh-js-env-no-fallback" + } + + fn severity(&self) -> Severity { + Severity::Medium + } + + fn check(&self, input: &AuditInput) -> Vec { + if input.kind != ExtensionKind::Mcp { + return vec![]; + } + let lines: Vec<&str> = input.raw_config.lines().collect(); + let mut findings = Vec::new(); + for (i, line) in lines.iter().enumerate() { + let Some(tag_pos) = line.find("!!js") else { continue }; + let after = &line[tag_pos + "!!js".len()..]; + let mut expr = String::from(after); + let trimmed_after = after.trim(); + if trimmed_after.starts_with('>') || trimmed_after.starts_with('|') { + // Block scalar: the expression is the following lines that + // are indented deeper than the tag line. + let indent = line.len() - line.trim_start().len(); + for cont in lines.iter().skip(i + 1) { + let cont_indent = cont.len() - cont.trim_start().len(); + if cont.trim().is_empty() || cont_indent > indent { + expr.push(' '); + expr.push_str(cont.trim()); + } else { + break; + } + } + } + if expr.contains("process.env.") && !expr.contains("??") && !expr.contains("||") { + findings.push(AuditFinding { + rule_id: self.id().into(), + severity: self.severity(), + message: format!( + "`!!js` expression reads process.env without a `??`/`||` fallback — \ + if the variable is unset at mount time the whole dsh boot fails: {}", + line.trim() + ), + location: format!("{}:{}", input.file_path, i + 1), + }); + } + } + findings + } +} + #[cfg(test)] mod tests { use super::*; @@ -650,6 +748,22 @@ mod tests { assert!(rule.check(&skill_input(body_mention)).is_empty()); } + #[test] + fn dsh_drops_skill_for_invocation_key_matches_the_rule_frontmatter_scan() { + assert!(dsh_drops_skill_for_invocation_key( + "---\nname: x\nuserInvocable: true\n---\nbody\n" + )); + // Frontmatter-only scan: clean frontmatter and no-frontmatter files hit nothing. + assert!(!dsh_drops_skill_for_invocation_key( + "---\nname: x\n---\nuserInvocable: true\n" + )); + assert!(!dsh_drops_skill_for_invocation_key("no frontmatter here")); + // Agrees with the audit rule on every case above — one vocabulary. + let flagged = "---\nname: x\ndisableModelInvocation: true\n---\n"; + assert!(dsh_drops_skill_for_invocation_key(flagged)); + assert!(!SkillInvocationKeyCase.check(&skill_input(flagged)).is_empty()); + } + #[test] fn test_skill_with_cli_parent_still_audited() { let rule = PromptInjection; @@ -660,4 +774,63 @@ mod tests { "CLI child skill should still be audited" ); } + + /// A dsh MCP row: the patch text rides `raw_config`, never `content` — + /// mirroring service::audit_extensions, so this helper also pins that + /// no other content rule can see the shared file. + fn dsh_mcp_input(patch_text: &str) -> AuditInput { + let mut input = mcp_input("npx", vec![], vec![]); + input.raw_config = patch_text.into(); + input.file_path = "/home/u/.dsh/cordis.patch.yml".into(); + input + } + + #[test] + fn dsh_js_env_no_fallback_flags_bare_env_reads_with_file_line() { + let rule = DshJsEnvNoFallback; + let content = "- insert:\n - id: mcp-github\n config:\n env:\n GITHUB_TOKEN: !!js process.env.GITHUB_TOKEN\n"; + let findings = rule.check(&dsh_mcp_input(content)); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0].severity, Severity::Medium); + assert_eq!(findings[0].location, "/home/u/.dsh/cordis.patch.yml:5"); + } + + #[test] + fn dsh_js_env_no_fallback_accepts_fallbacks_and_block_scalars() { + let rule = DshJsEnvNoFallback; + // Same-line fallbacks. + assert!(rule + .check(&dsh_mcp_input("k: !!js process.env.X ?? 'default'\n")) + .is_empty()); + assert!(rule + .check(&dsh_mcp_input("k: !!js process.env.X || fallback()\n")) + .is_empty()); + // Block-scalar form with the fallback on a continuation line + // (copied from dsh's own mcp-memory example). + let block = " MEMORY_FILE_PATH: !!js >-\n process.env.MEMORY_FILE_PATH?.trim() ||\n fallback()\n"; + assert!(rule.check(&dsh_mcp_input(block)).is_empty()); + // Block-scalar WITHOUT a fallback is flagged at the tag line. + let bad_block = " T: !!js >-\n process.env.T\n"; + let findings = rule.check(&dsh_mcp_input(bad_block)); + assert_eq!(findings.len(), 1); + assert!(findings[0].location.ends_with(":1")); + // Non-env !!js expressions are fine. + assert!(rule.check(&dsh_mcp_input("k: !!js process.cwd()\n")).is_empty()); + // Non-MCP inputs are out of scope. + let mut skill = dsh_mcp_input("k: !!js process.env.X\n"); + skill.kind = ExtensionKind::Skill; + assert!(rule.check(&skill).is_empty()); + } + + #[test] + fn raw_config_is_invisible_to_content_scanning_rules() { + // The whole point of the dedicated field: the cordis patch file is + // shared by every dsh MCP row, so a NEIGHBOUR server's token in it + // must not be reported on this row (and this row's own env secret + // must not be reported twice, once from env and once from the file). + let input = dsh_mcp_input( + " env:\n NEIGHBOUR: ghp_abc123def456ghi789jkl012mno345pqr678\n", + ); + assert!(PlaintextSecrets.check(&input).is_empty()); + } } diff --git a/crates/hk-core/src/auditor/rules/test_support.rs b/crates/hk-core/src/auditor/rules/test_support.rs index 50dade38..989f5388 100644 --- a/crates/hk-core/src/auditor/rules/test_support.rs +++ b/crates/hk-core/src/auditor/rules/test_support.rs @@ -7,6 +7,7 @@ pub(super) fn skill_input(content: &str) -> AuditInput { kind: ExtensionKind::Skill, name: "test-skill".into(), content: content.into(), + raw_config: String::new(), source: Source { origin: SourceOrigin::Local, url: None, @@ -34,6 +35,7 @@ pub(super) fn mcp_input(command: &str, args: Vec<&str>, env: Vec<(&str, &str)>) kind: ExtensionKind::Mcp, name: "test-mcp".into(), content: String::new(), + raw_config: String::new(), source: Source { origin: SourceOrigin::Local, url: None, diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index ffb1b3c8..d30f0954 100644 --- a/crates/hk-core/src/deployer.rs +++ b/crates/hk-core/src/deployer.rs @@ -2,7 +2,7 @@ use crate::HkError; use crate::adapter::{HookEntry, HookFormat, McpFormat, McpServerEntry, McpTransport, RemoteMcpSchema}; use fs2::FileExt; use std::io::{Read as _, Seek as _, SeekFrom, Write as _}; -use std::path::Path; +use std::path::{Path, PathBuf}; pub fn deploy_skill(source_path: &Path, target_skill_dir: &Path) -> Result { std::fs::create_dir_all(target_skill_dir)?; @@ -159,7 +159,9 @@ fn json_top_key(format: McpFormat) -> &'static str { } McpFormat::DshCordis => { unreachable!( - "DshCordis never reaches the JSON writers — install/remove are refused, \ + "DshCordis never reaches the JSON writers — install/remove \ + route through the dedicated cordis writers \ + (deploy_mcp_server_dsh_cordis / remove_mcp_server_dsh_cordis); \ toggling uses the native patch-layer path (set_dsh_mcp_enabled)" ) } @@ -190,12 +192,7 @@ pub fn deploy_mcp_server( McpFormat::Toml => deploy_mcp_server_toml(config_path, entry), McpFormat::Opencode => deploy_mcp_server_opencode(config_path, entry), McpFormat::HermesYaml => deploy_mcp_server_hermes_yaml(config_path, entry), - McpFormat::DshCordis => Err(HkError::Validation( - "dsh MCP servers are composition rows in cordis.patch.yml; \ - installing or removing them for dsh is not supported yet — \ - use enable/disable (native patch-layer path) or edit the file" - .into(), - )), + McpFormat::DshCordis => deploy_mcp_server_dsh_cordis(config_path, entry), } } @@ -215,7 +212,9 @@ fn validate_remote_mcp_target( RemoteMcpSchema::Unsupported => Err(HkError::Validation(format!( "{agent_name} does not support remote (HTTP/SSE) MCP servers" ))), - RemoteMcpSchema::Toml if entry.transport == McpTransport::Sse => { + RemoteMcpSchema::Toml | RemoteMcpSchema::DshTransport + if entry.transport == McpTransport::Sse => + { Err(HkError::Validation(format!( "{agent_name} supports Streamable HTTP MCP servers only, not SSE" ))) @@ -269,6 +268,7 @@ fn build_mcp_json_value( RemoteMcpSchema::Toml | RemoteMcpSchema::OpencodeRemote | RemoteMcpSchema::HermesUrl + | RemoteMcpSchema::DshTransport | RemoteMcpSchema::Unsupported => { return Err(HkError::Internal(format!( "remote JSON value requested for non-JSON schema {remote:?}" @@ -663,6 +663,67 @@ pub fn set_omp_mcp_enabled( const DSH_BLOCK_BEGIN: &str = "# >>> managed by HarnessKit — do not edit this block >>>"; const DSH_BLOCK_END: &str = "# <<< managed by HarnessKit <<<"; +/// Structured model of the HK-owned managed block at the end of the home +/// `cordis.patch.yml`. ONE engine serves the MCP toggle, the MCP insert +/// writer, and the plugin toggle — no second block format, no second +/// marker pair. +#[derive(Debug, Default)] +struct DshManagedBlock { + /// Id-targeted `{id, disabled}` override entries. BTreeMap keeps the + /// render order deterministic (sorted by row id). + toggles: std::collections::BTreeMap, + /// Full HK-authored insert ROWS in insertion order. Each renders as its + /// own `- insert:` group holding exactly one row mapping, and each row + /// is `{id, name, config}` (+ optional `disabled`) per the mcp-client + /// schema. Toggling an HK-inserted server edits the `disabled` field of + /// its own row — never a separate override entry. + inserts: Vec, +} + +impl DshManagedBlock { + fn is_empty(&self) -> bool { + self.toggles.is_empty() && self.inserts.is_empty() + } + + /// serverName of an insert row, but ONLY for mcp-client plugin rows — + /// the same gate the reader applies, so every block-side matcher + /// (find/remove/list) agrees with the reader's definition of an MCP row + /// and can never match a non-MCP plugin insert. + fn insert_server_name(row: &serde_yaml::Mapping) -> Option<&str> { + if row.get("name")?.as_str()? != crate::adapter::dsh::MCP_CLIENT_PLUGIN { + return None; + } + row.get("config")?.get("serverName")?.as_str() + } + + fn find_insert_mut(&mut self, server_name: &str) -> Option<&mut serde_yaml::Mapping> { + self.inserts + .iter_mut() + .find(|row| Self::insert_server_name(row) == Some(server_name)) + } + + fn remove_insert(&mut self, server_name: &str) -> bool { + let before = self.inserts.len(); + self.inserts + .retain(|row| Self::insert_server_name(row) != Some(server_name)); + self.inserts.len() != before + } + + fn insert_row_ids(&self) -> Vec { + self.inserts + .iter() + .filter_map(|row| row.get("id").and_then(|v| v.as_str()).map(String::from)) + .collect() + } + + fn insert_server_names(&self) -> Vec { + self.inserts + .iter() + .filter_map(|row| Self::insert_server_name(row).map(String::from)) + .collect() + } +} + /// Flip a dsh MCP server via the official patch-layer mechanism: an /// id-targeted `disabled:` override inside an HK-owned marked block at the /// END of the home-level `cordis.patch.yml` (the last always-applied user @@ -683,18 +744,21 @@ pub fn set_dsh_mcp_enabled( ) -> Result<(), HkError> { use crate::adapter::dsh::DshAdapter; - let original = match std::fs::read_to_string(home_patch) { - Ok(text) => text, - // Absent file = dsh not yet seeded its template; start from the - // valid empty form. Any other IO error must surface, not be - // mistaken for an empty file. - // Intentional error normalization: this text never reaches the write - // path — an empty patch has no rows, so the row-id lookup below fails - // first and the user sees "server not found" rather than a raw IO error. - Err(e) if e.kind() == std::io::ErrorKind::NotFound => "[]\n".to_string(), - Err(e) => return Err(e.into()), - }; - let (user_text, mut managed) = split_dsh_managed_block(&original)?; + // The install writer stores the SANITIZED serverName; match it on lookup. + let server_name = &normalize_dsh_server_name(server_name); + + let (user_text, mut block) = read_and_split_home_patch(home_patch)?; + + // An HK-inserted server (Task-8 install writer) is toggled by editing the + // `disabled` field of its OWN insert row — no separate override entry. + if let Some(row) = block.find_insert_mut(server_name) { + if enabled { + row.remove("disabled"); + } else { + row.insert(serde_yaml::Value::from("disabled"), serde_yaml::Value::from(true)); + } + return write_dsh_patch(home_patch, &user_text, &block); + } let row_id = DshAdapter::mcp_row_id_in_text(&user_text, server_name).ok_or_else(|| { HkError::NotFound(format!( @@ -710,38 +774,342 @@ pub fn set_dsh_mcp_enabled( .unwrap_or(true); if base_enabled == enabled { - managed.remove(&row_id); + block.toggles.remove(&row_id); } else { - managed.insert(row_id, !enabled); // value = disabled flag + block.toggles.insert(row_id, !enabled); // value = disabled flag } - let new_text = render_dsh_patch(&user_text, &managed); - let parsed: Result = serde_yaml::from_str(&new_text); - if !matches!(parsed, Ok(serde_yaml::Value::Sequence(_))) { - return Err(HkError::ConfigCorrupted(format!( - "refusing to write {}: edited content is not a YAML list", + write_dsh_patch(home_patch, &user_text, &block) +} + +/// Flip a dsh PLUGIN row via the same official patch-layer mechanism as the +/// MCP toggle: an id-targeted `disabled:` override inside the HK block at +/// the end of the home `cordis.patch.yml`. The home layer is applied after +/// every profile's layer, so the WRITE is machine-global — the one override +/// affects that row id in every profile that contains it (upstream +/// precedent: dsh's own web-app bundle disables base rows exactly this way; +/// hot-reload applies it live). Accepted side effect: disabling a row that +/// exists only in profile A leaves the override "dangling" from profile B's +/// perspective — dsh warn-skips it per boot; upstream cosmetic noise, not +/// surfaced by HK. +/// +/// The BASE state, by contrast, is per-profile: dsh boots ONE profile at a +/// time and composes only the layers of THAT profile plus the home patch +/// (upstream composeProfile), so a sibling profile's file is never loaded +/// alongside it and must never be folded in. `base_layers` is that profile's +/// ordered chain below the home patch — each mounted bundle's own patch file +/// in `bundles` order, then the profile's `cordis.patch.yml` — exactly what +/// the UI row carried as `PluginEntry::base_layers`. The fold is +/// `base_layers ++ [home user text]`, our own managed block stripped. +/// +/// The chain, not just the defining layer: `hmr` is DEFINED by +/// `@deepseek-ai/dsh-base` (enabled) and DISABLED by +/// `@deepseek-ai/dsh-web-app` two layers later. Folding only the definition +/// would read `hmr` as enabled, so "enable" would match the base state, drop +/// the override, and silently leave the row disabled. +/// +/// Bundle patch files are read-only inputs here — HK never writes any layer +/// but the home patch, and never `/cordis.yml` (dsh overwrites +/// that on boot). +/// +/// `home_patch` is taken as a path, not derived from a dsh home, so that it +/// is the SAME value the adapter hands out in `base_layers` — the +/// `layer == home_patch` test below must compare like with like. A re-derived +/// path (trailing slash, symlinked `$DSH_HOME`) would compare unequal for a +/// home-defined row, send us down the re-read branch, and fold HK's own +/// managed block back in as base state — every toggle a silent no-op. +pub fn set_dsh_plugin_enabled( + home_patch: &Path, + row_id: &str, + enabled: bool, + base_layers: &[PathBuf], +) -> Result<(), HkError> { + use crate::adapter::dsh::DshAdapter; + + let (user_text, mut block) = read_and_split_home_patch(home_patch)?; + + // Fold every layer below the home patch, then the home user text. The + // home patch is read through read_and_split_home_patch above (block + // stripped), so never re-read it here: folding our own managed block + // back in would make every toggle look like the base state. + let mut texts: Vec = base_layers + .iter() + .filter(|layer| layer.as_path() != home_patch) + .map(|layer| std::fs::read_to_string(layer).unwrap_or_default()) + .collect(); + texts.push(user_text.clone()); + let mut defined = false; + let mut disabled_state: Option = None; + for text in &texts { + let (layer_defined, layer_state) = DshAdapter::plugin_row_state_in_text(text, row_id); + defined |= layer_defined; + if let Some(d) = layer_state { + disabled_state = Some(d); + } + } + if !defined { + let layers = base_layers + .iter() + .map(|p| p.display().to_string()) + .collect::>() + .join(", "); + return Err(HkError::NotFound(format!( + "plugin row '{row_id}' is not defined by any of [{layers}] or {}", home_patch.display() ))); } - atomic_write(home_patch, &new_text) + let base_enabled = !disabled_state.unwrap_or(false); + + if base_enabled == enabled { + block.toggles.remove(row_id); + } else { + block.toggles.insert(row_id.to_string(), !enabled); + } + write_dsh_patch(home_patch, &user_text, &block) +} + +/// serverName must satisfy mcp-client's `/^[A-Za-z0-9_-]{1,32}$/` +/// (source-verified: packages/mcp/mcp-client/src/index.ts). Map every other +/// char to `-`, cap at 32; a name with no valid alphanumeric at all errors. +fn sanitize_dsh_server_name(name: &str) -> Result { + let cleaned: String = name + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '_' || c == '-' { + c + } else { + '-' + } + }) + .take(32) + .collect(); + if !cleaned.chars().any(|c| c.is_ascii_alphanumeric()) { + return Err(HkError::Validation(format!( + "cannot derive a valid dsh serverName from '{name}' \ + (needs at least one of A-Za-z0-9; pattern [A-Za-z0-9_-], max 32 chars)" + ))); + } + Ok(cleaned) +} + +/// Lookup-side normalization: identity for valid names; an unsanitizable +/// name is kept raw — it can't match any written row, so callers fall +/// through to "not found"/"no conflict". The install writer deliberately +/// does NOT use this (it must error on unsanitizable input). +pub(crate) fn normalize_dsh_server_name(name: &str) -> String { + sanitize_dsh_server_name(name).unwrap_or_else(|_| name.to_string()) +} + +/// One full mcp-client insert row per the source-verified Config schema: +/// always an explicit `transport` discriminant and `serverName`; optional +/// keys only when non-empty (the schema defaults them). This is the ONLY +/// site that builds insert rows, so the key order (id, name, config) is +/// fixed here once (serde_yaml Mapping preserves insertion order — the +/// rendered byte format depends on it); env/header keys are sorted for the +/// same determinism. +/// +/// Name round-trip: `serverName` must match mcp-client's +/// `/^[A-Za-z0-9_-]{1,32}$/`, so `microsoft/markitdown` is stored as +/// `microsoft-markitdown`. When sanitizing changed the name, the ORIGINAL is +/// recorded as `_hk_name` right after it so the reader can hand the scanner +/// the unsanitized name (`adapter::dsh::mcp_entries_in_text`). Without it the +/// scanner reads the row back under a different name and HK models it as a +/// SECOND extension — the ghost-duplicate-row bug. Same conditional as +/// Codex's `upsert_mcp_server_toml`: written ONLY when the name changed, so +/// already-valid names keep their exact previous bytes. +fn build_dsh_insert_row( + row_id: &str, + server_name: &str, + entry: &McpServerEntry, +) -> serde_yaml::Mapping { + use crate::adapter::dsh::HK_NAME_CONFIG_KEY; + use serde_yaml::{Mapping, Value}; + // Placed immediately after `serverName` in both transport branches — the + // key it qualifies. + let insert_hk_name = |config: &mut Mapping| { + if server_name != entry.name { + config.insert( + Value::from(HK_NAME_CONFIG_KEY), + Value::from(entry.name.clone()), + ); + } + }; + let mut config = Mapping::new(); + if entry.transport == McpTransport::Stdio { + config.insert(Value::from("transport"), Value::from("stdio")); + config.insert(Value::from("serverName"), Value::from(server_name)); + insert_hk_name(&mut config); + config.insert(Value::from("command"), Value::from(entry.command.clone())); + if !entry.args.is_empty() { + config.insert( + Value::from("args"), + Value::Sequence(entry.args.iter().map(|a| Value::from(a.clone())).collect()), + ); + } + if !entry.env.is_empty() { + let mut env = Mapping::new(); + let mut keys: Vec<&String> = entry.env.keys().collect(); + keys.sort(); + for k in keys { + env.insert(Value::from(k.clone()), Value::from(entry.env[k].clone())); + } + config.insert(Value::from("env"), Value::Mapping(env)); + } + } else { + // Both Http and (schema-rejected upstream of this fn) Sse spell the + // written transport as streamable-http — dsh ships no SSE transport, + // and validate_remote_mcp_target refuses Sse before this point. + config.insert(Value::from("transport"), Value::from("streamable-http")); + config.insert(Value::from("serverName"), Value::from(server_name)); + insert_hk_name(&mut config); + config.insert( + Value::from("url"), + Value::from(entry.url.clone().unwrap_or_default()), + ); + if !entry.headers.is_empty() { + let mut headers = Mapping::new(); + let mut keys: Vec<&String> = entry.headers.keys().collect(); + keys.sort(); + for k in keys { + headers.insert(Value::from(k.clone()), Value::from(entry.headers[k].clone())); + } + config.insert(Value::from("headers"), Value::Mapping(headers)); + } + } + let mut row = Mapping::new(); + row.insert(Value::from("id"), Value::from(row_id)); + row.insert( + Value::from("name"), + Value::from(crate::adapter::dsh::MCP_CLIENT_PLUGIN), + ); + row.insert(Value::from("config"), Value::Mapping(config)); + row +} + +/// dsh MCP install: append a full `insert:` row (an mcp-client plugin row) +/// inside the HK managed block of the home `cordis.patch.yml`. Global scope +/// only — `mcp_config_path_for(Project)` stays `None` for dsh. User text +/// outside the block is byte-preserved, exactly as in the P0 toggle. +fn deploy_mcp_server_dsh_cordis( + config_path: &Path, + entry: &McpServerEntry, +) -> Result<(), HkError> { + use crate::adapter::dsh::DshAdapter; + + let (user_text, mut block) = read_and_split_home_patch(config_path)?; + + let server_name = sanitize_dsh_server_name(&entry.name)?; + // Collision domain is the STORED `serverName`, so re-installing the same + // ORIGINAL name sanitizes to the same key and is caught here — one row, + // never a silent second one (the `_hk_name` round-trip only affects what + // the READER reports, never how rows are keyed). + if DshAdapter::mcp_enabled_in_text(&user_text).contains_key(&server_name) + || block.insert_server_names().contains(&server_name) + { + // Name the original input too when sanitizing changed it — the + // caller may otherwise not recognize the colliding name as theirs. + let from = if server_name == entry.name { + String::new() + } else { + format!(" (from '{}')", entry.name) + }; + return Err(HkError::Validation(format!( + "dsh already has an MCP server named '{server_name}'{from} in {}", + config_path.display() + ))); + } + // Generated row id: mcp-, kebab. Collision with ANY existing + // row id is an error — a duplicate id would make dsh treat the second + // definition as a malformed collision. The id namespace spans every + // layer dsh composes, not just this file: profile patches are applied + // BEFORE the home patch, so a profile row with the same id collides just + // as hard. Checked here, in the ONE place that generates ids. + let row_id = format!("mcp-{}", server_name.to_lowercase().replace('_', "-")); + let profile_row_ids: std::collections::HashSet = config_path + .parent() + .map(DshAdapter::profile_patch_texts) + .unwrap_or_default() + .iter() + .flat_map(|text| DshAdapter::row_ids_in_text(text)) + .collect(); + if DshAdapter::row_ids_in_text(&user_text).contains(&row_id) + || block.toggles.contains_key(&row_id) + || block.insert_row_ids().contains(&row_id) + || profile_row_ids.contains(&row_id) + { + return Err(HkError::Validation(format!( + "row id '{row_id}' already exists in the dsh patch layers of {} — \ + rename the server or the existing row", + config_path.display() + ))); + } + block.inserts.push(build_dsh_insert_row(&row_id, &server_name, entry)); + write_dsh_patch(config_path, &user_text, &block) } -/// Split file text into (user text without the managed block, managed -/// entries id → disabled). Unrecognized lines inside the block are dropped — -/// the block is HK-owned by contract. `split_inclusive` keeps user lines -/// byte-exact (including CRLF endings). +/// dsh MCP removal: HK-inserted rows (inside the managed block) are removed; +/// user-authored rows keep the Validation refusal — HK never rewrites user +/// YAML. An absent name is a no-op, matching every other format. +fn remove_mcp_server_dsh_cordis(config_path: &Path, server_name: &str) -> Result<(), HkError> { + use crate::adapter::dsh::DshAdapter; + // The block stores the SANITIZED serverName; removal must map to it. + let server_name = &normalize_dsh_server_name(server_name); + let (user_text, mut block) = read_and_split_home_patch(config_path)?; + if block.remove_insert(server_name) { + return write_dsh_patch(config_path, &user_text, &block); + } + if DshAdapter::mcp_enabled_in_text(&user_text).contains_key(server_name) { + return Err(HkError::Validation(format!( + "'{server_name}' is a user-authored row in cordis.patch.yml; \ + HarnessKit never rewrites user YAML — remove the row in the file itself" + ))); + } + Ok(()) +} + +/// Shared prologue of every dsh home-patch writer: read the file, split off +/// the HK managed block, and name the file in any block-corruption error. +/// +/// - Absent file = dsh has not seeded its template yet; start from the valid +/// empty form (`[]`). Any other IO error must surface, not be mistaken for +/// an empty file. The toggles never write this synthesized text — an empty +/// patch has no rows, so their row lookup fails first with "not found" — +/// while the install writer proceeds and creates the file, which is exactly +/// the desired first-install behavior. The remove writer relies on the +/// same synthesis for its idempotent no-op: an absent file has no rows, so +/// removal finds nothing and returns Ok instead of an IO error. +/// - The block parser is path-agnostic, so this call site owns naming the +/// file and the remediation hint on `ConfigCorrupted`. +fn read_and_split_home_patch(home_patch: &Path) -> Result<(String, DshManagedBlock), HkError> { + let original = match std::fs::read_to_string(home_patch) { + Ok(text) => text, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => "[]\n".to_string(), + Err(e) => return Err(e.into()), + }; + split_dsh_managed_block(&original).map_err(|e| match e { + HkError::ConfigCorrupted(msg) => HkError::ConfigCorrupted(format!( + "{msg} (in {}; fix or remove the content between the \ + '>>> managed by HarnessKit' markers)", + home_patch.display() + )), + other => other, + }) +} + +/// Split file text into (user text without the managed block, structured +/// block model). The block body is parsed as YAML: HK owns every byte +/// inside the markers, so content it would not itself render is a hard +/// `ConfigCorrupted` — refusing to write beats silently discarding block +/// entries. `split_inclusive` keeps user lines byte-exact (including CRLF). /// /// Unbalanced markers are a hard `ConfigCorrupted` error: a BEGIN without a /// matching END would otherwise swallow every user line to EOF (and the /// rewritten file could still parse as a valid YAML sequence, so the /// caller's post-edit guard would not catch the loss). -fn split_dsh_managed_block( - text: &str, -) -> Result<(String, std::collections::BTreeMap), HkError> { +fn split_dsh_managed_block(text: &str) -> Result<(String, DshManagedBlock), HkError> { let mut user = String::new(); - let mut managed = std::collections::BTreeMap::new(); + let mut body = String::new(); let mut in_block = false; - let mut current_id: Option = None; for raw in text.split_inclusive('\n') { let line = raw.trim(); if line == DSH_BLOCK_BEGIN { @@ -764,17 +1132,10 @@ fn split_dsh_managed_block( )); } in_block = false; - current_id = None; continue; } if in_block { - if let Some(id) = line.strip_prefix("- id: ") { - current_id = Some(id.trim().to_string()); - } else if let Some(flag) = line.strip_prefix("disabled: ") { - if let (Some(id), Ok(b)) = (current_id.take(), flag.trim().parse::()) { - managed.insert(id, b); - } - } + body.push_str(raw); } else { user.push_str(raw); } @@ -786,19 +1147,94 @@ fn split_dsh_managed_block( .into(), )); } - Ok((user, managed)) + Ok((user, parse_dsh_block_body(&body)?)) } -/// Reassemble user text + managed block. Structural rules: +/// Parse the marker-stripped block body into the structured model. +fn parse_dsh_block_body(body: &str) -> Result { + let mut block = DshManagedBlock::default(); + if body.trim().is_empty() { + return Ok(block); + } + let corrupted = |detail: String| { + HkError::ConfigCorrupted(format!( + "HarnessKit managed block in cordis.patch.yml is not valid: {detail}" + )) + }; + let doc: serde_yaml::Value = + serde_yaml::from_str(body).map_err(|e| corrupted(e.to_string()))?; + let Some(items) = doc.as_sequence() else { + return Err(corrupted("block body is not a YAML list".into())); + }; + // Entries must carry EXACTLY the keys HK itself renders. Extra keys in an + // id-targeted entry are LIVE dsh patch semantics (they would patch the + // target row), so silently dropping them on re-render would alter the + // user's effective config — hard error instead. + let extra_keys = |map: &serde_yaml::Mapping, allowed: &[&str]| -> String { + map.keys() + .map(|k| k.as_str().unwrap_or("").to_string()) + .filter(|k| !allowed.contains(&k.as_str())) + .collect::>() + .join(", ") + }; + for item in items { + let Some(map) = item.as_mapping() else { + return Err(corrupted("block entry is not a mapping".into())); + }; + if let Some(rows) = map.get("insert").and_then(|v| v.as_sequence()) { + if map.len() != 1 { + return Err(corrupted(format!( + "insert group has keys besides insert: {}", + extra_keys(map, &["insert"]) + ))); + } + for row in rows { + let Some(rm) = row.as_mapping() else { + return Err(corrupted("insert row is not a mapping".into())); + }; + block.inserts.push(rm.clone()); + } + continue; + } + // `as_bool()` rejects `disabled: null`, which the READER + // (adapter::dsh::yaml_disabled) accepts as `false` per upstream: HK + // owns every byte between the markers and only ever writes literal + // booleans, so a null in here means the block was hand-edited or + // corrupted — refuse it rather than guess. + match ( + map.get("id").and_then(|v| v.as_str()), + map.get("disabled").and_then(|v| v.as_bool()), + ) { + (Some(id), Some(disabled)) => { + if map.len() != 2 { + return Err(corrupted(format!( + "toggle entry has keys besides id/disabled: {}", + extra_keys(map, &["id", "disabled"]) + ))); + } + block.toggles.insert(id.to_string(), disabled); + } + _ => { + return Err(corrupted( + "block entry is neither an {id, disabled} toggle nor an insert group".into(), + )) + } + } + } + Ok(block) +} + +/// Reassemble user text + managed block. Structural rules (unchanged from P0): /// - Block present → any lone `[]` placeholder line is dropped (it can't /// coexist with block-style entries in one document). /// - Block absent → if the remaining text has no non-comment content, append /// `[]` (an empty/comment-only patch file is a dsh boot error). -fn render_dsh_patch( - user_text: &str, - managed: &std::collections::BTreeMap, -) -> String { - if managed.is_empty() { +/// +/// Rendering is deterministic: toggles sorted by id (BTreeMap) in the P0 +/// byte format, then insert groups in insertion order with fixed key order +/// (serde_yaml Mapping preserves insertion order). +fn render_dsh_patch(user_text: &str, block: &DshManagedBlock) -> String { + if block.is_empty() { let has_content = user_text .lines() .any(|l| !l.trim().is_empty() && !l.trim().starts_with('#') && l.trim() != "[]"); @@ -814,14 +1250,22 @@ fn render_dsh_patch( return out; } - let mut block = String::new(); - block.push_str(DSH_BLOCK_BEGIN); - block.push('\n'); - for (id, disabled) in managed { - block.push_str(&format!("- id: {id}\n disabled: {disabled}\n")); + let mut body = String::new(); + for (id, disabled) in &block.toggles { + body.push_str(&format!("- id: {id}\n disabled: {disabled}\n")); + } + for row in &block.inserts { + let mut group = serde_yaml::Mapping::new(); + group.insert( + serde_yaml::Value::from("insert"), + serde_yaml::Value::Sequence(vec![serde_yaml::Value::Mapping(row.clone())]), + ); + let rendered = serde_yaml::to_string(&serde_yaml::Value::Sequence(vec![ + serde_yaml::Value::Mapping(group), + ])) + .expect("HK-built YAML mapping always serializes"); + body.push_str(&rendered); } - block.push_str(DSH_BLOCK_END); - block.push('\n'); // Drop `[]` placeholder lines byte-preservingly (keep every other raw line). let mut out = String::new(); @@ -833,10 +1277,34 @@ fn render_dsh_patch( if !out.is_empty() && !out.ends_with('\n') { out.push('\n'); } - out.push_str(&block); + out.push_str(DSH_BLOCK_BEGIN); + out.push('\n'); + out.push_str(&body); + out.push_str(DSH_BLOCK_END); + out.push('\n'); out } +/// Re-parse guard + atomic write shared by every dsh patch writer: the +/// edited text must stay a valid top-level YAML sequence, else nothing is +/// written (a broken file would make dsh keep last-good config and silently +/// ignore all future edits). +fn write_dsh_patch( + path: &Path, + user_text: &str, + block: &DshManagedBlock, +) -> Result<(), HkError> { + let new_text = render_dsh_patch(user_text, block); + let parsed: Result = serde_yaml::from_str(&new_text); + if !matches!(parsed, Ok(serde_yaml::Value::Sequence(_))) { + return Err(HkError::ConfigCorrupted(format!( + "refusing to write {}: edited content is not a YAML list", + path.display() + ))); + } + atomic_write(path, &new_text) +} + /// Flip a Kiro IDE hook's native `enabled` flag in place, keeping the entry /// in the file — mirrors Kiro's own panel toggle ("skip without deleting"). pub fn set_kiro_hook_enabled( @@ -1256,12 +1724,7 @@ pub fn remove_mcp_server( } Ok(()) }), - McpFormat::DshCordis => Err(HkError::Validation( - "dsh MCP servers are composition rows in cordis.patch.yml; \ - installing or removing them for dsh is not supported yet — \ - use enable/disable (native patch-layer path) or edit the file" - .into(), - )), + McpFormat::DshCordis => remove_mcp_server_dsh_cordis(config_path, server_name), _ => locked_modify_json(config_path, |config| { let key = json_top_key(format); if let Some(servers) = config.get_mut(key).and_then(|v| v.as_object_mut()) { @@ -2286,6 +2749,12 @@ mod tests { assert!(matches!(&err, HkError::Validation(m) if m.contains("not SSE"))); validate_remote_mcp_target(&http, "codex", RemoteMcpSchema::Toml).unwrap(); + // dsh (DshTransport) is HTTP-only too. + let err = + validate_remote_mcp_target(&sse, "dsh", RemoteMcpSchema::DshTransport).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("not SSE"))); + assert!(validate_remote_mcp_target(&http, "dsh", RemoteMcpSchema::DshTransport).is_ok()); + // Remote without url is corrupt regardless of target. let mut broken = remote_entry(McpTransport::Http); broken.url = None; @@ -4258,6 +4727,69 @@ mod dsh_toggle_tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), text); } + #[test] + fn malformed_block_content_errors_and_leaves_the_file_untouched() { + // HK owns every byte inside the markers, so anything it could not have + // rendered is corruption: refuse to write rather than re-render the + // block without it. Each case is a DIFFERENT way that can happen. + for (case, body) in [ + // Unparseable YAML. + ("unclosed flow sequence", "- id: [unclosed\n"), + // Parses, but HK never renders an entry shaped like this — a + // silent drop here would delete whatever the user meant by it. + ("entry HK never renders", "- surprise: true\n"), + // Extra keys on an id-targeted entry are LIVE dsh patch semantics + // (they patch the target row), so dropping them on re-render would + // alter the user's effective config. + ( + "extra keys on a toggle entry", + "- id: mcp-github\n disabled: true\n command: pwned\n", + ), + ( + "extra keys beside an insert group", + "- insert:\n - id: mcp-x\n after: mcp-github\n", + ), + ] { + let text = format!("{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n{body}{DSH_BLOCK_END}\n"); + let (_tmp, path) = patch_file(&text); + let err = set_dsh_mcp_enabled(&path, "github", false).unwrap_err(); + assert!(matches!(err, HkError::ConfigCorrupted(_)), "{case}: {err:?}"); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + text, + "{case}: file must be untouched" + ); + } + } + + #[test] + fn hk_inserted_server_toggles_via_own_row_and_render_is_byte_stable() { + // Pins the serde_yaml insert byte format BEFORE T8 depends on it. + let text = format!( + "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- insert:\n - id: mcp-web\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: web\n transport: streamable-http\n url: http://localhost:3000/mcp\n{DSH_BLOCK_END}\n" + ); + let (_tmp, path) = patch_file(&text); + set_dsh_mcp_enabled(&path, "web", false).unwrap(); + let out = std::fs::read_to_string(&path).unwrap(); + assert!(out.starts_with(HOME_WITH_GH), "user bytes preserved"); + + // Disable lands on the insert row's OWN `disabled` field — never a + // separate toggle entry. + let (user_text, block) = split_dsh_managed_block(&out).unwrap(); + assert!(block.toggles.is_empty(), "no separate toggle entry"); + assert_eq!(block.inserts.len(), 1); + assert_eq!( + block.inserts[0].get("disabled").and_then(|v| v.as_bool()), + Some(true) + ); + + let parsed: serde_yaml::Value = serde_yaml::from_str(&out).unwrap(); + assert!(parsed.is_sequence(), "file must stay a valid YAML list"); + + // Byte-stable: a second split→render reproduces the file exactly. + assert_eq!(render_dsh_patch(&user_text, &block), out); + } + #[test] fn user_content_after_block_survives_roundtrip() { // Documented out-vote mechanism: user lines AFTER the managed block @@ -4289,14 +4821,644 @@ mod dsh_toggle_tests { let parsed: serde_yaml::Value = serde_yaml::from_str(&out).unwrap(); assert!(parsed.is_sequence()); } +} + +#[cfg(test)] +mod dsh_plugin_toggle_tests { + use super::*; + + const PROFILE_PATCH: &str = + "- insert:\n - id: tool-policy\n name: dsh-plugin-tool\n config:\n mode: strict\n"; + + fn dsh_home_with_profile( + patch: &str, + home_patch: Option<&str>, + ) -> (tempfile::TempDir, std::path::PathBuf) { + let tmp = tempfile::tempdir().unwrap(); + let dsh_home = tmp.path().join(".dsh"); + std::fs::create_dir_all(dsh_home.join("profiles/web")).unwrap(); + std::fs::write(dsh_home.join("profiles/web/cordis.patch.yml"), patch).unwrap(); + if let Some(h) = home_patch { + std::fs::write(dsh_home.join("cordis.patch.yml"), h).unwrap(); + } + (tmp, dsh_home) + } + + /// The layer that DEFINES the row in the `dsh_home_with_profile` fixture + /// — what the dsh adapter puts on the entry the UI toggled. + fn web_layer(dsh_home: &Path) -> std::path::PathBuf { + dsh_home.join("profiles/web/cordis.patch.yml") + } + + /// The home patch the writer edits — what `manager::toggle_plugin` passes + /// straight through from the dsh adapter's `mcp_config_path()`. + fn home_patch(dsh_home: &Path) -> std::path::PathBuf { + dsh_home.join("cordis.patch.yml") + } #[test] - fn remove_mcp_server_refuses_dsh_cordis() { - // Pin: the generic remove path must never touch the dsh patch file — - // create the file so the early-return-on-missing-file path isn't taken. - let (_tmp, path) = patch_file(HOME_WITH_GH); + fn disable_profile_row_writes_home_block_and_enable_removes_it() { + let (_tmp, dsh_home) = dsh_home_with_profile(PROFILE_PATCH, None); + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + false, + &[web_layer(&dsh_home)], + ) + .unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(home.contains("managed by HarnessKit")); + assert!(home.contains("- id: tool-policy\n disabled: true")); + // ONLY the home file is written — profile patch stays byte-identical. + assert_eq!( + std::fs::read_to_string(dsh_home.join("profiles/web/cordis.patch.yml")).unwrap(), + PROFILE_PATCH + ); + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + true, + &[web_layer(&dsh_home)], + ) + .unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(!home.contains("managed by HarnessKit"), "back to base → entry removed"); + let parsed: serde_yaml::Value = serde_yaml::from_str(&home).unwrap(); + assert!(parsed.is_sequence(), "file must stay a valid YAML list"); + } + + #[test] + fn enable_user_disabled_row_writes_disabled_false_override() { + // The row is disabled IN THE PROFILE FILE by the user; HK enable must + // write an explicit `disabled: false` override (last layer wins). + let patch = format!("{PROFILE_PATCH}- id: tool-policy\n disabled: true\n"); + let (_tmp, dsh_home) = dsh_home_with_profile(&patch, None); + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + true, + &[web_layer(&dsh_home)], + ) + .unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(home.contains("- id: tool-policy\n disabled: false")); + } + + #[test] + fn home_user_override_is_part_of_base_state() { + // User already disabled the row from their home patch text: HK + // disable is then a no-op (no block written). + let (_tmp, dsh_home) = dsh_home_with_profile( + PROFILE_PATCH, + Some("- id: tool-policy\n disabled: true\n"), + ); + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + false, + &[web_layer(&dsh_home)], + ) + .unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(!home.contains("managed by HarnessKit"), "already disabled at base"); + } + + #[test] + fn unknown_row_errors_not_found_and_writes_nothing() { + let (_tmp, dsh_home) = dsh_home_with_profile(PROFILE_PATCH, None); + let err = + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "nope", + false, + &[web_layer(&dsh_home)], + ) + .unwrap_err(); + assert!(matches!(err, HkError::NotFound(_))); + assert!(!dsh_home.join("cordis.patch.yml").exists(), "nothing written"); + } + + #[test] + fn sibling_profile_override_is_not_part_of_base_state() { + // Row DEFINED (enabled) in profile `alpha`, separately overridden + // `disabled: true` in profile `beta`. dsh boots ONE profile at a + // time — beta's patch is never loaded next to alpha's — so from + // alpha's entry the base state is ENABLED and disabling it must + // actually write an override. Folding beta in would compute + // base=disabled and silently write nothing, leaving the plugin + // loaded in alpha. + let tmp = tempfile::tempdir().unwrap(); + let dsh_home = tmp.path().join(".dsh"); + std::fs::create_dir_all(dsh_home.join("profiles/alpha")).unwrap(); + std::fs::create_dir_all(dsh_home.join("profiles/beta")).unwrap(); + std::fs::write(dsh_home.join("profiles/alpha/cordis.patch.yml"), PROFILE_PATCH).unwrap(); + std::fs::write( + dsh_home.join("profiles/beta/cordis.patch.yml"), + "- id: tool-policy\n disabled: true\n", + ) + .unwrap(); + let alpha_layer = dsh_home.join("profiles/alpha/cordis.patch.yml"); + + set_dsh_plugin_enabled(&home_patch(&dsh_home), "tool-policy", false, std::slice::from_ref(&alpha_layer)).unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!( + home.contains("- id: tool-policy\n disabled: true"), + "alpha's row is enabled at base, so disable must write: {home}" + ); + + // And back: the row returns to alpha's own base → block removed. A + // fold that consulted beta would instead leave a gratuitous + // `disabled: false`, overriding beta's own choice machine-wide. + set_dsh_plugin_enabled(&home_patch(&dsh_home), "tool-policy", true, std::slice::from_ref(&alpha_layer)).unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!( + !home.contains("managed by HarnessKit"), + "back to alpha's base → entry removed: {home}" + ); + assert!(!home.contains("disabled: false"), "no gratuitous override: {home}"); + } + + #[test] + fn own_layer_override_after_the_definition_is_part_of_base_state() { + // Same row defined AND overridden inside the toggled entry's own + // layer: that override is loaded with the definition, so it does + // count — the per-profile rule narrows the fold, it does not drop + // in-layer ordering. + let patch = format!("{PROFILE_PATCH}- id: tool-policy\n disabled: true\n"); + let (_tmp, dsh_home) = dsh_home_with_profile(&patch, None); + set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + false, + &[web_layer(&dsh_home)], + ) + .unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")) + .unwrap_or_else(|_| String::new()); + assert!( + !home.contains("managed by HarnessKit"), + "the row's own layer already disables it at base: {home}" + ); + } + + #[test] + fn corrupted_block_error_names_the_home_patch_path() { + // The call-site map_err must append the file path + remediation hint + // to the path-agnostic block parser's ConfigCorrupted. + let bad_home = format!("{DSH_BLOCK_BEGIN}\n- surprise: 1\n{DSH_BLOCK_END}\n"); + let (_tmp, dsh_home) = dsh_home_with_profile(PROFILE_PATCH, Some(&bad_home)); + let err = set_dsh_plugin_enabled( + &home_patch(&dsh_home), + "tool-policy", + false, + &[web_layer(&dsh_home)], + ) + .unwrap_err(); + let HkError::ConfigCorrupted(msg) = err else { + panic!("expected ConfigCorrupted, got {err:?}"); + }; + let home_path = dsh_home.join("cordis.patch.yml").display().to_string(); + assert!(msg.contains(&home_path), "message names the file: {msg}"); + assert!(msg.contains("fix or remove"), "message carries the hint: {msg}"); + // Nothing written: the corrupt file is untouched. + assert_eq!( + std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(), + bad_home + ); + } + + #[test] + fn home_defined_row_toggles_too() { + // Rows defined directly in the home layer are also valid targets: + // the owning layer IS the home patch, and the writer must then read + // that layer only through the block-stripped user text. + let (_tmp, dsh_home) = dsh_home_with_profile( + "[]\n", + Some("- insert:\n - id: theme-row\n name: dsh-plugin-theme\n"), + ); + let home_layer = dsh_home.join("cordis.patch.yml"); + set_dsh_plugin_enabled(&home_patch(&dsh_home), "theme-row", false, std::slice::from_ref(&home_layer)).unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(home.contains("- id: theme-row\n disabled: true")); + assert!(home.starts_with("- insert:"), "user bytes preserved"); + // Re-enable with our own block already in the file: the block must + // never be folded back in as "base", or this would look like a no-op. + set_dsh_plugin_enabled(&home_patch(&dsh_home), "theme-row", true, std::slice::from_ref(&home_layer)).unwrap(); + let home = std::fs::read_to_string(dsh_home.join("cordis.patch.yml")).unwrap(); + assert!(!home.contains("managed by HarnessKit"), "back to base → entry removed"); + } +} + +#[cfg(test)] +mod dsh_insert_writer_tests { + use super::*; + use crate::adapter::dsh::DshAdapter; + + const USER_GH: &str = r#"# precious comment +- insert: + - id: mcp-github + name: '@deepseek-ai/dsh-mcp-client' + config: + serverName: github + transport: stdio + command: npx +"#; + + fn patch_file(text: &str) -> (tempfile::TempDir, std::path::PathBuf) { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("cordis.patch.yml"); + std::fs::write(&path, text).unwrap(); + (tmp, path) + } + + fn stdio_entry(name: &str) -> McpServerEntry { + McpServerEntry { + name: name.into(), + command: "npx".into(), + args: vec!["-y".into(), "@modelcontextprotocol/server-github".into()], + env: std::collections::HashMap::from([( + "GITHUB_TOKEN".to_string(), + "tok".to_string(), + )]), + transport: McpTransport::Stdio, + url: None, + headers: Default::default(), + enabled: true, + } + } + + #[test] + fn stdio_install_round_trips_through_the_dsh_reader() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("cordis.patch.yml"); + // Missing file: writer starts from the valid empty form. + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github2")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("managed by HarnessKit")); + // Markers are YAML comments, so the whole file parses as one + // document and the P0 reader sees the new row with no extra plumbing. + assert_eq!(DshAdapter::mcp_enabled_in_text(&text).get("github2"), Some(&true)); + assert_eq!( + DshAdapter::mcp_row_id_in_text(&text, "github2").as_deref(), + Some("mcp-github2") + ); + // Explicit discriminant + serverName always (mcp-client schema). + assert!(text.contains("transport: stdio")); + assert!(text.contains("serverName: github2")); + assert!(text.contains("command: npx")); + assert!(text.contains("GITHUB_TOKEN: tok")); + } + + #[test] + fn install_preserves_user_bytes_and_drops_placeholder() { + let (_tmp, path) = patch_file("# my notes\n[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github2")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.starts_with("# my notes\n")); + assert!( + !text.lines().any(|l| l.trim() == "[]"), + "placeholder can't coexist with entries" + ); + let parsed: serde_yaml::Value = serde_yaml::from_str(&text).unwrap(); + assert!(parsed.is_sequence()); + } + + #[test] + fn server_name_is_sanitized_to_the_mcp_client_pattern() { + // /^[A-Za-z0-9_-]{1,32}$/ — invalid chars map to '-', 32-char cap. + let (_tmp, path) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("My Server/rocks!")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("serverName: My-Server-rocks-")); + + // A name with no valid character at all cannot be sanitized. + let err = deploy_mcp_server_dsh_cordis(&path, &stdio_entry("///")).unwrap_err(); + assert!(matches!(err, HkError::Validation(_))); + } + + #[test] + fn server_name_and_row_id_collisions_error() { + // serverName collision with a user-authored row. + let (_tmp, path) = patch_file(USER_GH); + let err = deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github")).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("github"))); + + // Row-id collision with an unrelated user row occupying the generated id. + let user2 = "- insert:\n - id: mcp-github2\n name: dsh-plugin-tool\n config:\n mode: x\n"; + let (_tmp2, path2) = patch_file(user2); + let err = deploy_mcp_server_dsh_cordis(&path2, &stdio_entry("github2")).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("mcp-github2"))); + + // Double-install of the same HK server collides with its own block row. + let (_tmp3, path3) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path3, &stdio_entry("github2")).unwrap(); + let err = deploy_mcp_server_dsh_cordis(&path3, &stdio_entry("github2")).unwrap_err(); + assert!(matches!(err, HkError::Validation(_))); + } + + #[test] + fn toggle_of_hk_inserted_server_edits_its_own_insert_entry() { + let (_tmp, path) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github2")).unwrap(); + set_dsh_mcp_enabled(&path, "github2", false).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("disabled: true")); + assert_eq!( + text.matches("id: mcp-github2").count(), + 1, + "no separate override row — the insert row itself carries disabled" + ); + assert_eq!(DshAdapter::mcp_enabled_in_text(&text).get("github2"), Some(&false)); + + // ENABLE path of an HK-inserted row: the disabled key is removed from + // the row itself and the reader sees the server enabled again. + set_dsh_mcp_enabled(&path, "github2", true).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(!text.contains("disabled"), "re-enable removes the disabled key"); + assert_eq!(DshAdapter::mcp_enabled_in_text(&text).get("github2"), Some(&true)); + } + + #[test] + fn user_row_toggle_back_to_base_keeps_hk_insert_rows() { + // Spec-pinned: removing a toggle entry must NOT delete co-resident + // HK insert rows in the same block. + let (_tmp, path) = patch_file(USER_GH); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github2")).unwrap(); + set_dsh_mcp_enabled(&path, "github", false).unwrap(); + set_dsh_mcp_enabled(&path, "github", true).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.starts_with(USER_GH), "user bytes preserved"); + assert!(text.contains("serverName: github2"), "HK insert row survives"); + assert!( + !text.contains("- id: mcp-github\n disabled"), + "toggle entry for the user row is gone" + ); + } + + #[test] + fn remove_deletes_hk_row_refuses_user_row_ignores_absent() { + let (_tmp, path) = patch_file(USER_GH); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("github2")).unwrap(); + + // HK-inserted row: removed; block (now empty) disappears; user bytes intact. + remove_mcp_server(&path, "github2", McpFormat::DshCordis).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.starts_with(USER_GH)); + assert!(!text.contains("managed by HarnessKit")); + + // User-authored row: Validation refusal, file untouched. let err = remove_mcp_server(&path, "github", McpFormat::DshCordis).unwrap_err(); assert!(matches!(&err, HkError::Validation(m) if m.contains("cordis.patch.yml"))); - assert_eq!(std::fs::read_to_string(&path).unwrap(), HOME_WITH_GH); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + + // Absent name: idempotent no-op, like every other format. + remove_mcp_server(&path, "nope", McpFormat::DshCordis).unwrap(); + } + + #[test] + fn crlf_user_file_survives_install_byte_for_byte() { + let user = "# note\r\n- insert:\r\n - id: mcp-github\r\n name: '@deepseek-ai/dsh-mcp-client'\r\n config:\r\n serverName: github\r\n transport: stdio\r\n command: npx\r\n"; + let (_tmp, path) = patch_file(user); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("web2")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.starts_with(user), "CRLF user bytes preserved verbatim"); + let parsed: serde_yaml::Value = serde_yaml::from_str(&text).unwrap(); + assert!(parsed.is_sequence()); + } + + #[test] + fn deploy_mcp_server_dispatch_routes_dsh_cordis() { + use crate::adapter::AgentAdapter; + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(tmp.path().join(".dsh")).unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let path = adapter.mcp_config_path(); + deploy_mcp_server(&path, &stdio_entry("github2"), &adapter).unwrap(); + assert_eq!(adapter.read_mcp_servers().len(), 1); + } + + /// The bug this pins: installing `microsoft/markitdown` used to write a + /// row named `microsoft-markitdown` with no record of the original, so + /// the scanner read back a DIFFERENT extension — the source row's DSH + /// button never turned ✓, a re-install failed on the serverName + /// collision, and the list grew a ghost `microsoft-markitdown` row. + #[test] + fn install_records_the_original_name_and_the_reader_round_trips_it() { + use crate::adapter::AgentAdapter; + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(tmp.path().join(".dsh")).unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let path = adapter.mcp_config_path(); + + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("microsoft/markitdown")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + // On disk: the sanitized name mcp-client accepts, plus the original. + assert!(text.contains("serverName: microsoft-markitdown"), "{text}"); + assert!(text.contains("_hk_name: microsoft/markitdown"), "{text}"); + + // Read back: the ORIGINAL name, so the extension groups with the + // other agents' rows instead of forming a second one. + let servers = adapter.read_mcp_servers(); + assert_eq!(servers.len(), 1); + assert_eq!(servers[0].name, "microsoft/markitdown"); + + // Deployer-side lookups still key on the STORED serverName. + assert_eq!( + DshAdapter::mcp_row_id_in_text(&text, "microsoft-markitdown").as_deref(), + Some("mcp-microsoft-markitdown") + ); + assert!(DshAdapter::mcp_enabled_in_text(&text).contains_key("microsoft-markitdown")); + } + + #[test] + fn install_omits_hk_name_when_the_name_needs_no_sanitizing() { + // Same conditional as Codex: unchanged names keep their exact bytes. + let (_tmp, path) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("my_server-1")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("serverName: my_server-1"), "{text}"); + assert!(!text.contains("_hk_name"), "{text}"); + } + + #[test] + fn installing_the_same_original_name_twice_collides_and_adds_no_second_row() { + use crate::adapter::AgentAdapter; + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(tmp.path().join(".dsh")).unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let path = adapter.mcp_config_path(); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("microsoft/markitdown")).unwrap(); + + let err = + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("microsoft/markitdown")).unwrap_err(); + // The message names both the stored name and the original input. + assert!(matches!(&err, HkError::Validation(m) + if m.contains("microsoft-markitdown") && m.contains("microsoft/markitdown"))); + assert_eq!(adapter.read_mcp_servers().len(), 1, "no ghost second row"); + } + + #[test] + fn remove_and_toggle_by_original_name_hit_the_sanitized_row() { + // Name symmetry: deploy writes the SANITIZED serverName, so remove + // and toggle called with the ORIGINAL input must normalize the same + // way — otherwise `remove("My Server")` silently returns Ok while + // the "My-Server" row stays installed. Asserted on the raw bytes, + // BELOW the reader, so a reader that also normalized would not hide + // a writer that did not. + let (_tmp, path) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("My Server")).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("serverName: My-Server")); + + set_dsh_mcp_enabled(&path, "My Server", false).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert_eq!(DshAdapter::mcp_enabled_in_text(&text).get("My-Server"), Some(&false)); + + set_dsh_mcp_enabled(&path, "My Server", true).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert_eq!(DshAdapter::mcp_enabled_in_text(&text).get("My-Server"), Some(&true)); + + remove_mcp_server(&path, "My Server", McpFormat::DshCordis).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(!text.contains("My-Server"), "row actually gone: {text}"); + } + + #[test] + fn build_dsh_insert_row_streamable_http_pins_rendered_bytes() { + // Byte-level pin of the remote row format, now that dsh advertises + // RemoteMcpSchema::DshTransport and installs can reach this arm. + let entry = McpServerEntry { + name: "web".into(), + command: String::new(), + args: vec![], + env: Default::default(), + transport: McpTransport::Http, + url: Some("https://example.com/mcp".into()), + headers: std::collections::HashMap::from([ + ("X-Api".to_string(), "v1".to_string()), + ("Authorization".to_string(), "Bearer tok".to_string()), + ]), + enabled: true, + }; + let mut block = DshManagedBlock::default(); + block.inserts.push(build_dsh_insert_row("mcp-web", "web", &entry)); + let out = render_dsh_patch("", &block); + let expected = format!( + "{DSH_BLOCK_BEGIN}\n\ + - insert:\n\ + \x20 - id: mcp-web\n\ + \x20 name: '@deepseek-ai/dsh-mcp-client'\n\ + \x20 config:\n\ + \x20 transport: streamable-http\n\ + \x20 serverName: web\n\ + \x20 url: https://example.com/mcp\n\ + \x20 headers:\n\ + \x20 Authorization: Bearer tok\n\ + \x20 X-Api: v1\n\ + {DSH_BLOCK_END}\n" + ); + assert_eq!(out, expected); + } + + #[test] + fn remote_streamable_http_installs_and_sse_is_rejected() { + use crate::adapter::AgentAdapter; + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(tmp.path().join(".dsh")).unwrap(); + let adapter = DshAdapter::with_home(tmp.path().to_path_buf()); + let path = adapter.mcp_config_path(); + + let http = McpServerEntry { + name: "web2".into(), + command: String::new(), + args: vec![], + env: Default::default(), + transport: McpTransport::Http, + url: Some("https://example.com/mcp".into()), + headers: std::collections::HashMap::from([( + "Authorization".to_string(), + "Bearer x".to_string(), + )]), + enabled: true, + }; + deploy_mcp_server(&path, &http, &adapter).unwrap(); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains("transport: streamable-http")); + assert!(text.contains("url: https://example.com/mcp")); + assert!(text.contains("Authorization: Bearer x")); + + let sse = McpServerEntry { + name: "sse2".into(), + command: String::new(), + args: vec![], + env: Default::default(), + transport: McpTransport::Sse, + url: Some("https://example.com/sse".into()), + headers: Default::default(), + enabled: true, + }; + let err = deploy_mcp_server(&path, &sse, &adapter).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("not SSE"))); + // Rejection happens before any write — the patch file is untouched. + let after = std::fs::read_to_string(&path).unwrap(); + assert_eq!(after, text); + } + + #[test] + fn distinct_inputs_sanitizing_to_the_same_name_collide() { + // "My Server" and "My/Server" both sanitize to "My-Server" — the + // second install must error (collision), never clobber the first, + // and the message names both the sanitized and the original form. + let (_tmp, path) = patch_file("[]\n"); + deploy_mcp_server_dsh_cordis(&path, &stdio_entry("My Server")).unwrap(); + let before = std::fs::read_to_string(&path).unwrap(); + let err = deploy_mcp_server_dsh_cordis(&path, &stdio_entry("My/Server")).unwrap_err(); + assert!( + matches!(&err, HkError::Validation(m) + if m.contains("'My-Server'") && m.contains("(from 'My/Server')")), + "got: {err:?}" + ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), before, "no clobber"); + } + + #[test] + fn stale_toggle_occupying_the_generated_row_id_errors() { + // A block toggle can outlive the user row it targeted (dsh warn-skips + // dangling overrides). Its id still occupies the collision domain: an + // install deriving the same row id must error, not double-define it. + let stale = format!("{DSH_BLOCK_BEGIN}\n- id: mcp-x\n disabled: true\n{DSH_BLOCK_END}\n"); + let (_tmp, path) = patch_file(&stale); + let err = deploy_mcp_server_dsh_cordis(&path, &stdio_entry("x")).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("mcp-x")), "got: {err:?}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), stale, "file untouched"); + } + + #[test] + fn profile_layer_row_id_occupies_the_collision_domain() { + // Profile patches are applied BEFORE the home patch, so their row ids + // share one namespace with it: generating `mcp-x` while a profile + // already defines `mcp-x` would be a duplicate definition for dsh. + let (_tmp, path) = patch_file("[]\n"); + let profile = path.parent().unwrap().join("profiles/web"); + std::fs::create_dir_all(&profile).unwrap(); + std::fs::write( + profile.join("cordis.patch.yml"), + "- insert:\n - id: mcp-x\n name: dsh-plugin-tool\n", + ) + .unwrap(); + let err = deploy_mcp_server_dsh_cordis(&path, &stdio_entry("x")).unwrap_err(); + assert!(matches!(&err, HkError::Validation(m) if m.contains("mcp-x")), "got: {err:?}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "[]\n", "file untouched"); + } + + #[test] + fn remove_on_a_wholly_absent_file_is_ok() { + // Pins the writer-level idempotency (NotFound → "[]" synthesis in + // read_and_split_home_patch) so it can't regress to an IO error — + // independent of the dispatch-level exists() early return. + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("cordis.patch.yml"); + remove_mcp_server_dsh_cordis(&path, "anything").unwrap(); + assert!(!path.exists(), "no file conjured by a no-op removal"); } } diff --git a/crates/hk-core/src/kits/install_plan.rs b/crates/hk-core/src/kits/install_plan.rs index ca33df09..26a0b45f 100644 --- a/crates/hk-core/src/kits/install_plan.rs +++ b/crates/hk-core/src/kits/install_plan.rs @@ -62,9 +62,19 @@ fn mcp_entry_exists(config_path: &Path, name: &str, format: McpFormat) -> bool { .and_then(|v| v.get(name)) .is_some() } - // dsh MCP can't be Kit-installed (cordis patch files are never a Kit - // install target), so no conflict is ever detectable. - McpFormat::DshCordis => false, + McpFormat::DshCordis => { + // Kits route through deploy_mcp_server, so dsh MCP Kit-install + // works since the insert writer landed. HK-inserted rows live in + // the managed block, but the markers are YAML comments — the + // whole file parses as one document, so a single folded lookup + // covers user rows and HK rows alike. The writer stores the + // SANITIZED serverName; normalize before the lookup. + let Ok(s) = std::fs::read_to_string(config_path) else { + return false; + }; + let lookup = crate::deployer::normalize_dsh_server_name(name); + crate::adapter::dsh::DshAdapter::mcp_enabled_in_text(&s).contains_key(&lookup) + } } } @@ -219,3 +229,65 @@ pub fn compute_kit_install_plan( Ok(items) } + +#[cfg(test)] +mod dsh_conflict_tests { + use super::*; + + #[test] + fn dsh_cordis_conflict_detects_existing_server_name() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("cordis.patch.yml"); + std::fs::write( + &path, + "- insert:\n - id: mcp-github\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: github\n transport: stdio\n command: npx\n", + ) + .unwrap(); + assert!(mcp_entry_exists(&path, "github", McpFormat::DshCordis)); + assert!(!mcp_entry_exists(&path, "web", McpFormat::DshCordis)); + // Missing file → no conflict (early-return path). + assert!(!mcp_entry_exists( + &tmp.path().join("absent.yml"), + "github", + McpFormat::DshCordis + )); + } + + #[test] + fn dsh_cordis_conflict_matches_sanitized_server_name() { + // The install writer sanitizes the serverName it writes + // ("my/server" → "my-server"), so the conflict check must + // normalize the same way or a Kit carrying the original name + // would miss the very row the writer installed for it. + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("cordis.patch.yml"); + std::fs::write( + &path, + "- insert:\n - id: mcp-my-server\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: my-server\n transport: stdio\n command: npx\n", + ) + .unwrap(); + assert!(mcp_entry_exists(&path, "my/server", McpFormat::DshCordis)); + } + + #[test] + fn dsh_cordis_conflict_sees_rows_the_real_writer_installed() { + // Roundtrip: the writer puts rows inside the HK managed block + // (comment markers) — the kits conflict check must still see them. + let tmp = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(tmp.path().join(".dsh")).unwrap(); + let adapter = crate::adapter::dsh::DshAdapter::with_home(tmp.path().to_path_buf()); + let path = adapter.mcp_config_path(); + let entry = crate::adapter::McpServerEntry { + name: "github".into(), + command: "npx".into(), + args: vec![], + env: Default::default(), + transport: crate::adapter::McpTransport::Stdio, + url: None, + headers: Default::default(), + enabled: true, + }; + crate::deployer::deploy_mcp_server(&path, &entry, &adapter).unwrap(); + assert!(mcp_entry_exists(&path, "github", McpFormat::DshCordis)); + } +} diff --git a/crates/hk-core/src/manager.rs b/crates/hk-core/src/manager.rs index 76e9506e..1b007f80 100644 --- a/crates/hk-core/src/manager.rs +++ b/crates/hk-core/src/manager.rs @@ -467,6 +467,58 @@ fn disabled_plugin_name(path: &Path) -> String { .unwrap_or_else(|| base.to_string()) } +/// Resolve a dsh plugin toggle to `(patch-row id, base layer chain)`, or the +/// reason it cannot be toggled. The chain travels with the id because the +/// writer's base state is per-profile AND multi-layer: dsh boots one profile +/// at a time and composes each mounted bundle's patch, then the profile's +/// patch, then the home patch — a row can be defined in one layer and +/// disabled in a later one (`hmr`), so only the whole chain gives the right +/// base state, and a sibling profile's layers must never join it. +/// +/// Rows provided by a BUNDLE's patch file are toggled exactly like user rows: +/// they have ids, and the home managed block overrides them — which is +/// precisely how dsh's own web-app bundle disables base rows. The bundle's +/// patch file is a read-only input; the write still goes to the home patch. +/// +/// One non-toggleable class, with its own backend `Validation` error (UI +/// graying alone is not a guard): anonymous rows (`uri` None) have no id to +/// target — advise adding one (P0 MCP wording pattern). +fn dsh_plugin_toggle_target( + plugin: &adapter::PluginEntry, +) -> Result<(String, Vec), HkError> { + match &plugin.uri { + // Every id-bearing row comes from patch layers the dsh adapter read, + // so the chain is always present here; an empty one is an adapter + // bug, not a user-facing state. + Some(row_id) => { + if plugin.base_layers.is_empty() { + return Err(HkError::Internal(format!( + "dsh plugin row '{row_id}' carries no base patch layers" + ))); + } + Ok((row_id.clone(), plugin.base_layers.clone())) + } + None => Err(HkError::Validation(format!( + "the insert row for '{}' has no id — add an `id:` to the row in cordis.patch.yml so a patch override can target it ({})", + plugin.name, plugin.source + ))), + } +} + +/// The adapter-reported plugin whose scanner identity matches `ext`. The id +/// must be recomputed exactly as `scanner::scan_plugins` builds it +/// (":"); both sides call `scanner::plugin_extension_id`, so +/// that pairing lives in one place. +fn find_plugin_for_ext<'a>( + plugins: &'a [adapter::PluginEntry], + ext: &Extension, + agent: &str, +) -> Option<&'a adapter::PluginEntry> { + plugins + .iter() + .find(|p| scanner::plugin_extension_id(&p.name, &p.source, agent) == ext.id) +} + fn toggle_plugin( ext: &Extension, enabled: bool, @@ -501,13 +553,8 @@ fn toggle_plugin( // If so, toggle via state.vscdb. Otherwise fall through to manifest rename. // Cache read_plugins result to avoid scanning twice for CLI plugins. let plugins = a.read_plugins(); - let plugin_uri = plugins - .iter() - .find(|p| { - let id_name = format!("{}:{}", p.name, p.source); - scanner::stable_id_for(&id_name, "plugin", a.name()) == ext.id - }) - .and_then(|p| p.uri.clone()); + let plugin_uri = + find_plugin_for_ext(&plugins, ext, a.name()).and_then(|p| p.uri.clone()); if let Some(uri) = plugin_uri { let vscode_user_dir = a.vscode_user_dir().ok_or_else(|| { HkError::Internal("Copilot adapter missing vscode_user_dir".into()) @@ -518,6 +565,31 @@ fn toggle_plugin( // Copilot CLI plugin — reuse cached plugins to avoid second scan toggle_plugin_manifest(ext, enabled, store, a.as_ref(), Some(plugins))?; } + } else if a.name() == "dsh" { + // Explicit branch BEFORE the generic manifest-rename fallback — + // mandatory ordering, not style: the fallback probes package dirs + // for a plugin.json to rename `.disabled`, which would corrupt + // dsh's pnpm-managed node_modules tree. + let plugins = a.read_plugins(); + let plugin = find_plugin_for_ext(&plugins, ext, a.name()).ok_or_else(|| { + HkError::NotFound(format!("dsh plugin '{}' not found on disk", ext.name)) + })?; + // The matched entry names its own layer chain: read_plugins + // emits one entry per (profile, row) carrying exactly the layers + // dsh composes for that profile below the home patch. + let (row_id, base_layers) = dsh_plugin_toggle_target(plugin)?; + // Pass the adapter's OWN home-patch path: `base_layers` came + // from the same accessor, so the writer's home-defined-row test + // compares two identical values instead of a re-derived one. + deployer::set_dsh_plugin_enabled( + &a.mcp_config_path(), + &row_id, + enabled, + &base_layers, + )?; + // State lives in the patch file and is read back on rescan — no + // DB snapshot; clear any legacy disabled_config. + store.set_disabled_config(&ext.id, None)?; } else { // Generic: manifest rename for Cursor, Copilot CLI, etc. toggle_plugin_manifest(ext, enabled, store, a.as_ref(), None)?; @@ -569,30 +641,22 @@ fn toggle_plugin_manifest( } else { // Disable: find plugin via live scan, rename manifest, save path let plugins = prefetched_plugins.unwrap_or_else(|| adapter.read_plugins()); - let mut found = false; - for plugin in plugins { - let plugin_id_name = format!("{}:{}", plugin.name, plugin.source); - if scanner::stable_id_for(&plugin_id_name, "plugin", adapter.name()) != ext.id { - continue; - } - if let Some(ref path) = plugin.path - && let Some(manifest) = plugin_toggle_target(path) - { - let disabled_manifest = disabled_plugin_target(&manifest); - let saved = - serde_json::json!({ "manifest_path": disabled_manifest.to_string_lossy() }); - store.set_disabled_config(&ext.id, Some(&saved.to_string()))?; - std::fs::rename(&manifest, &disabled_manifest)?; - found = true; - } - break; - } - if !found { + // Only the FIRST identity match is considered (the old `for … break` + // semantics): a second entry with the same id would be the same + // plugin, and renaming its manifest twice cannot help. + let manifest = find_plugin_for_ext(&plugins, ext, adapter.name()) + .and_then(|plugin| plugin.path.as_deref()) + .and_then(plugin_toggle_target); + let Some(manifest) = manifest else { return Err(HkError::NotFound(format!( "No plugin file or manifest found for plugin '{}' — cannot disable", ext.name ))); - } + }; + let disabled_manifest = disabled_plugin_target(&manifest); + let saved = serde_json::json!({ "manifest_path": disabled_manifest.to_string_lossy() }); + store.set_disabled_config(&ext.id, Some(&saved.to_string()))?; + std::fs::rename(&manifest, &disabled_manifest)?; } Ok(()) } @@ -620,8 +684,12 @@ fn find_disabled_plugin_path(adapter: &dyn adapter::AgentAdapter, ext_id: &str) .map(|n| n.to_string_lossy().to_string()) .unwrap_or_default() }; - let id_name = format!("{}:{}", disabled_plugin_name(&path), source); - if scanner::stable_id_for(&id_name, "plugin", adapter.name()) == ext_id { + if scanner::plugin_extension_id( + &disabled_plugin_name(&path), + &source, + adapter.name(), + ) == ext_id + { return Some(path); } continue; @@ -636,8 +704,9 @@ fn find_disabled_plugin_path(adapter: &dyn adapter::AgentAdapter, ext_id: &str) continue; } let dir_name = entry.file_name().to_string_lossy().to_string(); - let id_name = format!("{dir_name}:local"); - if scanner::stable_id_for(&id_name, "plugin", adapter.name()) == ext_id { + if scanner::plugin_extension_id(&dir_name, "local", adapter.name()) + == ext_id + { return Some(disabled); } } @@ -671,8 +740,8 @@ fn find_disabled_plugin_path(adapter: &dyn adapter::AgentAdapter, ext_id: &str) } else { &dir_name }; - let id_name = format!("{}:{}", name, source); - if scanner::stable_id_for(&id_name, "plugin", adapter.name()) == ext_id + if scanner::plugin_extension_id(name, source, adapter.name()) + == ext_id { return Some(disabled); } @@ -683,8 +752,9 @@ fn find_disabled_plugin_path(adapter: &dyn adapter::AgentAdapter, ext_id: &str) .file_name() .map(|n| n.to_string_lossy().to_string()) .unwrap_or_default(); - let id_name = format!("{}:{}", dir_name_str, source); - if scanner::stable_id_for(&id_name, "plugin", adapter.name()) == ext_id { + if scanner::plugin_extension_id(&dir_name_str, &source, adapter.name()) + == ext_id + { return Some(disabled); } } @@ -2905,3 +2975,101 @@ mod tests { assert!(!p3.enabled); } } + +#[cfg(test)] +mod dsh_plugin_dispatch_tests { + use super::*; + + /// `name` mirrors what the adapter emits: an id-bearing row is named by + /// its row id (dsh's own plugin list labels it that way), an anonymous + /// row by the package it instantiates. + fn entry(name: &str, source: &str, uri: Option<&str>) -> adapter::PluginEntry { + adapter::PluginEntry { + name: name.into(), + source: source.into(), + enabled: true, + path: None, + source_url: None, + uri: uri.map(String::from), + installed_at: None, + updated_at: None, + // Every id-bearing row the adapter emits names its profile's + // whole layer chain; the row tests below set it explicitly. + base_layers: vec![], + pack: None, + } + } + + #[test] + fn row_plugins_toggle_via_their_row_id_and_their_profiles_layer_chain() { + // The chain must travel with the id: the writer folds base state + // from THIS profile's layers only, never a sibling profile's — and + // it needs every layer, not just the defining one. + let layers = vec![ + PathBuf::from("/home/u/.dsh/profiles/web/cordis.patch.yml"), + ]; + let mut e = entry( + "tool-policy", + "profile web, package dsh-plugin-tool", + Some("tool-policy"), + ); + e.base_layers = layers.clone(); + assert_eq!( + dsh_plugin_toggle_target(&e).unwrap(), + ("tool-policy".to_string(), layers) + ); + } + + #[test] + fn bundle_provided_rows_are_toggleable_like_any_other_row() { + // A bundle is a LAYER, not a plugin — but the rows it inserts are + // ordinary toggleable rows (dsh's own web-app bundle disables base + // rows exactly this way). The bundle's patch is a read-only input in + // the chain; the write goes to the home patch. + let layers = vec![ + PathBuf::from("/home/u/.dsh/profiles/node_modules/@deepseek-ai/dsh-base/cordis.patch.yml"), + PathBuf::from("/home/u/.dsh/profiles/node_modules/@deepseek-ai/dsh-web-app/cordis.patch.yml"), + PathBuf::from("/home/u/.dsh/profiles/web/cordis.patch.yml"), + ]; + let mut e = entry( + "hmr", + "profile web, bundle @deepseek-ai/dsh-base, package @deepseek-ai/cordis-plugin-hmr", + Some("hmr"), + ); + e.base_layers = layers.clone(); + assert_eq!( + dsh_plugin_toggle_target(&e).unwrap(), + ("hmr".to_string(), layers) + ); + } + + #[test] + fn row_without_base_layers_is_an_internal_error() { + // Adapter bug, not a user-facing state — must not silently fall back + // to folding every profile. + let e = dsh_plugin_toggle_target(&entry( + "tool-policy", + "profile web, package dsh-plugin-tool", + Some("tool-policy"), + )) + .unwrap_err(); + assert!(matches!(&e, HkError::Internal(m) if m.contains("base patch layers"))); + } + + #[test] + fn an_anonymous_row_gets_an_actionable_backend_error() { + // Backend guard, not just UI graying — direct API calls must fail + // with an actionable message (spec §2). Bundles are not a class here: + // they are layers and are not emitted as entries at all. + let e = dsh_plugin_toggle_target(&entry( + "dsh-plugin-anon", + "profile web, anonymous row", + None, + )) + .unwrap_err(); + assert!(matches!( + &e, + HkError::Validation(m) if m.contains("no id") && m.contains("(profile web, anonymous row)") + )); + } +} diff --git a/crates/hk-core/src/models.rs b/crates/hk-core/src/models.rs index 0aafcd9a..589d56ee 100644 --- a/crates/hk-core/src/models.rs +++ b/crates/hk-core/src/models.rs @@ -371,6 +371,12 @@ pub struct AgentCapabilities { /// http/sse MCP servers to this agent. #[serde(default)] pub mcp_remote: RemoteTransportFlags, + /// Packs whose plugins ship with the agent. `delete_extension` refuses + /// these, so the UI greys out delete on exactly the same rows rather than + /// letting the user hit the error. Empty for every agent whose baseline + /// never surfaces as an extension. + #[serde(default)] + pub vendor_baseline_packs: Vec, } /// Remote MCP transports an agent supports. Both false = stdio-only. diff --git a/crates/hk-core/src/scanner.rs b/crates/hk-core/src/scanner.rs index b729c8f6..4d3b795a 100644 --- a/crates/hk-core/src/scanner.rs +++ b/crates/hk-core/src/scanner.rs @@ -91,6 +91,15 @@ pub fn stable_id_for(name: &str, kind: &str, agent: &str) -> String { stable_id(name, kind, agent) } +/// The extension id of a plugin, from the `(name, source)` pair an adapter's +/// `read_plugins` reports. Plugin identity is `":"` fed to +/// `stable_id` — a two-part key, unlike every other kind — so the producer +/// (`scan_plugins`) and every consumer that has to re-derive an id from disk +/// go through this one function instead of re-spelling the `format!`. +pub fn plugin_extension_id(name: &str, source: &str, agent: &str) -> String { + stable_id(&format!("{name}:{source}"), "plugin", agent) +} + /// Public wrapper for `stable_id_with_scope`. Use this when the caller /// already knows whether the ID it needs to match is global or project-scoped. pub fn stable_id_with_scope_for( @@ -167,6 +176,17 @@ pub fn scan_skill_dir(dir: &Path, agent_name: &str) -> Vec { let Ok(content) = std::fs::read_to_string(&skill_file) else { continue; }; + // dsh REJECTS camelCase invocation-key aliases by dropping the whole + // skill (a log warning at boot and nothing else), so for dsh this + // skill does not exist — emit no extension for it. "If dsh itself + // doesn't show it, HarnessKit doesn't show it." Consequence, intended: + // in a shared root (~/.agents/skills) only dsh drops off the skill's + // agent list; a dsh-only skill disappears from HK entirely. + if agent_name == "dsh" + && crate::auditor::rules::dsh_drops_skill_for_invocation_key(&content) + { + continue; + } let (name, description, _requires_bins) = parse_skill_frontmatter(&content).unwrap_or_else(|| { @@ -550,14 +570,15 @@ pub fn scan_plugins(adapter: &dyn AgentAdapter) -> Vec { from_manifest: false, }), }; - let pack = source.url.as_deref().and_then(extract_pack_from_url); + // An adapter that knows its provider wins; otherwise fall back to + // the git-URL derivation, which only fires for git-checkout plugins. + let pack = plugin + .pack + .clone() + .or_else(|| source.url.as_deref().and_then(extract_pack_from_url)); Extension { - id: stable_id( - &format!("{}:{}", plugin.name, plugin.source), - "plugin", - adapter.name(), - ), + id: plugin_extension_id(&plugin.name, &plugin.source, adapter.name()), kind: ExtensionKind::Plugin, name: plugin.name, description, @@ -2752,6 +2773,64 @@ mod tests { ); } + #[test] + fn dsh_skips_skills_it_drops_for_camelcase_invocation_keys() { + // dsh rejects camelCase invocation-key aliases by dropping the WHOLE + // skill, so HK must not list it under dsh: "if dsh itself doesn't + // show it, HarnessKit doesn't show it." + let dir = TempDir::new().unwrap(); + for (name, frontmatter) in [ + ("legacy-skill", "name: legacy-skill\nuserInvocable: true"), + ("clean-skill", "name: clean-skill"), + ] { + let skill = dir.path().join(name); + std::fs::create_dir_all(&skill).unwrap(); + std::fs::write( + skill.join("SKILL.md"), + format!("---\n{frontmatter}\n---\nbody\n"), + ) + .unwrap(); + } + + // Shape 1 — shared root (e.g. ~/.agents/skills): only dsh drops the + // skill; every other agent still lists it, so a shared skill merely + // loses dsh from its agent list. + let dsh: Vec = super::scan_skill_dir(dir.path(), "dsh") + .into_iter() + .map(|e| e.name) + .collect(); + assert_eq!(dsh, vec!["clean-skill".to_string()]); + let mut claude: Vec = super::scan_skill_dir(dir.path(), "claude") + .into_iter() + .map(|e| e.name) + .collect(); + claude.sort(); + assert_eq!(claude, vec!["clean-skill".to_string(), "legacy-skill".to_string()]); + + // Shape 2 — a dsh-only root: the skill disappears from HK entirely. + let only = TempDir::new().unwrap(); + let skill = only.path().join("legacy-skill"); + std::fs::create_dir_all(&skill).unwrap(); + std::fs::write( + skill.join("SKILL.md"), + "---\nname: legacy-skill\ndisableModelInvocation: true\n---\nbody\n", + ) + .unwrap(); + assert!(super::scan_skill_dir(only.path(), "dsh").is_empty()); + + // A DISABLED dropped skill is skipped too — dsh would not load it + // even if it were re-enabled. + let off = only.path().join("off-skill"); + std::fs::create_dir_all(&off).unwrap(); + std::fs::write( + off.join("SKILL.md.disabled"), + "---\nname: off-skill\nuserInvocable: true\n---\nbody\n", + ) + .unwrap(); + assert!(super::scan_skill_dir(only.path(), "dsh").is_empty()); + assert_eq!(super::scan_skill_dir(only.path(), "claude").len(), 2); + } + #[test] fn test_disabled_skill_same_id_as_enabled() { let dir = TempDir::new().unwrap(); diff --git a/crates/hk-core/src/service.rs b/crates/hk-core/src/service.rs index dbaa23b7..4fe9d5d1 100644 --- a/crates/hk-core/src/service.rs +++ b/crates/hk-core/src/service.rs @@ -500,14 +500,26 @@ pub fn audit_extensions( ) -> Vec { let auditor = Auditor::new(); let mut inputs = Vec::new(); + // The dsh home patch file is shared by ALL dsh MCP rows; attach its raw + // text to exactly ONE of them so shared-file findings are reported (and + // deducted from trust) once, not once per row. The host is the smallest + // extension id, NOT "whichever row the caller listed first" — otherwise + // the finding and its trust deduction migrate between rows whenever the + // slice order changes (store ordering, scope filter, a new install). + let dsh_patch_host: Option<&str> = extensions + .iter() + .filter(|e| e.kind == ExtensionKind::Mcp && e.agents.iter().any(|a| a == "dsh")) + .map(|e| e.id.as_str()) + .min(); for ext in extensions { - let (content, mcp_command, mcp_args, mcp_env, file_path) = match ext.kind { + let (content, raw_config, mcp_command, mcp_args, mcp_env, file_path) = match ext.kind { ExtensionKind::Skill => { let (skill_content, skill_path) = find_skill_content(adapters, &ext.id, &ext.agents); ( skill_content, + String::new(), None, vec![], Default::default(), @@ -518,6 +530,8 @@ pub fn audit_extensions( let mut cmd = None; let mut args = vec![]; let mut env = std::collections::HashMap::new(); + let mut raw_config = String::new(); + let mut file_path = ext.name.clone(); for a in adapters { if !ext.agents.contains(&a.name().to_string()) { continue; @@ -531,11 +545,30 @@ pub fn audit_extensions( // not env — merge them in so the secret-scanning // audit rules cover Authorization tokens too. env.extend(server.headers); + // dsh: feed the raw home patch text so the + // `!!js`-aware boot-risk rule sees the tags (YAML + // parsing strips them) and reports real file:line. + // It rides `raw_config`, NOT `content`, so no + // other rule sees a whole file of other servers' + // rows on this one extension. + // Attribution semantics: the patch file is shared + // by every dsh MCP row, so its findings attach to + // one designated row only (also reads the file + // once); file:line points at the offending line, + // which may belong to another server's block. + if a.name() == "dsh" && dsh_patch_host == Some(ext.id.as_str()) { + let patch = a.mcp_config_path(); + raw_config = + std::fs::read_to_string(&patch).unwrap_or_default(); + file_path = patch.to_string_lossy().to_string(); + } break; } } } - (String::new(), cmd, args, env, ext.name.clone()) + // MCP rows carry no `content`: their risk surface is the + // command/args/env fields, not a document. + (String::new(), raw_config, cmd, args, env, file_path) } ExtensionKind::Hook => { let raw_command = ext @@ -546,6 +579,7 @@ pub fn audit_extensions( .to_string(); ( raw_command, + String::new(), None, vec![], Default::default(), @@ -556,9 +590,10 @@ pub fn audit_extensions( let plugin_dir = ext.source_path.as_deref().unwrap_or(&ext.name); let content = read_plugin_content(plugin_dir); let file_path = ext.source_path.clone().unwrap_or_else(|| ext.name.clone()); - (content, None, vec![], Default::default(), file_path) + (content, String::new(), None, vec![], Default::default(), file_path) } ExtensionKind::Cli => ( + String::new(), String::new(), None, vec![], @@ -572,6 +607,7 @@ pub fn audit_extensions( kind: ext.kind, name: ext.name.clone(), content, + raw_config, source: ext.source.clone(), file_path, mcp_command, @@ -612,6 +648,8 @@ fn audit_extension_by_name( kind: ext.kind, name: ext.name.clone(), content, + // Skills-only path: no agent config file is involved. + raw_config: String::new(), source: ext.source.clone(), file_path: file_path.unwrap_or_else(|| ext.name.clone()), mcp_command: None, @@ -696,6 +734,26 @@ fn read_plugin_content(plugin_path: &str) -> String { parts.join("\n") } +/// Delegate a plugin uninstall to the agent's own CLI (see +/// [`adapter::PluginRemoval::Command`]). A missing binary is reported with the +/// exact command to run by hand rather than silently falling back to deleting +/// files, which is precisely what the agent's uninstaller exists to avoid. +fn run_agent_uninstall(program: &str, args: &[String]) -> Result<(), HkError> { + let display = format!("{program} {}", args.join(" ")); + match std::process::Command::new(program).args(args).output() { + Ok(out) if out.status.success() => Ok(()), + Ok(out) => Err(HkError::CommandFailed(format!( + "`{display}` failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + ))), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(HkError::Validation(format!( + "`{program}` is not on PATH, and removing the files directly would leave \ + a broken install — run `{display}` yourself, then rescan." + ))), + Err(e) => Err(HkError::CommandFailed(e.to_string())), + } +} + fn remove_path(path: &std::path::Path) -> Result<(), HkError> { if path.is_dir() { std::fs::remove_dir_all(path)?; @@ -947,11 +1005,8 @@ pub fn delete_extension( continue; } for plugin in adapter.read_plugins() { - if scanner::stable_id_for( - &format!("{}:{}", plugin.name, plugin.source), - "plugin", - adapter.name(), - ) != id + if scanner::plugin_extension_id(&plugin.name, &plugin.source, adapter.name()) + != id { continue; } @@ -1022,8 +1077,28 @@ pub fn delete_extension( &plugin.name, false, )?; - } else if let Some(ref path) = plugin.path { - remove_path(path)?; + } else { + // Everyone else answers through the adapter, so an + // agent whose plugins are not simply directories is + // never handed to the file fallback below. + match adapter.plugin_removal(&plugin) { + crate::adapter::PluginRemoval::Shipped => { + return Err(HkError::Validation(format!( + "'{}' ships with {} — it is not HarnessKit's to delete. \ + Disable it instead.", + ext.name, + adapter.name() + ))); + } + crate::adapter::PluginRemoval::Command { program, args } => { + run_agent_uninstall(&program, &args)?; + } + crate::adapter::PluginRemoval::Files => { + if let Some(ref path) = plugin.path { + remove_path(path)?; + } + } + } } } } @@ -1222,11 +1297,8 @@ pub fn get_extension_content( continue; } for plugin in adapter.read_plugins() { - if scanner::stable_id_for( - &format!("{}:{}", plugin.name, plugin.source), - "plugin", - adapter.name(), - ) == id + if scanner::plugin_extension_id(&plugin.name, &plugin.source, adapter.name()) + == id { let path_str = plugin .path @@ -1578,6 +1650,88 @@ mod tests { assert!(!is_update_eligible(&mcp)); } + #[test] + fn test_audit_dsh_patch_findings_attach_to_first_mcp_row_only() { + use crate::adapter::dsh::DshAdapter; + + let dir = TempDir::new().unwrap(); + std::fs::create_dir_all(dir.path().join(".dsh")).unwrap(); + // Two dsh MCP servers share ONE patch file with ONE bad `!!js` line + // (on github) and ONE plaintext secret (on memory). + std::fs::write( + dir.path().join(".dsh/cordis.patch.yml"), + r#"- insert: + - id: mcp-github + name: '@deepseek-ai/dsh-mcp-client' + config: + serverName: github + transport: stdio + command: npx + env: + GITHUB_TOKEN: !!js process.env.GITHUB_TOKEN + - id: mcp-memory + name: '@deepseek-ai/dsh-mcp-client' + config: + serverName: memory + transport: stdio + command: npx + env: + MEMORY_TOKEN: ghp_abc123def456ghi789jkl012mno345pqr678 +"#, + ) + .unwrap(); + + let adapter = DshAdapter::with_home(dir.path().to_path_buf()); + let extensions = crate::scanner::scan_mcp_servers(&adapter); + assert_eq!(extensions.len(), 2, "both servers should scan"); + + let adapters: Vec> = + vec![Box::new(DshAdapter::with_home(dir.path().to_path_buf()))]; + let results = audit_extensions(&extensions, &adapters); + + // The shared patch file's findings attach to exactly ONE row (the + // first dsh MCP row) — never duplicated onto every dsh row. The + // file:line may still point into another server's block; that is the + // remaining, documented imprecision of reading one shared file. + let total_rule_findings: usize = results + .iter() + .flat_map(|r| &r.findings) + .filter(|f| f.rule_id == "dsh-js-env-no-fallback") + .count(); + assert_eq!(total_rule_findings, 1); + let rows_with_rule = results + .iter() + .filter(|r| { + r.findings + .iter() + .any(|f| f.rule_id == "dsh-js-env-no-fallback") + }) + .count(); + assert_eq!(rows_with_rule, 1); + + // The patch text rides `raw_config`, so no OTHER rule sees it: the + // row that carries the file must not inherit its neighbour's secret. + let github = results + .iter() + .find(|r| r.extension_id == extensions[0].id) + .expect("first row audited"); + assert!( + !github + .findings + .iter() + .any(|f| f.rule_id == "plaintext-secrets"), + "neighbour's token leaked onto the row carrying the shared file: {:?}", + github.findings + ); + // …while the row that actually owns the secret still reports it once. + let secret_reports: usize = results + .iter() + .flat_map(|r| &r.findings) + .filter(|f| f.rule_id == "plaintext-secrets") + .count(); + assert_eq!(secret_reports, 1); + } + #[test] fn test_record_skill_revision_stamps_all_siblings() { // Regression: after a delegated (skills-CLI) update, the deployed files @@ -2733,6 +2887,100 @@ mod tests { ); } + #[test] + fn run_agent_uninstall_reports_a_missing_binary_without_deleting_anything() { + // The whole point of delegating is that the agent's own uninstaller + // reconciles manifests HarnessKit must not edit. If its binary is + // absent, falling back to deleting files would do exactly the damage + // the delegation exists to prevent — so this must fail loud and name + // the command the user can run instead. + let err = run_agent_uninstall( + "hk-nonexistent-agent-binary", + &["plugin".into(), "remove".into(), "x".into()], + ) + .unwrap_err(); + let HkError::Validation(msg) = err else { + panic!("a missing binary is a user-actionable refusal, got {err:?}"); + }; + assert!(msg.contains("not on PATH"), "{msg}"); + assert!( + msg.contains("hk-nonexistent-agent-binary plugin remove x"), + "the message must carry the exact command: {msg}" + ); + } + + #[test] + fn run_agent_uninstall_maps_a_nonzero_exit_to_a_failure() { + let err = run_agent_uninstall("sh", &["-c".into(), "echo boom >&2; exit 1".into()]) + .unwrap_err(); + let HkError::CommandFailed(msg) = err else { + panic!("expected CommandFailed, got {err:?}"); + }; + assert!(msg.contains("boom"), "stderr reaches the user: {msg}"); + } + + #[test] + fn test_delete_extension_refuses_dsh_plugin_and_keeps_its_package() { + use crate::adapter; + + let dir = TempDir::new().unwrap(); + let home = dir.path(); + let farm = home.join(".dsh/profiles/node_modules"); + + // A bundle whose patch defines one row, plus the package that row + // instantiates — the layout every one of dsh's own rows has. + let bundle = farm.join("@deepseek-ai/dsh-base"); + std::fs::create_dir_all(&bundle).unwrap(); + std::fs::write( + bundle.join("package.json"), + r#"{"name": "@deepseek-ai/dsh-base", "dsh": {"bundle": {"patch": "./cordis.patch.yml"}}}"#, + ) + .unwrap(); + std::fs::write( + bundle.join("cordis.patch.yml"), + "- insert:\n - id: timer\n name: '@deepseek-ai/cordis-plugin-timer'\n", + ) + .unwrap(); + let package = farm.join("@deepseek-ai/cordis-plugin-timer"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write(package.join("package.json"), r#"{"name": "timer"}"#).unwrap(); + + let profile = home.join(".dsh/profiles/web"); + std::fs::create_dir_all(&profile).unwrap(); + std::fs::write( + profile.join("package.json"), + r#"{"dsh": {"profile": {"bundles": ["@deepseek-ai/dsh-base"]}}}"#, + ) + .unwrap(); + std::fs::write(profile.join("cordis.patch.yml"), "[]\n").unwrap(); + + let store = Mutex::new(Store::open(&home.join("test.db")).unwrap()); + let adapters: Vec> = vec![Box::new( + adapter::dsh::DshAdapter::with_home(home.to_path_buf()), + )]; + store + .lock() + .sync_extensions(&scanner::scan_all(&adapters, &[])) + .unwrap(); + let all = store.lock().list_extensions(None, None).unwrap(); + let id = all + .iter() + .find(|e| e.kind == ExtensionKind::Plugin && e.name == "timer") + .expect("scanned dsh plugin row should be in the store") + .id + .clone(); + + // The generic fallback would remove_dir_all() the package the row + // still names, breaking dsh's boot while the row itself survives. + let err = delete_extension(&store, &adapters, &id).unwrap_err(); + assert!(matches!(err, HkError::Validation(_)), "got {err:?}"); + assert!(package.is_dir(), "the package must survive the refusal"); + assert!( + store.lock().get_extension(&id).unwrap().is_some(), + "a refused delete must not drop the DB row either" + ); + } + // ---- install_to_agent target_scope ------------------------------------- /// Register `path` in the projects table and return the matching scope. diff --git a/crates/hk-core/tests/toggle_integration.rs b/crates/hk-core/tests/toggle_integration.rs index 676744e9..0969c419 100644 --- a/crates/hk-core/tests/toggle_integration.rs +++ b/crates/hk-core/tests/toggle_integration.rs @@ -445,3 +445,180 @@ fn test_dsh_mcp_native_toggle_roundtrip() { let servers = DshAdapter::with_home(dir.path().to_path_buf()).read_mcp_servers(); assert!(servers[0].enabled); } + +/// Cross-layer joint test for the dsh plugin toggle: the SCANNER +/// (`adapter::dsh::read_plugins`, which folds profile row + home override and +/// deliberately INCLUDES HK's managed block, because the block is effective +/// state dsh applies) against the WRITER (`deployer::set_dsh_plugin_enabled`, +/// which folds owning layer + home *user* text and deliberately EXCLUDES the +/// block, because the block is HK's own output, not base state). +/// +/// Both implement the same upstream compose rule with opposite block +/// treatment, and each is otherwise only covered in isolation — so the round +/// trip is the only thing that catches them drifting apart. In particular, if +/// the writer ever folded its own block back in, the re-enable below would +/// compute "already at base", write nothing, and the plugin would stay +/// disabled forever. +#[test] +fn test_dsh_profile_plugin_toggle_roundtrip_scanner_and_writer_agree() { + use hk_core::adapter::dsh::DshAdapter; + use hk_core::adapter::AgentAdapter; + + let dir = TempDir::new().unwrap(); + let store = Store::open(&dir.path().join("test.db")).unwrap(); + + // A real .dsh tree: one profile whose patch layer DEFINES the plugin row + // (`tool-policy`), with the package present in its node_modules. No home + // patch yet — the writer must create it. + let profile = dir.path().join(".dsh/profiles/web"); + std::fs::create_dir_all(profile.join("node_modules/dsh-plugin-tool")).unwrap(); + std::fs::write( + profile.join("package.json"), + r#"{"dependencies": {"dsh-plugin-tool": "1.0.0"}, "dsh": {"profile": {"bundles": []}}}"#, + ) + .unwrap(); + let profile_patch = profile.join("cordis.patch.yml"); + let profile_patch_text = + "- insert:\n - id: tool-policy\n name: dsh-plugin-tool\n config:\n mode: strict\n"; + std::fs::write(&profile_patch, profile_patch_text).unwrap(); + + let adapter = || DshAdapter::with_home(dir.path().to_path_buf()); + let adapters: Vec> = vec![Box::new(adapter())]; + let rescan = || { + let exts = hk_core::scanner::scan_plugins(&adapter()); + // Named by its patch row id, exactly as dsh's own plugin list shows + // it; the package (`dsh-plugin-tool`) rides in the description. + let row = exts + .into_iter() + .find(|e| e.name == "tool-policy") + .expect("profile row must survive every rescan"); + (row.id.clone(), row.enabled) + }; + + // Scan → enabled, and the row is stored the way the manager will find it. + let (ext_id, enabled) = rescan(); + assert!(enabled, "a plain profile row starts enabled"); + store + .sync_extensions(&hk_core::scanner::scan_plugins(&adapter())) + .unwrap(); + + // Toggle off → rescan reads the writer's block back as disabled. + hk_core::manager::toggle_extension_with_adapters(&store, &adapters, &ext_id, false).unwrap(); + let (id_after, enabled) = rescan(); + assert_eq!(id_after, ext_id, "identity is stable across the toggle"); + assert!(!enabled, "scanner must see the writer's managed block"); + let home = std::fs::read_to_string(dir.path().join(".dsh/cordis.patch.yml")).unwrap(); + assert!(home.contains("- id: tool-policy\n disabled: true"), "{home}"); + // Only the home file is written; the profile layer is untouched. + assert_eq!( + std::fs::read_to_string(&profile_patch).unwrap(), + profile_patch_text + ); + + // Toggle on → back to the profile's own base state, so the whole block + // goes away rather than becoming a redundant `disabled: false`. + hk_core::manager::toggle_extension_with_adapters(&store, &adapters, &ext_id, true).unwrap(); + let (id_after, enabled) = rescan(); + assert_eq!(id_after, ext_id); + assert!(enabled, "re-enable must be visible to the scanner"); + let home = std::fs::read_to_string(dir.path().join(".dsh/cordis.patch.yml")).unwrap(); + assert!(!home.contains("managed by HarnessKit"), "block gone: {home}"); + assert!(!home.contains("disabled"), "no leftover override: {home}"); +} + +/// A row provided by a mounted BUNDLE is toggleable exactly like a user row: +/// the write lands in the HOME patch and the bundle's own patch file — which +/// HK must never edit — stays byte-identical. Also pins the `hmr` case: the +/// row is DEFINED enabled by one bundle and DISABLED by a later one, so the +/// base state only comes out right if the whole layer chain is folded. +#[test] +fn test_dsh_bundle_row_toggle_writes_home_patch_and_never_the_bundle_patch() { + use hk_core::adapter::dsh::DshAdapter; + use hk_core::adapter::AgentAdapter; + + let dir = TempDir::new().unwrap(); + let store = Store::open(&dir.path().join("test.db")).unwrap(); + + // dsh's symlink farm with two in-box bundles, mirroring rc.6: base + // defines `timer` and `hmr`; web-app disables `hmr` two layers later. + let farm = dir.path().join(".dsh/profiles/node_modules/@deepseek-ai"); + let base_patch_text = "- insert:\n - id: timer\n name: '@deepseek-ai/cordis-plugin-timer'\n - id: hmr\n name: '@deepseek-ai/cordis-plugin-hmr'\n"; + let web_app_patch_text = "- id: hmr\n disabled: true\n"; + for (pkg, patch) in [ + ("dsh-base", base_patch_text), + ("dsh-web-app", web_app_patch_text), + ] { + let d = farm.join(pkg); + std::fs::create_dir_all(&d).unwrap(); + std::fs::write( + d.join("package.json"), + r#"{"dsh": {"bundle": {"patch": "./cordis.patch.yml"}}}"#, + ) + .unwrap(); + std::fs::write(d.join("cordis.patch.yml"), patch).unwrap(); + } + let base_patch = farm.join("dsh-base/cordis.patch.yml"); + let web_app_patch = farm.join("dsh-web-app/cordis.patch.yml"); + + let profile = dir.path().join(".dsh/profiles/web"); + std::fs::create_dir_all(&profile).unwrap(); + std::fs::write( + profile.join("package.json"), + r#"{"dependencies": {}, "dsh": {"profile": {"bundles": ["@deepseek-ai/dsh-base", "@deepseek-ai/dsh-web-app"]}}}"#, + ) + .unwrap(); + + let adapter = || DshAdapter::with_home(dir.path().to_path_buf()); + let adapters: Vec> = vec![Box::new(adapter())]; + let home = dir.path().join(".dsh/cordis.patch.yml"); + let rescan = |name: &str| { + hk_core::scanner::scan_plugins(&adapter()) + .into_iter() + .find(|e| e.name == name) + .map(|e| (e.id.clone(), e.enabled)) + .expect("bundle row must survive every rescan") + }; + let bundle_patches_untouched = || { + assert_eq!(std::fs::read_to_string(&base_patch).unwrap(), base_patch_text); + assert_eq!( + std::fs::read_to_string(&web_app_patch).unwrap(), + web_app_patch_text + ); + }; + + // The bundle itself is not an extension; only its rows are — under + // neither its package name nor the `package ` slot of a row source. + let exts = hk_core::scanner::scan_plugins(&adapter()); + assert!(exts.iter().all(|e| e.name != "@deepseek-ai/dsh-base" + && !e.description.ends_with("package @deepseek-ai/dsh-base"))); + store.sync_extensions(&exts).unwrap(); + + // `timer`: enabled at base state → disable writes an override. + let (timer_id, enabled) = rescan("timer"); + assert!(enabled); + hk_core::manager::toggle_extension_with_adapters(&store, &adapters, &timer_id, false).unwrap(); + let (_, enabled) = rescan("timer"); + assert!(!enabled, "scanner must see the writer's managed block"); + let home_text = std::fs::read_to_string(&home).unwrap(); + assert!(home_text.contains("- id: timer\n disabled: true"), "{home_text}"); + bundle_patches_untouched(); + + // Back to base state → the block goes away entirely. + hk_core::manager::toggle_extension_with_adapters(&store, &adapters, &timer_id, true).unwrap(); + assert!(rescan("timer").1); + let home_text = std::fs::read_to_string(&home).unwrap(); + assert!(!home_text.contains("managed by HarnessKit"), "{home_text}"); + bundle_patches_untouched(); + + // `hmr`: base state is DISABLED (by the second bundle), so enabling must + // write an explicit `disabled: false` rather than being a silent no-op — + // the whole-chain fold is what makes this correct. + let (hmr_id, enabled) = rescan("hmr"); + assert!(!enabled, "the web-app bundle layer disables hmr"); + hk_core::manager::toggle_extension_with_adapters(&store, &adapters, &hmr_id, true).unwrap(); + let (_, enabled) = rescan("hmr"); + assert!(enabled, "enable must survive the rescan, not fold back to base"); + let home_text = std::fs::read_to_string(&home).unwrap(); + assert!(home_text.contains("- id: hmr\n disabled: false"), "{home_text}"); + bundle_patches_untouched(); +} diff --git a/src/components/extensions/extension-detail.tsx b/src/components/extensions/extension-detail.tsx index b6b62f0d..17405ae8 100644 --- a/src/components/extensions/extension-detail.tsx +++ b/src/components/extensions/extension-detail.tsx @@ -27,6 +27,7 @@ import { ScopeTargetField } from "@/components/shared/scope-target-field"; import { canInstallAtScope, canReceiveMcpTransport, + isVendorBaseline, } from "@/lib/agent-capabilities"; import { copyPathToClipboard } from "@/lib/copy-path"; import i18n from "@/lib/i18n"; @@ -982,13 +983,35 @@ export function ExtensionDetail() { {/* 10. Delete trigger */}
- + {(() => { + // A plugin that ships with its agent has no delete: the backend + // refuses it, because removing the files would leave the agent's + // own manifests naming a package that is gone. + const shipped = isVendorBaseline( + group.pack, + agents.flatMap((a) => a.capabilities?.vendor_baseline_packs ?? []), + ); + return ( + + ); + })()}
{/* Delete confirmation dialog */} diff --git a/src/components/extensions/extension-filters.tsx b/src/components/extensions/extension-filters.tsx index bbda6d89..9a6974dd 100644 --- a/src/components/extensions/extension-filters.tsx +++ b/src/components/extensions/extension-filters.tsx @@ -5,6 +5,7 @@ import { useTranslation } from "react-i18next"; import { agentDisplayName, type ExtensionKind, sortAgents } from "@/lib/types"; import { isWeb as web, webSelectStyle } from "@/lib/web-select"; import { useAgentStore } from "@/stores/agent-store"; +import { BUILTIN_PACK_PREFIX } from "@/stores/extension-helpers"; import { useExtensionStore } from "@/stores/extension-store"; import { useScopeStore } from "@/stores/scope-store"; @@ -63,13 +64,33 @@ export function ExtensionFilters() { const grouped = useExtensionStore((s) => s.grouped); const filtered = useExtensionStore((s) => s.filtered); const scope = useScopeStore((s) => s.current); + const hideVendorBaseline = useExtensionStore((s) => s.hideVendorBaseline); + const setHideVendorBaseline = useExtensionStore( + (s) => s.setHideVendorBaseline, + ); + const agents = useAgentStore((s) => s.agents); + /** `pack -> the agent that ships it`, from the backend capabilities. */ + const shippedBy = useMemo(() => { + const map = new Map(); + for (const a of agents) { + for (const p of a.capabilities?.vendor_baseline_packs ?? []) { + map.set(p, a.name); + } + } + return map; + }, [agents]); // Source dropdown options + counts are scoped: a project shouldn't show // packs that only exist globally (and vice versa). We deliberately don't // narrow by kind/agent/tag/search — those filter the rows further; the // dropdown options should stay stable as the user toggles them. + // + // An agent's shipped bundles collapse into ONE option: dsh spreads its + // baseline over three of them, which is an implementation detail of how it + // ships, not three sources the user chose between. // biome-ignore lint/correctness/useExhaustiveDependencies: `extensions` is a trigger sentinel — grouped() reads it via Zustand closure; needed in deps so the memo re-runs on store updates. - const { scopedPacks, packCounts } = useMemo(() => { + const { scopedPacks, packCounts, builtinCounts } = useMemo(() => { const counts = new Map(); + const builtins = new Map(); for (const g of grouped()) { if (!g.pack) continue; if (scope.type !== "all") { @@ -80,14 +101,23 @@ export function ExtensionFilters() { }); if (!matches) continue; } - counts.set(g.pack, (counts.get(g.pack) ?? 0) + 1); + const owner = shippedBy.get(g.pack); + if (owner) { + builtins.set(owner, (builtins.get(owner) ?? 0) + 1); + } else { + counts.set(g.pack, (counts.get(g.pack) ?? 0) + 1); + } } return { scopedPacks: [...counts.keys()].sort(), packCounts: counts, + builtinCounts: builtins, }; - }, [grouped, extensions, scope]); - const agents = useAgentStore((s) => s.agents); + }, [grouped, extensions, scope, shippedBy]); + const hideableCount = useMemo( + () => [...builtinCounts.values()].reduce((a, b) => a + b, 0), + [builtinCounts], + ); const agentOrder = useAgentStore((s) => s.agentOrder); const enabledAgents = useMemo( () => @@ -99,8 +129,14 @@ export function ExtensionFilters() { ); const resultCount = filtered().length; + // The built-in group value is synthetic and never appears in `scopedPacks`, + // so it has to survive the stale-filter reset below. useEffect(() => { - if (packFilter && !scopedPacks.includes(packFilter)) { + if ( + packFilter && + !packFilter.startsWith(BUILTIN_PACK_PREFIX) && + !scopedPacks.includes(packFilter) + ) { setPackFilter(null); } }, [packFilter, scopedPacks, setPackFilter]); @@ -133,6 +169,32 @@ export function ExtensionFilters() { {t("filters.resultCount", { count: resultCount })} + {/* A view mode, deliberately outside the filter set: it carries its + own always-visible lit state, so Clear filters neither advertises + nor resets it. + + Read like the kind pills — the label names the mode and never + changes, the tint means it is on. Flipping the label to the next + action instead ("Show built-in") would contradict the tint, since + a lit control reads as "on" while the verb claims the opposite. + + No count: the result count sits right next to it and already moves + when this is toggled. Rendered only where something is actually + hideable, so agents that ship no extensions never see it. */} + {hideableCount > 0 && ( + + )} {(kindFilter || agentFilter || packFilter || searchQuery) && (