From 47d10e1d5fff2fb39ba3dcb670e68934b809d3b7 Mon Sep 17 00:00:00 2001 From: Tuo Shi <67766569+tshi92@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:31:57 +0800 Subject: [PATCH] fix(store): prune ghost rows left by disabling then deleting a skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disabling a skill renames SKILL.md to SKILL.md.disabled, and the scanner reports the renamed file with enabled=false — so a disabled skill still on disk appears in every scan and never looks stale. The blanket `if !enabled { return false }` guard therefore did nothing for it, and only fired once the .disabled file was gone too: exactly the case where the row should go. Deleting a disabled skill outside HarnessKit left a row no rescan could clear, and clicking "enable" on it silently did nothing while flipping the toggle on, so the list claimed an active skill that did not exist. Narrow the guard to rows whose disabled state the store alone holds, which is what `disabled_config` marks. Skills never set it — disabling one leaves its bytes on disk under another name — so they become collectable, while rows that would lose their only copy stay exempt. UPSERT_EXTENSION_SQL already uses the same predicate to stop a scan from overwriting `enabled`, so this keeps one notion of "the store owns this row" rather than two. Also stop reporting every disabled file-backed row as missing. The scanner deliberately records the *enabled* filename for a disabled skill so its id survives toggling, so testing source_path alone always failed and the transient-scan-gap exemption never reached disabled rows. Check the .disabled sibling too. Verified against a real store: the ghost row cleared on the next scan, and all 85 disabled rows with a source path still resolved on disk and were left alone. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hk-core/src/store.rs | 165 ++++++++++++++++++++++++++++-------- 1 file changed, 130 insertions(+), 35 deletions(-) diff --git a/crates/hk-core/src/store.rs b/crates/hk-core/src/store.rs index 16704ec..e7cab90 100644 --- a/crates/hk-core/src/store.rs +++ b/crates/hk-core/src/store.rs @@ -927,35 +927,58 @@ impl Store { Ok(()) } + /// Does this row's file still exist on disk, in either toggle state? + /// + /// HarnessKit disables file-backed extensions by renaming them with a + /// `.disabled` suffix, and for a disabled skill the scanner deliberately + /// records the *enabled* filename so the row's id survives toggling (see + /// `scanner::scan_skill_dir`). Testing `source_path` alone would therefore + /// report every disabled skill as missing. Check both names so "still on + /// disk" means what it says. + fn source_still_on_disk(source_path: &str) -> bool { + Path::new(source_path).exists() || Path::new(&format!("{source_path}.disabled")).exists() + } + /// Decide whether a stale extension row (one absent from the latest scan) /// should be pruned from the store. /// /// Kept (returns false): - /// - disabled rows — intentionally absent from scan results; + /// - disabled rows whose state the store alone holds — `disabled_config` is + /// where an MCP/hook entry goes once it is removed from the agent's config + /// file, and where a renamed plugin manifest's path is recorded. Pruning + /// those would destroy the only copy, so they are exempt. This is the same + /// predicate `UPSERT_EXTENSION_SQL` uses to stop a scan from overwriting + /// `enabled`; /// - CLI extensions with install_meta — their binary can transiently fail /// detection on startup, so one missing scan isn't proof of removal; - /// - file-backed install_meta rows whose `source_path` still exists on disk - /// (or is unknown) — a momentary scan gap, not a real uninstall. + /// - file-backed install_meta rows still on disk (or whose path is unknown) + /// — a momentary scan gap, not a real uninstall. + /// + /// Pruned (returns true): everything else that is gone, including skill and + /// plugin rows with install_meta whose files the user deleted (e.g. + /// `rm -rf ~/.claude`) — otherwise they linger forever as ghost rows. /// - /// Pruned (returns true): everything else that is enabled and gone, - /// including skill and plugin rows with install_meta whose files the user - /// deleted (e.g. `rm -rf ~/.claude`) — otherwise they linger forever as - /// ghost rows. Scanned MCP/hook entries carry no install_meta, so they take - /// the normal no-meta prune path rather than this `has_install_meta` branch. + /// Disabled *skills* fall in that last group by design: they carry no + /// `disabled_config`, and a disabled skill still on disk is reported by the + /// scanner as `SKILL.md.disabled` with `enabled = false`, so it is never + /// stale in the first place. Reaching this function means both filenames are + /// gone. Scanned MCP/hook entries carry no install_meta, so they take the + /// normal no-meta prune path rather than the `has_install_meta` branch. fn stale_row_should_prune( enabled: bool, + has_disabled_config: bool, has_install_meta: bool, kind: &str, source_path: Option<&str>, ) -> bool { - if !enabled { + if !enabled && has_disabled_config { return false; } if has_install_meta { if kind == ExtensionKind::Cli.as_str() { return false; } - if source_path.is_none_or(|p| Path::new(p).exists()) { + if source_path.is_none_or(Self::source_still_on_disk) { return false; } } @@ -1014,32 +1037,37 @@ impl Store { )?; // Remove stale extensions no longer on disk. The keep/prune decision - // lives in `stale_row_should_prune`: disabled rows and CLI binaries with - // install_meta are always kept; file-backed install_meta rows are kept - // only while their source_path still exists, so a manual delete (e.g. - // `rm -rf ~/.claude`) no longer leaves ghost rows behind. + // lives in `stale_row_should_prune`: rows whose disabled state the store + // alone holds, and CLI binaries with install_meta, are always kept; + // file-backed install_meta rows are kept only while their file is still + // there, so a manual delete (e.g. `rm -rf ~/.claude`) no longer leaves + // ghost rows behind. let scanned_ids: std::collections::HashSet<&str> = extensions.iter().map(|e| e.id.as_str()).collect(); - let stale_ids: Vec<(String, bool, bool, String, Option)> = { + let stale_ids: Vec<(String, bool, bool, bool, String, Option)> = { let mut stmt = tx.prepare( - "SELECT id, enabled, (install_type IS NOT NULL) as has_meta, kind, source_path FROM extensions" + "SELECT id, enabled, (disabled_config IS NOT NULL) as has_disabled_config, + (install_type IS NOT NULL) as has_meta, kind, source_path + FROM extensions" )?; stmt.query_map([], |row| { Ok(( row.get::<_, String>(0)?, row.get::<_, bool>(1)?, row.get::<_, bool>(2)?, - row.get::<_, String>(3)?, - row.get::<_, Option>(4)?, + row.get::<_, bool>(3)?, + row.get::<_, String>(4)?, + row.get::<_, Option>(5)?, )) })? .filter_map(|r| r.map_err(|e| eprintln!("[hk] row error: {e}")).ok()) .collect() }; - for (id, enabled, has_install_meta, kind, source_path) in &stale_ids { + for (id, enabled, has_disabled_config, has_install_meta, kind, source_path) in &stale_ids { if !scanned_ids.contains(id.as_str()) && Self::stale_row_should_prune( *enabled, + *has_disabled_config, *has_install_meta, kind, source_path.as_deref(), @@ -1130,14 +1158,17 @@ impl Store { } // Remove stale extensions for THIS agent only, using the same keep/prune - // rule as sync_extensions (see `stale_row_should_prune`): disabled rows - // and CLI binaries with install_meta stay; file-backed install_meta rows - // stay only while their source_path still exists on disk. + // rule as sync_extensions (see `stale_row_should_prune`): rows whose + // disabled state the store alone holds, and CLI binaries with + // install_meta, stay; file-backed install_meta rows stay only while + // their file is still on disk. let scanned_ids: std::collections::HashSet<&str> = extensions.iter().map(|e| e.id.as_str()).collect(); - let stale_ids: Vec<(String, bool, bool, String, Option)> = { + let stale_ids: Vec<(String, bool, bool, bool, String, Option)> = { let mut stmt = tx.prepare( - "SELECT DISTINCT e.id, e.enabled, (e.install_type IS NOT NULL) as has_meta, + "SELECT DISTINCT e.id, e.enabled, + (e.disabled_config IS NOT NULL) as has_disabled_config, + (e.install_type IS NOT NULL) as has_meta, e.kind, e.source_path FROM extensions e INNER JOIN extension_agents ea ON e.id = ea.extension_id @@ -1148,17 +1179,19 @@ impl Store { row.get::<_, String>(0)?, row.get::<_, bool>(1)?, row.get::<_, bool>(2)?, - row.get::<_, String>(3)?, - row.get::<_, Option>(4)?, + row.get::<_, bool>(3)?, + row.get::<_, String>(4)?, + row.get::<_, Option>(5)?, )) })? .filter_map(|r| r.ok()) .collect() }; - for (id, enabled, has_install_meta, kind, source_path) in &stale_ids { + for (id, enabled, has_disabled_config, has_install_meta, kind, source_path) in &stale_ids { if !scanned_ids.contains(id.as_str()) && Self::stale_row_should_prune( *enabled, + *has_disabled_config, *has_install_meta, kind, source_path.as_deref(), @@ -2413,13 +2446,20 @@ mod tests { fn test_sync_preserves_disabled_extensions() { let (store, _dir) = test_store(); - // Insert an extension and disable it + // Insert an extension and disable it the way `manager::toggle_mcp` does: + // the entry is lifted out of the agent's config file and parked in + // `disabled_config`, which makes this row the only copy of it. That is + // what earns the row its exemption from stale pruning — flipping the + // `enabled` flag alone would not, and must not. let mut ext = sample_extension(); ext.id = "disabled-mcp".into(); ext.kind = ExtensionKind::Mcp; ext.name = "my-mcp".into(); store.insert_extension(&ext).unwrap(); store.set_enabled("disabled-mcp", false).unwrap(); + store + .set_disabled_config("disabled-mcp", Some(r#"{"command":"my-mcp"}"#)) + .unwrap(); // Sync with an empty scan result (simulating MCP removed from config) store.sync_extensions(&[]).unwrap(); @@ -3100,23 +3140,28 @@ mod tests { #[test] fn test_stale_row_should_prune_decision() { + // Args: (enabled, has_disabled_config, has_install_meta, kind, source_path) let cli = ExtensionKind::Cli.as_str(); let skill = ExtensionKind::Skill.as_str(); let exists = env!("CARGO_MANIFEST_DIR"); // guaranteed to exist let missing = "/nonexistent/harnesskit/ghost/SKILL.md"; - // Disabled rows are intentionally absent from scans — always kept. - assert!(!Store::stale_row_should_prune(false, true, skill, Some(missing))); + // Disabled row whose state only the store holds (MCP/hook entry pulled + // out of the agent's config, or a renamed plugin manifest) — kept. + assert!(!Store::stale_row_should_prune(false, true, true, skill, Some(missing))); + // Disabled row with no such state — a skill whose SKILL.md and + // SKILL.md.disabled are both gone — is a ghost and gets pruned. + assert!(Store::stale_row_should_prune(false, false, true, skill, Some(missing))); // Sourceless rows (no install_meta) are pruned when gone — prior behavior. - assert!(Store::stale_row_should_prune(true, false, skill, None)); + assert!(Store::stale_row_should_prune(true, false, false, skill, None)); // CLI with install_meta is kept even when absent (flaky binary detection). - assert!(!Store::stale_row_should_prune(true, true, cli, Some(missing))); + assert!(!Store::stale_row_should_prune(true, false, true, cli, Some(missing))); // File-backed install_meta row whose file is gone → pruned (the ghost fix). - assert!(Store::stale_row_should_prune(true, true, skill, Some(missing))); + assert!(Store::stale_row_should_prune(true, false, true, skill, Some(missing))); // File-backed install_meta row whose file still exists → kept (scan gap). - assert!(!Store::stale_row_should_prune(true, true, skill, Some(exists))); + assert!(!Store::stale_row_should_prune(true, false, true, skill, Some(exists))); // Unknown source_path → kept (can't prove removal). - assert!(!Store::stale_row_should_prune(true, true, skill, None)); + assert!(!Store::stale_row_should_prune(true, false, true, skill, None)); } #[test] @@ -3168,6 +3213,31 @@ mod tests { cli.install_meta = meta(); store.insert_extension(&cli).unwrap(); + // Disabled skill the user then deleted outside HarnessKit. Disabling a + // skill only renames its file, so nothing is parked in disabled_config + // and the store holds no state worth saving — once both filenames are + // gone the row is a ghost like any other. + let mut disabled_ghost = sample_extension(); + disabled_ghost.id = "disabled-ghost".into(); + disabled_ghost.name = "disabled-ghost".into(); + disabled_ghost.enabled = false; + disabled_ghost.source_path = Some("/nonexistent/harnesskit/off/SKILL.md".into()); + disabled_ghost.install_meta = meta(); + store.insert_extension(&disabled_ghost).unwrap(); + + // Disabled skill still on disk as SKILL.md.disabled — a scan gap must + // not take it, even though its recorded source_path names SKILL.md. + let off_dir = dir.path().join("off-skill"); + std::fs::create_dir_all(&off_dir).unwrap(); + std::fs::write(off_dir.join("SKILL.md.disabled"), "x").unwrap(); + let mut disabled_live = sample_extension(); + disabled_live.id = "disabled-live".into(); + disabled_live.name = "disabled-live".into(); + disabled_live.enabled = false; + disabled_live.source_path = Some(off_dir.join("SKILL.md").to_string_lossy().into_owned()); + disabled_live.install_meta = meta(); + store.insert_extension(&disabled_live).unwrap(); + // Empty scan = nothing found on disk this round. store.sync_extensions(&[]).unwrap(); @@ -3181,6 +3251,14 @@ mod tests { !ids.contains(&"ghost-skill".to_string()), "ghost skill with deleted files should be pruned" ); + assert!( + !ids.contains(&"disabled-ghost".to_string()), + "disabled skill whose files are gone should be pruned too" + ); + assert!( + ids.contains(&"disabled-live".to_string()), + "disabled skill still on disk as .disabled should be kept" + ); assert!( ids.contains(&"live-skill".to_string()), "skill whose file still exists should be kept" @@ -3566,6 +3644,19 @@ mod tests { live.install_meta = meta(); store.insert_extension(&live).unwrap(); + // Disabled row whose entry the store alone holds — exercises the + // disabled_config column in this query's own SELECT list. + let mut agent_off = sample_extension(); + agent_off.id = "agent-off".into(); + agent_off.kind = ExtensionKind::Mcp; + agent_off.agents = vec!["claude".into()]; + agent_off.enabled = false; + agent_off.source_path = Some("/nonexistent/harnesskit/off/.mcp.json".into()); + store.insert_extension(&agent_off).unwrap(); + store + .set_disabled_config("agent-off", Some(r#"{"command":"x"}"#)) + .unwrap(); + store.sync_extensions_for_agent("claude", &[]).unwrap(); let ids: Vec = store @@ -3582,6 +3673,10 @@ mod tests { ids.contains(&"agent-live".to_string()), "skill whose file still exists should be kept" ); + assert!( + ids.contains(&"agent-off".to_string()), + "row whose disabled entry lives only in the store should be kept" + ); } #[test]