diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index f15a7711..fb11f9f2 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -230,11 +230,17 @@ fn check_skill_up_to_date() -> CheckResult { } }; let report = crate::skill_install::sync::sync_installed_skills(&home); + let newer_bsk = crate::cli::update::cached_newer_version(env!("CARGO_PKG_VERSION")); - skill_check_from_report(&report) + skill_check_from_report(&report, newer_bsk.as_deref()) } -fn skill_check_from_report(report: &crate::skill_install::sync::SyncReport) -> CheckResult { +/// `newer_bsk` is the version the update cache names, if it names a newer one. +/// Passed in rather than read here so the tests stay off the filesystem. +fn skill_check_from_report( + report: &crate::skill_install::sync::SyncReport, + newer_bsk: Option<&str>, +) -> CheckResult { let name = "agent skill up to date"; let mut details = Vec::new(); for (label, harnesses) in [ @@ -280,7 +286,25 @@ fn skill_check_from_report(report: &crate::skill_install::sync::SyncReport) -> C } 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) + // "up to date" is measured against the skill bundled in *this* build, + // not against upstream. An old bsk therefore reports a stale skill as + // current, and agents act on that: a 0.2.1 install kept a SKILL.md + // documenting a --full-page flag its own CLI did not have. + // + // A warning, not a failure: the skill really is synced for the build + // that is installed, and the cache is only consulted when it is fresh, + // so an unknown state stays quiet rather than crying stale. + match newer_bsk { + Some(latest) => CheckResult::warn( + name, + format!( + "{detail}; matches the skill bundled with bsk {current}, but bsk {latest} is available", + current = env!("CARGO_PKG_VERSION"), + ), + format!("run `bsk update` to install the {latest} skill, then re-run `bsk doctor`"), + ), + None => CheckResult::ok(name, detail), + } } else if !details.is_empty() { CheckResult::na(name, detail) } else { @@ -547,7 +571,7 @@ mod m2_tests { } else { report.up_to_date.push(HarnessId::ClaudeCode); } - let check = skill_check_from_report(&report); + let check = skill_check_from_report(&report, None); assert_eq!(check.status, CheckStatus::Ok); assert!(check.detail.contains("claude-code")); assert!( @@ -566,17 +590,80 @@ mod m2_tests { } } + #[test] + fn skill_check_warns_when_the_bundled_skill_comes_from_an_older_bsk() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + // A synced skill is only as current as the build that bundled it. + // Both outcomes that previously reported plain "ok" must warn here. + for report in [ + SyncReport { + up_to_date: vec![HarnessId::ClaudeCode], + ..Default::default() + }, + SyncReport { + updated: vec![HarnessId::ClaudeCode], + ..Default::default() + }, + ] { + let check = skill_check_from_report(&report, Some("9.9.9")); + assert_eq!(check.status, CheckStatus::Warning, "{}", check.detail); + // still not a failure: the skill is correct for what is installed + assert!(check.ok); + assert!(check.detail.contains("9.9.9"), "{}", check.detail); + assert!( + check.detail.contains(env!("CARGO_PKG_VERSION")), + "{}", + check.detail + ); + assert!(check.hint.unwrap().contains("bsk update")); + } + } + + #[test] + fn skill_check_stays_ok_when_no_newer_bsk_is_known() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + // A missing or stale update cache reads as None. Staying quiet is the + // point: doctor must not cry stale just because it cannot tell. + let check = skill_check_from_report( + &SyncReport { + up_to_date: vec![HarnessId::ClaudeCode], + ..Default::default() + }, + None, + ); + assert_eq!(check.status, CheckStatus::Ok); + assert!(check.hint.is_none()); + } + + #[test] + fn a_newer_bsk_does_not_downgrade_a_sync_failure() { + use crate::skill_install::{HarnessId, sync::SyncReport}; + let check = skill_check_from_report( + &SyncReport { + up_to_date: vec![HarnessId::ClaudeCode], + errors: vec![(HarnessId::Workbuddy, "permission denied".into())], + ..Default::default() + }, + Some("9.9.9"), + ); + assert_eq!(check.status, CheckStatus::Fail); + assert!(!check.ok); + } + #[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())], - paused: Vec::new(), - }); + 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())], + paused: Vec::new(), + }, + None, + ); assert_eq!(check.status, CheckStatus::Fail); for text in [ "synced: claude-code", @@ -610,7 +697,7 @@ mod m2_tests { if updated { report.updated.push(HarnessId::ClaudeCode); } - let check = skill_check_from_report(&report); + let check = skill_check_from_report(&report, None); assert_eq!(check.status, CheckStatus::Warning); assert!(check.detail.contains("automatic updates paused for cursor")); assert!(check.detail.contains(reason.description())); @@ -629,7 +716,7 @@ mod m2_tests { report .errors .push((HarnessId::PiAgent, "permission denied".into())); - let failed = skill_check_from_report(&report); + let failed = skill_check_from_report(&report, None); assert_eq!(failed.status, CheckStatus::Fail); assert!( failed @@ -655,13 +742,13 @@ mod m2_tests { ..Default::default() }, ] { - let check = skill_check_from_report(&report); + let check = skill_check_from_report(&report, None); 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()); + let empty = skill_check_from_report(&SyncReport::default(), None); assert_eq!(empty.status, CheckStatus::NotApplicable); assert_eq!(empty.detail, "no agent skill installed"); } diff --git a/crates/bsk-cli/src/cli/update.rs b/crates/bsk-cli/src/cli/update.rs index f219aa75..36d0281f 100644 --- a/crates/bsk-cli/src/cli/update.rs +++ b/crates/bsk-cli/src/cli/update.rs @@ -646,6 +646,25 @@ fn cached_update_hint( Ok(update_hint_for_cache(&cache, current_version, auto_update)) } +/// The newer version the cache names, if any. Same read-only, offline +/// contract as `print_update_hint_from_cache`: the daemon owns refreshing +/// `~/.bsk/update-check.json`, and a missing or stale cache means "say +/// nothing" rather than "up to date". +/// +/// `doctor` needs the version itself rather than the CLI hint wording, to +/// explain that the bundled agent skill is only as current as the build it +/// shipped with. +pub(crate) fn cached_newer_version(current_version: &str) -> Option { + let cache_path = crate::daemon::paths::update_check_path().ok()?; + let cache = read_update_cache(&cache_path).ok()??; + if !cache.is_fresh(now_epoch_secs(), UPDATE_CHECK_INTERVAL) { + return None; + } + let current = Version::parse(current_version.trim_start_matches('v')).ok()?; + let latest = Version::parse(cache.latest_version.trim_start_matches('v')).ok()?; + (latest > current).then(|| latest.to_string()) +} + fn confirm_update(candidate: &UpdateCandidate) -> Result { dialoguer::Confirm::new() .with_prompt(format!(