From 377880e0137aaf1b712b571c5a5fccb677cb9445 Mon Sep 17 00:00:00 2001 From: NianJiuZst <3235467914@qq.com> Date: Fri, 10 Jul 2026 22:20:14 +0800 Subject: [PATCH 1/6] fix(skill): preserve custom installed instructions --- crates/bsk-cli/src/cli/doctor.rs | 13 +++++ crates/bsk-cli/src/skill_install/mod.rs | 65 ++++++++++++++++++++++++ crates/bsk-cli/src/skill_install/sync.rs | 56 +++++++++++++++++++- 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index c4aa2a2b..b390a3e2 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -269,6 +269,19 @@ fn check_skill_up_to_date() -> CheckResult { ); } + if !report.protected.is_empty() { + let names = report + .protected + .iter() + .map(|h| h.cli_name()) + .collect::>() + .join(", "); + return CheckResult::na( + name, + format!("custom or unmanaged skill preserved in: {names}"), + ); + } + CheckResult::na(name, "no agent skill installed") } diff --git a/crates/bsk-cli/src/skill_install/mod.rs b/crates/bsk-cli/src/skill_install/mod.rs index b52432e3..c96119ab 100644 --- a/crates/bsk-cli/src/skill_install/mod.rs +++ b/crates/bsk-cli/src/skill_install/mod.rs @@ -15,6 +15,9 @@ pub use harness::{HarnessId, HarnessReport, all_harness_reports, parse_harness_i pub const SKILL_DIR_NAME: &str = "browser-skill"; pub const DEFAULT_SKILL_MD: &str = include_str!("../../skill/SKILL.md"); +pub const SOURCE_MARKER_FILE: &str = ".bsk-source"; +pub const SOURCE_BUNDLED: &str = "bundled\n"; +pub const SOURCE_CUSTOM: &str = "custom\n"; #[derive(Debug, Clone, Serialize)] pub struct InstallResult { @@ -135,6 +138,13 @@ fn install_one_at_home( let existed = dest_file.exists(); fs::create_dir_all(&dest_dir).with_context(|| format!("create {}", dest_dir.display()))?; fs::write(&dest_file, source).with_context(|| format!("write {}", dest_file.display()))?; + let source_kind = if source == DEFAULT_SKILL_MD { + SOURCE_BUNDLED + } else { + SOURCE_CUSTOM + }; + let marker = dest_dir.join(SOURCE_MARKER_FILE); + fs::write(&marker, source_kind).with_context(|| format!("write {}", marker.display()))?; let status = if existed { InstallStatus::Updated @@ -317,6 +327,10 @@ mod tests { assert!(out.errors.is_empty()); assert_eq!(out.results.len(), 1); assert!(skills.join(SKILL_DIR_NAME).join("SKILL.md").is_file()); + assert_eq!( + fs::read_to_string(skills.join(SKILL_DIR_NAME).join(SOURCE_MARKER_FILE)).unwrap(), + SOURCE_CUSTOM + ); } #[test] @@ -367,6 +381,57 @@ mod tests { ); assert_eq!(out.results[0].status, InstallStatus::Updated); assert_eq!(fs::read_to_string(&dest).unwrap(), "new"); + assert_eq!( + fs::read_to_string(dest.parent().unwrap().join(SOURCE_MARKER_FILE)).unwrap(), + SOURCE_CUSTOM + ); + } + + #[test] + fn bundled_install_is_marked_as_managed() { + let tmp = TempDir::new().unwrap(); + let home = tmp.path().to_path_buf(); + let harness = HarnessId::Cursor; + + let out = install_to_harnesses_at_home( + &home, + &InstallOptions { + harnesses: &[harness], + source: DEFAULT_SKILL_MD, + force: false, + home: Some(&home), + }, + ); + + assert!(out.errors.is_empty()); + let marker = harness + .skill_dest_dir_for_home(&home) + .join(SOURCE_MARKER_FILE); + assert_eq!(fs::read_to_string(marker).unwrap(), SOURCE_BUNDLED); + } + + #[test] + fn custom_install_survives_automatic_bundled_sync() { + let tmp = TempDir::new().unwrap(); + let home = tmp.path().to_path_buf(); + let harness = HarnessId::Cursor; + let dest = harness.skill_dest_dir_for_home(&home).join("SKILL.md"); + + let out = install_to_harnesses_at_home( + &home, + &InstallOptions { + harnesses: &[harness], + source: "custom instructions", + force: false, + home: Some(&home), + }, + ); + assert!(out.errors.is_empty()); + + let report = sync::sync_with_source(&home, "new bundled instructions"); + + assert_eq!(report.protected, vec![HarnessId::Cursor]); + assert_eq!(fs::read_to_string(dest).unwrap(), "custom instructions"); } #[test] diff --git a/crates/bsk-cli/src/skill_install/sync.rs b/crates/bsk-cli/src/skill_install/sync.rs index dbd7e68b..2289532b 100644 --- a/crates/bsk-cli/src/skill_install/sync.rs +++ b/crates/bsk-cli/src/skill_install/sync.rs @@ -3,7 +3,7 @@ use std::path::Path; -use super::{DEFAULT_SKILL_MD, harness::HarnessId}; +use super::{DEFAULT_SKILL_MD, SOURCE_BUNDLED, SOURCE_MARKER_FILE, harness::HarnessId}; /// Per-harness outcome of a sync pass. #[derive(Debug, Clone, Default, PartialEq, Eq)] @@ -13,6 +13,9 @@ pub struct SyncReport { /// Harnesses whose on-disk `SKILL.md` already matched the bundled /// content; no write happened, mtime preserved. pub up_to_date: Vec, + /// Custom or historical untracked installations that must not be + /// overwritten by automatic bundled-skill synchronization. + pub protected: Vec, /// Harnesses that have an installed `SKILL.md` but the sync attempt /// failed with an I/O error. The string is a human-readable detail. pub errors: Vec<(HarnessId, String)>, @@ -33,6 +36,7 @@ pub(crate) fn sync_with_source(home: &Path, source: &str) -> SyncReport { SyncOne::Missing => continue, SyncOne::UpToDate => report.up_to_date.push(harness), SyncOne::Updated => report.updated.push(harness), + SyncOne::Protected => report.protected.push(harness), SyncOne::Error(msg) => report.errors.push((harness, msg)), } } @@ -43,6 +47,7 @@ enum SyncOne { Missing, UpToDate, Updated, + Protected, Error(String), } @@ -57,6 +62,16 @@ fn sync_one(dest: &Path, source: &str) -> SyncOne { if on_disk == source { return SyncOne::UpToDate; } + let marker = dest + .parent() + .expect("SKILL.md destination must have a parent") + .join(SOURCE_MARKER_FILE); + match std::fs::read_to_string(&marker) { + Ok(value) if value == SOURCE_BUNDLED => {} + Ok(_) => return SyncOne::Protected, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return SyncOne::Protected, + Err(err) => return SyncOne::Error(format!("read {}: {err}", marker.display())), + } // Atomic replace: write tmp, rename over. Including pid in the // tmp suffix avoids concurrent processes racing on the same path. let tmp = dest.with_extension(format!("md.tmp.{}", std::process::id())); @@ -82,6 +97,14 @@ mod tests { use super::*; use tempfile::TempDir; + fn mark_bundled(dest: &Path) { + std::fs::write( + dest.parent().unwrap().join(SOURCE_MARKER_FILE), + SOURCE_BUNDLED, + ) + .unwrap(); + } + #[test] fn sync_skips_uninstalled_harness() { let tmp = TempDir::new().unwrap(); @@ -108,6 +131,7 @@ mod tests { std::fs::create_dir_all(&dest_dir).unwrap(); let dest = dest_dir.join("SKILL.md"); std::fs::write(&dest, b"old content").unwrap(); + mark_bundled(&dest); let report = sync_with_source(home, "fresh content"); @@ -137,6 +161,7 @@ mod tests { std::fs::create_dir_all(&dest_dir).unwrap(); let dest = dest_dir.join("SKILL.md"); std::fs::write(&dest, "frozen content").unwrap(); + mark_bundled(&dest); let mtime_before = std::fs::metadata(&dest).unwrap().modified().unwrap(); // Sleep enough that any rewrite would visibly change mtime on @@ -167,12 +192,14 @@ mod tests { let cursor_dir = HarnessId::Cursor.skill_dest_dir_for_home(home); std::fs::create_dir_all(&cursor_dir).unwrap(); std::fs::write(cursor_dir.join("SKILL.md"), "old").unwrap(); + mark_bundled(&cursor_dir.join("SKILL.md")); // Codex: parent dir set to r-x. Reads still succeed, but creating // SKILL.md.tmp fails → exercises sync_one's write-tmp error branch. let codex_dir = HarnessId::Codex.skill_dest_dir_for_home(home); std::fs::create_dir_all(&codex_dir).unwrap(); std::fs::write(codex_dir.join("SKILL.md"), "old").unwrap(); + mark_bundled(&codex_dir.join("SKILL.md")); let mut perms = std::fs::metadata(&codex_dir).unwrap().permissions(); perms.set_mode(0o500); // r-x: blocks tmp creation in this dir std::fs::set_permissions(&codex_dir, perms).unwrap(); @@ -188,4 +215,31 @@ mod tests { assert_eq!(report.errors.len(), 1); assert_eq!(report.errors[0].0, HarnessId::Codex); } + + #[test] + fn sync_preserves_custom_and_untracked_skills() { + let tmp = TempDir::new().unwrap(); + let home = tmp.path(); + + let custom_dir = HarnessId::Cursor.skill_dest_dir_for_home(home); + std::fs::create_dir_all(&custom_dir).unwrap(); + std::fs::write(custom_dir.join("SKILL.md"), "custom content").unwrap(); + std::fs::write(custom_dir.join(SOURCE_MARKER_FILE), "custom\n").unwrap(); + + let untracked_dir = HarnessId::Codex.skill_dest_dir_for_home(home); + std::fs::create_dir_all(&untracked_dir).unwrap(); + std::fs::write(untracked_dir.join("SKILL.md"), "historical content").unwrap(); + + let report = sync_with_source(home, "new bundled content"); + + assert_eq!(report.protected, vec![HarnessId::Codex, HarnessId::Cursor]); + assert_eq!( + std::fs::read_to_string(custom_dir.join("SKILL.md")).unwrap(), + "custom content" + ); + assert_eq!( + std::fs::read_to_string(untracked_dir.join("SKILL.md")).unwrap(), + "historical content" + ); + } } From 959f4129891a67c9f9d84d03d8071cf462bf2798 Mon Sep 17 00:00:00 2001 From: NianJiuZst <3235467914@qq.com> Date: Fri, 10 Jul 2026 22:40:16 +0800 Subject: [PATCH 2/6] chore(ci): match Biome 2.4 formatting --- apps/extension/vitest.config.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/extension/vitest.config.ts b/apps/extension/vitest.config.ts index 2e1c157e..00060bf3 100644 --- a/apps/extension/vitest.config.ts +++ b/apps/extension/vitest.config.ts @@ -21,9 +21,7 @@ export default defineConfig({ }, define: { __BSK_EXT_VERSION__: JSON.stringify(pkg.version), - __BSK_DAEMON_WS_URL__: JSON.stringify( - process.env.BSK_DAEMON_WS_URL ?? "ws://127.0.0.1:52800", - ), + __BSK_DAEMON_WS_URL__: JSON.stringify(process.env.BSK_DAEMON_WS_URL ?? "ws://127.0.0.1:52800"), }, resolve: { alias: { From 9a1a8ad71ceca24e87bd1b55ac467aac0502ec75 Mon Sep 17 00:00:00 2001 From: NianJiuZst <3235467914@qq.com> Date: Fri, 10 Jul 2026 22:50:10 +0800 Subject: [PATCH 3/6] ci: retry stalled Rust job From bf45ebe4f4e52555b7c0f32dc64bb600500d4e95 Mon Sep 17 00:00:00 2001 From: NianJiuZst <3235467914@qq.com> Date: Fri, 10 Jul 2026 23:06:02 +0800 Subject: [PATCH 4/6] test: bind daemon ports without probe races --- crates/bsk-cli/tests/browser_list_ordering.rs | 5 +---- crates/bsk-cli/tests/browser_wait.rs | 5 +---- crates/bsk-cli/tests/cancel_forwarding.rs | 5 +---- crates/bsk-cli/tests/handshake_compat.rs | 5 +---- crates/bsk-cli/tests/per_session_queue.rs | 5 +---- crates/bsk-cli/tests/session_user_interrupt.rs | 5 +---- crates/bsk-cli/tests/sessions_ipc.rs | 5 +---- crates/bsk-cli/tests/tools_ipc.rs | 5 +---- crates/bsk-cli/tests/tools_m7_ipc.rs | 5 +---- crates/bsk-cli/tests/tools_m8_ipc.rs | 5 +---- crates/bsk-cli/tests/tools_m9_ipc.rs | 5 +---- crates/bsk-cli/tests/ws_handshake.rs | 5 +---- 12 files changed, 12 insertions(+), 48 deletions(-) diff --git a/crates/bsk-cli/tests/browser_list_ordering.rs b/crates/bsk-cli/tests/browser_list_ordering.rs index 1c5199b9..f19f320f 100644 --- a/crates/bsk-cli/tests/browser_list_ordering.rs +++ b/crates/bsk-cli/tests/browser_list_ordering.rs @@ -16,7 +16,6 @@ use bsk_protocol::system::BrowserStatusEntry; use bsk_protocol::{ErrorCode, Method}; use rand::Rng; use serde::Deserialize; -use tokio::net::TcpListener; use tokio::sync::mpsc; fn tempfile_path(prefix: &str) -> PathBuf { @@ -30,9 +29,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); let sock = tempfile_path("bsk-test-browser-list"); diff --git a/crates/bsk-cli/tests/browser_wait.rs b/crates/bsk-cli/tests/browser_wait.rs index 632a7e8c..16513e0f 100644 --- a/crates/bsk-cli/tests/browser_wait.rs +++ b/crates/bsk-cli/tests/browser_wait.rs @@ -15,7 +15,6 @@ use bsk_protocol::system::{BrowserListParams, HandshakeParams, HandshakeResult, use bsk_protocol::{BrowserPeerInfo, Method, RequestFrame, ResponseBody, ResponseFrame}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -77,9 +76,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); let sock = tempfile_path("bsk-test-browser-wait"); diff --git a/crates/bsk-cli/tests/cancel_forwarding.rs b/crates/bsk-cli/tests/cancel_forwarding.rs index b9913caa..e8a0aafb 100644 --- a/crates/bsk-cli/tests/cancel_forwarding.rs +++ b/crates/bsk-cli/tests/cancel_forwarding.rs @@ -20,7 +20,6 @@ use bsk_protocol::{ use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde_json::json; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -40,9 +39,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); let sock = tempfile_path("bsk-test-cancel"); diff --git a/crates/bsk-cli/tests/handshake_compat.rs b/crates/bsk-cli/tests/handshake_compat.rs index 07e14e4d..596d35e7 100644 --- a/crates/bsk-cli/tests/handshake_compat.rs +++ b/crates/bsk-cli/tests/handshake_compat.rs @@ -10,7 +10,6 @@ use bsk_protocol::system::{HandshakeParams, HandshakeResult, StatusResult}; use bsk_protocol::{BrowserPeerInfo, ErrorCode, Method, RequestFrame, ResponseBody, ResponseFrame}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -28,9 +27,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); let sock = tempfile_path("bsk-test-handshake"); diff --git a/crates/bsk-cli/tests/per_session_queue.rs b/crates/bsk-cli/tests/per_session_queue.rs index f548ee62..79b53559 100644 --- a/crates/bsk-cli/tests/per_session_queue.rs +++ b/crates/bsk-cli/tests/per_session_queue.rs @@ -27,7 +27,6 @@ use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde::Deserialize; use serde_json::json; -use tokio::net::TcpListener; use tokio::sync::{Mutex, mpsc}; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; @@ -46,9 +45,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let sock = tempfile_path("bsk-test-queue"); let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone())) .await diff --git a/crates/bsk-cli/tests/session_user_interrupt.rs b/crates/bsk-cli/tests/session_user_interrupt.rs index 2960814f..b7d6e61f 100644 --- a/crates/bsk-cli/tests/session_user_interrupt.rs +++ b/crates/bsk-cli/tests/session_user_interrupt.rs @@ -26,7 +26,6 @@ use bsk_protocol::{ use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde_json::json; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -46,9 +45,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); let sock = tempfile_path("bsk-test-user-interrupt"); diff --git a/crates/bsk-cli/tests/sessions_ipc.rs b/crates/bsk-cli/tests/sessions_ipc.rs index a6865203..99b15352 100644 --- a/crates/bsk-cli/tests/sessions_ipc.rs +++ b/crates/bsk-cli/tests/sessions_ipc.rs @@ -15,7 +15,6 @@ use bsk_protocol::tools::{SessionStartParams, SessionStartResult, SessionStopPar use bsk_protocol::{BrowserPeerInfo, Frame, Method, RequestFrame, ResponseBody, ResponseFrame}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -39,9 +38,7 @@ async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { } async fn spawn_daemon_with_connect_wait(connect_wait: Duration) -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port).with_extension_connect_wait(connect_wait); let sock = tempfile_path("bsk-test-ipc"); diff --git a/crates/bsk-cli/tests/tools_ipc.rs b/crates/bsk-cli/tests/tools_ipc.rs index 32b77326..2857c62f 100644 --- a/crates/bsk-cli/tests/tools_ipc.rs +++ b/crates/bsk-cli/tests/tools_ipc.rs @@ -21,7 +21,6 @@ use futures_util::stream::{SplitSink, SplitStream}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde_json::{Value, json}; -use tokio::net::TcpListener; use tokio::sync::Mutex; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; @@ -40,9 +39,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let sock = tempfile_path("bsk-test-tools"); let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone())) .await diff --git a/crates/bsk-cli/tests/tools_m7_ipc.rs b/crates/bsk-cli/tests/tools_m7_ipc.rs index acb94818..ee74b5e5 100644 --- a/crates/bsk-cli/tests/tools_m7_ipc.rs +++ b/crates/bsk-cli/tests/tools_m7_ipc.rs @@ -25,7 +25,6 @@ use futures_util::stream::{SplitSink, SplitStream}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde_json::{Value, json}; -use tokio::net::TcpListener; use tokio::sync::Mutex; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; @@ -44,9 +43,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let sock = tempfile_path("bsk-test-tools-m7"); let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone())) .await diff --git a/crates/bsk-cli/tests/tools_m8_ipc.rs b/crates/bsk-cli/tests/tools_m8_ipc.rs index 5308f7bc..14e7c72a 100644 --- a/crates/bsk-cli/tests/tools_m8_ipc.rs +++ b/crates/bsk-cli/tests/tools_m8_ipc.rs @@ -24,7 +24,6 @@ use futures_util::stream::{SplitSink, SplitStream}; use futures_util::{SinkExt, StreamExt}; use rand::Rng; use serde_json::Value; -use tokio::net::TcpListener; use tokio::sync::Mutex; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; @@ -43,9 +42,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let sock = tempfile_path("bsk-test-tools-m8"); let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone())) .await diff --git a/crates/bsk-cli/tests/tools_m9_ipc.rs b/crates/bsk-cli/tests/tools_m9_ipc.rs index 6dd5d33d..2f8b46d9 100644 --- a/crates/bsk-cli/tests/tools_m9_ipc.rs +++ b/crates/bsk-cli/tests/tools_m9_ipc.rs @@ -30,7 +30,6 @@ use rand::Rng; use serde_json::{Value, json}; #[cfg(unix)] use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; -use tokio::net::TcpListener; #[cfg(unix)] use tokio::net::UnixStream; use tokio::sync::Mutex; @@ -53,9 +52,7 @@ fn tempfile_path(prefix: &str) -> PathBuf { } async fn spawn_daemon() -> (daemon::DaemonHandle, PathBuf) { - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let sock = tempfile_path("bsk-test-tools-m9"); let handle = daemon::run(DaemonConfig::new(port), Some(sock.clone())) .await diff --git a/crates/bsk-cli/tests/ws_handshake.rs b/crates/bsk-cli/tests/ws_handshake.rs index 146ef339..c6e2da14 100644 --- a/crates/bsk-cli/tests/ws_handshake.rs +++ b/crates/bsk-cli/tests/ws_handshake.rs @@ -8,7 +8,6 @@ use bsk::daemon::{self, DaemonConfig}; use bsk_protocol::system::{HandshakeParams, HandshakeResult}; use bsk_protocol::{BrowserPeerInfo, Method, RequestFrame, ResponseBody, ResponseFrame}; use futures_util::{SinkExt, StreamExt}; -use tokio::net::TcpListener; use tokio_tungstenite::tungstenite::handshake::client::generate_key; use tokio_tungstenite::tungstenite::http::Request; use tokio_tungstenite::tungstenite::protocol::Message; @@ -17,9 +16,7 @@ pub const TEST_EXT_ID: &str = "abcdefghijklmnopabcdefghijklmnop"; // 32 chars in pub async fn spawn_daemon() -> daemon::DaemonHandle { // Bind to any free TCP port. - let probe = TcpListener::bind("127.0.0.1:0").await.unwrap(); - let port = probe.local_addr().unwrap().port(); - drop(probe); + let port = 0; let config = DaemonConfig::new(port); daemon::run(config, None).await.unwrap() From e3df84c365c19c724f2830bd9bf6afc85c8f22ef Mon Sep 17 00:00:00 2001 From: drakezhang Date: Thu, 10 Sep 2026 11:52:52 +0800 Subject: [PATCH 5/6] fix(skill): make custom installation and sync safe Pass explicit source provenance from the CLI and coordinate installation and sync with per-skill locks. Atomically replace content and markers in a failure-safe order, and report every doctor outcome. Cover identical custom sources, replacement failures, and deterministic install/sync interleavings. Document how existing installations opt into managed updates. --- README.md | 15 ++ README.zh-CN.md | 13 + crates/bsk-cli/Cargo.toml | 2 +- crates/bsk-cli/src/cli/doctor.rs | 150 +++++++---- crates/bsk-cli/src/cli/install_skill.rs | 51 +++- crates/bsk-cli/src/skill_install/mod.rs | 262 +++++++++++++++++++- crates/bsk-cli/src/skill_install/storage.rs | 156 ++++++++++++ crates/bsk-cli/src/skill_install/sync.rs | 202 ++++++++++----- 8 files changed, 740 insertions(+), 111 deletions(-) create mode 100644 crates/bsk-cli/src/skill_install/storage.rs diff --git a/README.md b/README.md index 919a220a..faf86cbd 100644 --- a/README.md +++ b/README.md @@ -129,6 +129,21 @@ Use Space to select the Agent harness you want to install into, then press Enter to install the skill. Run `bsk install-skill --list` to see internal variants and install paths. +To install your own instructions, use `bsk install-skill --harness cursor --source ./SKILL.md`. +An explicit `--source` stays custom even if its contents match the bundled skill. +Existing installations are skipped unless you add `--force`. + +Daemon startup, `session start`, and `doctor` automatically update only installations +marked as bundled. Custom installations and older or manual installations without +a `.bsk-source` marker are preserved. `doctor` reports preserved installations and +any sync deferred because another install or sync is running. + +To replace an existing installation with the bundled skill and enable automatic +updates, run `bsk install-skill --harness cursor --force` without `--source`. +This overwrites the existing instructions. To customize a managed installation, +install your edited file with `--source` and `--force`; directly editing a bundled +installation does not change its ownership and it can still be overwritten by sync. + Other shell-capable agent harnesses are supported too. Copy [`skill/SKILL.md`](skill/SKILL.md) into your harness's skills directory as `browser-skill/SKILL.md` to install the skill manually. DeepSeek Harness uses a diff --git a/README.zh-CN.md b/README.zh-CN.md index 2d2bd3c7..7d5812e4 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -113,6 +113,19 @@ bsk install-skill 用 Space 选择需要安装的 Agent harness，然后按 Enter 安装 skill。运行 `bsk install-skill --list` 可查看 internal 变体及安装路径。 +安装自定义指令可运行 `bsk install-skill --harness cursor --source ./SKILL.md`。 +显式指定 `--source` 的安装始终视为自定义，即使内容与内置 skill 相同。 +已有安装默认跳过，添加 `--force` 才会覆盖。 + +daemon 启动、`session start` 和 `doctor` 只自动更新标记为内置的安装。 +自定义安装，以及没有 `.bsk-source` 标记的历史或手动安装，都会保留原内容。 +`doctor` 会列出受保护的安装，以及因其他安装或同步正在进行而推迟的同步。 + +如需用内置 skill 替换现有安装并启用自动更新，运行 +`bsk install-skill --harness cursor --force`，不带 `--source`。 +这会覆盖现有指令。修改受管理的安装时，请通过 `--source` 和 `--force` 安装编辑后的文件； +直接修改内置安装的文件不会改变其归属，仍可能被自动同步覆盖。 + 其他支持 Shell 的 Agent harness 也可使用 BrowserSkill，但需手动将 [`skill/SKILL.md`](skill/SKILL.md) 复制到对应 skills 目录下的 `browser-skill/SKILL.md`。DeepSeek Harness 走独立插件，见 [DeepSeek Harness 插件](#deepseek-harness-插件)。 diff --git a/crates/bsk-cli/Cargo.toml b/crates/bsk-cli/Cargo.toml index e24ef887..291901ae 100644 --- a/crates/bsk-cli/Cargo.toml +++ b/crates/bsk-cli/Cargo.toml @@ -45,6 +45,7 @@ console = { workspace = true } dialoguer = { workspace = true } dirs = { workspace = true } fs2 = { workspace = true } +tempfile = { workspace = true } rand = { workspace = true } uuid = { workspace = true } bsk-protocol = { workspace = true } @@ -68,7 +69,6 @@ windows-sys = { version = "0.59", features = [ ] } [dev-dependencies] -tempfile = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true } tokio-tungstenite = { workspace = true } diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index f2dd4999..b4efb842 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -247,54 +247,45 @@ fn check_skill_up_to_date() -> CheckResult { }; let report = crate::skill_install::sync::sync_installed_skills(&home); - if !report.errors.is_empty() { - let detail = report - .errors - .iter() - .map(|(h, msg)| format!("{}: {msg}", h.cli_name())) - .collect::>() - .join("; "); - return CheckResult::fail( - name, - detail, - "re-run `bsk install-skill --force --harness ` for the failing harness", - ); - } + skill_check_from_report(&report) +} - if !report.updated.is_empty() { - let names = report - .updated - .iter() - .map(|h| h.cli_name()) - .collect::>() - .join(", "); - return CheckResult::ok( - name, - format!("synced {} harness(es): {names}", report.updated.len()), - ); +fn skill_check_from_report(report: &crate::skill_install::sync::SyncReport) -> CheckResult { + let name = "agent skill up to date"; + let mut details = Vec::new(); + for (label, harnesses) in [ + ("synced", &report.updated), + ("up to date", &report.up_to_date), + ("custom or unmanaged skill preserved in", &report.protected), + ("busy, sync deferred in", &report.busy), + ] { + if !harnesses.is_empty() { + let names = harnesses + .iter() + .map(|h| h.cli_name()) + .collect::>() + .join(", "); + details.push(format!("{label}: {names}")); + } } - - if !report.up_to_date.is_empty() { - return CheckResult::ok( - name, - format!("up to date in {} harness(es)", report.up_to_date.len()), - ); + for (harness, message) in &report.errors { + details.push(format!("sync failed for {}: {message}", harness.cli_name())); } + let detail = details.join("; "); - if !report.protected.is_empty() { - let names = report - .protected - .iter() - .map(|h| h.cli_name()) - .collect::>() - .join(", "); - return CheckResult::na( + if !report.errors.is_empty() { + CheckResult::fail( name, - format!("custom or unmanaged skill preserved in: {names}"), - ); + detail, + "check filesystem access for the failing harness, then re-run `bsk doctor`", + ) + } else if !report.updated.is_empty() || !report.up_to_date.is_empty() { + CheckResult::ok(name, detail) + } else if !details.is_empty() { + CheckResult::na(name, detail) + } else { + CheckResult::na(name, "no agent skill installed") } - - CheckResult::na(name, "no agent skill installed") } fn check_daemon_running(state: &DaemonState) -> CheckResult { @@ -509,6 +500,81 @@ mod m2_tests { } } + #[test] + fn skill_check_reports_protected_harnesses_with_managed_results() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + for updated in [true, false] { + let mut report = SyncReport { + protected: vec![HarnessId::Cursor], + ..Default::default() + }; + if updated { + report.updated.push(HarnessId::ClaudeCode); + } else { + report.up_to_date.push(HarnessId::ClaudeCode); + } + let check = skill_check_from_report(&report); + assert_eq!(check.status, CheckStatus::Ok); + assert!(check.detail.contains("claude-code")); + assert!(check.detail.contains("preserved in: cursor")); + let json = serde_json::to_value(&check).unwrap(); + assert_eq!(json["status"], "ok"); + assert!( + json["detail"] + .as_str() + .unwrap() + .contains("preserved in: cursor") + ); + } + } + + #[test] + fn skill_check_keeps_all_outcomes_when_another_harness_fails() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + let check = skill_check_from_report(&SyncReport { + updated: vec![HarnessId::ClaudeCode], + up_to_date: vec![HarnessId::PiAgent], + protected: vec![HarnessId::Cursor], + busy: vec![HarnessId::Hermes], + errors: vec![(HarnessId::Workbuddy, "permission denied".into())], + }); + assert_eq!(check.status, CheckStatus::Fail); + for text in [ + "synced: claude-code", + "up to date: pi", + "preserved in: cursor", + "sync deferred in: hermes", + "workbuddy: permission denied", + ] { + assert!(check.detail.contains(text), "{}", check.detail); + } + assert!(!check.hint.unwrap().contains("--force")); + } + + #[test] + fn protected_or_busy_skills_are_informational() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + for report in [ + SyncReport { + protected: vec![HarnessId::Cursor], + ..Default::default() + }, + SyncReport { + busy: vec![HarnessId::Cursor], + ..Default::default() + }, + ] { + let check = skill_check_from_report(&report); + assert_eq!(check.status, CheckStatus::NotApplicable); + assert!(check.detail.contains("cursor")); + assert!(!has_failures(std::slice::from_ref(&check))); + assert_eq!(serde_json::to_value(&check).unwrap()["status"], "na"); + } + let empty = skill_check_from_report(&SyncReport::default()); + assert_eq!(empty.status, CheckStatus::NotApplicable); + assert_eq!(empty.detail, "no agent skill installed"); + } + #[test] fn extension_check_includes_store_url_when_no_browser_connected() { let status = fake_status(Vec::new(), Vec::new()); diff --git a/crates/bsk-cli/src/cli/install_skill.rs b/crates/bsk-cli/src/cli/install_skill.rs index 96675a42..9c520fab 100644 --- a/crates/bsk-cli/src/cli/install_skill.rs +++ b/crates/bsk-cli/src/cli/install_skill.rs @@ -11,7 +11,7 @@ use serde::Serialize; use crate::cli::error::CliError; use crate::cli::status::Output; use crate::skill_install::{ - InstallOptions, all_harness_reports, + InstallOptions, SkillSource, all_harness_reports, harness::{HarnessId, parse_harness_id}, load_source, print_harness_table, run_interactive_prompt, }; @@ -43,6 +43,16 @@ pub struct InstallSkillArgs { pub force: bool, } +impl InstallSkillArgs { + fn source_kind(&self) -> SkillSource { + if self.source.is_some() { + SkillSource::Custom + } else { + SkillSource::Bundled + } + } +} + #[derive(Debug, Serialize)] struct ListOutput { harnesses: Vec, @@ -61,6 +71,7 @@ pub fn dispatch(args: InstallSkillArgs, output: Output) -> Result<(), CliError> let install_output = crate::skill_install::install_to_harnesses(&InstallOptions { harnesses: &harnesses, source: &source, + source_kind: args.source_kind(), force: args.force, home: None, }); @@ -202,6 +213,44 @@ mod tests { } } + #[test] + fn identical_explicit_source_remains_custom_after_install_and_upgrade() { + use crate::skill_install::{DEFAULT_SKILL_MD, SOURCE_CUSTOM, SOURCE_MARKER_FILE, sync}; + let home = tempfile::tempdir().unwrap(); + let path = home.path().join("my-skill.md"); + std::fs::write(&path, DEFAULT_SKILL_MD).unwrap(); + let mut args = args(false, false); + assert_eq!(args.source_kind(), SkillSource::Bundled); + args.source = Some(path); + let source = load_source(args.source.as_deref()).unwrap(); + let result = crate::skill_install::install_to_harnesses(&InstallOptions { + harnesses: &[HarnessId::Cursor], + source: &source, + source_kind: args.source_kind(), + force: false, + home: Some(home.path()), + }); + assert!(result.success()); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + assert_eq!( + std::fs::read_to_string(dir.join(SOURCE_MARKER_FILE)).unwrap(), + SOURCE_CUSTOM + ); + // Matching bytes do not turn a custom installation into an up-to-date managed one. + assert_eq!( + sync::sync_installed_skills(home.path()).protected, + vec![HarnessId::Cursor] + ); + assert_eq!( + sync::sync_with_source(home.path(), "next bundled version").protected, + vec![HarnessId::Cursor] + ); + assert_eq!( + std::fs::read_to_string(dir.join("SKILL.md")).unwrap(), + DEFAULT_SKILL_MD + ); + } + #[test] fn all_targets_detected_harnesses_only() { let reports = vec![ diff --git a/crates/bsk-cli/src/skill_install/mod.rs b/crates/bsk-cli/src/skill_install/mod.rs index c96119ab..4e1ab11f 100644 --- a/crates/bsk-cli/src/skill_install/mod.rs +++ b/crates/bsk-cli/src/skill_install/mod.rs @@ -1,6 +1,7 @@ -//! Install the bundled browser-skill `SKILL.md` into agent harness skill directories. +//! Install bundled or custom browser-skill instructions into agent skill directories. pub mod harness; +mod storage; pub mod sync; use std::fs; @@ -19,6 +20,13 @@ pub const SOURCE_MARKER_FILE: &str = ".bsk-source"; pub const SOURCE_BUNDLED: &str = "bundled\n"; pub const SOURCE_CUSTOM: &str = "custom\n"; +/// Installation provenance is explicit: even an identical `--source` is custom. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillSource { + Bundled, + Custom, +} + #[derive(Debug, Clone, Serialize)] pub struct InstallResult { pub harness: String, @@ -77,6 +85,7 @@ pub struct InstallError { pub struct InstallOptions<'a> { pub harnesses: &'a [HarnessId], pub source: &'a str, + pub source_kind: SkillSource, pub force: bool, /// When `Some`, installs under this home instead of the real `$HOME`. pub home: Option<&'a Path>, @@ -106,7 +115,7 @@ pub fn install_to_harnesses_at_home(home: &Path, opts: &InstallOptions<'_>) -> I let mut errors = Vec::new(); for harness in opts.harnesses { - match install_one_at_home(home, *harness, opts.source, opts.force) { + match install_one_at_home(home, *harness, opts.source, opts.source_kind, opts.force) { Ok((path, status)) => results.push(InstallResult { harness: harness.cli_name().to_string(), path, @@ -114,7 +123,7 @@ pub fn install_to_harnesses_at_home(home: &Path, opts: &InstallOptions<'_>) -> I }), Err(err) => errors.push(InstallError { harness: harness.cli_name().to_string(), - message: err.to_string(), + message: format!("{err:#}"), }), } } @@ -126,25 +135,45 @@ fn install_one_at_home( home: &Path, harness: HarnessId, source: &str, + source_kind: SkillSource, force: bool, ) -> Result<(PathBuf, InstallStatus)> { let dest_dir = harness.skill_dest_dir_for_home(home); let dest_file = dest_dir.join("SKILL.md"); + // A no-op install needs no write access. Recheck under the lock before writing + // so two installers that both observed a missing file cannot overwrite it. + if dest_file.exists() && !force { + return Ok((dest_file, InstallStatus::Skipped)); + } + fs::create_dir_all(&dest_dir).with_context(|| format!("create {}", dest_dir.display()))?; + let _lock = storage::SkillLock::acquire(&dest_dir) + .with_context(|| format!("lock {}", dest_dir.display()))?; + if dest_file.exists() && !force { return Ok((dest_file, InstallStatus::Skipped)); } let existed = dest_file.exists(); - fs::create_dir_all(&dest_dir).with_context(|| format!("create {}", dest_dir.display()))?; - fs::write(&dest_file, source).with_context(|| format!("write {}", dest_file.display()))?; - let source_kind = if source == DEFAULT_SKILL_MD { - SOURCE_BUNDLED - } else { - SOURCE_CUSTOM - }; + let content = storage::PendingWrite::prepare(&dest_file, source)?; let marker = dest_dir.join(SOURCE_MARKER_FILE); - fs::write(&marker, source_kind).with_context(|| format!("write {}", marker.display()))?; + match source_kind { + SkillSource::Custom => { + // Protection must be established before any custom content appears. + storage::PendingWrite::prepare(&marker, SOURCE_CUSTOM)?.commit()?; + content + .commit() + .context("custom protection recorded, but skill content was not replaced")?; + } + SkillSource::Bundled => { + let marker = storage::PendingWrite::prepare(&marker, SOURCE_BUNDLED)?; + // Do not authorize sync until the old custom content is replaced. + content.commit()?; + marker.commit().context( + "bundled skill content installed, but its source marker was not updated", + )?; + } + } let status = if existed { InstallStatus::Updated @@ -320,6 +349,7 @@ mod tests { &InstallOptions { harnesses: &[harness], source: "# test skill\n", + source_kind: SkillSource::Custom, force: false, home: Some(&home), }, @@ -350,6 +380,7 @@ mod tests { &InstallOptions { harnesses: &[harness], source: "new", + source_kind: SkillSource::Custom, force: false, home: Some(&home), }, @@ -375,6 +406,7 @@ mod tests { &InstallOptions { harnesses: &[harness], source: "new", + source_kind: SkillSource::Custom, force: true, home: Some(&home), }, @@ -398,6 +430,7 @@ mod tests { &InstallOptions { harnesses: &[harness], source: DEFAULT_SKILL_MD, + source_kind: SkillSource::Bundled, force: false, home: Some(&home), }, @@ -422,6 +455,7 @@ mod tests { &InstallOptions { harnesses: &[harness], source: "custom instructions", + source_kind: SkillSource::Custom, force: false, home: Some(&home), }, @@ -434,6 +468,212 @@ mod tests { assert_eq!(fs::read_to_string(dest).unwrap(), "custom instructions"); } + #[test] + fn skipped_install_does_not_claim_or_change_provenance() { + for marker in [None, Some(SOURCE_CUSTOM), Some(SOURCE_BUNDLED)] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("SKILL.md"), "keep").unwrap(); + if let Some(marker) = marker { + fs::write(dir.join(SOURCE_MARKER_FILE), marker).unwrap(); + } + let (_, status) = install_one_at_home( + home.path(), + HarnessId::Cursor, + DEFAULT_SKILL_MD, + SkillSource::Bundled, + false, + ) + .unwrap(); + assert_eq!(status, InstallStatus::Skipped); + assert_eq!(fs::read_to_string(dir.join("SKILL.md")).unwrap(), "keep"); + assert!(!dir.join(".bsk.lock").exists()); + assert_eq!( + fs::read_to_string(dir.join(SOURCE_MARKER_FILE)) + .ok() + .as_deref(), + marker + ); + } + } + + #[test] + fn failed_install_keeps_content_and_provenance_safe() { + use storage::test_support::{assert_no_temporary_files, with_replace_hook}; + + for kind in [SkillSource::Custom, SkillSource::Bundled] { + for failed_file in ["SKILL.md", SOURCE_MARKER_FILE] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + let old_kind = if kind == SkillSource::Custom { + SkillSource::Bundled + } else { + SkillSource::Custom + }; + install_one_at_home( + home.path(), + HarnessId::Cursor, + "old content", + old_kind, + false, + ) + .unwrap(); + let error = with_replace_hook( + move |dest| { + if dest.file_name().unwrap() == failed_file { + Err(std::io::Error::other("injected replacement failure")) + } else { + Ok(()) + } + }, + || { + install_one_at_home( + home.path(), + HarnessId::Cursor, + "new content", + kind, + true, + ) + }, + ) + .unwrap_err(); + assert!(format!("{error:#}").contains("injected replacement failure")); + let bundled_content_installed = + kind == SkillSource::Bundled && failed_file == SOURCE_MARKER_FILE; + let expected_content = if bundled_content_installed { + "new content" + } else { + "old content" + }; + assert_eq!( + fs::read_to_string(dir.join("SKILL.md")).unwrap(), + expected_content + ); + if bundled_content_installed { + assert!( + error + .to_string() + .contains("bundled skill content installed") + ); + } + let still_bundled = + kind == SkillSource::Custom && failed_file == SOURCE_MARKER_FILE; + let expected_marker = if still_bundled { + SOURCE_BUNDLED + } else { + SOURCE_CUSTOM + }; + assert_eq!( + fs::read_to_string(dir.join(SOURCE_MARKER_FILE)).unwrap(), + expected_marker + ); + assert_no_temporary_files(&dir); + let report = sync::sync_with_source(home.path(), "next bundled version"); + if still_bundled { + assert_eq!(report.updated, vec![HarnessId::Cursor]); + } else { + assert_eq!(report.protected, vec![HarnessId::Cursor]); + assert_eq!( + fs::read_to_string(dir.join("SKILL.md")).unwrap(), + expected_content + ); + } + } + } + } + + #[test] + fn sync_defers_during_custom_install_then_preserves_it() { + use std::sync::mpsc; + use std::time::Duration; + use storage::test_support::with_replace_hook; + + let home = TempDir::new().unwrap(); + install_one_at_home( + home.path(), + HarnessId::Cursor, + "old bundled", + SkillSource::Bundled, + false, + ) + .unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + let worker_home = home.path().to_path_buf(); + let (ready_tx, ready_rx) = mpsc::channel(); + let (resume_tx, resume_rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + with_replace_hook( + move |dest| { + if dest.file_name().unwrap() == "SKILL.md" { + ready_tx.send(()).unwrap(); + resume_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + } + Ok(()) + }, + || { + install_one_at_home( + &worker_home, + HarnessId::Cursor, + "custom", + SkillSource::Custom, + true, + ) + }, + ) + }); + ready_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + let during = sync::sync_with_source(home.path(), "new bundled"); + assert_eq!(during.busy, vec![HarnessId::Cursor]); + assert!(during.updated.is_empty()); + assert!(during.errors.is_empty()); + assert_eq!( + fs::read_to_string(dir.join("SKILL.md")).unwrap(), + "old bundled" + ); + assert_eq!( + fs::read_to_string(dir.join(SOURCE_MARKER_FILE)).unwrap(), + SOURCE_CUSTOM + ); + resume_tx.send(()).unwrap(); + worker.join().unwrap().unwrap(); + let after = sync::sync_with_source(home.path(), "new bundled"); + assert_eq!(after.protected, vec![HarnessId::Cursor]); + assert_eq!(fs::read_to_string(dir.join("SKILL.md")).unwrap(), "custom"); + } + + #[test] + fn forced_bundled_install_resumes_management() { + let home = TempDir::new().unwrap(); + install_one_at_home( + home.path(), + HarnessId::Cursor, + "custom", + SkillSource::Custom, + false, + ) + .unwrap(); + install_one_at_home( + home.path(), + HarnessId::Cursor, + DEFAULT_SKILL_MD, + SkillSource::Bundled, + true, + ) + .unwrap(); + let report = sync::sync_with_source(home.path(), "new bundled"); + assert_eq!(report.updated, vec![HarnessId::Cursor]); + assert_eq!( + fs::read_to_string( + HarnessId::Cursor + .skill_dest_dir_for_home(home.path()) + .join("SKILL.md") + ) + .unwrap(), + "new bundled" + ); + } + #[test] fn interactive_candidates_omit_undetected() { let reports = vec![ diff --git a/crates/bsk-cli/src/skill_install/storage.rs b/crates/bsk-cli/src/skill_install/storage.rs new file mode 100644 index 00000000..9caba570 --- /dev/null +++ b/crates/bsk-cli/src/skill_install/storage.rs @@ -0,0 +1,156 @@ +//! File operations shared by installation and automatic synchronization. + +use std::fs::{File, OpenOptions}; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use fs2::FileExt; +use tempfile::NamedTempFile; + +/// All readers that may replace a skill hold this lock until their writes finish. +/// Keep the lock file in place: deleting it could let two processes lock different +/// files at the same path. Closing the handle releases the OS lock. +pub(super) struct SkillLock { + _file: File, +} + +impl SkillLock { + fn open(dir: &Path) -> io::Result { + OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(dir.join(".bsk.lock")) + } + + pub(super) fn acquire(dir: &Path) -> io::Result { + let file = Self::open(dir)?; + file.lock_exclusive()?; + Ok(Self { _file: file }) + } + + /// Automatic sync is best-effort and must not wait on another installer. + pub(super) fn try_acquire(dir: &Path) -> io::Result> { + let file = Self::open(dir)?; + match file.try_lock_exclusive() { + Ok(()) => Ok(Some(Self { _file: file })), + Err(err) if err.raw_os_error() == fs2::lock_contended_error().raw_os_error() => { + Ok(None) + } + Err(err) => Err(err), + } + } +} + +/// Prepare complete content before changing the destination. The unique sibling +/// temporary file is removed on error or drop, including a failed replacement. +pub(super) struct PendingWrite { + file: NamedTempFile, + dest: PathBuf, +} + +impl PendingWrite { + pub(super) fn prepare(dest: &Path, content: &str) -> Result { + let mut file = tempfile::Builder::new() + .prefix(".bsk-tmp-") + .tempfile_in(dest.parent().context("skill destination has no parent")?) + .with_context(|| format!("prepare {}", dest.display()))?; + file.write_all(content.as_bytes()) + .with_context(|| format!("write temporary file for {}", dest.display()))?; + Ok(Self { + file, + dest: dest.to_path_buf(), + }) + } + + pub(super) fn commit(self) -> Result<()> { + #[cfg(test)] + test_support::before_replace(&self.dest)?; + self.file + .persist(&self.dest) + .map_err(|err| err.error) + .with_context(|| format!("replace {}", self.dest.display()))?; + Ok(()) + } +} + +#[cfg(test)] +pub(super) mod test_support { + use super::*; + use std::cell::RefCell; + + type ReplaceHook = Box io::Result<()>>; + thread_local! { + // Thread-local injection keeps failure and interleaving tests independent. + static REPLACE_HOOK: RefCell> = RefCell::new(None); + } + + pub(super) fn before_replace(dest: &Path) -> io::Result<()> { + REPLACE_HOOK.with_borrow(|hook| match hook { + Some(hook) => hook(dest), + None => Ok(()), + }) + } + + pub(crate) fn with_replace_hook( + hook: impl Fn(&Path) -> io::Result<()> + 'static, + run: impl FnOnce() -> T, + ) -> T { + struct Reset(Option); + impl Drop for Reset { + fn drop(&mut self) { + REPLACE_HOOK.set(self.0.take()); + } + } + let _reset = Reset(REPLACE_HOOK.replace(Some(Box::new(hook)))); + run() + } + + pub(crate) fn assert_no_temporary_files(dir: &Path) { + for entry in std::fs::read_dir(dir).unwrap() { + assert!( + !entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".bsk-tmp-") + ); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn failed_atomic_replace_preserves_destination_and_cleans_up() { + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("SKILL.md"); + std::fs::create_dir(&dest).unwrap(); + std::fs::write(dest.join("sentinel"), "keep").unwrap(); + assert!( + PendingWrite::prepare(&dest, "new") + .unwrap() + .commit() + .is_err() + ); + assert_eq!( + std::fs::read_to_string(dest.join("sentinel")).unwrap(), + "keep" + ); + test_support::assert_no_temporary_files(dir.path()); + } + + #[test] + fn lock_is_shared_by_separate_handles_and_released_on_drop() { + let dir = tempfile::tempdir().unwrap(); + let lock = SkillLock::acquire(dir.path()).unwrap(); + assert!(SkillLock::try_acquire(dir.path()).unwrap().is_none()); + drop(lock); + assert!(dir.path().join(".bsk.lock").exists()); + assert!(SkillLock::try_acquire(dir.path()).unwrap().is_some()); + } +} diff --git a/crates/bsk-cli/src/skill_install/sync.rs b/crates/bsk-cli/src/skill_install/sync.rs index 2289532b..38ac6910 100644 --- a/crates/bsk-cli/src/skill_install/sync.rs +++ b/crates/bsk-cli/src/skill_install/sync.rs @@ -3,19 +3,27 @@ use std::path::Path; -use super::{DEFAULT_SKILL_MD, SOURCE_BUNDLED, SOURCE_MARKER_FILE, harness::HarnessId}; +use anyhow::{Context, Result}; + +use super::{ + DEFAULT_SKILL_MD, SOURCE_BUNDLED, SOURCE_MARKER_FILE, + harness::HarnessId, + storage::{PendingWrite, SkillLock}, +}; /// Per-harness outcome of a sync pass. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct SyncReport { /// Harnesses whose on-disk `SKILL.md` differed and was rewritten. pub updated: Vec, - /// Harnesses whose on-disk `SKILL.md` already matched the bundled + /// Managed harnesses whose on-disk `SKILL.md` already matched the bundled /// content; no write happened, mtime preserved. pub up_to_date: Vec, /// Custom or historical untracked installations that must not be /// overwritten by automatic bundled-skill synchronization. pub protected: Vec, + /// Another install/sync holds the lock; retry on a later sync pass. + pub busy: Vec, /// Harnesses that have an installed `SKILL.md` but the sync attempt /// failed with an I/O error. The string is a human-readable detail. pub errors: Vec<(HarnessId, String)>, @@ -33,11 +41,12 @@ pub(crate) fn sync_with_source(home: &Path, source: &str) -> SyncReport { for &harness in HarnessId::ALL { let dest = harness.skill_dest_dir_for_home(home).join("SKILL.md"); match sync_one(&dest, source) { - SyncOne::Missing => continue, - SyncOne::UpToDate => report.up_to_date.push(harness), - SyncOne::Updated => report.updated.push(harness), - SyncOne::Protected => report.protected.push(harness), - SyncOne::Error(msg) => report.errors.push((harness, msg)), + Ok(SyncOne::Missing) => continue, + Ok(SyncOne::UpToDate) => report.up_to_date.push(harness), + Ok(SyncOne::Updated) => report.updated.push(harness), + Ok(SyncOne::Protected) => report.protected.push(harness), + Ok(SyncOne::Busy) => report.busy.push(harness), + Err(err) => report.errors.push((harness, format!("{err:#}"))), } } report @@ -48,48 +57,39 @@ enum SyncOne { UpToDate, Updated, Protected, - Error(String), + Busy, } -fn sync_one(dest: &Path, source: &str) -> SyncOne { +fn sync_one(dest: &Path, source: &str) -> Result { + // Do not create directories or locks for uninstalled harnesses. if !dest.is_file() { - return SyncOne::Missing; + return Ok(SyncOne::Missing); } - let on_disk = match std::fs::read_to_string(dest) { - Ok(s) => s, - Err(err) => return SyncOne::Error(format!("read {}: {err}", dest.display())), + let dir = dest.parent().context("skill destination has no parent")?; + let Some(_lock) = + SkillLock::try_acquire(dir).with_context(|| format!("lock {}", dir.display()))? + else { + return Ok(SyncOne::Busy); }; - if on_disk == source { - return SyncOne::UpToDate; - } - let marker = dest - .parent() - .expect("SKILL.md destination must have a parent") - .join(SOURCE_MARKER_FILE); + + // Check ownership under the lock, even when the content is identical. + let marker = dir.join(SOURCE_MARKER_FILE); match std::fs::read_to_string(&marker) { Ok(value) if value == SOURCE_BUNDLED => {} - Ok(_) => return SyncOne::Protected, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return SyncOne::Protected, - Err(err) => return SyncOne::Error(format!("read {}: {err}", marker.display())), + Ok(_) => return Ok(SyncOne::Protected), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(SyncOne::Protected), + Err(err) => return Err(err).with_context(|| format!("read {}", marker.display())), } - // Atomic replace: write tmp, rename over. Including pid in the - // tmp suffix avoids concurrent processes racing on the same path. - let tmp = dest.with_extension(format!("md.tmp.{}", std::process::id())); - if let Err(err) = std::fs::write(&tmp, source) { - // Best-effort cleanup of any partial tmp left by a half-written attempt. - let _ = std::fs::remove_file(&tmp); - return SyncOne::Error(format!("write {}: {err}", tmp.display())); - } - if let Err(err) = std::fs::rename(&tmp, dest) { - // Best-effort cleanup of the orphan tmp file. - let _ = std::fs::remove_file(&tmp); - return SyncOne::Error(format!( - "rename {} -> {}: {err}", - tmp.display(), - dest.display() - )); + let on_disk = match std::fs::read_to_string(dest) { + Ok(content) => content, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(SyncOne::Missing), + Err(err) => return Err(err).with_context(|| format!("read {}", dest.display())), + }; + if on_disk == source { + return Ok(SyncOne::UpToDate); } - SyncOne::Updated + PendingWrite::prepare(dest, source)?.commit()?; + Ok(SyncOne::Updated) } #[cfg(test)] @@ -118,8 +118,8 @@ mod tests { .skill_dest_dir_for_home(tmp.path()) .join("SKILL.md"); assert!( - !dest.exists(), - "sync should not create files for uninstalled harnesses" + !dest.parent().unwrap().exists(), + "sync should not create directories or locks for uninstalled harnesses" ); } @@ -139,18 +139,7 @@ mod tests { assert!(report.up_to_date.is_empty()); assert!(report.errors.is_empty()); assert_eq!(std::fs::read_to_string(&dest).unwrap(), "fresh content"); - // Atomicity guard: no orphan tmp files matching SKILL.md.tmp.* should - // remain after a successful sync (regardless of pid suffix). - let leftovers: Vec<_> = std::fs::read_dir(&dest_dir) - .unwrap() - .filter_map(|entry| entry.ok()) - .map(|entry| entry.file_name()) - .filter(|name| name.to_string_lossy().starts_with("SKILL.md.tmp")) - .collect(); - assert!( - leftovers.is_empty(), - "no SKILL.md.tmp.* files should remain after sync, found: {leftovers:?}" - ); + super::super::storage::test_support::assert_no_temporary_files(&dest_dir); } #[test] @@ -194,8 +183,8 @@ mod tests { std::fs::write(cursor_dir.join("SKILL.md"), "old").unwrap(); mark_bundled(&cursor_dir.join("SKILL.md")); - // Codex: parent dir set to r-x. Reads still succeed, but creating - // SKILL.md.tmp fails → exercises sync_one's write-tmp error branch. + // Codex: a read-only directory prevents lock creation. The other + // harness must still update, and the failure must not become Busy. let codex_dir = HarnessId::Codex.skill_dest_dir_for_home(home); std::fs::create_dir_all(&codex_dir).unwrap(); std::fs::write(codex_dir.join("SKILL.md"), "old").unwrap(); @@ -242,4 +231,105 @@ mod tests { "historical content" ); } + #[test] + fn unknown_and_missing_markers_protect_even_identical_content() { + for marker in [None, Some(""), Some("unknown\n")] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("SKILL.md"), "same").unwrap(); + if let Some(marker) = marker { + std::fs::write(dir.join(SOURCE_MARKER_FILE), marker).unwrap(); + } + for source in ["same", "new bundled"] { + let report = sync_with_source(home.path(), source); + assert_eq!(report.protected, vec![HarnessId::Cursor]); + assert!(report.up_to_date.is_empty()); + assert!(report.errors.is_empty()); + assert_eq!( + std::fs::read_to_string(dir.join("SKILL.md")).unwrap(), + "same" + ); + } + assert_eq!( + std::fs::read_to_string(dir.join(SOURCE_MARKER_FILE)) + .ok() + .as_deref(), + marker + ); + } + } + + #[test] + fn marker_read_error_is_reported_without_changing_content() { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(dir.join(SOURCE_MARKER_FILE)).unwrap(); + std::fs::write(dir.join("SKILL.md"), "keep").unwrap(); + let report = sync_with_source(home.path(), "new bundled"); + assert_eq!(report.errors.len(), 1); + assert_eq!(report.errors[0].0, HarnessId::Cursor); + assert!(report.errors[0].1.contains(SOURCE_MARKER_FILE)); + assert_eq!( + std::fs::read_to_string(dir.join("SKILL.md")).unwrap(), + "keep" + ); + } + + #[test] + fn failed_sync_replace_preserves_old_content_and_releases_lock() { + use super::super::storage::test_support::{assert_no_temporary_files, with_replace_hook}; + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("SKILL.md"); + std::fs::write(&dest, "old").unwrap(); + mark_bundled(&dest); + let report = with_replace_hook( + |_| Err(std::io::Error::other("injected replacement failure")), + || sync_with_source(home.path(), "new"), + ); + assert_eq!(report.errors.len(), 1); + assert!(report.updated.is_empty()); + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "old"); + assert_no_temporary_files(&dir); + assert_eq!( + sync_with_source(home.path(), "new").updated, + vec![HarnessId::Cursor] + ); + } + + #[test] + fn sync_holds_lock_until_content_replacement_finishes() { + use super::super::storage::test_support::with_replace_hook; + use std::sync::mpsc; + use std::time::Duration; + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("SKILL.md"); + std::fs::write(&dest, "old").unwrap(); + mark_bundled(&dest); + let worker_home = home.path().to_path_buf(); + let (ready_tx, ready_rx) = mpsc::channel(); + let (resume_tx, resume_rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + with_replace_hook( + move |_| { + ready_tx.send(()).unwrap(); + resume_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + Ok(()) + }, + || sync_with_source(&worker_home, "first update"), + ) + }); + ready_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + let during = sync_with_source(home.path(), "second update"); + assert_eq!(during.busy, vec![HarnessId::Cursor]); + assert!(during.errors.is_empty()); + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "old"); + resume_tx.send(()).unwrap(); + assert_eq!(worker.join().unwrap().updated, vec![HarnessId::Cursor]); + assert_eq!(std::fs::read_to_string(&dest).unwrap(), "first update"); + } } From 32b9652dc140eea4469c773e99013e80fec85315 Mon Sep 17 00:00:00 2001 From: drakezhang Date: Thu, 10 Sep 2026 12:34:22 +0800 Subject: [PATCH 6/6] fix(skill): migrate legacy installs and preserve local edits Adopt byte-identical legacy skills without rewriting their content. Record the last managed content digest and pause automatic updates when local edits or untrusted metadata are detected. Report actionable doctor warnings without failing the health check, and surface paused updates during session startup. Cover migration, marker failures, and baseline recovery in regression tests. --- README.md | 27 +- README.zh-CN.md | 21 +- crates/bsk-cli/src/cli/doctor.rs | 131 ++++++-- crates/bsk-cli/src/cli/session.rs | 7 + crates/bsk-cli/src/daemon/start.rs | 4 + crates/bsk-cli/src/skill_install/mod.rs | 41 ++- .../bsk-cli/src/skill_install/provenance.rs | 76 +++++ crates/bsk-cli/src/skill_install/sync.rs | 302 ++++++++++++++++-- 8 files changed, 527 insertions(+), 82 deletions(-) create mode 100644 crates/bsk-cli/src/skill_install/provenance.rs diff --git a/README.md b/README.md index faf86cbd..2704accc 100644 --- a/README.md +++ b/README.md @@ -133,16 +133,23 @@ To install your own instructions, use `bsk install-skill --harness cursor --sour An explicit `--source` stays custom even if its contents match the bundled skill. Existing installations are skipped unless you add `--force`. -Daemon startup, `session start`, and `doctor` automatically update only installations -marked as bundled. Custom installations and older or manual installations without -a `.bsk-source` marker are preserved. `doctor` reports preserved installations and -any sync deferred because another install or sync is running. - -To replace an existing installation with the bundled skill and enable automatic -updates, run `bsk install-skill --harness cursor --force` without `--source`. -This overwrites the existing instructions. To customize a managed installation, -install your edited file with `--source` and `--force`; directly editing a bundled -installation does not change its ownership and it can still be overwritten by sync. +Daemon startup, `session start`, and `doctor` automatically update managed skills +only when their contents still match the last installed version. Local edits are +preserved and automatic updates pause. An older installation without a content +baseline is enrolled automatically only if it exactly matches the current bundled +skill; this writes the source marker without rewriting `SKILL.md`. Explicit custom +installations stay custom even when their contents match. + +For differing historical files, local edits, or an unrecognized source marker, +`doctor` shows `WARN` with the reason and recovery options. These warnings do not +make the health check fail (`--json` reports `status: "warn"` and `ok: true`). +A concurrent install or sync is reported as deferred and retried on a later pass. + +To keep your current instructions as an explicit customization, run +`bsk install-skill --harness cursor --source --force`, replacing +`` with the path to your existing file. To restore the bundled +skill and resume automatic updates, run `bsk install-skill --harness cursor --force` +without `--source`. This second command overwrites the existing instructions. Other shell-capable agent harnesses are supported too. Copy [`skill/SKILL.md`](skill/SKILL.md) into your harness's skills directory as diff --git a/README.zh-CN.md b/README.zh-CN.md index 7d5812e4..fd2664ef 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -117,14 +117,19 @@ bsk install-skill 显式指定 `--source` 的安装始终视为自定义，即使内容与内置 skill 相同。 已有安装默认跳过，添加 `--force` 才会覆盖。 -daemon 启动、`session start` 和 `doctor` 只自动更新标记为内置的安装。 -自定义安装，以及没有 `.bsk-source` 标记的历史或手动安装，都会保留原内容。 -`doctor` 会列出受保护的安装，以及因其他安装或同步正在进行而推迟的同步。 - -如需用内置 skill 替换现有安装并启用自动更新，运行 -`bsk install-skill --harness cursor --force`，不带 `--source`。 -这会覆盖现有指令。修改受管理的安装时，请通过 `--source` 和 `--force` 安装编辑后的文件； -直接修改内置安装的文件不会改变其归属，仍可能被自动同步覆盖。 +daemon 启动、`session start` 和 `doctor` 会检查已安装的 skill：只有文件内容仍与 +上次安装或同步时的内容一致，才继续自动更新。检测到本地编辑时会保留文件并暂停更新。 +没有内容基线的历史安装，只有与当前内置 skill 字节级一致时才自动纳入管理；此时只补齐 +来源标记，不重写 `SKILL.md`。明确的自定义安装即使内容相同，也不会被自动纳入管理。 + +对于内容不同的历史文件、本地编辑或无法识别的来源标记，`doctor` 会显示 `WARN`， +说明暂停原因及恢复方法。这类警告不会让健康检查失败（`--json` 中为 `status: "warn"`、 +`ok: true`）。其他安装或同步正在进行时，本次同步会推迟到后续再试。 + +如需将当前指令保留为明确的自定义安装，运行 +`bsk install-skill --harness cursor --source --force`，将 +`` 替换为现有文件路径。如需恢复内置 skill 并重新启用自动更新，运行 +`bsk install-skill --harness cursor --force`，不带 `--source`。后一条命令会覆盖现有指令。 其他支持 Shell 的 Agent harness 也可使用 BrowserSkill，但需手动将 [`skill/SKILL.md`](skill/SKILL.md) 复制到对应 skills 目录下的 `browser-skill/SKILL.md`。DeepSeek Harness 走独立插件，见 [DeepSeek Harness 插件](#deepseek-harness-插件)。 diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index b4efb842..fcd0b1e6 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -24,17 +24,15 @@ const EXTENSION_STORE_URL_EDGE: &str = "https://microsoftedge.microsoft.com/addo /// Store listings highlighted in repair hints, in the order they appear. const EXTENSION_STORE_URLS: [&str; 2] = [EXTENSION_STORE_URL, EXTENSION_STORE_URL_EDGE]; -/// Status of a single doctor check. `Ok` / `Fail` are the legacy two -/// states; `NotApplicable` (review M2) is reported as "N/A" in human -/// output and as `"status": "na"` in `--json` output, so a check that -/// has nothing to compare against (e.g. browsers protocol-compat with -/// zero connected browsers) does not falsely report green. +/// A warning needs attention but does not fail the overall health check. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize)] #[serde(rename_all = "snake_case")] pub enum CheckStatus { #[default] Ok, Fail, + #[serde(rename = "warn")] + Warning, /// The check could not run because its precondition is absent. /// Treated as informational and never flips an exit code. #[serde(rename = "na")] @@ -44,21 +42,14 @@ pub enum CheckStatus { #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct CheckResult { pub name: String, - /// Pre-M2 boolean status, retained for backwards-compatible JSON - /// consumers. `true` means the check passed or was not applicable - /// (i.e. nothing flips the overall doctor verdict red); `false` - /// means the check actively failed. New consumers should read the - /// `status` field below for the tri-state (`ok` / `fail` / `na`) - /// distinction — the legacy boolean intentionally collapses - /// `ok` and `na` so a doctor run that includes an N/A check does - /// not regress for callers that still consult `ok` only. + /// Backwards-compatible verdict: only a failure is false. Read `status` + /// to distinguish a warning or a check that is not applicable. pub ok: bool, - /// Tri-state status (review M2): `ok` / `fail` / `na`. + /// `ok`, `fail`, `warn`, or `na`. #[serde(default)] pub status: CheckStatus, pub detail: String, - /// User-facing repair hint (only meaningful when `status` is - /// `Fail`). + /// Actionable guidance for a failure or warning. pub hint: Option, } @@ -83,6 +74,16 @@ impl CheckResult { } } + fn warn(name: impl Into, detail: impl Into, hint: impl Into) -> Self { + Self { + name: name.into(), + ok: true, + status: CheckStatus::Warning, + detail: detail.into(), + hint: Some(hint.into()), + } + } + fn na(name: impl Into, detail: impl Into) -> Self { Self { name: name.into(), @@ -111,7 +112,7 @@ pub fn run(output: Output) -> Result> { } /// Whether the rendered doctor report contains an active failure. -/// `NotApplicable` remains informational and must not change the exit code. +/// Warnings and `NotApplicable` remain informational and do not change the exit code. pub fn has_failures(checks: &[CheckResult]) -> bool { checks.iter().any(|check| check.status == CheckStatus::Fail) } @@ -256,7 +257,10 @@ fn skill_check_from_report(report: &crate::skill_install::sync::SyncReport) -> C for (label, harnesses) in [ ("synced", &report.updated), ("up to date", &report.up_to_date), - ("custom or unmanaged skill preserved in", &report.protected), + ( + "custom skill preserved (automatic updates disabled) in", + &report.protected, + ), ("busy, sync deferred in", &report.busy), ] { if !harnesses.is_empty() { @@ -268,17 +272,30 @@ fn skill_check_from_report(report: &crate::skill_install::sync::SyncReport) -> C details.push(format!("{label}: {names}")); } } + let mut hints = Vec::new(); + for (harness, reason) in &report.paused { + let id = harness.cli_name(); + details.push(format!( + "automatic updates paused for {id}: {}; content preserved", + reason.description() + )); + hints.push(format!( + "{id}: keep your instructions with `bsk install-skill --harness {id} --source --force`, or restore the bundled skill with `bsk install-skill --harness {id} --force` (overwrites existing instructions)" + )); + } for (harness, message) in &report.errors { details.push(format!("sync failed for {}: {message}", harness.cli_name())); } let detail = details.join("; "); if !report.errors.is_empty() { - CheckResult::fail( - name, - detail, - "check filesystem access for the failing harness, then re-run `bsk doctor`", - ) + hints.insert( + 0, + "check filesystem access for the failing harness, then re-run `bsk doctor`".into(), + ); + CheckResult::fail(name, detail, hints.join("; ")) + } else if !report.paused.is_empty() { + CheckResult::warn(name, detail, hints.join("; ")) } else if !report.updated.is_empty() || !report.up_to_date.is_empty() { CheckResult::ok(name, detail) } else if !details.is_empty() { @@ -460,10 +477,13 @@ fn render_human(checks: &[CheckResult]) { let mark = match c.status { CheckStatus::Ok => "ok ", CheckStatus::Fail => "FAIL", + CheckStatus::Warning => "WARN", CheckStatus::NotApplicable => "N/A ", }; let detail = match (&c.hint, c.status) { - (Some(h), CheckStatus::Fail) => format!("{} — hint: {}", c.detail, style_hint(h)), + (Some(h), CheckStatus::Fail | CheckStatus::Warning) => { + format!("{} — hint: {}", c.detail, style_hint(h)) + } _ => c.detail.clone(), }; let name = &c.name; @@ -516,14 +536,18 @@ mod m2_tests { let check = skill_check_from_report(&report); assert_eq!(check.status, CheckStatus::Ok); assert!(check.detail.contains("claude-code")); - assert!(check.detail.contains("preserved in: cursor")); + assert!( + check + .detail + .contains("preserved (automatic updates disabled) in: cursor") + ); let json = serde_json::to_value(&check).unwrap(); assert_eq!(json["status"], "ok"); assert!( json["detail"] .as_str() .unwrap() - .contains("preserved in: cursor") + .contains("preserved (automatic updates disabled) in: cursor") ); } } @@ -537,12 +561,13 @@ mod m2_tests { protected: vec![HarnessId::Cursor], busy: vec![HarnessId::Hermes], errors: vec![(HarnessId::Workbuddy, "permission denied".into())], + paused: Vec::new(), }); assert_eq!(check.status, CheckStatus::Fail); for text in [ "synced: claude-code", "up to date: pi", - "preserved in: cursor", + "preserved (automatic updates disabled) in: cursor", "sync deferred in: hermes", "workbuddy: permission denied", ] { @@ -551,6 +576,58 @@ mod m2_tests { assert!(!check.hint.unwrap().contains("--force")); } + #[test] + fn paused_skills_warn_with_actions_even_alongside_successful_updates() { + use crate::skill_install::{ + HarnessId, + sync::{PauseReason, SyncReport}, + }; + for reason in [ + PauseReason::Untracked, + PauseReason::MissingBaseline, + PauseReason::LocalChanges, + PauseReason::InvalidMarker, + ] { + for updated in [false, true] { + let mut report = SyncReport { + paused: vec![(HarnessId::Cursor, reason)], + ..Default::default() + }; + if updated { + report.updated.push(HarnessId::ClaudeCode); + } + let check = skill_check_from_report(&report); + assert_eq!(check.status, CheckStatus::Warning); + assert!(check.detail.contains("automatic updates paused for cursor")); + assert!(check.detail.contains(reason.description())); + if updated { + assert!(check.detail.contains("synced: claude-code")); + } + assert!(!has_failures(std::slice::from_ref(&check))); + let json = serde_json::to_value(&check).unwrap(); + assert_eq!(json["status"], "warn"); + assert_eq!(json["ok"], true); + let hint = json["hint"].as_str().unwrap(); + assert!(hint.contains("--harness cursor --source --force")); + assert!(hint.contains("--harness cursor --force")); + assert!(hint.contains("overwrites existing instructions")); + // An I/O failure takes precedence without hiding paused installations. + report + .errors + .push((HarnessId::PiAgent, "permission denied".into())); + let failed = skill_check_from_report(&report); + assert_eq!(failed.status, CheckStatus::Fail); + assert!( + failed + .detail + .contains("automatic updates paused for cursor") + ); + assert!(failed.hint.as_ref().unwrap().contains("--harness cursor")); + assert!(has_failures(&[failed])); + } + } + } + #[test] fn protected_or_busy_skills_are_informational() { use crate::skill_install::{HarnessId, sync::SyncReport}; diff --git a/crates/bsk-cli/src/cli/session.rs b/crates/bsk-cli/src/cli/session.rs index 5365282c..6e552633 100644 --- a/crates/bsk-cli/src/cli/session.rs +++ b/crates/bsk-cli/src/cli/session.rs @@ -542,6 +542,13 @@ fn run_skill_sync_for_session_start(format: Format) { for harness in &report.updated { eprintln!("≈ skill updated for {}", harness.cli_name()); } + for (harness, reason) in &report.paused { + eprintln!( + "! skill auto-update paused for {}: {}; run `bsk doctor` for options", + harness.cli_name(), + reason.description() + ); + } } for (harness, msg) in &report.errors { tracing::warn!(harness = harness.cli_name(), error = %msg, "skill sync failed"); diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index 017c55d0..4d584575 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -279,6 +279,10 @@ pub fn run_foreground(cfg: DaemonConfig) -> Result<()> { for harness in &report.updated { info!(harness = harness.cli_name(), "skill synced"); } + for (harness, reason) in &report.paused { + warn!(harness = harness.cli_name(), reason = reason.description(), + "skill auto-update paused; content preserved; run `bsk doctor` for options"); + } for (harness, msg) in &report.errors { warn!(harness = harness.cli_name(), error = %msg, "skill sync failed"); } diff --git a/crates/bsk-cli/src/skill_install/mod.rs b/crates/bsk-cli/src/skill_install/mod.rs index 4e1ab11f..456fdc9f 100644 --- a/crates/bsk-cli/src/skill_install/mod.rs +++ b/crates/bsk-cli/src/skill_install/mod.rs @@ -1,6 +1,7 @@ //! Install bundled or custom browser-skill instructions into agent skill directories. pub mod harness; +mod provenance; mod storage; pub mod sync; @@ -10,7 +11,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; use console::{Style, style}; use dialoguer::{MultiSelect, theme::ColorfulTheme}; -use serde::Serialize; +use serde::{Deserialize, Serialize}; pub use harness::{HarnessId, HarnessReport, all_harness_reports, parse_harness_id}; @@ -21,7 +22,8 @@ pub const SOURCE_BUNDLED: &str = "bundled\n"; pub const SOURCE_CUSTOM: &str = "custom\n"; /// Installation provenance is explicit: even an identical `--source` is custom. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] pub enum SkillSource { Bundled, Custom, @@ -155,10 +157,10 @@ fn install_one_at_home( } let existed = dest_file.exists(); - let content = storage::PendingWrite::prepare(&dest_file, source)?; let marker = dest_dir.join(SOURCE_MARKER_FILE); match source_kind { SkillSource::Custom => { + let content = storage::PendingWrite::prepare(&dest_file, source)?; // Protection must be established before any custom content appears. storage::PendingWrite::prepare(&marker, SOURCE_CUSTOM)?.commit()?; content @@ -166,12 +168,7 @@ fn install_one_at_home( .context("custom protection recorded, but skill content was not replaced")?; } SkillSource::Bundled => { - let marker = storage::PendingWrite::prepare(&marker, SOURCE_BUNDLED)?; - // Do not authorize sync until the old custom content is replaced. - content.commit()?; - marker.commit().context( - "bundled skill content installed, but its source marker was not updated", - )?; + write_bundled_skill(&dest_file, source)?; } } @@ -183,6 +180,22 @@ fn install_one_at_home( Ok((dest_file, status)) } +/// Callers hold the skill lock. Publish the baseline only after its content; +/// a failed marker replacement leaves a conservative, detectable mismatch. +fn write_bundled_skill(dest: &Path, source: &str) -> Result<()> { + let content = storage::PendingWrite::prepare(dest, source)?; + let marker = dest + .parent() + .context("skill destination has no parent")? + .join(SOURCE_MARKER_FILE); + let metadata = provenance::bundled_marker(source.as_bytes())?; + let marker = storage::PendingWrite::prepare(&marker, &metadata)?; + content.commit()?; + marker + .commit() + .context("bundled skill content installed, but its source marker was not updated") +} + /// Harnesses visible in the interactive installer (detected on this machine only). pub fn interactive_candidates(reports: &[HarnessReport]) -> Vec<&HarnessReport> { reports.iter().filter(|report| report.detected).collect() @@ -440,7 +453,12 @@ mod tests { let marker = harness .skill_dest_dir_for_home(&home) .join(SOURCE_MARKER_FILE); - assert_eq!(fs::read_to_string(marker).unwrap(), SOURCE_BUNDLED); + assert_eq!( + provenance::read(&marker).unwrap(), + provenance::Provenance::Bundled { + sha256: provenance::digest(DEFAULT_SKILL_MD.as_bytes()), + } + ); } #[test] @@ -519,6 +537,7 @@ mod tests { false, ) .unwrap(); + let original_marker = fs::read_to_string(dir.join(SOURCE_MARKER_FILE)).unwrap(); let error = with_replace_hook( move |dest| { if dest.file_name().unwrap() == failed_file { @@ -560,7 +579,7 @@ mod tests { let still_bundled = kind == SkillSource::Custom && failed_file == SOURCE_MARKER_FILE; let expected_marker = if still_bundled { - SOURCE_BUNDLED + original_marker.as_str() } else { SOURCE_CUSTOM }; diff --git a/crates/bsk-cli/src/skill_install/provenance.rs b/crates/bsk-cli/src/skill_install/provenance.rs new file mode 100644 index 00000000..991e8d5c --- /dev/null +++ b/crates/bsk-cli/src/skill_install/provenance.rs @@ -0,0 +1,76 @@ +//! Read legacy source markers and record the last managed content in the same +//! atomically replaced marker. A baseline describes what we wrote, not what the +//! current binary happens to bundle. + +use std::path::Path; + +use anyhow::{Context, Result}; +use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; + +use super::{SOURCE_BUNDLED, SOURCE_CUSTOM, SkillSource}; + +#[derive(Debug, PartialEq, Eq)] +pub(super) enum Provenance { + Missing, + Custom, + LegacyBundled, + Bundled { sha256: String }, + Invalid, +} + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct Marker { + version: u8, + source: SkillSource, + #[serde(default, skip_serializing_if = "Option::is_none")] + sha256: Option, +} + +pub(super) fn digest(content: &[u8]) -> String { + Sha256::digest(content) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + +pub(super) fn bundled_marker(content: &[u8]) -> Result { + let marker = Marker { + version: 1, + source: SkillSource::Bundled, + sha256: Some(digest(content)), + }; + Ok(format!("{}\n", serde_json::to_string(&marker)?)) +} + +pub(super) fn read(marker: &Path) -> Result { + let bytes = match std::fs::read(marker) { + Ok(bytes) => bytes, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(Provenance::Missing), + Err(err) => return Err(err).with_context(|| format!("read {}", marker.display())), + }; + if bytes == SOURCE_CUSTOM.as_bytes() { + return Ok(Provenance::Custom); + } + if bytes == SOURCE_BUNDLED.as_bytes() { + return Ok(Provenance::LegacyBundled); + } + match serde_json::from_slice::(&bytes) { + Ok(Marker { + version: 1, + source: SkillSource::Custom, + .. + }) => Ok(Provenance::Custom), + Ok(Marker { + version: 1, + source: SkillSource::Bundled, + sha256: Some(hash), + }) if hash.len() == 64 && hash.bytes().all(|byte| byte.is_ascii_hexdigit()) => { + Ok(Provenance::Bundled { + sha256: hash.to_ascii_lowercase(), + }) + } + _ => Ok(Provenance::Invalid), + } +} diff --git a/crates/bsk-cli/src/skill_install/sync.rs b/crates/bsk-cli/src/skill_install/sync.rs index 38ac6910..b98140ce 100644 --- a/crates/bsk-cli/src/skill_install/sync.rs +++ b/crates/bsk-cli/src/skill_install/sync.rs @@ -6,8 +6,9 @@ use std::path::Path; use anyhow::{Context, Result}; use super::{ - DEFAULT_SKILL_MD, SOURCE_BUNDLED, SOURCE_MARKER_FILE, + DEFAULT_SKILL_MD, SOURCE_MARKER_FILE, harness::HarnessId, + provenance::{self, Provenance}, storage::{PendingWrite, SkillLock}, }; @@ -19,9 +20,10 @@ pub struct SyncReport { /// Managed harnesses whose on-disk `SKILL.md` already matched the bundled /// content; no write happened, mtime preserved. pub up_to_date: Vec, - /// Custom or historical untracked installations that must not be - /// overwritten by automatic bundled-skill synchronization. + /// Explicit custom installations that intentionally opt out of updates. pub protected: Vec, + /// Content preserved because safe automatic updates need user attention. + pub paused: Vec<(HarnessId, PauseReason)>, /// Another install/sync holds the lock; retry on a later sync pass. pub busy: Vec, /// Harnesses that have an installed `SKILL.md` but the sync attempt @@ -29,6 +31,25 @@ pub struct SyncReport { pub errors: Vec<(HarnessId, String)>, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PauseReason { + Untracked, + MissingBaseline, + LocalChanges, + InvalidMarker, +} + +impl PauseReason { + pub fn description(self) -> &'static str { + match self { + Self::Untracked => "untracked installation differs from the bundled skill", + Self::MissingBaseline => "older bundled installation has no content baseline", + Self::LocalChanges => "local changes detected", + Self::InvalidMarker => "unrecognized or damaged source marker", + } + } +} + /// Iterates `HarnessId::ALL`, syncing harnesses with an existing /// `SKILL.md` and leaving the rest untouched. pub fn sync_installed_skills(home: &Path) -> SyncReport { @@ -45,6 +66,7 @@ pub(crate) fn sync_with_source(home: &Path, source: &str) -> SyncReport { Ok(SyncOne::UpToDate) => report.up_to_date.push(harness), Ok(SyncOne::Updated) => report.updated.push(harness), Ok(SyncOne::Protected) => report.protected.push(harness), + Ok(SyncOne::Paused(reason)) => report.paused.push((harness, reason)), Ok(SyncOne::Busy) => report.busy.push(harness), Err(err) => report.errors.push((harness, format!("{err:#}"))), } @@ -57,6 +79,7 @@ enum SyncOne { UpToDate, Updated, Protected, + Paused(PauseReason), Busy, } @@ -72,23 +95,38 @@ fn sync_one(dest: &Path, source: &str) -> Result { return Ok(SyncOne::Busy); }; - // Check ownership under the lock, even when the content is identical. + // Explicit custom intent wins over byte equality; unknown metadata is never + // silently claimed. Only missing or recognized legacy markers can migrate. let marker = dir.join(SOURCE_MARKER_FILE); - match std::fs::read_to_string(&marker) { - Ok(value) if value == SOURCE_BUNDLED => {} - Ok(_) => return Ok(SyncOne::Protected), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(SyncOne::Protected), - Err(err) => return Err(err).with_context(|| format!("read {}", marker.display())), + let ownership = provenance::read(&marker)?; + match ownership { + Provenance::Custom => return Ok(SyncOne::Protected), + Provenance::Invalid => return Ok(SyncOne::Paused(PauseReason::InvalidMarker)), + _ => {} } - let on_disk = match std::fs::read_to_string(dest) { + let on_disk = match std::fs::read(dest) { Ok(content) => content, Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(SyncOne::Missing), Err(err) => return Err(err).with_context(|| format!("read {}", dest.display())), }; - if on_disk == source { + let matches_baseline = matches!(&ownership, Provenance::Bundled { sha256 } + if *sha256 == provenance::digest(&on_disk)); + if on_disk == source.as_bytes() { + if !matches_baseline { + // Also recovers a content update whose final marker write failed. + // Neither adoption nor recovery rewrites SKILL.md or its mtime. + let metadata = provenance::bundled_marker(&on_disk)?; + PendingWrite::prepare(&marker, &metadata)?.commit()?; + } return Ok(SyncOne::UpToDate); } - PendingWrite::prepare(dest, source)?.commit()?; + match ownership { + Provenance::Missing => return Ok(SyncOne::Paused(PauseReason::Untracked)), + Provenance::LegacyBundled => return Ok(SyncOne::Paused(PauseReason::MissingBaseline)), + Provenance::Bundled { .. } if matches_baseline => {} + _ => return Ok(SyncOne::Paused(PauseReason::LocalChanges)), + } + super::write_bundled_skill(dest, source)?; Ok(SyncOne::Updated) } @@ -100,7 +138,7 @@ mod tests { fn mark_bundled(dest: &Path) { std::fs::write( dest.parent().unwrap().join(SOURCE_MARKER_FILE), - SOURCE_BUNDLED, + provenance::bundled_marker(&std::fs::read(dest).unwrap()).unwrap(), ) .unwrap(); } @@ -221,7 +259,11 @@ mod tests { let report = sync_with_source(home, "new bundled content"); - assert_eq!(report.protected, vec![HarnessId::Codex, HarnessId::Cursor]); + assert_eq!(report.protected, vec![HarnessId::Cursor]); + assert_eq!( + report.paused, + vec![(HarnessId::Codex, PauseReason::Untracked)] + ); assert_eq!( std::fs::read_to_string(custom_dir.join("SKILL.md")).unwrap(), "custom content" @@ -232,18 +274,26 @@ mod tests { ); } #[test] - fn unknown_and_missing_markers_protect_even_identical_content() { - for marker in [None, Some(""), Some("unknown\n")] { + fn unknown_markers_are_not_claimed_even_when_content_matches() { + for marker in [ + "", + "unknown\n", + r#"{"version":2,"source":"bundled","sha256":"abc"}"#, + r#"{"version":1,"source":"bundled","sha256":"invalid"}"#, + r#"{"version":1,"source":"bundled"}"#, + r#"{"version":1,"source":"custom","unexpected":true}"#, + ] { let home = TempDir::new().unwrap(); let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); std::fs::create_dir_all(&dir).unwrap(); std::fs::write(dir.join("SKILL.md"), "same").unwrap(); - if let Some(marker) = marker { - std::fs::write(dir.join(SOURCE_MARKER_FILE), marker).unwrap(); - } + std::fs::write(dir.join(SOURCE_MARKER_FILE), marker).unwrap(); for source in ["same", "new bundled"] { let report = sync_with_source(home.path(), source); - assert_eq!(report.protected, vec![HarnessId::Cursor]); + assert_eq!( + report.paused, + vec![(HarnessId::Cursor, PauseReason::InvalidMarker)] + ); assert!(report.up_to_date.is_empty()); assert!(report.errors.is_empty()); assert_eq!( @@ -252,9 +302,7 @@ mod tests { ); } assert_eq!( - std::fs::read_to_string(dir.join(SOURCE_MARKER_FILE)) - .ok() - .as_deref(), + std::fs::read_to_string(dir.join(SOURCE_MARKER_FILE)).unwrap(), marker ); } @@ -315,9 +363,11 @@ mod tests { let (resume_tx, resume_rx) = mpsc::channel(); let worker = std::thread::spawn(move || { with_replace_hook( - move |_| { - ready_tx.send(()).unwrap(); - resume_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + move |dest| { + if dest.file_name().unwrap() == "SKILL.md" { + ready_tx.send(()).unwrap(); + resume_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + } Ok(()) }, || sync_with_source(&worker_home, "first update"), @@ -332,4 +382,204 @@ mod tests { assert_eq!(worker.join().unwrap().updated, vec![HarnessId::Cursor]); assert_eq!(std::fs::read_to_string(&dest).unwrap(), "first update"); } + #[test] + fn matching_legacy_installations_are_adopted_without_rewriting_content() { + use std::time::{Duration, SystemTime}; + for legacy in [None, Some(super::super::SOURCE_BUNDLED)] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("SKILL.md"); + let marker = dir.join(SOURCE_MARKER_FILE); + std::fs::write(&dest, "current bundle").unwrap(); + std::fs::OpenOptions::new() + .write(true) + .open(&dest) + .unwrap() + .set_modified(SystemTime::UNIX_EPOCH + Duration::from_secs(1_600_000_000)) + .unwrap(); + let mtime = std::fs::metadata(&dest).unwrap().modified().unwrap(); + if let Some(legacy) = legacy { + std::fs::write(&marker, legacy).unwrap(); + } + let report = sync_with_source(home.path(), "current bundle"); + assert_eq!(report.up_to_date, vec![HarnessId::Cursor]); + assert!(report.paused.is_empty()); + assert_eq!(std::fs::metadata(&dest).unwrap().modified().unwrap(), mtime); + assert_eq!( + provenance::read(&marker).unwrap(), + Provenance::Bundled { + sha256: provenance::digest(b"current bundle") + } + ); + let marker_mtime = std::fs::metadata(&marker).unwrap().modified().unwrap(); + assert_eq!( + sync_with_source(home.path(), "current bundle").up_to_date, + vec![HarnessId::Cursor] + ); + assert_eq!( + std::fs::metadata(&marker).unwrap().modified().unwrap(), + marker_mtime + ); + assert_eq!( + sync_with_source(home.path(), "next bundle").updated, + vec![HarnessId::Cursor] + ); + assert_eq!(std::fs::read(&dest).unwrap(), b"next bundle"); + } + } + + #[test] + fn differing_legacy_content_is_preserved_with_a_specific_pause_reason() { + for (legacy, reason) in [ + (None, PauseReason::Untracked), + ( + Some(super::super::SOURCE_BUNDLED), + PauseReason::MissingBaseline, + ), + ] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(SOURCE_MARKER_FILE); + // Even a trailing newline is a byte difference, not an adoption match. + std::fs::write(dir.join("SKILL.md"), "bundle\n").unwrap(); + if let Some(legacy) = legacy { + std::fs::write(&marker, legacy).unwrap(); + } + let report = sync_with_source(home.path(), "bundle"); + assert_eq!(report.paused, vec![(HarnessId::Cursor, reason)]); + assert!(report.updated.is_empty()); + assert!(report.errors.is_empty()); + assert_eq!( + std::fs::read_to_string(dir.join("SKILL.md")).unwrap(), + "bundle\n" + ); + assert_eq!(std::fs::read_to_string(&marker).ok().as_deref(), legacy); + } + } + + #[test] + fn failed_adoption_keeps_legacy_content_and_marker_intact() { + use super::super::storage::test_support::{assert_no_temporary_files, with_replace_hook}; + for legacy in [None, Some(super::super::SOURCE_BUNDLED)] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("SKILL.md"), "bundle").unwrap(); + let marker = dir.join(SOURCE_MARKER_FILE); + if let Some(legacy) = legacy { + std::fs::write(&marker, legacy).unwrap(); + } + let report = with_replace_hook( + |_| Err(std::io::Error::other("injected marker failure")), + || sync_with_source(home.path(), "bundle"), + ); + assert_eq!(report.errors.len(), 1); + assert!(report.up_to_date.is_empty()); + assert_eq!(std::fs::read_to_string(&marker).ok().as_deref(), legacy); + assert_eq!( + std::fs::read_to_string(dir.join("SKILL.md")).unwrap(), + "bundle" + ); + assert_no_temporary_files(&dir); + assert_eq!( + sync_with_source(home.path(), "bundle").up_to_date, + vec![HarnessId::Cursor] + ); + } + } + + #[test] + fn local_edits_are_preserved_until_the_recorded_content_is_restored() { + for edited in [b"my instructions".as_slice(), b"bundle v1\n", b"\xff"] { + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("SKILL.md"); + std::fs::write(&dest, "bundle v1").unwrap(); + mark_bundled(&dest); + let original_marker = std::fs::read(dir.join(SOURCE_MARKER_FILE)).unwrap(); + std::fs::write(&dest, edited).unwrap(); + for target in ["bundle v1", "bundle v2"] { + let report = sync_with_source(home.path(), target); + assert_eq!( + report.paused, + vec![(HarnessId::Cursor, PauseReason::LocalChanges)] + ); + assert!(report.errors.is_empty()); + assert_eq!(std::fs::read(&dest).unwrap(), edited); + assert_eq!( + std::fs::read(dir.join(SOURCE_MARKER_FILE)).unwrap(), + original_marker + ); + } + std::fs::write(&dest, "bundle v1").unwrap(); + assert_eq!( + sync_with_source(home.path(), "bundle v2").updated, + vec![HarnessId::Cursor] + ); + assert_eq!( + provenance::read(&dir.join(SOURCE_MARKER_FILE)).unwrap(), + Provenance::Bundled { + sha256: provenance::digest(b"bundle v2") + } + ); + // The updated baseline must protect edits made after an upgrade, too. + std::fs::write(&dest, "v2 with local edits").unwrap(); + assert_eq!( + sync_with_source(home.path(), "bundle v3").paused, + vec![(HarnessId::Cursor, PauseReason::LocalChanges)] + ); + } + } + + #[test] + fn interrupted_sync_preserves_content_and_repairs_only_a_matching_bundle() { + use super::super::storage::test_support::{assert_no_temporary_files, with_replace_hook}; + let home = TempDir::new().unwrap(); + let dir = HarnessId::Cursor.skill_dest_dir_for_home(home.path()); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("SKILL.md"); + std::fs::write(&dest, "old bundle").unwrap(); + mark_bundled(&dest); + let old_marker = std::fs::read(dir.join(SOURCE_MARKER_FILE)).unwrap(); + let report = with_replace_hook( + |dest| { + if dest.file_name().unwrap() == SOURCE_MARKER_FILE { + Err(std::io::Error::other("injected marker failure")) + } else { + Ok(()) + } + }, + || sync_with_source(home.path(), "new bundle"), + ); + assert_eq!(report.errors.len(), 1); + assert!( + report.errors[0] + .1 + .contains("bundled skill content installed") + ); + assert_eq!(std::fs::read(&dest).unwrap(), b"new bundle"); + assert_eq!( + std::fs::read(dir.join(SOURCE_MARKER_FILE)).unwrap(), + old_marker + ); + assert_no_temporary_files(&dir); + // A different binary cannot guess the origin of this mismatch. + assert_eq!( + sync_with_source(home.path(), "another bundle").paused, + vec![(HarnessId::Cursor, PauseReason::LocalChanges)] + ); + let mtime = std::fs::metadata(&dest).unwrap().modified().unwrap(); + assert_eq!( + sync_with_source(home.path(), "new bundle").up_to_date, + vec![HarnessId::Cursor] + ); + assert_eq!(std::fs::metadata(&dest).unwrap().modified().unwrap(), mtime); + assert_eq!( + sync_with_source(home.path(), "another bundle").updated, + vec![HarnessId::Cursor] + ); + } }