From 03f5847b3bf798e6220b4d6068162a5684094617 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 21:04:54 +0800 Subject: [PATCH 01/11] feat: scan dsh plugins from composed patch rows and skip dropped skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dsh's own vocabulary is the composed cordis patch ROW: Settings → Plugins lists one entry per row (`timer`, `hmr`, `llm`, …) and never lists a bundle, because a bundle is a patch LAYER that inserts rows. `read_plugins` mirrors that with two sources per profile: rows defined by each mounted bundle's own patch file (resolved via `dsh.bundle.patch`, from the symlink farm for in-box bundles) and rows defined by the user's own home/profile patch files. Composition follows dsh's: bundle patches in `bundles` order, then the profile patch, then the home patch. The earliest layer defining a row id owns the entry (upstream, a later restatement can only override), while `enabled` folds `disabled` across the WHOLE chain — which is why `hmr`, defined enabled by dsh-base and disabled by dsh-web-app, reads as disabled exactly like dsh shows it. Identity carries profile + bundle + row id, since two profiles compose different chains and can disagree about one row. Packages that are profile dependencies but which no layer mounts are NOT listed: dsh warns once at install time and then never loads or displays them. Same principle applies to skills — dsh drops a skill whose frontmatter uses a camelCase invocation key, so `scan_skill_dir` emits no extension for it under dsh. In a shared skills root that removes only dsh from the skill's agent list; a dsh-only skill disappears from HK entirely. The detector is a small `pub(crate)` predicate beside `CAMELCASE_INVOCATION_KEYS`, so the rejected-key vocabulary keeps one home while the audit rule's own scan is untouched. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/adapter/dsh.rs | 1019 ++++++++++++++++++- crates/hk-core/src/auditor/rules.rs | 4 + crates/hk-core/src/auditor/rules/content.rs | 37 + crates/hk-core/src/scanner.rs | 84 +- 4 files changed, 1090 insertions(+), 54 deletions(-) diff --git a/crates/hk-core/src/adapter/dsh.rs b/crates/hk-core/src/adapter/dsh.rs index 76f9a96..92aa0b4 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -82,25 +82,97 @@ 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"; + +/// 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 +194,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 +262,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 +478,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 { @@ -290,6 +544,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 { @@ -338,6 +621,171 @@ 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, + source_url: None, + uri, + installed_at: None, + updated_at: None, + }); + } + + // --- 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)); + + // 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, + }); + } + } + entries + } + fn hook_format(&self) -> HookFormat { HookFormat::None } @@ -590,6 +1038,15 @@ 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 mcp_row_id_lookup_by_server_name() { assert_eq!( @@ -645,6 +1102,470 @@ 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); + + // 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 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; diff --git a/crates/hk-core/src/auditor/rules.rs b/crates/hk-core/src/auditor/rules.rs index 46e34cc..f264d77 100644 --- a/crates/hk-core/src/auditor/rules.rs +++ b/crates/hk-core/src/auditor/rules.rs @@ -16,6 +16,10 @@ pub use content::{ CredentialTheft, DangerousCommands, 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, diff --git a/crates/hk-core/src/auditor/rules/content.rs b/crates/hk-core/src/auditor/rules/content.rs index c0b58a1..eeb9c7c 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" @@ -650,6 +671,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; diff --git a/crates/hk-core/src/scanner.rs b/crates/hk-core/src/scanner.rs index b729c8f..ad99791 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(|| { @@ -553,11 +573,7 @@ pub fn scan_plugins(adapter: &dyn AgentAdapter) -> Vec { let pack = 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 +2768,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(); From 05238954de1448eee517ccd71c4fd2a9d3a48403 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 09:01:10 +0800 Subject: [PATCH 02/11] refactor: generalize dsh managed block to structured toggle+insert model Co-Authored-By: Claude Fable 5 --- crates/hk-core/src/deployer.rs | 337 ++++++++++++++++++++++++++++----- 1 file changed, 293 insertions(+), 44 deletions(-) diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index ffb1b3c..657f7f7 100644 --- a/crates/hk-core/src/deployer.rs +++ b/crates/hk-core/src/deployer.rs @@ -663,6 +663,66 @@ 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() + } + + fn insert_server_name(row: &serde_yaml::Mapping) -> Option<&str> { + 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)) + } + + // consumed by T7/T8; attribute self-expires when they land + #[expect(dead_code)] + 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 + } + + // consumed by T7/T8; attribute self-expires when they land + #[expect(dead_code)] + 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() + } + + // consumed by T7/T8; attribute self-expires when they land + #[expect(dead_code)] + 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 @@ -694,7 +754,27 @@ pub fn set_dsh_mcp_enabled( 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)?; + // T7/T8 copy this pattern: the block parser is path-agnostic, so the call + // site owns naming the file and the remediation hint. + let (user_text, mut block) = 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, + })?; + + // 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 +790,28 @@ 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", - home_patch.display() - ))); - } - atomic_write(home_patch, &new_text) + write_dsh_patch(home_patch, &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). +/// 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 +834,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 +849,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 +952,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 +979,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( @@ -4258,6 +4428,85 @@ mod dsh_toggle_tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), text); } + #[test] + fn corrupted_block_yaml_errors_and_leaves_file_untouched() { + // HK owns every byte inside the markers: unparseable block content is + // a hard ConfigCorrupted, refuse to write. + let text = format!( + "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- id: [unclosed\n{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(_))); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + + #[test] + fn unrecognized_block_entry_errors_instead_of_silent_drop() { + // Behavior change pinned on purpose: entries HK did not render are + // corruption, not noise to discard. + let text = format!( + "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- surprise: true\n{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(_))); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + + #[test] + fn toggle_entry_with_extra_keys_errors() { + // Extra keys on an id-targeted entry are LIVE dsh patch semantics + // (they would patch the target row) — dropping them on re-render + // would alter the user's effective config, so they are corruption. + let text = format!( + "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- id: mcp-github\n disabled: true\n command: pwned\n{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(_))); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + + #[test] + fn insert_group_with_extra_keys_errors() { + let text = format!( + "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- insert:\n - id: mcp-x\n after: mcp-github\n{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(_))); + assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + } + + #[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 From 3edd68b1fe45f77d0c21df0a5a5c22f1ed0e16f4 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 21:07:46 +0800 Subject: [PATCH 03/11] feat: dsh plugin enable/disable via home patch managed block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toggling a dsh plugin appends an `{id, disabled}` override inside HarnessKit's comment-marker block at the end of the HOME `cordis.patch.yml` — the precedent is dsh's own web-app bundle, which disables base rows exactly that way. The home layer is applied after every profile layer, so the toggle is machine-global, like the P0 MCP toggle, and hot-reload picks it up live. What is new over the P0 helpers is the base state. P0 folded row ids in the home file's text alone; a plugin row can live in a bundle patch or a profile patch, and its base state is the fold across the WHOLE layer chain, not the layer that defines it. `hmr` is the proof: dsh-base defines it enabled and dsh-web-app disables it two layers later, so folding only the defining layer would read it as enabled, make "enable" look like a no-op against base, drop the override and leave the row disabled. `PluginEntry` therefore carries `base_layers` — that profile's ordered layers below the home patch, produced by the scan — and `deployer::set_dsh_plugin_enabled` folds exactly those plus the home patch while writing only the home file. Bundle patch files stay read-only inputs. The `layer == home_patch` identity guard is load-bearing: a home-defined row must fold the block-stripped user text, not HK's own block. Bundle-provided rows are toggleable like any other row. Anonymous rows (no `id:`) are not, and get a backend `Validation` error advising an `id:` — UI graying alone is not a guard. `toggle_plugin` gains an explicit dsh branch BEFORE the generic manifest-rename fallback, which would otherwise hunt for a `plugin.json` in node_modules and rename it, corrupting a pnpm-managed tree. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/adapter/claude.rs | 1 + crates/hk-core/src/adapter/codex.rs | 1 + crates/hk-core/src/adapter/copilot.rs | 2 + crates/hk-core/src/adapter/cursor.rs | 2 + crates/hk-core/src/adapter/dsh.rs | 15 + crates/hk-core/src/adapter/gemini.rs | 1 + crates/hk-core/src/adapter/hermes.rs | 1 + crates/hk-core/src/adapter/mod.rs | 23 +- crates/hk-core/src/adapter/omp.rs | 2 + crates/hk-core/src/adapter/opencode.rs | 1 + crates/hk-core/src/deployer.rs | 328 ++++++++++++++++++++- crates/hk-core/src/manager.rs | 237 ++++++++++++--- crates/hk-core/src/service.rs | 14 +- crates/hk-core/tests/toggle_integration.rs | 177 +++++++++++ 14 files changed, 757 insertions(+), 48 deletions(-) diff --git a/crates/hk-core/src/adapter/claude.rs b/crates/hk-core/src/adapter/claude.rs index e2b49e6..d4584da 100644 --- a/crates/hk-core/src/adapter/claude.rs +++ b/crates/hk-core/src/adapter/claude.rs @@ -373,6 +373,7 @@ impl AgentAdapter for ClaudeAdapter { uri: None, installed_at, updated_at, + base_layers: vec![], }); } entries diff --git a/crates/hk-core/src/adapter/codex.rs b/crates/hk-core/src/adapter/codex.rs index 7211f98..5dc0566 100644 --- a/crates/hk-core/src/adapter/codex.rs +++ b/crates/hk-core/src/adapter/codex.rs @@ -435,6 +435,7 @@ impl AgentAdapter for CodexAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); 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 eebdaba..cb7ddd0 100644 --- a/crates/hk-core/src/adapter/copilot.rs +++ b/crates/hk-core/src/adapter/copilot.rs @@ -265,6 +265,7 @@ impl AgentAdapter for CopilotAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } } @@ -316,6 +317,7 @@ impl AgentAdapter for CopilotAdapter { uri: Some(plugin_uri.to_string()), installed_at: None, updated_at: None, + base_layers: vec![], }); } } diff --git a/crates/hk-core/src/adapter/cursor.rs b/crates/hk-core/src/adapter/cursor.rs index 8e40ce2..b9eb97f 100644 --- a/crates/hk-core/src/adapter/cursor.rs +++ b/crates/hk-core/src/adapter/cursor.rs @@ -241,6 +241,7 @@ impl AgentAdapter for CursorAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } } @@ -279,6 +280,7 @@ impl AgentAdapter for CursorAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } } diff --git a/crates/hk-core/src/adapter/dsh.rs b/crates/hk-core/src/adapter/dsh.rs index 92aa0b4..4f513eb 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -680,6 +680,9 @@ impl AgentAdapter for DshAdapter { 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()], }); } @@ -721,6 +724,7 @@ impl AgentAdapter for DshAdapter { }) .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 @@ -780,6 +784,7 @@ impl AgentAdapter for DshAdapter { uri, installed_at: None, updated_at: None, + base_layers: base_layers.clone(), }); } } @@ -1199,6 +1204,16 @@ mod tests { ); 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(); diff --git a/crates/hk-core/src/adapter/gemini.rs b/crates/hk-core/src/adapter/gemini.rs index 9fe6ee3..feb11a4 100644 --- a/crates/hk-core/src/adapter/gemini.rs +++ b/crates/hk-core/src/adapter/gemini.rs @@ -253,6 +253,7 @@ impl AgentAdapter for GeminiAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } entries diff --git a/crates/hk-core/src/adapter/hermes.rs b/crates/hk-core/src/adapter/hermes.rs index ddb168d..017b8c5 100644 --- a/crates/hk-core/src/adapter/hermes.rs +++ b/crates/hk-core/src/adapter/hermes.rs @@ -113,6 +113,7 @@ impl HermesAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }) } } diff --git a/crates/hk-core/src/adapter/mod.rs b/crates/hk-core/src/adapter/mod.rs index 0aeaba7..c04241a 100644 --- a/crates/hk-core/src/adapter/mod.rs +++ b/crates/hk-core/src/adapter/mod.rs @@ -255,13 +255,32 @@ 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, } /// Format used by an agent for hook configuration files. diff --git a/crates/hk-core/src/adapter/omp.rs b/crates/hk-core/src/adapter/omp.rs index 8b1bb39..670a2bf 100644 --- a/crates/hk-core/src/adapter/omp.rs +++ b/crates/hk-core/src/adapter/omp.rs @@ -246,6 +246,7 @@ impl AgentAdapter for OmpAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } else if path.is_dir() { // Directory-form extension: /index.{ts,js}, TypeScript @@ -280,6 +281,7 @@ impl AgentAdapter for OmpAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } } diff --git a/crates/hk-core/src/adapter/opencode.rs b/crates/hk-core/src/adapter/opencode.rs index d30edbb..628e650 100644 --- a/crates/hk-core/src/adapter/opencode.rs +++ b/crates/hk-core/src/adapter/opencode.rs @@ -231,6 +231,7 @@ impl AgentAdapter for OpencodeAdapter { uri: None, installed_at: None, updated_at: None, + base_layers: vec![], }); } } diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index 657f7f7..004331f 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)?; @@ -798,6 +798,106 @@ pub fn set_dsh_mcp_enabled( 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 original = match std::fs::read_to_string(home_patch) { + Ok(text) => text, + // Absent file = dsh has not seeded its template yet; a missing home + // layer still lets profile-defined rows be toggled from a fresh block. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => "[]\n".to_string(), + Err(e) => return Err(e.into()), + }; + let (user_text, mut block) = 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, + })?; + + // 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() + ))); + } + 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) +} + /// 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 @@ -4549,3 +4649,229 @@ mod dsh_toggle_tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), HOME_WITH_GH); } } + +#[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 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"); + } +} diff --git a/crates/hk-core/src/manager.rs b/crates/hk-core/src/manager.rs index 76e9506..c530dbd 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,100 @@ 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![], + } + } + + #[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/service.rs b/crates/hk-core/src/service.rs index dbaa23b..13e78e8 100644 --- a/crates/hk-core/src/service.rs +++ b/crates/hk-core/src/service.rs @@ -947,11 +947,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; } @@ -1222,11 +1219,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 diff --git a/crates/hk-core/tests/toggle_integration.rs b/crates/hk-core/tests/toggle_integration.rs index 676744e..0969c41 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(); +} From da10ddf3df38459a37f10fb15c4ebaa5753c6235 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 09:48:19 +0800 Subject: [PATCH 04/11] feat: dsh MCP insert writer for install, remove, and native toggle --- crates/hk-core/src/adapter/dsh.rs | 74 +++- crates/hk-core/src/deployer.rs | 701 +++++++++++++++++++++++++++--- 2 files changed, 709 insertions(+), 66 deletions(-) diff --git a/crates/hk-core/src/adapter/dsh.rs b/crates/hk-core/src/adapter/dsh.rs index 4f513eb..3befae8 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -167,6 +167,22 @@ impl DshAdapter { /// dsh's `@deepseek-ai/dsh-mcp-client` plugin name — the marker for MCP rows. 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. @@ -496,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 @@ -1052,6 +1087,43 @@ mod tests { 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!( diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index 004331f..05e861c 100644 --- a/crates/hk-core/src/deployer.rs +++ b/crates/hk-core/src/deployer.rs @@ -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), } } @@ -685,7 +682,14 @@ impl DshManagedBlock { 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() } @@ -695,8 +699,6 @@ impl DshManagedBlock { .find(|row| Self::insert_server_name(row) == Some(server_name)) } - // consumed by T7/T8; attribute self-expires when they land - #[expect(dead_code)] fn remove_insert(&mut self, server_name: &str) -> bool { let before = self.inserts.len(); self.inserts @@ -704,8 +706,6 @@ impl DshManagedBlock { self.inserts.len() != before } - // consumed by T7/T8; attribute self-expires when they land - #[expect(dead_code)] fn insert_row_ids(&self) -> Vec { self.inserts .iter() @@ -713,8 +713,6 @@ impl DshManagedBlock { .collect() } - // consumed by T7/T8; attribute self-expires when they land - #[expect(dead_code)] fn insert_server_names(&self) -> Vec { self.inserts .iter() @@ -743,27 +741,10 @@ 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()), - }; - // T7/T8 copy this pattern: the block parser is path-agnostic, so the call - // site owns naming the file and the remediation hint. - let (user_text, mut block) = 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, - })?; + // 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. @@ -842,21 +823,7 @@ pub fn set_dsh_plugin_enabled( ) -> Result<(), HkError> { use crate::adapter::dsh::DshAdapter; - let original = match std::fs::read_to_string(home_patch) { - Ok(text) => text, - // Absent file = dsh has not seeded its template yet; a missing home - // layer still lets profile-defined rows be toggled from a fresh block. - Err(e) if e.kind() == std::io::ErrorKind::NotFound => "[]\n".to_string(), - Err(e) => return Err(e.into()), - }; - let (user_text, mut block) = 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, - })?; + 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 @@ -898,6 +865,235 @@ pub fn set_dsh_plugin_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 once + // the Task-9 remote schema lands (until then it refuses all remotes). + 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) +} + +/// 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 @@ -1526,12 +1722,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()) { @@ -4638,16 +4829,6 @@ mod dsh_toggle_tests { let parsed: serde_yaml::Value = serde_yaml::from_str(&out).unwrap(); assert!(parsed.is_sequence()); } - - #[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); - 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); - } } #[cfg(test)] @@ -4875,3 +5056,393 @@ mod dsh_plugin_toggle_tests { 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(), 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 toggle_and_remove_work_through_the_original_name() { + use crate::adapter::AgentAdapter; + // The scanner now hands the manager the ORIGINAL name, so every + // by-name path must resolve it to the row stored under the sanitized + // `serverName` — and must still be a no-op-free round trip. + 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(); + + set_dsh_mcp_enabled(&path, "microsoft/markitdown", false).unwrap(); + let disabled = adapter.read_mcp_servers(); + assert_eq!(disabled[0].name, "microsoft/markitdown"); + assert!(!disabled[0].enabled, "toggle by original name reached the row"); + + set_dsh_mcp_enabled(&path, "microsoft/markitdown", true).unwrap(); + assert!(adapter.read_mcp_servers()[0].enabled); + + remove_mcp_server(&path, "microsoft/markitdown", McpFormat::DshCordis).unwrap(); + assert!(adapter.read_mcp_servers().is_empty()); + let text = std::fs::read_to_string(&path).unwrap(); + assert!(!text.contains("markitdown"), "row actually gone: {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. + 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)); + + 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 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"); + } +} From 39b0a00208fd32e5600e67a0ca657a3491a044b3 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 21:11:26 +0800 Subject: [PATCH 05/11] feat: advertise dsh remote MCP and detect kit-install conflicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes wiring dsh MCP install now that the insert writer works. dsh's remote rows are `{transport, url, headers}` YAML, which no existing `RemoteMcpSchema` variant describes — reusing `Toml` would break the documented "Codex is the only Toml agent" invariant — so add a `DshTransport` variant and update BOTH sites that switch on the schema: the SSE capability derivation in `AgentCapabilities::from_adapter` (http true, sse false) and `validate_remote_mcp_target`, which now refuses SSE for dsh with a clear message before anything is written. Missing either would let SSE servers look installable and reach the writer. The two now-stale `Toml`-is-the-only-HTTP comments in adapter/mod.rs are corrected alongside. `remote_mcp_schema()` is flipped last on purpose: advertising it before the writer existed would have offered an install path that always errored. Kit installs route through the same `deploy_mcp_server`, so dsh Kit MCP install starts working implicitly — which means the `kits/install_plan.rs` conflict-detection arm can no longer answer a hardcoded `false`. It now looks the serverName up in the home patch text, normalizing the query the same way the writer sanitizes what it stores, so a conflict is reported for the row that actually exists on disk. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/adapter/dsh.rs | 19 ++++++ crates/hk-core/src/adapter/mod.rs | 28 ++++----- crates/hk-core/src/deployer.rs | 58 +++++++++++++++++- crates/hk-core/src/kits/install_plan.rs | 78 ++++++++++++++++++++++++- 4 files changed, 163 insertions(+), 20 deletions(-) diff --git a/crates/hk-core/src/adapter/dsh.rs b/crates/hk-core/src/adapter/dsh.rs index 3befae8..f3fc697 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -839,6 +839,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()) } @@ -1664,4 +1670,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/mod.rs b/crates/hk-core/src/adapter/mod.rs index c04241a..276b1ca 100644 --- a/crates/hk-core/src/adapter/mod.rs +++ b/crates/hk-core/src/adapter/mod.rs @@ -352,7 +352,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 { @@ -370,6 +371,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, } @@ -683,13 +689,15 @@ 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. + // 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 ), }, } @@ -763,25 +771,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()); diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index 05e861c..d7a3348 100644 --- a/crates/hk-core/src/deployer.rs +++ b/crates/hk-core/src/deployer.rs @@ -212,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" ))) @@ -266,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:?}" @@ -955,8 +958,7 @@ fn build_dsh_insert_row( } 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 once - // the Task-9 remote schema lands (until then it refuses all remotes). + // 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); @@ -2747,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; @@ -5388,6 +5396,50 @@ mod dsh_insert_writer_tests { 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 diff --git a/crates/hk-core/src/kits/install_plan.rs b/crates/hk-core/src/kits/install_plan.rs index ca33df0..26a0b45 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)); + } +} From 26fb00b2d148cddf8c029b4991ac4c87c874a450 Mon Sep 17 00:00:00 2001 From: RealZST Date: Mon, 17 Aug 2026 21:11:56 +0800 Subject: [PATCH 06/11] feat: add dsh-js-env-no-fallback audit rule with its display MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dsh evaluates `!!js` config expressions 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. The rule reports it as a warning with real file:line. The patch text rides a dedicated `AuditInput::raw_config` field rather than `content`: the cordis patch file is shared by every dsh MCP row, so putting it in `content` would let the content-scanning rules report a neighbouring server's token on this row. `service::run_audit` attaches the raw text to the first dsh MCP row only; the reported file:line may therefore point into another server's block, which is accepted. A rule ships with the surface that displays it, so this also registers `dsh-js-env-no-fallback` in the frontend `AUDIT_RULES` registry and adds its label/description to all three locales (en, zh, zh-TW), with an i18n parity test so a rule can never again reach the Audit page untranslated. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/auditor/mod.rs | 16 ++- crates/hk-core/src/auditor/rules.rs | 5 +- crates/hk-core/src/auditor/rules/content.rs | 136 ++++++++++++++++++ .../hk-core/src/auditor/rules/test_support.rs | 2 + crates/hk-core/src/service.rs | 126 +++++++++++++++- src/lib/__tests__/i18n.test.ts | 60 ++++++++ src/lib/i18n/locales/en/audit.json | 4 + src/lib/i18n/locales/zh-TW/audit.json | 4 + src/lib/i18n/locales/zh/audit.json | 4 + src/pages/__tests__/audit-utils.test.ts | 8 ++ src/pages/audit-utils.ts | 14 ++ src/pages/audit.tsx | 6 +- 12 files changed, 373 insertions(+), 12 deletions(-) diff --git a/crates/hk-core/src/auditor/mod.rs b/crates/hk-core/src/auditor/mod.rs index 6492b33..5e1888d 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 f264d77..c7f9533 100644 --- a/crates/hk-core/src/auditor/rules.rs +++ b/crates/hk-core/src/auditor/rules.rs @@ -13,8 +13,8 @@ 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 @@ -35,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 eeb9c7c..5a59e0e 100644 --- a/crates/hk-core/src/auditor/rules/content.rs +++ b/crates/hk-core/src/auditor/rules/content.rs @@ -463,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::*; @@ -697,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 50dade3..989f538 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/service.rs b/crates/hk-core/src/service.rs index 13e78e8..7b87532 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, @@ -1572,6 +1610,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 diff --git a/src/lib/__tests__/i18n.test.ts b/src/lib/__tests__/i18n.test.ts index ca320f1..9a6d05d 100644 --- a/src/lib/__tests__/i18n.test.ts +++ b/src/lib/__tests__/i18n.test.ts @@ -105,3 +105,63 @@ describe("i18n language preference helpers", () => { expect(i18n.resolvedLanguage).toBe("zh"); }); }); + +describe("audit rules locale parity", () => { + // Every AUDIT_RULES entry renders its label/description through + // `rules..*`, but audit.tsx passes `defaultValue: rule.label`, + // so a locale missing the key silently renders the English registry string + // instead of failing loudly. Drive the check off the registry (not off the + // English bundle) so a rule added with no i18n at all is caught too. + it("defines label and description for every rule in every supported language", async () => { + const { default: i18n, SUPPORTED_LANGUAGES } = await import("../i18n"); + // Import the SAME transform audit.tsx renders with — a local copy could + // drift and leave this guard green while the UI reads untranslated keys. + const { AUDIT_RULES, ruleI18nKey } = await import("@/pages/audit-utils"); + + expect(AUDIT_RULES.map((r) => r.id)).toContain("dsh-js-env-no-fallback"); + + for (const lang of SUPPORTED_LANGUAGES) { + for (const rule of AUDIT_RULES) { + for (const field of ["label", "description"]) { + const fullKey = `rules.${ruleI18nKey(rule.id)}.${field}`; + // Own bundle only — the zh-TW → zh → en fallback chain (and the + // defaultValue in audit.tsx) would mask a missing translation. + expect( + typeof i18n.getResource(lang, "audit", fullKey), + `${lang} is missing ${fullKey}`, + ).toBe("string"); + } + } + } + }); + + it("has no orphan rules.* keys and keeps en labels in sync with the registry", async () => { + const { default: i18n } = await import("../i18n"); + const { AUDIT_RULES, ruleI18nKey } = await import("@/pages/audit-utils"); + const englishRules = (i18n.getResource("en", "audit", "rules") ?? + {}) as Record; + const registryKeys = new Set(AUDIT_RULES.map((r) => ruleI18nKey(r.id))); + + // Reverse direction: an i18n entry whose rule was renamed or removed is + // dead copy that no longer renders anywhere. + for (const key of Object.keys(englishRules)) { + expect(registryKeys, `rules.${key} matches no AUDIT_RULES id`).toContain( + key, + ); + } + + // audit.tsx falls back to the registry strings via defaultValue, so both + // fields must agree or the UI text silently changes with the user's + // language. + for (const rule of AUDIT_RULES) { + expect( + englishRules[ruleI18nKey(rule.id)]?.label, + `en label for ${rule.id} differs from the registry`, + ).toBe(rule.label); + expect( + englishRules[ruleI18nKey(rule.id)]?.description, + `en description for ${rule.id} differs from the registry`, + ).toBe(rule.description); + } + }); +}); diff --git a/src/lib/i18n/locales/en/audit.json b/src/lib/i18n/locales/en/audit.json index b1c4a07..55c910f 100644 --- a/src/lib/i18n/locales/en/audit.json +++ b/src/lib/i18n/locales/en/audit.json @@ -103,6 +103,10 @@ "label": "Skill Invocation Key Case", "description": "Frontmatter uses a camelCase invocation key that DeepSeek Harness silently rejects, dropping the whole skill" }, + "dshJsEnvNoFallback": { + "label": "dsh !!js Env Without Fallback", + "description": "A !!js config expression reads process.env without a ??/|| fallback — if the variable is unset the whole dsh boot fails at mount time; give the expression a default, e.g. process.env.X ?? \"\"." + }, "cliCredentialStorage": { "label": "CLI Credential Storage", "description": "CLI credential file has overly permissive permissions or unknown storage location" diff --git a/src/lib/i18n/locales/zh-TW/audit.json b/src/lib/i18n/locales/zh-TW/audit.json index 3459ea8..db49ab8 100644 --- a/src/lib/i18n/locales/zh-TW/audit.json +++ b/src/lib/i18n/locales/zh-TW/audit.json @@ -103,6 +103,10 @@ "label": "Skill 呼叫鍵大小寫", "description": "Frontmatter 使用了 camelCase 呼叫鍵，DeepSeek Harness 會靜默捨棄整個 skill" }, + "dshJsEnvNoFallback": { + "label": "dsh !!js 環境變數缺少備用值", + "description": "!!js 設定表達式讀取 process.env 卻沒有 ??/|| 備用值——變數未設定時會在掛載階段使整個 dsh 啟動失敗；請為表達式補上備用值，例如 process.env.X ?? \"\"。" + }, "cliCredentialStorage": { "label": "CLI 憑證儲存", "description": "CLI 憑證檔權限過寬或儲存位置不明" diff --git a/src/lib/i18n/locales/zh/audit.json b/src/lib/i18n/locales/zh/audit.json index fd1eb12..c1e722d 100644 --- a/src/lib/i18n/locales/zh/audit.json +++ b/src/lib/i18n/locales/zh/audit.json @@ -103,6 +103,10 @@ "label": "Skill 调用键大小写", "description": "Frontmatter 使用了 camelCase 调用键，DeepSeek Harness 会静默丢弃整个 skill" }, + "dshJsEnvNoFallback": { + "label": "dsh !!js 环境变量缺少回退", + "description": "!!js 配置表达式读取 process.env 却没有 ??/|| 回退——变量未设置时会在挂载阶段使整个 dsh 启动失败；请为表达式补上回退值，例如 process.env.X ?? \"\"。" + }, "cliCredentialStorage": { "label": "CLI 凭据存储", "description": "CLI 凭据文件权限过宽或存储位置未知" diff --git a/src/pages/__tests__/audit-utils.test.ts b/src/pages/__tests__/audit-utils.test.ts index d3a446e..6232e05 100644 --- a/src/pages/__tests__/audit-utils.test.ts +++ b/src/pages/__tests__/audit-utils.test.ts @@ -166,4 +166,12 @@ describe("AUDIT_RULES", () => { expect(valid.has(rule.severity)).toBe(true); } }); + + it("includes dsh-js-env-no-fallback scoped to MCP at Medium", () => { + const rule = AUDIT_RULES.find((r) => r.id === "dsh-js-env-no-fallback"); + expect(rule).toBeDefined(); + expect(rule?.kinds).toEqual(["mcp"]); + expect(rule?.severity).toBe("Medium"); + expect(rule?.deduction).toBe(8); + }); }); diff --git a/src/pages/audit-utils.ts b/src/pages/audit-utils.ts index ecf06a7..bf20edb 100644 --- a/src/pages/audit-utils.ts +++ b/src/pages/audit-utils.ts @@ -2,6 +2,11 @@ import type { AuditFinding, ExtensionKind, Severity } from "@/lib/types"; type Kind = ExtensionKind; +/** kebab-case rule id → camelCase i18n key (e.g. "prompt-injection" → "promptInjection") */ +export function ruleI18nKey(id: string): string { + return id.replace(/-(\w)/g, (_, c: string) => c.toUpperCase()); +} + export const AUDIT_RULES = [ { id: "prompt-injection", @@ -93,6 +98,15 @@ export const AUDIT_RULES = [ "Frontmatter uses a camelCase invocation key that DeepSeek Harness silently rejects, dropping the whole skill", kinds: ["skill"] as Kind[], }, + { + id: "dsh-js-env-no-fallback", + label: "dsh !!js Env Without Fallback", + severity: "Medium" as Severity, + deduction: 8, + description: + 'A !!js config expression reads process.env without a ??/|| fallback — if the variable is unset the whole dsh boot fails at mount time; give the expression a default, e.g. process.env.X ?? "".', + kinds: ["mcp"] as Kind[], + }, { id: "cli-credential-storage", label: "CLI Credential Storage", diff --git a/src/pages/audit.tsx b/src/pages/audit.tsx index 9f33710..a622496 100644 --- a/src/pages/audit.tsx +++ b/src/pages/audit.tsx @@ -32,16 +32,12 @@ import { AUDIT_RULES, type GroupedResult, maxSeverity, + ruleI18nKey, rulesForKind, severityBadgeClass, severityIconColor, } from "./audit-utils"; -/** kebab-case rule id → camelCase i18n key (e.g. "prompt-injection" → "promptInjection") */ -function ruleI18nKey(id: string): string { - return id.replace(/-(\w)/g, (_, c: string) => c.toUpperCase()); -} - function IndeterminateBar({ className = "" }: { className?: string }) { return (
Date: Tue, 18 Aug 2026 10:26:13 +0800 Subject: [PATCH 07/11] refactor(test): fold duplicate dsh managed-block tests Four block-corruption tests were byte-identical apart from the malformed input, so they become one table-driven test whose case labels carry the reason each input is corruption. `toggle_and_remove_work_through_the_ original_name` duplicated the path `remove_and_toggle_by_original_name_ hit_the_sanitized_row` already walks; its one extra assertion (toggle back on) moves into the survivor, which keeps asserting on raw bytes so a reader that also normalized could not hide a writer that did not. The reader round-trip it covered is `install_records_the_original_name_and_ the_reader_round_trips_it`. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/deployer.rs | 114 +++++++++++---------------------- 1 file changed, 39 insertions(+), 75 deletions(-) diff --git a/crates/hk-core/src/deployer.rs b/crates/hk-core/src/deployer.rs index d7a3348..d30f095 100644 --- a/crates/hk-core/src/deployer.rs +++ b/crates/hk-core/src/deployer.rs @@ -4728,54 +4728,38 @@ mod dsh_toggle_tests { } #[test] - fn corrupted_block_yaml_errors_and_leaves_file_untouched() { - // HK owns every byte inside the markers: unparseable block content is - // a hard ConfigCorrupted, refuse to write. - let text = format!( - "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- id: [unclosed\n{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(_))); - assert_eq!(std::fs::read_to_string(&path).unwrap(), text); - } - - #[test] - fn unrecognized_block_entry_errors_instead_of_silent_drop() { - // Behavior change pinned on purpose: entries HK did not render are - // corruption, not noise to discard. - let text = format!( - "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- surprise: true\n{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(_))); - assert_eq!(std::fs::read_to_string(&path).unwrap(), text); - } - - #[test] - fn toggle_entry_with_extra_keys_errors() { - // Extra keys on an id-targeted entry are LIVE dsh patch semantics - // (they would patch the target row) — dropping them on re-render - // would alter the user's effective config, so they are corruption. - let text = format!( - "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- id: mcp-github\n disabled: true\n command: pwned\n{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(_))); - assert_eq!(std::fs::read_to_string(&path).unwrap(), text); - } - - #[test] - fn insert_group_with_extra_keys_errors() { - let text = format!( - "{HOME_WITH_GH}{DSH_BLOCK_BEGIN}\n- insert:\n - id: mcp-x\n after: mcp-github\n{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(_))); - assert_eq!(std::fs::read_to_string(&path).unwrap(), text); + 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] @@ -5296,32 +5280,6 @@ mod dsh_insert_writer_tests { assert!(!text.contains("_hk_name"), "{text}"); } - #[test] - fn toggle_and_remove_work_through_the_original_name() { - use crate::adapter::AgentAdapter; - // The scanner now hands the manager the ORIGINAL name, so every - // by-name path must resolve it to the row stored under the sanitized - // `serverName` — and must still be a no-op-free round trip. - 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(); - - set_dsh_mcp_enabled(&path, "microsoft/markitdown", false).unwrap(); - let disabled = adapter.read_mcp_servers(); - assert_eq!(disabled[0].name, "microsoft/markitdown"); - assert!(!disabled[0].enabled, "toggle by original name reached the row"); - - set_dsh_mcp_enabled(&path, "microsoft/markitdown", true).unwrap(); - assert!(adapter.read_mcp_servers()[0].enabled); - - remove_mcp_server(&path, "microsoft/markitdown", McpFormat::DshCordis).unwrap(); - assert!(adapter.read_mcp_servers().is_empty()); - let text = std::fs::read_to_string(&path).unwrap(); - assert!(!text.contains("markitdown"), "row actually gone: {text}"); - } - #[test] fn installing_the_same_original_name_twice_collides_and_adds_no_second_row() { use crate::adapter::AgentAdapter; @@ -5344,7 +5302,9 @@ mod dsh_insert_writer_tests { // 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. + // 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(); @@ -5354,6 +5314,10 @@ mod dsh_insert_writer_tests { 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}"); From 03e7a0ec8e9d8edc1ec4f537396d17a4c294d593 Mon Sep 17 00:00:00 2001 From: RealZST Date: Tue, 18 Aug 2026 10:28:17 +0800 Subject: [PATCH 08/11] feat: protect agent-shipped plugins from deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dsh plugin is a ROW in a composed patch layer naming an npm package, reached through two manifest records — the profile's `dependencies` and its `dsh.profile.bundles` layer list. `delete_extension`'s fallback deleted `PluginEntry::path`, which for dsh is that package inside a SHARED `profiles/node_modules` farm: both records kept naming it, dsh failed to boot (a configured-but-absent package is a hard mount error), sibling rows instantiating the same package broke with it, and the next scan re-reported the row from the patch text anyway. Destructive and ineffective. Reachable only since this branch made dsh report plugins. Adapters now answer `plugin_removal()` — `Files` (the default, unchanged for every directory-based agent), `Command` (delegate to the agent's own uninstaller: `dsh plugin remove` runs pnpm AND reconciles `bundles` in one step), or `Shipped`. In-box bundles are not dependencies, so dsh's own reconcile never touches them either; refusing them mirrors the vendor's constraint rather than inventing one. A missing binary reports the exact command instead of falling back to deleting files. `PluginEntry::pack` carries the bundle that provided a row, since the git-URL derivation only fires for git checkouts and would leave the whole vendor baseline unattributed. `AgentCapabilities::vendor_baseline_packs` publishes the `Shipped` set so the detail panel greys out delete on exactly the rows the backend refuses. Deleting also reported success when it failed: the optimistic removal drops rows and toasts five seconds BEFORE the request, and the rejection was dropped as an unhandled rejection from a timer callback. Failures now restore the rows and say why — pre-existing, but this refusal is the first routine way to hit it. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/adapter/claude.rs | 1 + crates/hk-core/src/adapter/codex.rs | 1 + crates/hk-core/src/adapter/copilot.rs | 2 + crates/hk-core/src/adapter/cursor.rs | 2 + crates/hk-core/src/adapter/dsh.rs | 130 ++++++++++++++++- crates/hk-core/src/adapter/gemini.rs | 1 + crates/hk-core/src/adapter/hermes.rs | 1 + crates/hk-core/src/adapter/mod.rs | 55 +++++++ crates/hk-core/src/adapter/omp.rs | 2 + crates/hk-core/src/adapter/opencode.rs | 1 + crates/hk-core/src/manager.rs | 1 + crates/hk-core/src/models.rs | 6 + crates/hk-core/src/scanner.rs | 7 +- crates/hk-core/src/service.rs | 138 +++++++++++++++++- .../extensions/extension-detail.tsx | 37 ++++- src/lib/agent-capabilities.ts | 22 +++ src/lib/i18n/locales/en/extensions.json | 2 + src/lib/i18n/locales/zh-TW/extensions.json | 2 + src/lib/i18n/locales/zh/extensions.json | 2 + src/lib/types.ts | 5 + src/stores/__tests__/extension-store.test.ts | 85 +++++++++++ src/stores/extension-store.ts | 56 ++++++- 22 files changed, 542 insertions(+), 17 deletions(-) create mode 100644 src/stores/__tests__/extension-store.test.ts diff --git a/crates/hk-core/src/adapter/claude.rs b/crates/hk-core/src/adapter/claude.rs index d4584da..5866750 100644 --- a/crates/hk-core/src/adapter/claude.rs +++ b/crates/hk-core/src/adapter/claude.rs @@ -374,6 +374,7 @@ impl AgentAdapter for ClaudeAdapter { 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 5dc0566..86f36fc 100644 --- a/crates/hk-core/src/adapter/codex.rs +++ b/crates/hk-core/src/adapter/codex.rs @@ -436,6 +436,7 @@ impl AgentAdapter for CodexAdapter { 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 cb7ddd0..c0b6be7 100644 --- a/crates/hk-core/src/adapter/copilot.rs +++ b/crates/hk-core/src/adapter/copilot.rs @@ -266,6 +266,7 @@ impl AgentAdapter for CopilotAdapter { installed_at: None, updated_at: None, base_layers: vec![], + pack: None, }); } } @@ -318,6 +319,7 @@ impl AgentAdapter for CopilotAdapter { 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 b9eb97f..d0bbabc 100644 --- a/crates/hk-core/src/adapter/cursor.rs +++ b/crates/hk-core/src/adapter/cursor.rs @@ -242,6 +242,7 @@ impl AgentAdapter for CursorAdapter { installed_at: None, updated_at: None, base_layers: vec![], + pack: None, }); } } @@ -281,6 +282,7 @@ impl AgentAdapter for CursorAdapter { 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 f3fc697..5366e7a 100644 --- a/crates/hk-core/src/adapter/dsh.rs +++ b/crates/hk-core/src/adapter/dsh.rs @@ -645,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 @@ -711,6 +761,10 @@ impl AgentAdapter for DshAdapter { // 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, @@ -820,6 +874,11 @@ impl AgentAdapter for DshAdapter { 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, }); } } @@ -897,7 +956,7 @@ impl AgentAdapter for DshAdapter { #[cfg(test)] mod tests { - use super::super::AgentAdapter; + use super::super::{AgentAdapter, PluginRemoval}; use super::*; #[test] @@ -1317,6 +1376,75 @@ mod tests { 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 diff --git a/crates/hk-core/src/adapter/gemini.rs b/crates/hk-core/src/adapter/gemini.rs index feb11a4..2807f54 100644 --- a/crates/hk-core/src/adapter/gemini.rs +++ b/crates/hk-core/src/adapter/gemini.rs @@ -254,6 +254,7 @@ impl AgentAdapter for GeminiAdapter { 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 017b8c5..3355ebd 100644 --- a/crates/hk-core/src/adapter/hermes.rs +++ b/crates/hk-core/src/adapter/hermes.rs @@ -114,6 +114,7 @@ impl HermesAdapter { 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 276b1ca..780531f 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 { @@ -281,6 +300,18 @@ pub struct PluginEntry { /// `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. @@ -393,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`). @@ -689,6 +734,7 @@ impl crate::models::AgentCapabilities { }, hooks_supported: a.hook_format() != HookFormat::None, global_hook_install: a.supports_global_hook_install(), + 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 { @@ -910,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 670a2bf..8f9e4a6 100644 --- a/crates/hk-core/src/adapter/omp.rs +++ b/crates/hk-core/src/adapter/omp.rs @@ -247,6 +247,7 @@ impl AgentAdapter for OmpAdapter { installed_at: None, updated_at: None, base_layers: vec![], + pack: None, }); } else if path.is_dir() { // Directory-form extension: /index.{ts,js}, TypeScript @@ -282,6 +283,7 @@ impl AgentAdapter for OmpAdapter { 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 628e650..106d592 100644 --- a/crates/hk-core/src/adapter/opencode.rs +++ b/crates/hk-core/src/adapter/opencode.rs @@ -232,6 +232,7 @@ impl AgentAdapter for OpencodeAdapter { installed_at: None, updated_at: None, base_layers: vec![], + pack: None, }); } } diff --git a/crates/hk-core/src/manager.rs b/crates/hk-core/src/manager.rs index c530dbd..1b007f8 100644 --- a/crates/hk-core/src/manager.rs +++ b/crates/hk-core/src/manager.rs @@ -2996,6 +2996,7 @@ mod dsh_plugin_dispatch_tests { // 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, } } diff --git a/crates/hk-core/src/models.rs b/crates/hk-core/src/models.rs index 0aafcd9..589d56e 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 ad99791..4d3b795 100644 --- a/crates/hk-core/src/scanner.rs +++ b/crates/hk-core/src/scanner.rs @@ -570,7 +570,12 @@ 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: plugin_extension_id(&plugin.name, &plugin.source, adapter.name()), diff --git a/crates/hk-core/src/service.rs b/crates/hk-core/src/service.rs index 7b87532..4fe9d5d 100644 --- a/crates/hk-core/src/service.rs +++ b/crates/hk-core/src/service.rs @@ -734,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)?; @@ -1057,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)?; + } + } + } } } } @@ -2847,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/src/components/extensions/extension-detail.tsx b/src/components/extensions/extension-detail.tsx index b6b62f0..17405ae 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/lib/agent-capabilities.ts b/src/lib/agent-capabilities.ts index 43d1df5..c59c95f 100644 --- a/src/lib/agent-capabilities.ts +++ b/src/lib/agent-capabilities.ts @@ -19,6 +19,28 @@ export function canReceiveMcpTransport( return transport === "http" ? flags.http : flags.sse; } +/** Whether this plugin ships WITH its agent and so cannot be deleted. + * + * Mirrors `AgentAdapter::plugin_removal` returning `Shipped`: the backend + * refuses these, and the same list arrives as + * `capabilities.vendor_baseline_packs`, so the greyed-out button and the + * refusal can never disagree. dsh is the only agent with a non-empty list + * today — its in-box bundles (`@deepseek-ai/dsh-base`, …) contribute most of + * its plugin rows, while a plugin the user installed carries the pack of the + * third-party bundle that brought it and stays deletable. + * + * Keyed on the pack alone — never on the kind, and never on which agent owns + * the row. `getCachedFiltered` tests the same flat set of shipped packs, so + * the greyed-out button and the hide filter can never disagree about a row; + * an agent that later ships built-in skills instead of plugins is covered + * without a change here either. */ +export function isVendorBaseline( + pack: string | null | undefined, + shippedPacks: Iterable, +): boolean { + return !!pack && new Set(shippedPacks).has(pack); +} + /** Whether `agent` can take an install of `kind` at `scope`. * * Reads the backend-derived `AgentInfo.capabilities` (computed from the diff --git a/src/lib/i18n/locales/en/extensions.json b/src/lib/i18n/locales/en/extensions.json index 5695b31..926035e 100644 --- a/src/lib/i18n/locales/en/extensions.json +++ b/src/lib/i18n/locales/en/extensions.json @@ -97,7 +97,9 @@ "documentation": "Documentation", "openInFinder": "Open in Finder", "deleteButton": "Delete...", + "deleteShippedTip": "{{agent}} ships this plugin — it can be disabled but not deleted", "deleteSuccess": "Extension deleted. Takes effect in new sessions", + "deleteFailedReason": "Could not delete {{name}}: {{msg}}", "deleteFromAgentsSuccess": "Deleted from {{agents}}. Takes effect in new sessions", "deleteFailed": "Failed to delete" }, diff --git a/src/lib/i18n/locales/zh-TW/extensions.json b/src/lib/i18n/locales/zh-TW/extensions.json index 53b3b15..b0531f2 100644 --- a/src/lib/i18n/locales/zh-TW/extensions.json +++ b/src/lib/i18n/locales/zh-TW/extensions.json @@ -97,7 +97,9 @@ "documentation": "文件", "openInFinder": "在 Finder 中開啟", "deleteButton": "刪除...", + "deleteShippedTip": "{{agent}} 內建此外掛——可以停用,但無法刪除", "deleteSuccess": "擴充已刪除。將在新 session 生效", + "deleteFailedReason": "無法刪除 {{name}}：{{msg}}", "deleteFromAgentsSuccess": "已從 {{agents}} 刪除。將在新 session 生效", "deleteFailed": "刪除失敗" }, diff --git a/src/lib/i18n/locales/zh/extensions.json b/src/lib/i18n/locales/zh/extensions.json index b812ff9..9352a4e 100644 --- a/src/lib/i18n/locales/zh/extensions.json +++ b/src/lib/i18n/locales/zh/extensions.json @@ -97,7 +97,9 @@ "documentation": "文档", "openInFinder": "在访达中打开", "deleteButton": "删除...", + "deleteShippedTip": "{{agent}} 自带此插件——可以停用,但无法删除", "deleteSuccess": "扩展已删除。将在新会话中生效", + "deleteFailedReason": "无法删除 {{name}}：{{msg}}", "deleteFromAgentsSuccess": "已从 {{agents}} 中删除。将在新会话中生效", "deleteFailed": "删除失败" }, diff --git a/src/lib/types.ts b/src/lib/types.ts index 56d0203..0ff9829 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -337,6 +337,11 @@ export interface AgentCapabilities { /** Which remote MCP transports the agent's config can express. Absent * on responses from pre-transport backends — treat as stdio-only. */ mcp_remote?: RemoteTransportFlags; + /** Packs whose plugins ship WITH the agent. `delete_extension` refuses + * these, so delete is greyed out on exactly the same rows instead of + * letting the user hit the error. Empty (or absent, on older backends) + * for agents whose baseline never surfaces as an extension. */ + vendor_baseline_packs?: string[]; } export interface RemoteTransportFlags { diff --git a/src/stores/__tests__/extension-store.test.ts b/src/stores/__tests__/extension-store.test.ts new file mode 100644 index 0000000..d9cc962 --- /dev/null +++ b/src/stores/__tests__/extension-store.test.ts @@ -0,0 +1,85 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { api } from "@/lib/invoke"; +import type { Extension } from "@/lib/types"; +import { useExtensionStore } from "../extension-store"; +import { toast } from "../toast-store"; + +vi.mock("@/lib/invoke"); + +const dshPlugin: Extension = { + id: "p1", + kind: "plugin", + name: "timer", + description: "Plugin from profile web, bundle @deepseek-ai/dsh-base", + source: { origin: "agent", url: null, version: null, commit_hash: null }, + agents: ["dsh"], + tags: [], + pack: "@deepseek-ai/dsh-base", + permissions: [], + enabled: true, + trust_score: null, + installed_at: "2026-08-01T00:00:00Z", + updated_at: "2026-08-01T00:00:00Z", + source_path: null, + cli_parent_id: null, + cli_meta: null, + install_meta: null, + scope: { type: "global" }, +}; + +describe("extension-store confirmDelete", () => { + beforeEach(() => { + useExtensionStore.setState({ extensions: [], pendingDelete: null }); + vi.resetAllMocks(); + }); + + // Deletion is optimistic: the row leaves the list and the success toast + // fires five seconds BEFORE the request goes out. A refusal that only got + // logged left the UI asserting a deletion that never happened — the row + // stayed gone until a manual reload, and nothing told the user why. + it("puts the rows back and reports why when the backend refuses", async () => { + const errorToast = vi.spyOn(toast, "error").mockImplementation(() => {}); + vi.mocked(api.deleteExtension).mockRejectedValue( + '{"kind":"Validation","message":"\'timer\' is a dsh plugin row"}', + ); + // State right after the optimistic removal: gone from the list, parked + // in pendingDelete. + useExtensionStore.setState({ + extensions: [], + pendingDelete: { + ids: new Set(["p1"]), + extensions: [dshPlugin], + timer: 0 as unknown as ReturnType, + }, + }); + + await expect( + useExtensionStore.getState().confirmDelete(), + ).resolves.toBeUndefined(); + + expect(useExtensionStore.getState().extensions).toEqual([dshPlugin]); + expect(errorToast).toHaveBeenCalledTimes(1); + // The backend's reason has to reach the toast, not a generic failure. + expect(errorToast.mock.calls[0][0]).toContain("dsh plugin row"); + // A failed delete must not leave a rescan running over a stale list. + expect(api.scanAndSync).not.toHaveBeenCalled(); + }); + + it("does not restore anything when the delete succeeds", async () => { + vi.mocked(api.deleteExtension).mockResolvedValue(undefined); + vi.mocked(api.scanAndSync).mockResolvedValue(0); + vi.mocked(api.listExtensions).mockResolvedValue([]); + useExtensionStore.setState({ + extensions: [], + pendingDelete: { + ids: new Set(["p1"]), + extensions: [dshPlugin], + timer: 0 as unknown as ReturnType, + }, + }); + + await useExtensionStore.getState().confirmDelete(); + + expect(useExtensionStore.getState().extensions).toEqual([]); + }); +}); diff --git a/src/stores/extension-store.ts b/src/stores/extension-store.ts index ea8607b..cc3c19d 100644 --- a/src/stores/extension-store.ts +++ b/src/stores/extension-store.ts @@ -1,4 +1,5 @@ import { create } from "zustand"; +import { parseError } from "@/lib/error-types"; import i18n from "@/lib/i18n"; import { api } from "@/lib/invoke"; import type { @@ -20,6 +21,27 @@ import { toast } from "./toast-store"; export { buildGroups } from "./extension-helpers"; +/** + * Run a pending delete's backend calls, reporting the first failure instead of + * throwing. + * + * Deletion is optimistic: the rows leave the list and the success toast fires + * the moment the user confirms, five seconds BEFORE the request goes out. So a + * rejection that is merely logged (or, worse, dropped as an unhandled rejection + * from a timer callback) leaves the UI asserting a deletion that never + * happened — the caller must put the rows back and say why. A backend that + * refuses on purpose (a dsh plugin row, which names a package HarnessKit does + * not own) is the routine case, but so is any read-only or locked path. + */ +async function runPendingDeletes(ids: Iterable): Promise { + try { + await Promise.all([...ids].map((id) => api.deleteExtension(id))); + return null; + } catch (e) { + return e; + } +} + const MIN_CHECK_UPDATES_VISIBLE_MS = 600; interface PendingDelete { @@ -351,7 +373,17 @@ export const useExtensionStore = create((set, get) => ({ if (!pending) return; clearTimeout(pending.timer); set({ pendingDelete: null }); - await Promise.all([...pending.ids].map((id) => api.deleteExtension(id))); + const failure = await runPendingDeletes(pending.ids); + if (failure) { + set((s) => ({ extensions: [...s.extensions, ...pending.extensions] })); + toast.error( + i18n.t("extensions:detail.deleteFailedReason", { + name: pending.extensions[0]?.name ?? "", + msg: parseError(failure).message, + }), + ); + return; + } // Remove CLI binary only on full uninstall (CLI parent is in the set, not just children) for (const ext of pending.extensions) { if ( @@ -564,14 +596,26 @@ export const useExtensionStore = create((set, get) => ({ const prev = get().pendingDelete; if (prev) { clearTimeout(prev.timer); - try { - await Promise.all([...prev.ids].map((id) => api.deleteExtension(id))); - } catch (e) { - console.error("Failed to finalize previous deletion:", e); + const failure = await runPendingDeletes(prev.ids); + if (failure) { + // Same contract as confirmDelete: the earlier rows were removed + // optimistically too, so a refusal has to put them back. + set((s) => ({ extensions: [...s.extensions, ...prev.extensions] })); + toast.error( + i18n.t("extensions:detail.deleteFailedReason", { + name: prev.extensions[0]?.name ?? "", + msg: parseError(failure).message, + }), + ); } } const timer = setTimeout(() => { - get().confirmDelete(); + // confirmDelete reports its own failures; this guards the rescan tail. + get() + .confirmDelete() + .catch((e) => { + console.error("Failed to finalize deletion:", e); + }); }, 5000); set({ pendingDelete: { ids, extensions: toDelete, timer } }); }, From 26cfb4d0ca0617a06d31f83803563b67a805e94e Mon Sep 17 00:00:00 2001 From: RealZST Date: Tue, 18 Aug 2026 10:28:32 +0800 Subject: [PATCH 09/11] feat: separate an agent's shipped baseline in the Extensions list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dsh contributes ~130 plugin rows against ~20 from every other agent combined, because its whole product is plugins and its baseline ships as data rather than compiled in. Flat, that baseline IS the plugin list. Two additions, both driven by `capabilities.vendor_baseline_packs` so no UI code names an agent. An agent's in-box bundles collapse into one source option (`DeepSeek built-in (129)`) — three bundles is how dsh ships, not three sources the user chose between. And a "Hide built-in" toggle sits by the result count, rendered only where something is actually hideable, so the other eleven agents never see it. Shown by default: VS Code hides its built-ins and Obsidian gives core plugins their own tab, but dsh's own list shows everything, and hiding a row the user could re-enable would be worse than the noise. The label names the mode and never changes — flipping it to the next action would contradict the tint, since a lit control reads as "on" while the verb claims the opposite. Co-Authored-By: Claude Opus 5 (1M context) --- .../extensions/extension-filters.tsx | 86 ++++++++++++- src/lib/i18n/locales/en/extensions.json | 2 + src/lib/i18n/locales/zh-TW/extensions.json | 2 + src/lib/i18n/locales/zh/extensions.json | 2 + .../__tests__/extension-helpers.test.ts | 116 +++++++++++++++++- src/stores/extension-helpers.ts | 30 ++++- src/stores/extension-store.ts | 23 ++++ 7 files changed, 252 insertions(+), 9 deletions(-) diff --git a/src/components/extensions/extension-filters.tsx b/src/components/extensions/extension-filters.tsx index bbda6d8..9a6974d 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) && (