From b146bb5d911020ec4a040dde1b205606365aaa75 Mon Sep 17 00:00:00 2001 From: SherlockSalvatore Date: Wed, 24 Jun 2026 12:43:52 +0800 Subject: [PATCH 1/6] feat(scanner): derive skill/plugin source from install manifests HarnessKit inferred an extension's source by walking up to the nearest enclosing .git, mis-attributing everything under a dotfiles-managed agent home (e.g. ~/.claude) to that backup repo. Read each tool's own install manifest instead, falling back to .git detection only when absent. - Skills: override detect_source with /.skill-lock.json (the skills CLI lockfile), matched by on-disk folder name, cached per lockfile. - Plugins: PluginEntry gains source_url; the Claude adapter fills it from plugins/known_marketplaces.json (github marketplaces -> owner/repo); scan_plugins prefers it over the .git walk. Other adapters unchanged. Regression tests: lockfile beats a populated enclosing repo (and a sibling skill absent from the lock falls back); a plugin is attributed to its marketplace repo. Refs #89. Builds on #88 canonicalize in scan_skill_dir. --- crates/hk-core/src/adapter/claude.rs | 37 +++++ 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/gemini.rs | 1 + crates/hk-core/src/adapter/hermes.rs | 1 + crates/hk-core/src/adapter/mod.rs | 5 + crates/hk-core/src/adapter/opencode.rs | 1 + crates/hk-core/src/scanner.rs | 179 +++++++++++++++++++++++-- 9 files changed, 218 insertions(+), 11 deletions(-) diff --git a/crates/hk-core/src/adapter/claude.rs b/crates/hk-core/src/adapter/claude.rs index e135c603..28bc08b4 100644 --- a/crates/hk-core/src/adapter/claude.rs +++ b/crates/hk-core/src/adapter/claude.rs @@ -278,6 +278,11 @@ impl AgentAdapter for ClaudeAdapter { return vec![]; }; + // Map marketplace name → upstream "owner/repo" from the agent's own + // catalog, so each plugin is attributed to its real source instead of + // the `.git` its cache dir happens to sit under. + let marketplace_repo = read_marketplace_repos(&self.base_dir().join("plugins")); + // Also read enabledPlugins from settings.json to know which are enabled let enabled_set: std::collections::HashSet = self .read_settings() @@ -321,11 +326,16 @@ impl AgentAdapter for ClaudeAdapter { .and_then(|s| chrono::DateTime::parse_from_rfc3339(s).ok()) .map(|dt| dt.with_timezone(&chrono::Utc)); + let source_url = marketplace_repo + .get(&source) + .map(|repo| format!("https://github.com/{repo}")); + entries.push(PluginEntry { name, source: source.clone(), enabled: enabled_set.contains(key), path: install_path, + source_url, uri: None, installed_at, updated_at, @@ -335,6 +345,33 @@ impl AgentAdapter for ClaudeAdapter { } } +/// Parse `/known_marketplaces.json` into a `marketplace name → +/// "owner/repo"` map. Empty when the catalog is missing or unreadable. +fn read_marketplace_repos(plugins_dir: &Path) -> std::collections::HashMap { + let path = plugins_dir.join("known_marketplaces.json"); + let Ok(content) = std::fs::read_to_string(&path) else { + return std::collections::HashMap::new(); + }; + let Ok(json) = serde_json::from_str::(&content) else { + return std::collections::HashMap::new(); + }; + let Some(obj) = json.as_object() else { + return std::collections::HashMap::new(); + }; + obj.iter() + .filter_map(|(name, entry)| { + let src = entry.get("source")?; + // Only github marketplaces map cleanly to a github.com/ URL; + // skip others (gitlab, local, …) so we don't fabricate a wrong URL. + if src.get("source").and_then(|v| v.as_str()) != Some("github") { + return None; + } + let repo = src.get("repo")?.as_str()?.to_string(); + Some((name.clone(), repo)) + }) + .collect() +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/hk-core/src/adapter/codex.rs b/crates/hk-core/src/adapter/codex.rs index d3a8ab6d..ca37b28a 100644 --- a/crates/hk-core/src/adapter/codex.rs +++ b/crates/hk-core/src/adapter/codex.rs @@ -400,6 +400,7 @@ impl AgentAdapter for CodexAdapter { enabled: !disabled_plugins .contains(&format!("{}@{}", name, &marketplace_name)), path: Some(version_dir.path().to_path_buf()), // version level — matches manifest location + source_url: None, uri: None, installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/adapter/copilot.rs b/crates/hk-core/src/adapter/copilot.rs index fe914b47..0ec2fa6c 100644 --- a/crates/hk-core/src/adapter/copilot.rs +++ b/crates/hk-core/src/adapter/copilot.rs @@ -245,6 +245,7 @@ impl AgentAdapter for CopilotAdapter { source: mp_name.clone(), enabled: true, path: Some(plugin.path()), + source_url: None, uri: None, installed_at: None, updated_at: None, @@ -295,6 +296,7 @@ impl AgentAdapter for CopilotAdapter { source: marketplace.to_string(), enabled, path: Some(plugin_path), + source_url: None, uri: Some(plugin_uri.to_string()), installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/adapter/cursor.rs b/crates/hk-core/src/adapter/cursor.rs index 5f6b2ab4..e78ee5b6 100644 --- a/crates/hk-core/src/adapter/cursor.rs +++ b/crates/hk-core/src/adapter/cursor.rs @@ -236,6 +236,7 @@ impl AgentAdapter for CursorAdapter { source: "local".into(), enabled: true, path: Some(dir.path()), + source_url: None, uri: None, installed_at: None, updated_at: None, @@ -273,6 +274,7 @@ impl AgentAdapter for CursorAdapter { source: mp_name.clone(), enabled: true, path: Some(plugin.path()), + source_url: None, uri: None, installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/adapter/gemini.rs b/crates/hk-core/src/adapter/gemini.rs index 5b655b2f..9fb1876d 100644 --- a/crates/hk-core/src/adapter/gemini.rs +++ b/crates/hk-core/src/adapter/gemini.rs @@ -237,6 +237,7 @@ impl AgentAdapter for GeminiAdapter { source: "gemini".into(), enabled, path: Some(dir.path()), + source_url: None, uri: None, installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/adapter/hermes.rs b/crates/hk-core/src/adapter/hermes.rs index 61678051..c4d5c994 100644 --- a/crates/hk-core/src/adapter/hermes.rs +++ b/crates/hk-core/src/adapter/hermes.rs @@ -108,6 +108,7 @@ impl HermesAdapter { name, source: "hermes".into(), path: dir, + source_url: None, uri: None, installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/adapter/mod.rs b/crates/hk-core/src/adapter/mod.rs index da30ad07..88e8f4a5 100644 --- a/crates/hk-core/src/adapter/mod.rs +++ b/crates/hk-core/src/adapter/mod.rs @@ -73,6 +73,11 @@ pub struct PluginEntry { pub source: String, pub enabled: bool, pub path: Option, + /// Authoritative upstream URL resolved from the agent's own plugin manifest + /// (e.g. Claude's marketplace → repo mapping). When set, it overrides the + /// `.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. pub uri: Option, diff --git a/crates/hk-core/src/adapter/opencode.rs b/crates/hk-core/src/adapter/opencode.rs index 967d157f..fd43c880 100644 --- a/crates/hk-core/src/adapter/opencode.rs +++ b/crates/hk-core/src/adapter/opencode.rs @@ -214,6 +214,7 @@ impl AgentAdapter for OpencodeAdapter { source: "local".into(), enabled, path: Some(path), + source_url: None, uri: None, installed_at: None, updated_at: None, diff --git a/crates/hk-core/src/scanner.rs b/crates/hk-core/src/scanner.rs index c7b69b01..0375f01b 100644 --- a/crates/hk-core/src/scanner.rs +++ b/crates/hk-core/src/scanner.rs @@ -142,6 +142,10 @@ pub fn scan_skill_dir(dir: &Path, agent_name: &str) -> Vec { return extensions; }; + // Cache parsed `skills` CLI lockfiles by their path so we read each at most + // once per scanned directory (one lockfile serves many skills). + let mut lock_cache: HashMap>> = HashMap::new(); + for entry in entries.flatten() { let path = entry.path(); // Skills can be either: a directory containing SKILL.md (or SKILL.md.disabled), or a standalone .md file @@ -183,8 +187,26 @@ pub fn scan_skill_dir(dir: &Path, agent_name: &str) -> Vec { // walking up for `.git`. Plain (non-symlinked) skills canonicalize to // the same real tree, so their detection is unchanged. let resolved = std::fs::canonicalize(&path).unwrap_or_else(|_| path.clone()); - let source = detect_source(&resolved, true); - let pack = source.url.as_deref().and_then(extract_pack_from_url); + let mut source = detect_source(&resolved, true); + let mut pack = source.url.as_deref().and_then(extract_pack_from_url); + // Authoritative override: a skill installed by the `skills` CLI is + // recorded in `/.skill-lock.json` with its true upstream source. + // That beats `detect_source`, which only ever reports whatever `.git` + // the files happen to sit under (e.g. a dotfiles backup repo when the + // shared skills root is itself version-controlled). + // Match by the on-disk folder name — how the `skills` CLI keys the + // lockfile — not the frontmatter `name`, which may differ or be absent. + let lock_key = path.file_name().and_then(|n| n.to_str()).unwrap_or(name.as_str()); + if let Some(locked) = skill_lock_source(&resolved, lock_key, &mut lock_cache) { + pack = extract_pack_from_url(&locked.url).or(Some(locked.source)); + source = Source { + origin: SourceOrigin::Git, + url: Some(locked.url), + version: None, + // Keep a real commit hash if the files are an actual git checkout. + commit_hash: source.commit_hash.take(), + }; + } extensions.push(Extension { id: stable_id(&name, "skill", agent_name), kind: ExtensionKind::Skill, @@ -435,17 +457,27 @@ pub fn scan_plugins(adapter: &dyn AgentAdapter) -> Vec { .unwrap_or_else(|| (Utc::now(), Utc::now())), }; - // Detect git source from plugin path (e.g. VS Code agent-plugins have .git) - let source = plugin - .path - .as_ref() - .map(|p| detect_source(p, true)) - .unwrap_or(Source { - origin: SourceOrigin::Agent, - url: None, + // Prefer the agent manifest's authoritative source (e.g. Claude's + // marketplace → repo mapping); fall back to detecting a `.git` from + // the plugin path (e.g. VS Code agent-plugins that are git clones). + let source = match plugin.source_url { + Some(url) => Source { + origin: SourceOrigin::Git, + url: Some(url), version: None, commit_hash: None, - }); + }, + None => plugin + .path + .as_ref() + .map(|p| detect_source(p, true)) + .unwrap_or(Source { + origin: SourceOrigin::Agent, + url: None, + version: None, + commit_hash: None, + }), + }; let pack = source.url.as_deref().and_then(extract_pack_from_url); Extension { @@ -1498,6 +1530,58 @@ pub fn detect_source_for(path: &Path) -> Source { detect_source(path, false) } +/// One skill's authoritative source as recorded by the `skills` CLI in +/// `/.skill-lock.json` (e.g. `~/.agents/.skill-lock.json`). +#[derive(Clone)] +struct SkillLock { + /// Upstream URL, e.g. `https://github.com/mattpocock/skills.git`. + url: String, + /// Short `owner/repo`, used as the pack when the URL can't be parsed. + source: String, +} + +/// Look up a skill's lockfile-recorded source. `resolved` is the skill's real +/// (symlink-resolved) path; the lockfile lives two levels up — beside the +/// `skills/` folder that holds the skill. Parsed lockfiles are cached in +/// `cache` by path. Returns `None` when there is no lockfile or no entry for +/// `name`. +fn skill_lock_source( + resolved: &Path, + name: &str, + cache: &mut HashMap>>, +) -> Option { + // `resolved` = /skills/[/SKILL.md] → the lock sits at . + let lock_path = resolved.parent()?.parent()?.join(".skill-lock.json"); + cache + .entry(lock_path.clone()) + .or_insert_with(|| parse_skill_lock(&lock_path)) + .as_ref()? + .get(name) + .cloned() +} + +/// Parse a `skills` CLI lockfile into `skill name → source`. `None` when the +/// file is missing or malformed. +fn parse_skill_lock(lock_path: &Path) -> Option> { + let content = std::fs::read_to_string(lock_path).ok()?; + let json: serde_json::Value = serde_json::from_str(&content).ok()?; + let skills = json.get("skills")?.as_object()?; + Some( + skills + .iter() + .filter_map(|(name, entry)| { + let source = entry.get("source")?.as_str()?.to_string(); + let url = entry + .get("sourceUrl") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + .unwrap_or_else(|| source.clone()); + Some((name.clone(), SkillLock { url, source })) + }) + .collect(), + ) +} + fn detect_source(path: &Path, agent_managed: bool) -> Source { // Check the path itself and all parent directories for .git let mut dir = path.to_path_buf(); @@ -2074,6 +2158,79 @@ mod tests { assert!(exts[0].pack.is_none()); } + #[test] + fn test_skill_lock_overrides_enclosing_git_source() { + // The `skills` CLI lockfile is authoritative: a skill it installed must + // be attributed to its recorded upstream, not to whatever `.git` the + // shared skills root happens to sit under (e.g. a dotfiles backup repo). + // A sibling skill absent from the lock still falls back to that `.git`. + let dir = TempDir::new().unwrap(); + let root = dir.path(); + // Enclosing dotfiles repo with a real remote. + std::fs::create_dir_all(root.join(".git")).unwrap(); + std::fs::write( + root.join(".git").join("config"), + "[remote \"origin\"]\n\turl = https://github.com/octo/dotfiles.git\n", + ) + .unwrap(); + let skills = root.join("skills"); + // `tdd`'s frontmatter name deliberately differs from its folder name to + // prove the lock is matched by folder (how the CLI keys it), not name. + std::fs::create_dir_all(skills.join("tdd")).unwrap(); + std::fs::write( + skills.join("tdd").join("SKILL.md"), + "---\nname: test-driven-development\n---\n", + ) + .unwrap(); + std::fs::create_dir_all(skills.join("foo")).unwrap(); + std::fs::write(skills.join("foo").join("SKILL.md"), "---\nname: foo\n---\n").unwrap(); + std::fs::write( + root.join(".skill-lock.json"), + r#"{"version":3,"skills":{"tdd":{"source":"mattpocock/skills","sourceType":"github","sourceUrl":"https://github.com/mattpocock/skills.git","skillPath":"skills/tdd/SKILL.md"}}}"#, + ) + .unwrap(); + + let exts = scan_skill_dir(&skills, "claude"); + let tdd = exts + .iter() + .find(|e| e.name == "test-driven-development") + .unwrap(); + assert_eq!(tdd.pack.as_deref(), Some("mattpocock/skills")); + assert_eq!( + tdd.source.url.as_deref(), + Some("https://github.com/mattpocock/skills.git") + ); + // The skill not in the lock falls back to the enclosing repo. + let foo = exts.iter().find(|e| e.name == "foo").unwrap(); + assert_eq!(foo.pack.as_deref(), Some("octo/dotfiles")); + } + + #[test] + fn test_scan_plugins_attributes_to_marketplace_repo() { + // A plugin cached under `~/.claude/plugins/cache/...` must be attributed + // to its marketplace's upstream repo (from known_marketplaces.json), not + // to the dotfiles `.git` the cache dir sits inside. + let dir = TempDir::new().unwrap(); + let plugins = dir.path().join(".claude").join("plugins"); + std::fs::create_dir_all(&plugins).unwrap(); + std::fs::write( + plugins.join("installed_plugins.json"), + r#"{"version":2,"plugins":{"code-review@claude-plugins-official":[{"scope":"user","installPath":"/tmp/x/code-review/v1"}]}}"#, + ) + .unwrap(); + std::fs::write( + plugins.join("known_marketplaces.json"), + r#"{"claude-plugins-official":{"source":{"source":"github","repo":"anthropics/claude-plugins-official"}}}"#, + ) + .unwrap(); + let adapter = crate::adapter::claude::ClaudeAdapter::with_home(dir.path().to_path_buf()); + + let exts = scan_plugins(&adapter); + let p = exts.iter().find(|e| e.name == "code-review").unwrap(); + assert_eq!(p.source.origin, SourceOrigin::Git); + assert_eq!(p.pack.as_deref(), Some("anthropics/claude-plugins-official")); + } + #[test] fn test_scan_mcp_from_adapter() { let dir = TempDir::new().unwrap(); From 72a88d24c41f8bc6ef011a089f966bd1a67654b3 Mon Sep 17 00:00:00 2001 From: SherlockSalvatore Date: Wed, 24 Jun 2026 13:21:13 +0800 Subject: [PATCH 2/6] fix(store): realign git install_meta when its source repo was corrected The scanner now resolves the real source from install manifests, but the git-source backfill only writes install_meta when install_type IS NULL, so a row stamped in an earlier sync (e.g. a plugin first attributed to the enclosing dotfiles repo) keeps its stale install_url. deriveExtensionUrl prefers install_url, so the corrected source_json.url stayed shadowed and the extension lingered in the wrong group. Add refresh_stale_git_install_meta: for install_type='git' skill/plugin rows whose install_url owner/repo differs from the authoritative source_json.url owner/repo, realign install_url/revision and clear the now-stale branch/subpath + pack (re-derived by backfill_packs). Compare by pack, not raw URL string, so a legitimate install recorded as ".../repo" is not churned against the scanner's ".../repo.git" remote every sync (which would wipe its pinned revision and check state). Runs in both sync paths after the symlink heal, before backfill_packs. This is the store half of the manifest source-resolution fix (parallel to the #88 self-heal). --- crates/hk-core/src/store.rs | 199 ++++++++++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) diff --git a/crates/hk-core/src/store.rs b/crates/hk-core/src/store.rs index 2318c5b4..0c79c267 100644 --- a/crates/hk-core/src/store.rs +++ b/crates/hk-core/src/store.rs @@ -1065,6 +1065,11 @@ impl Store { // pack backfill so the cleared rows don't re-acquire a pack. Self::heal_symlinked_git_install_meta(&tx, extensions)?; + // Realign git install_meta the backfill stamped from a since-corrected + // source (e.g. plugins re-attributed to their marketplace repo). Run + // before pack backfill so pack re-derives from the refreshed URL. + Self::refresh_stale_git_install_meta(&tx)?; + // Backfill pack from install_url or source_json URL for deployed extensions // that lost their git context after being copied to agent directories Self::backfill_packs(&tx)?; @@ -1171,6 +1176,8 @@ impl Store { Self::heal_symlinked_git_install_meta(&tx, extensions)?; + Self::refresh_stale_git_install_meta(&tx)?; + Self::backfill_packs(&tx)?; tx.commit()?; @@ -1214,6 +1221,64 @@ impl Store { Ok(()) } + /// Refresh `git` install_meta that the source backfill stamped from a now- + /// corrected source. The backfill (above) only fires on `install_type IS + /// NULL`, so a row stamped in an earlier sync keeps its old `install_url` + /// even after the scanner learns the real source (e.g. a plugin first seen + /// as the enclosing dotfiles repo, now resolved to its marketplace repo via + /// the install manifest). `deriveExtensionUrl` prefers `install_url`, so the + /// stale value would keep the extension in the wrong group. + /// + /// We compare by **pack** (`owner/repo`), not by raw URL string: a genuine + /// git install records its URL verbatim (`…/repo`) while the scanner reports + /// the `.git/config` remote (`…/repo.git`), so a string compare would fire + /// every sync and wipe a legitimate install's pinned revision/check state. + /// Only a real owner/repo change realigns `install_url`/revision and clears + /// the now-stale branch/subpath + pack (re-derived by `backfill_packs`). + /// Limited to skills/plugins; marketplace/manual/cli installs are untouched. + fn refresh_stale_git_install_meta(conn: &rusqlite::Connection) -> Result<(), HkError> { + let mut stmt = conn.prepare( + "SELECT id, install_url, json_extract(source_json, '$.url'), + json_extract(source_json, '$.commit_hash') + FROM extensions + WHERE install_type = 'git' + AND kind IN ('skill', 'plugin') + AND json_extract(source_json, '$.url') IS NOT NULL", + )?; + let rows: Vec<(String, Option, String, Option)> = stmt + .query_map([], |row| { + Ok(( + row.get::<_, String>(0)?, + row.get::<_, Option>(1)?, + row.get::<_, String>(2)?, + row.get::<_, Option>(3)?, + )) + })? + .filter_map(|r| r.map_err(|e| eprintln!("[hk] row error: {e}")).ok()) + .collect(); + + for (id, install_url, source_url, source_commit) in &rows { + let new_pack = crate::scanner::extract_pack_from_url(source_url); + let old_pack = install_url + .as_deref() + .and_then(crate::scanner::extract_pack_from_url); + // Same repo (or unparseable new source) → leave the install record alone. + if new_pack.is_none() || new_pack == old_pack { + continue; + } + conn.execute( + "UPDATE extensions + SET install_url = ?1, install_url_resolved = NULL, + install_branch = NULL, install_subpath = NULL, + install_revision = ?2, remote_revision = NULL, + checked_at = NULL, check_error = NULL, pack = NULL + WHERE id = ?3", + params![source_url, source_commit, id], + )?; + } + Ok(()) + } + /// Backfill `pack` from install_url, source_json URL, or child extensions. /// Deployed skills lose their git context after being copied to agent directories, /// but install_url retains the repo URL. CLI parent extensions inherit pack from children. @@ -2736,6 +2801,140 @@ mod tests { ); } + #[test] + fn test_sync_refreshes_stale_git_install_meta() { + // Regression: a plugin first scanned as the enclosing dotfiles repo got + // install_type='git' + install_url/pack of that repo. After the scanner + // learns the real marketplace source, the backfill (install_type IS + // NULL only) can't update it, so the stale install_url kept the plugin + // in the dotfiles group. Sync must realign install_url + pack to the + // corrected source; a git row already in agreement stays untouched. + let (store, _dir) = test_store(); + + // Polluted plugin: stale dotfiles install_meta, but the scan now reports + // the real marketplace source. + let mut polluted = sample_extension(); + polluted.id = "plugin-cr".into(); + polluted.kind = ExtensionKind::Plugin; + polluted.name = "code-review".into(); + polluted.source.origin = SourceOrigin::Git; + polluted.source.url = Some("https://github.com/anthropics/claude-plugins-official".into()); + store.insert_extension(&polluted).unwrap(); + store + .set_install_meta( + "plugin-cr", + &InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/octo/dotfiles".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: None, + remote_revision: None, + checked_at: None, + check_error: None, + }, + ) + .unwrap(); + store.update_pack("plugin-cr", Some("octo/dotfiles")).unwrap(); + + // Consistent git install: install_url already matches its source. + let mut consistent = sample_extension(); + consistent.id = "plugin-ok".into(); + consistent.kind = ExtensionKind::Plugin; + consistent.name = "ok".into(); + consistent.source.origin = SourceOrigin::Git; + consistent.source.url = Some("https://github.com/real/repo".into()); + store.insert_extension(&consistent).unwrap(); + store + .set_install_meta( + "plugin-ok", + &InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/real/repo".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: None, + remote_revision: None, + checked_at: None, + check_error: None, + }, + ) + .unwrap(); + store.update_pack("plugin-ok", Some("real/repo")).unwrap(); + + // Same repo, different URL string form: a genuine git install records + // `.../repo` while the scanner reports the `.git/config` remote + // `.../repo.git`. Same pack → must NOT be touched (pinned revision and + // check state preserved), or every sync would churn legitimate installs. + let mut variant = sample_extension(); + variant.id = "plugin-variant".into(); + variant.kind = ExtensionKind::Plugin; + variant.name = "variant".into(); + variant.source.origin = SourceOrigin::Git; + variant.source.url = Some("https://github.com/owner/repo.git".into()); + store.insert_extension(&variant).unwrap(); + store + .set_install_meta( + "plugin-variant", + &InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/owner/repo".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: Some("pinned123".into()), + remote_revision: None, + checked_at: None, + check_error: None, + }, + ) + .unwrap(); + + // Re-sync as the scanner now reports them (install_meta carried in DB). + let mut s_polluted = polluted.clone(); + s_polluted.install_meta = None; + s_polluted.pack = None; + let mut s_consistent = consistent.clone(); + s_consistent.install_meta = None; + s_consistent.pack = None; + let mut s_variant = variant.clone(); + s_variant.install_meta = None; + store + .sync_extensions(&[s_polluted, s_consistent, s_variant]) + .unwrap(); + + let fixed = store.get_extension("plugin-cr").unwrap().unwrap(); + assert_eq!( + fixed.install_meta.expect("install_meta kept").url.as_deref(), + Some("https://github.com/anthropics/claude-plugins-official"), + "stale install_url must be realigned to the corrected source" + ); + assert_eq!( + fixed.pack.as_deref(), + Some("anthropics/claude-plugins-official"), + "pack must re-derive from the corrected source" + ); + + let kept = store.get_extension("plugin-ok").unwrap().unwrap(); + assert_eq!(kept.pack.as_deref(), Some("real/repo"), "consistent git row untouched"); + + // Same-repo string variant: install record (incl. pinned revision) intact. + let variant_kept = store.get_extension("plugin-variant").unwrap().unwrap(); + let vm = variant_kept.install_meta.expect("variant install_meta kept"); + assert_eq!( + vm.url.as_deref(), + Some("https://github.com/owner/repo"), + "same-repo URL string variant must not be rewritten" + ); + assert_eq!( + vm.revision.as_deref(), + Some("pinned123"), + "same-repo variant's pinned revision must survive" + ); + } + #[test] fn test_stale_row_should_prune_decision() { let cli = ExtensionKind::Cli.as_str(); From 2b29d5c01d531147d3baa8ee9a6bf570235898a3 Mon Sep 17 00:00:00 2001 From: SherlockSalvatore Date: Thu, 25 Jun 2026 09:58:40 +0800 Subject: [PATCH 3/6] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?case-insensitive=20pack=20compare,=20lock=20key=20for=20.md=20s?= =?UTF-8?q?kills?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the manifest source-resolution change: - refresh_stale_git_install_meta compared pack owner/repo case-sensitively, so a stored `Owner/Repo` vs a scanned `owner/repo` (GitHub is case-insensitive) would churn the row every sync, wiping its pinned revision/check state. Compare lowercased. Add a same-repo case-only regression row that must stay intact. - scan_skill_dir's lockfile key used path.file_name() unconditionally, so a standalone `.md` skill would key on `name.md` and miss its lock entry. Key dir skills by folder name (file_name, dot-safe) and standalone .md by stem. --- crates/hk-core/src/scanner.rs | 12 +++++++-- crates/hk-core/src/store.rs | 48 +++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/crates/hk-core/src/scanner.rs b/crates/hk-core/src/scanner.rs index 0375f01b..fe69a2f3 100644 --- a/crates/hk-core/src/scanner.rs +++ b/crates/hk-core/src/scanner.rs @@ -194,9 +194,17 @@ pub fn scan_skill_dir(dir: &Path, agent_name: &str) -> Vec { // That beats `detect_source`, which only ever reports whatever `.git` // the files happen to sit under (e.g. a dotfiles backup repo when the // shared skills root is itself version-controlled). - // Match by the on-disk folder name — how the `skills` CLI keys the + // Match by the on-disk entry name — how the `skills` CLI keys the // lockfile — not the frontmatter `name`, which may differ or be absent. - let lock_key = path.file_name().and_then(|n| n.to_str()).unwrap_or(name.as_str()); + // A dir skill keys on its folder (`file_name`, dot-safe); a standalone + // `.md` keys on its stem so the `.md` suffix is dropped. + let lock_key = if path.is_dir() { + path.file_name() + } else { + path.file_stem() + } + .and_then(|n| n.to_str()) + .unwrap_or(name.as_str()); if let Some(locked) = skill_lock_source(&resolved, lock_key, &mut lock_cache) { pack = extract_pack_from_url(&locked.url).or(Some(locked.source)); source = Source { diff --git a/crates/hk-core/src/store.rs b/crates/hk-core/src/store.rs index 0c79c267..2800dac8 100644 --- a/crates/hk-core/src/store.rs +++ b/crates/hk-core/src/store.rs @@ -1257,11 +1257,12 @@ impl Store { .filter_map(|r| r.map_err(|e| eprintln!("[hk] row error: {e}")).ok()) .collect(); + // GitHub owner/repo is case-insensitive, so compare lowercased to avoid + // churning a row whose stored URL only differs in case. + let norm = |url: &str| crate::scanner::extract_pack_from_url(url).map(|p| p.to_lowercase()); for (id, install_url, source_url, source_commit) in &rows { - let new_pack = crate::scanner::extract_pack_from_url(source_url); - let old_pack = install_url - .as_deref() - .and_then(crate::scanner::extract_pack_from_url); + let new_pack = norm(source_url); + let old_pack = install_url.as_deref().and_then(norm); // Same repo (or unparseable new source) → leave the install record alone. if new_pack.is_none() || new_pack == old_pack { continue; @@ -2892,6 +2893,32 @@ mod tests { ) .unwrap(); + // Same repo, case-only difference: GitHub owner/repo is case-insensitive, + // so a stored `Owner/Repo` vs scanned `owner/repo` must NOT churn. + let mut casevar = sample_extension(); + casevar.id = "plugin-case".into(); + casevar.kind = ExtensionKind::Plugin; + casevar.name = "case".into(); + casevar.source.origin = SourceOrigin::Git; + casevar.source.url = Some("https://github.com/owner/repo".into()); + store.insert_extension(&casevar).unwrap(); + store + .set_install_meta( + "plugin-case", + &InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/Owner/Repo".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: Some("casepin99".into()), + remote_revision: None, + checked_at: None, + check_error: None, + }, + ) + .unwrap(); + // Re-sync as the scanner now reports them (install_meta carried in DB). let mut s_polluted = polluted.clone(); s_polluted.install_meta = None; @@ -2901,8 +2928,10 @@ mod tests { s_consistent.pack = None; let mut s_variant = variant.clone(); s_variant.install_meta = None; + let mut s_casevar = casevar.clone(); + s_casevar.install_meta = None; store - .sync_extensions(&[s_polluted, s_consistent, s_variant]) + .sync_extensions(&[s_polluted, s_consistent, s_variant, s_casevar]) .unwrap(); let fixed = store.get_extension("plugin-cr").unwrap().unwrap(); @@ -2933,6 +2962,15 @@ mod tests { Some("pinned123"), "same-repo variant's pinned revision must survive" ); + + // Same-repo case-only variant: install record (incl. pinned revision) intact. + let case_kept = store.get_extension("plugin-case").unwrap().unwrap(); + let cm = case_kept.install_meta.expect("case variant install_meta kept"); + assert_eq!( + cm.revision.as_deref(), + Some("casepin99"), + "case-only repo variant must not be churned" + ); } #[test] From 23a8f92ce114de97ef99f561779e7565ba1c0c84 Mon Sep 17 00:00:00 2001 From: SherlockSalvatore Date: Thu, 25 Jun 2026 10:14:34 +0800 Subject: [PATCH 4/6] fix(update): guard single-update endpoints with is_update_eligible The web and desktop `update_extension` endpoints validated install_meta and install_type but not kind. The update path clones a repo and deploys it as a skill, so passing a non-skill id ran the skill-update logic on it. This became reachable once manifest source-resolution stamps plugins with install_type='git'. Add the same `is_update_eligible` gate the bulk update path already uses (rejects non-skill kinds), so a plugin id can no longer enter the skill clone+deploy flow. --- crates/hk-desktop/src/commands/extensions.rs | 9 +++++++++ crates/hk-web/src/handlers/install.rs | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/crates/hk-desktop/src/commands/extensions.rs b/crates/hk-desktop/src/commands/extensions.rs index 1ada3d47..de67f5ee 100644 --- a/crates/hk-desktop/src/commands/extensions.rs +++ b/crates/hk-desktop/src/commands/extensions.rs @@ -574,6 +574,15 @@ pub async fn update_extension( let ext = store .get_extension(&id)? .ok_or_else(|| HkError::NotFound(format!("Extension '{}' not found", id)))?; + // The update path clones a repo and deploys it as a skill, so guard + // against non-skill kinds (e.g. a plugin now carrying install_type + // 'git') reaching it — same gate the bulk update path uses. + if !service::is_update_eligible(&ext) { + return Err(HkError::Validation(format!( + "Extension '{}' is not eligible for update", + ext.name + ))); + } let meta = ext.install_meta.clone().ok_or_else(|| { HkError::NotFound("Extension has no install metadata — cannot update".into()) })?; diff --git a/crates/hk-web/src/handlers/install.rs b/crates/hk-web/src/handlers/install.rs index 6ffe1075..f325a755 100644 --- a/crates/hk-web/src/handlers/install.rs +++ b/crates/hk-web/src/handlers/install.rs @@ -305,6 +305,14 @@ pub async fn update_extension( let store = state.store.lock(); let ext = store.get_extension(¶ms.id)? .ok_or_else(|| hk_core::HkError::NotFound(format!("Extension '{}' not found", params.id)))?; + // The update path clones a repo and deploys it as a skill, so guard + // against non-skill kinds (e.g. a plugin now carrying install_type + // 'git') reaching it — same gate the bulk update path uses. + if !service::is_update_eligible(&ext) { + return Err(hk_core::HkError::Validation(format!( + "Extension '{}' is not eligible for update", ext.name + ))); + } let meta = ext.install_meta.clone().ok_or_else(|| { hk_core::HkError::NotFound("Extension has no install metadata — cannot update".into()) })?; From 432970e0004fcf47b3045536c3d9a17cb395548c Mon Sep 17 00:00:00 2001 From: RealZST Date: Thu, 25 Jun 2026 22:56:19 +0300 Subject: [PATCH 5/6] fix(store): only manifest-derived sources may correct an install record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refresh_stale_git_install_meta realigned a row's install_url to its scanned source whenever the owner/repo packs differed. But a `.git`-inferred source (e.g. an HK-git-installed skill that merely sits under a personal dotfiles repo) is not authoritative — trusting it clobbers the real install_url and wipes the pinned revision, re-attributing the skill to the dotfiles repo. Add `Source::from_manifest`, set only where the source is read from an authoritative install manifest (the skills CLI `.skill-lock.json`, or a plugin marketplace map), and gate the realignment on it so only a manifest-derived source may correct an install record. Inferred sources are left alone. --- crates/hk-core/src/auditor/rules.rs | 2 + crates/hk-core/src/kits/tests/service.rs | 3 +- crates/hk-core/src/manager.rs | 5 ++ crates/hk-core/src/models.rs | 10 ++++ crates/hk-core/src/scanner.rs | 12 ++++ crates/hk-core/src/store.rs | 69 +++++++++++++++++++++- crates/hk-core/tests/toggle_integration.rs | 3 + 7 files changed, 102 insertions(+), 2 deletions(-) diff --git a/crates/hk-core/src/auditor/rules.rs b/crates/hk-core/src/auditor/rules.rs index df69b651..effefbc3 100644 --- a/crates/hk-core/src/auditor/rules.rs +++ b/crates/hk-core/src/auditor/rules.rs @@ -998,6 +998,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, file_path: "SKILL.md".into(), mcp_command: None, @@ -1024,6 +1025,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, file_path: "config.json".into(), mcp_command: Some(command.into()), diff --git a/crates/hk-core/src/kits/tests/service.rs b/crates/hk-core/src/kits/tests/service.rs index 80354368..e574ffb9 100644 --- a/crates/hk-core/src/kits/tests/service.rs +++ b/crates/hk-core/src/kits/tests/service.rs @@ -1120,7 +1120,7 @@ fn make_skill_ext( kind: ExtensionKind::Skill, name: name.into(), description: String::new(), - source: Source { origin, url: url.map(String::from), version: None, commit_hash: None }, + source: Source { origin, url: url.map(String::from), version: None, commit_hash: None, from_manifest: false }, agents: vec![agent.into()], tags: vec![], pack: None, @@ -1301,6 +1301,7 @@ fn insert_kit_asset( url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], diff --git a/crates/hk-core/src/manager.rs b/crates/hk-core/src/manager.rs index 16569e72..a1d95f76 100644 --- a/crates/hk-core/src/manager.rs +++ b/crates/hk-core/src/manager.rs @@ -1138,6 +1138,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1184,6 +1185,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1245,6 +1247,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1896,6 +1899,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -2394,6 +2398,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], diff --git a/crates/hk-core/src/models.rs b/crates/hk-core/src/models.rs index a3bf1fd0..6f692b68 100644 --- a/crates/hk-core/src/models.rs +++ b/crates/hk-core/src/models.rs @@ -82,6 +82,16 @@ pub struct Source { pub url: Option, pub version: Option, pub commit_hash: Option, + /// True when `url` was read from an authoritative install manifest — the + /// `skills` CLI `.skill-lock.json` for skills, or a plugin marketplace map — + /// rather than inferred from the nearest enclosing `.git`. Only a + /// manifest-derived source is trusted to correct a stored install record, so + /// an HK-git-installed extension that merely sits under a dotfiles repo is + /// never re-attributed to it. For skills it also marks the row as externally + /// managed, which routes its update to the `skills` CLI (see + /// `service::is_externally_managed`). + #[serde(default)] + pub from_manifest: bool, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/hk-core/src/scanner.rs b/crates/hk-core/src/scanner.rs index fe69a2f3..99c58693 100644 --- a/crates/hk-core/src/scanner.rs +++ b/crates/hk-core/src/scanner.rs @@ -213,6 +213,7 @@ pub fn scan_skill_dir(dir: &Path, agent_name: &str) -> Vec { version: None, // Keep a real commit hash if the files are an actual git checkout. commit_hash: source.commit_hash.take(), + from_manifest: true, }; } extensions.push(Extension { @@ -337,6 +338,7 @@ pub fn scan_mcp_servers(adapter: &dyn AgentAdapter) -> Vec { url: Some(url), version: None, commit_hash: None, + from_manifest: false, }, pack, ) @@ -347,6 +349,7 @@ pub fn scan_mcp_servers(adapter: &dyn AgentAdapter) -> Vec { url: None, version: None, commit_hash: None, + from_manifest: false, }, None, ) @@ -413,6 +416,7 @@ pub fn scan_hooks(adapter: &dyn AgentAdapter) -> Vec { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec![adapter.name().to_string()], tags: vec![], @@ -474,6 +478,7 @@ pub fn scan_plugins(adapter: &dyn AgentAdapter) -> Vec { url: Some(url), version: None, commit_hash: None, + from_manifest: true, }, None => plugin .path @@ -484,6 +489,7 @@ pub fn scan_plugins(adapter: &dyn AgentAdapter) -> Vec { url: None, version: None, commit_hash: None, + from_manifest: false, }), }; let pack = source.url.as_deref().and_then(extract_pack_from_url); @@ -876,6 +882,7 @@ fn scan_cli_binaries( url: known.and_then(|k| k.repo_url.map(|u| u.to_string())), version: version.clone(), commit_hash: None, + from_manifest: false, }; let pack = source.url.as_deref().and_then(extract_pack_from_url); @@ -1015,6 +1022,7 @@ pub fn scan_project_extensions( url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec![adapter.name().to_string()], tags: vec![], @@ -1065,6 +1073,7 @@ pub fn scan_project_extensions( url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec![adapter.name().to_string()], tags: vec![], @@ -1600,6 +1609,7 @@ fn detect_source(path: &Path, agent_managed: bool) -> Source { url: read_git_remote(&dir), version: None, commit_hash: read_git_commit_hash(&dir), + from_manifest: false, }; } while dir.pop() { @@ -1609,6 +1619,7 @@ fn detect_source(path: &Path, agent_managed: bool) -> Source { url: read_git_remote(&dir), version: None, commit_hash: read_git_commit_hash(&dir), + from_manifest: false, }; } } @@ -1623,6 +1634,7 @@ fn detect_source(path: &Path, agent_managed: bool) -> Source { url: None, version: None, commit_hash: None, + from_manifest: false, } } diff --git a/crates/hk-core/src/store.rs b/crates/hk-core/src/store.rs index 2800dac8..5f5c3be4 100644 --- a/crates/hk-core/src/store.rs +++ b/crates/hk-core/src/store.rs @@ -1243,7 +1243,12 @@ impl Store { FROM extensions WHERE install_type = 'git' AND kind IN ('skill', 'plugin') - AND json_extract(source_json, '$.url') IS NOT NULL", + AND json_extract(source_json, '$.url') IS NOT NULL + -- Only an authoritative manifest source may correct an install + -- record. A `.git`-inferred source (e.g. an HK-git-installed + -- skill that merely sits under a dotfiles repo) must NOT overwrite + -- the real install_url it was recorded with. + AND json_extract(source_json, '$.from_manifest') = 1", )?; let rows: Vec<(String, Option, String, Option)> = stmt .query_map([], |row| { @@ -1904,6 +1909,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec!["test".into()], @@ -2920,9 +2926,12 @@ mod tests { .unwrap(); // Re-sync as the scanner now reports them (install_meta carried in DB). + // The corrected plugin source is manifest-derived (known_marketplaces.json), + // which is what licenses refresh to realign the stale install_url. let mut s_polluted = polluted.clone(); s_polluted.install_meta = None; s_polluted.pack = None; + s_polluted.source.from_manifest = true; let mut s_consistent = consistent.clone(); s_consistent.install_meta = None; s_consistent.pack = None; @@ -2973,6 +2982,60 @@ mod tests { ); } + #[test] + fn test_refresh_preserves_authoritative_install_url_for_inferred_source() { + // Regression: an HK-git-installed skill records the real upstream in + // install_meta. If the user keeps ~/.claude under a dotfiles git repo, + // the scanner (no .skill-lock.json entry for an HK install) infers the + // enclosing dotfiles repo as the source. refresh must NOT trust that + // inferred source over the authoritative install_url (which would + // re-attribute the skill to the dotfiles repo and wipe its pinned + // revision). Only manifest-derived sources may realign. + let (store, _dir) = test_store(); + let mut skill = sample_extension(); + skill.id = "hk-skill".into(); + skill.kind = ExtensionKind::Skill; + skill.name = "my-skill".into(); + skill.source.origin = SourceOrigin::Git; + skill.source.url = Some("https://github.com/octo/dotfiles".into()); + skill.source.from_manifest = false; // inferred from the enclosing .git + store.insert_extension(&skill).unwrap(); + store + .set_install_meta( + "hk-skill", + &InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/real/my-skill".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: Some("pinnedabc123".into()), + remote_revision: None, + checked_at: None, + check_error: None, + }, + ) + .unwrap(); + store.update_pack("hk-skill", Some("real/my-skill")).unwrap(); + + let mut scanned = skill.clone(); + scanned.install_meta = None; + store.sync_extensions(&[scanned]).unwrap(); + + let got = store.get_extension("hk-skill").unwrap().unwrap(); + let im = got.install_meta.expect("install_meta preserved"); + assert_eq!( + im.url.as_deref(), + Some("https://github.com/real/my-skill"), + "authoritative install_url must survive an inferred (non-manifest) source" + ); + assert_eq!( + im.revision.as_deref(), + Some("pinnedabc123"), + "pinned revision must not be wiped by an inferred source" + ); + } + #[test] fn test_stale_row_should_prune_decision() { let cli = ExtensionKind::Cli.as_str(); @@ -3080,6 +3143,7 @@ mod tests { url: Some("https://github.com/user/old-skill".into()), version: None, commit_hash: Some("aaa111".into()), + from_manifest: false, }; ext.install_meta = None; @@ -3112,6 +3176,7 @@ mod tests { url: Some("https://github.com/user/skill".into()), version: None, commit_hash: Some("new-scan-hash".into()), + from_manifest: false, }; ext.install_meta = Some(InstallMeta { install_type: "marketplace".into(), @@ -3151,6 +3216,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }; ext.install_meta = None; @@ -3264,6 +3330,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], diff --git a/crates/hk-core/tests/toggle_integration.rs b/crates/hk-core/tests/toggle_integration.rs index ad5aa2f7..a084ece9 100644 --- a/crates/hk-core/tests/toggle_integration.rs +++ b/crates/hk-core/tests/toggle_integration.rs @@ -94,6 +94,7 @@ fn test_disabled_mcp_survives_rescan() { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -153,6 +154,7 @@ fn test_shared_skill_sibling_detection() { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["cursor".into()], tags: vec![], @@ -208,6 +210,7 @@ fn sample_plugin(id: &str, agent: &str) -> Extension { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec![agent.into()], tags: vec![], From 099ad624f913aacf4eb682570242a60f6eac487c Mon Sep 17 00:00:00 2001 From: RealZST Date: Thu, 25 Jun 2026 22:56:37 +0300 Subject: [PATCH 6/6] feat(skills): delegate externally-managed skill updates to the skills CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills installed by the external `skills` CLI live in ~/.agents/skills/ and are tracked in ~/.agents/.skill-lock.json (detected via Source::from_manifest). Updating them with HK's own clone+deploy overwrites the CLI-managed copy and leaves the CLI's lockfile hash stale, drifting the two tools apart. Route such a skill's update to `skills update -g -y` (preferring a `skills` binary, falling back to `npx --yes skills`, and falling back to HK's own update when neither is available), so the CLI updates the files AND its own lockfile in one step. Delete stays native — HK's delete is agent/path-granular whereas `skills remove` removes from every agent, and removing a per-agent symlink doesn't touch the canonical copy (no lockfile drift). The CLI syncs the skill to its upstream HEAD, but the deployed files aren't a git checkout, so a rescan would leave install_revision NULL and the row stuck on "update available". After a delegated update, record the upstream HEAD across the skill's same-scope copies so the git-based update check clears the indicator — mirroring how the native path records its captured revision. --- crates/hk-core/src/lib.rs | 1 + crates/hk-core/src/service.rs | 139 +++++++++++++++++++ crates/hk-core/src/skills_cli.rs | 128 +++++++++++++++++ crates/hk-desktop/src/commands/extensions.rs | 12 ++ crates/hk-web/src/handlers/install.rs | 11 ++ 5 files changed, 291 insertions(+) create mode 100644 crates/hk-core/src/skills_cli.rs diff --git a/crates/hk-core/src/lib.rs b/crates/hk-core/src/lib.rs index 44839c70..03b6d028 100644 --- a/crates/hk-core/src/lib.rs +++ b/crates/hk-core/src/lib.rs @@ -10,6 +10,7 @@ pub mod models; pub mod sanitize; pub mod scanner; pub mod service; +pub mod skills_cli; pub mod store; pub use error::HkError; diff --git a/crates/hk-core/src/service.rs b/crates/hk-core/src/service.rs index fee69dfe..aadc70ec 100644 --- a/crates/hk-core/src/service.rs +++ b/crates/hk-core/src/service.rs @@ -3,8 +3,10 @@ use crate::{ adapter::AgentAdapter, auditor::{AuditInput, Auditor}, deployer, + manager, models::*, scanner, + skills_cli, store::Store, }; use parking_lot::Mutex; @@ -194,6 +196,83 @@ pub fn is_update_eligible(ext: &Extension) -> bool { matches!(ext.scope, ConfigScope::Global) || ext.install_meta.is_some() } +/// Whether a skill's files are owned by the external `skills` CLI, which installs +/// into `~/.agents/skills/` and tracks them in `~/.agents/.skill-lock.json`. +/// Such a skill's **update** is routed to that CLI (see [`try_delegate_skill_update`]) +/// so its lockfile stays in sync; delete stays native (it's agent-granular and +/// doesn't rewrite the canonical copy). Identified by a manifest-derived source +/// (`Source::from_manifest`) — matched in the lockfile during the scan. +pub fn is_externally_managed(ext: &Extension) -> bool { + ext.kind == ExtensionKind::Skill && ext.source.from_manifest +} + +/// If `ext` is a `skills`-CLI-managed skill, hand its update to that CLI (which +/// updates the files AND its own lockfile). Returns `Ok(true)` when the CLI did +/// the update — the caller should skip its own deploy and let the follow-up +/// rescan reflect the change — and `Ok(false)` when the extension isn't externally +/// managed OR the CLI is unavailable, in which case the caller does its own +/// update. Errors from a CLI that ran but failed propagate. +pub fn try_delegate_skill_update( + store: &Mutex, + ext: &Extension, +) -> Result { + if !is_externally_managed(ext) { + return Ok(false); + } + if !skills_cli::try_update(&ext.name, &ext.scope)? { + return Ok(false); // CLI unavailable → caller falls back to its own update + } + // `skills update` synced the skill to its upstream HEAD. HK's update check is + // git-revision based, but the deployed files aren't a git checkout, so a + // rescan would leave install_revision NULL and the row stuck on "update + // available". Record the upstream HEAD now (best-effort — the update already + // succeeded) so the check sees "up to date", mirroring the native path. + if let Some(url) = ext.install_meta.as_ref().and_then(|m| m.url.clone()).or_else(|| ext.source.url.clone()) { + match manager::get_remote_head(&url) { + Ok(rev) => record_skill_revision(store, ext, &rev)?, + Err(e) => eprintln!("[hk] delegated update: could not record revision: {e}"), + } + } + Ok(true) +} + +/// Stamp `revision` as the local install revision on every same-name, same-scope +/// skill row, so the git-based update check sees the skill as up to date after an +/// external (`skills` CLI) update. Mirrors how the native clone+deploy path +/// records the captured revision across sibling copies. +fn record_skill_revision(store: &Mutex, ext: &Extension, revision: &str) -> Result<(), HkError> { + let store = store.lock(); + let base = ext.install_meta.clone().unwrap_or_else(|| InstallMeta { + install_type: "git".into(), + url: ext.source.url.clone(), + url_resolved: None, + branch: None, + subpath: None, + revision: None, + remote_revision: None, + checked_at: None, + check_error: None, + }); + let updated = InstallMeta { + revision: Some(revision.to_string()), + remote_revision: None, + checked_at: None, + check_error: None, + ..base + }; + let siblings: Vec = store + .list_extensions(Some(ext.kind), None)? + .into_iter() + .filter(|e| e.name == ext.name && e.source_path.is_some() && same_scope(&e.scope, &ext.scope)) + .collect(); + for sib in &siblings { + if let Err(e) = store.set_install_meta(&sib.id, &updated) { + eprintln!("[hk] warning: {e}"); + } + } + Ok(()) +} + // --- Manual source binding --------------------------------------------------- // // Users with locally-scanned skills (no install_meta) can type a GitHub @@ -779,6 +858,11 @@ pub fn delete_extension( // Phase 2: filesystem / config-file mutation. No DB access here. match ext.kind { ExtensionKind::Skill => { + // Deleted natively even for skills-CLI-managed skills: HK's delete is + // per-agent/path (the user may remove one agent's copy), whereas + // `skills remove` always removes from every agent. Removing a symlink + // doesn't touch the CLI's canonical `~/.agents` copy, so no lockfile + // drift. (Update, which rewrites that copy, IS delegated.) if let Some(loc) = scanner::find_skill_by_id(adapters, id, &ext.agents, &projects) { if loc.entry_path.is_dir() { std::fs::remove_dir_all(&loc.entry_path)?; @@ -1373,6 +1457,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1434,6 +1519,57 @@ mod tests { assert!(!is_update_eligible(&mcp)); } + #[test] + fn test_record_skill_revision_stamps_all_siblings() { + // Regression: after a delegated (skills-CLI) update, the deployed files + // aren't a git checkout, so a rescan leaves install_revision NULL and the + // row stays stuck on "update available". record_skill_revision stamps the + // upstream HEAD across every same-name/same-scope copy so the git-based + // check sees "up to date". + let (store, _dir) = test_store(); + let git_meta = |rev: Option<&str>| InstallMeta { + install_type: "git".into(), + url: Some("https://github.com/owner/repo.git".into()), + url_resolved: None, + branch: None, + subpath: None, + revision: rev.map(String::from), + remote_revision: None, + checked_at: None, + check_error: None, + }; + // Two agent copies of one externally-managed skill, NULL local revision. + let mut claude = make_skill(ConfigScope::Global, Some(git_meta(None))); + claude.id = "skill-claude".into(); + claude.name = "my-skill".into(); + claude.source.from_manifest = true; + claude.source_path = Some("/home/u/.claude/skills/my-skill/SKILL.md".into()); + let mut codex = claude.clone(); + codex.id = "skill-codex".into(); + codex.source_path = Some("/home/u/.codex/skills/my-skill/SKILL.md".into()); + store.insert_extension(&claude).unwrap(); + store.insert_extension(&codex).unwrap(); + + let store = Mutex::new(store); + record_skill_revision(&store, &claude, "abc123def456").unwrap(); + + let store = store.lock(); + for id in ["skill-claude", "skill-codex"] { + let rev = store + .get_extension(id) + .unwrap() + .unwrap() + .install_meta + .unwrap() + .revision; + assert_eq!( + rev.as_deref(), + Some("abc123def456"), + "{id} should carry the recorded upstream revision" + ); + } + } + #[test] fn test_same_scope() { let g = ConfigScope::Global; @@ -1761,6 +1897,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1856,6 +1993,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], @@ -1970,6 +2108,7 @@ mod tests { url: None, version: None, commit_hash: None, + from_manifest: false, }, agents: vec!["claude".into()], tags: vec![], diff --git a/crates/hk-core/src/skills_cli.rs b/crates/hk-core/src/skills_cli.rs new file mode 100644 index 00000000..832e4a7b --- /dev/null +++ b/crates/hk-core/src/skills_cli.rs @@ -0,0 +1,128 @@ +//! Delegation to the external `skills` CLI (https://github.com/vercel-labs/skills). +//! +//! Skills installed by that CLI live in `~/.agents/skills/` and are tracked +//! in `~/.agents/.skill-lock.json` (recording each skill's `skillFolderHash` + +//! source). When HarnessKit updates such a skill itself, it rewrites the canonical +//! files but cannot update that lockfile's hash, so the two tools drift. +//! +//! Instead we hand the *update* to the `skills` CLI, which rewrites the files AND +//! its own lockfile in one step. We never parse or write `.skill-lock.json` — the +//! tool that owns the format owns the bookkeeping. When the CLI is unavailable the +//! caller falls back to HarnessKit's own deploy (the lockfile, if it was synced +//! from another machine, self-heals the next time the CLI runs there). +//! +//! Delete is intentionally NOT delegated: HK's delete is agent/path-granular, +//! whereas `skills remove` always removes from every agent. Removing a per-agent +//! symlink doesn't touch the CLI's canonical `~/.agents` copy (no lockfile drift), +//! so native removal is both safe and more precise. + +use std::process::Command; + +use crate::error::HkError; +use crate::models::ConfigScope; + +/// Scope restriction passed to the CLI so a Global op never touches a project +/// copy and vice versa. Externally-managed skills are detected only from the +/// global `~/.agents/.skill-lock.json`, so in practice this is always `-g` today +/// (HarnessKit doesn't read the CLI's per-project `skills-lock.json`); the +/// `Project` arm is kept for symmetry should that change. +fn scope_flag(scope: &ConfigScope) -> &'static str { + match scope { + ConfigScope::Global => "-g", + ConfigScope::Project { .. } => "-p", + } +} + +/// Build the `skills update` argument list for a single named skill, run +/// non-interactively (`-y`) and scope-restricted. Pure, so it is unit-tested. +fn build_update_args(skill_name: &str, scope: &ConfigScope) -> Vec { + vec![ + "update".to_string(), + skill_name.to_string(), + scope_flag(scope).to_string(), + "-y".to_string(), + ] +} + +/// The launchers tried in order: a `skills` binary on PATH, else `npx --yes +/// skills` (which fetches it on demand). +const LAUNCHERS: [(&str, &[&str]); 2] = [("skills", &[]), ("npx", &["--yes", "skills"])]; + +/// Run the `skills` CLI with `args` via the first available launcher. Returns +/// `Ok(true)` when a launcher ran and exited 0, `Ok(false)` when NO launcher is +/// installed (caller falls back to its own path), and `Err` when a launcher ran +/// but exited non-zero. +/// +/// Caveat: the `skills` CLI exits 0 even when an update changes nothing (e.g. +/// upstream unreachable) — it only prints a failure line. So `Err` here catches +/// launch/IO failures, not "the update did nothing". HarnessKit's follow-up +/// rescan is the source of truth for what actually changed on disk. +fn run(args: &[String]) -> Result { + run_with(&LAUNCHERS, args) +} + +/// [`run`] with an injectable launcher list, so the launch/fallback control flow +/// can be unit-tested with real shell utilities. +fn run_with(launchers: &[(&str, &[&str])], args: &[String]) -> Result { + for (program, lead) in launchers { + let mut cmd = Command::new(program); + cmd.args(*lead).args(args); + match cmd.output() { + Ok(out) if out.status.success() => return Ok(true), + Ok(out) => { + let stderr = String::from_utf8_lossy(&out.stderr); + return Err(HkError::CommandFailed(format!( + "`{program}` skills command failed: {}", + stderr.trim() + ))); + } + // This launcher isn't installed — try the next one. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue, + Err(e) => return Err(HkError::CommandFailed(e.to_string())), + } + } + Ok(false) +} + +/// Update one externally-managed skill via the CLI. See [`run`] for the return +/// contract (`Ok(false)` = CLI unavailable → caller falls back). +pub fn try_update(skill_name: &str, scope: &ConfigScope) -> Result { + run(&build_update_args(skill_name, scope)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn build_update_args_pins_the_cli_invocation() { + // The exact command string is the most breakage-prone part of delegation. + // Manifest skills are global-only, so `-g` is what's exercised in practice. + assert_eq!( + build_update_args("tdd", &ConfigScope::Global), + vec!["update", "tdd", "-g", "-y"] + ); + } + + #[test] + fn run_with_no_launcher_available_returns_false() { + // No launcher on PATH → Ok(false) so the caller falls back to its own path. + let launchers: [(&str, &[&str]); 1] = [("hk-no-such-binary-xyz", &[])]; + assert!(!run_with(&launchers, &["update".into()]).unwrap()); + } + + #[test] + fn run_with_falls_through_missing_launcher_to_present_one() { + // First launcher missing, second present and exits 0 → Ok(true). + let launchers: [(&str, &[&str]); 2] = [("hk-no-such-binary-xyz", &[]), ("true", &[])]; + assert!(run_with(&launchers, &[]).unwrap()); + } + + #[test] + fn run_with_nonzero_exit_is_error_not_fallthrough() { + // A launcher that runs but exits non-zero surfaces as Err — it must NOT + // fall through to the next launcher (don't mask a real failure). + let launchers: [(&str, &[&str]); 2] = [("false", &[]), ("true", &[])]; + assert!(run_with(&launchers, &[]).is_err()); + } +} diff --git a/crates/hk-desktop/src/commands/extensions.rs b/crates/hk-desktop/src/commands/extensions.rs index de67f5ee..d81dff09 100644 --- a/crates/hk-desktop/src/commands/extensions.rs +++ b/crates/hk-desktop/src/commands/extensions.rs @@ -597,6 +597,18 @@ pub async fn update_extension( } (ext, meta) }; + + // Skills-CLI-managed skills update via that CLI; caller's scan_and_sync + // reflects it. Falls through to HK's clone+deploy when delegation declines. + if service::try_delegate_skill_update(&store_clone, &ext)? { + return Ok(manager::InstallResult { + name: ext.name.clone(), + was_update: true, + revision: None, + skipped: false, + }); + } + let url = install_meta .url_resolved .as_deref() diff --git a/crates/hk-web/src/handlers/install.rs b/crates/hk-web/src/handlers/install.rs index f325a755..fb4b42bd 100644 --- a/crates/hk-web/src/handlers/install.rs +++ b/crates/hk-web/src/handlers/install.rs @@ -327,6 +327,17 @@ pub async fn update_extension( (ext, meta) }; + // Skills-CLI-managed skills update via that CLI; the follow-up rescan + // reflects it. Falls through to HK's clone+deploy when delegation declines. + if service::try_delegate_skill_update(&state.store, &ext)? { + return Ok(manager::InstallResult { + name: ext.name.clone(), + was_update: true, + revision: None, + skipped: false, + }); + } + let url = install_meta.url_resolved.as_deref() .or(install_meta.url.as_deref()) .ok_or_else(|| hk_core::HkError::NotFound("Extension has no remote URL".into()))?;