diff --git a/crates/runtime/src/skill_state.rs b/crates/runtime/src/skill_state.rs index 09cd65643c..f418d57e09 100644 --- a/crates/runtime/src/skill_state.rs +++ b/crates/runtime/src/skill_state.rs @@ -12,6 +12,14 @@ //! disabled = ["skill-name-1", "skill-name-2"] //! ``` //! +//! Reserved `!codewhale-skill-state:1:*` entries retain legacy vetoes and +//! exact enables in that same set. The leading `!` cannot be a discovered +//! skill name. v0.10.0 writers preserve these strings, unlike unknown TOML +//! fields. This preserves new-reader policy through their serialization; +//! it does not make old readers understand distinct Unicode identities or +//! old lossy/no-op toggles express the new per-skill choices. Upgrade all +//! runtimes sharing this directory for consistent activation controls. +//! //! Default state when the file does not exist: empty list (everything enabled). //! A present but unreadable or malformed file is an error. Callers may keep //! native Skills available for recovery, but reviewed plugin Skills must stay @@ -25,6 +33,9 @@ use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; const STATE_FILE_NAME: &str = "skills_state.toml"; +const MARKER_PREFIX: &str = "!codewhale-skill-state:"; +const ENABLED_PREFIX: &str = "!codewhale-skill-state:1:enabled:"; +const HISTORY_PREFIX: &str = "!codewhale-skill-state:1:history:"; #[derive(Debug, Clone)] pub struct SkillStateStore { @@ -50,7 +61,28 @@ impl SkillStateStore { } pub fn is_enabled(&self, skill_name: &str) -> bool { - !self.disabled.contains(skill_name) + self.is_enabled_with_legacy(skill_name, None) + } + + /// Raw exact denies win even over a retained enable (including a later + /// v0.10.0 toggle). Enables override only inherited legacy vetoes. + pub fn is_enabled_with_legacy(&self, skill_name: &str, legacy_name: Option<&str>) -> bool { + if self.disabled.contains(skill_name) { + return false; + } + if self + .disabled + .contains(&format!("{ENABLED_PREFIX}{skill_name}")) + { + return true; + } + !self + .disabled + .contains(&format!("{HISTORY_PREFIX}{skill_name}")) + && !legacy_name.is_some_and(|legacy| { + self.disabled.contains(legacy) + || self.disabled.contains(&format!("{HISTORY_PREFIX}{legacy}")) + }) } pub fn set_enabled(&mut self, skill_name: &str, enabled: bool) -> Result<()> { @@ -67,8 +99,14 @@ impl SkillStateStore { Ok(()) } + /// Raw exact denies only; inherited vetoes need discovered identity + /// metadata and cannot be enumerated as a list of effective skill names. pub fn disabled(&self) -> Vec { - self.disabled.iter().cloned().collect() + self.disabled + .iter() + .filter(|name| !name.starts_with(MARKER_PREFIX)) + .cloned() + .collect() } fn set_enabled_with_persist( @@ -77,6 +115,10 @@ impl SkillStateStore { enabled: bool, persist: impl FnOnce(&Path, &BTreeSet) -> Result<()>, ) -> Result<()> { + anyhow::ensure!( + !skill_name.is_empty() && !skill_name.starts_with(MARKER_PREFIX), + "invalid skill activation identity" + ); if let Some(parent) = self .path .parent() @@ -96,13 +138,26 @@ impl SkillStateStore { // requested exact-name change to this latest snapshot merges updates // from other Runtime API/TUI processes instead of replacing them with // the caller's possibly stale in-memory view. - let mut next = load_disabled_unlocked(&self.path)?; - let changed = if enabled { - next.remove(skill_name) + let previous = load_disabled_unlocked(&self.path)?; + let mut next = previous.clone(); + // A raw name may also govern an undiscovered legacy collision cohort. + // Preserve every veto before removing any, without discovery writes. + next.extend( + previous + .iter() + .filter(|name| !name.is_empty() && !name.starts_with(MARKER_PREFIX)) + .map(|name| format!("{HISTORY_PREFIX}{name}")), + ); + let exact_enable = format!("{ENABLED_PREFIX}{skill_name}"); + if enabled { + next.remove(skill_name); + next.insert(exact_enable); } else { - next.insert(skill_name.to_string()) - }; - if changed { + next.insert(skill_name.to_string()); + next.insert(format!("{HISTORY_PREFIX}{skill_name}")); + next.remove(&exact_enable); + } + if next != previous { // Disk is authoritative. Publish to memory only after the atomic // write succeeds so a failed persistence attempt cannot make this // process report a toggle that no other process can observe. @@ -146,6 +201,18 @@ fn load_disabled_unlocked(path: &Path) -> Result> { }; let parsed: OnDiskState = toml::from_str(&raw).with_context(|| format!("parse skill state at {}", path.display()))?; + for entry in &parsed.disabled { + if entry.starts_with(MARKER_PREFIX) { + let identity = entry + .strip_prefix(ENABLED_PREFIX) + .or_else(|| entry.strip_prefix(HISTORY_PREFIX)); + anyhow::ensure!( + identity.is_some_and(|name| !name.is_empty() && !name.starts_with(MARKER_PREFIX)), + "parse skill state at {}: unsupported or malformed activation marker", + path.display() + ); + } + } Ok(parsed.disabled.into_iter().collect()) } @@ -284,10 +351,142 @@ mod tests { } #[test] - fn redundant_toggle_is_noop() { - let (_dir, mut store) = fresh(); + fn explicit_enable_records_a_choice_then_repeated_toggle_is_noop() { + let (dir, mut store) = fresh(); store.set_enabled("foo", true).unwrap(); assert!(store.disabled().is_empty()); + let path = dir.path().join(STATE_FILE_NAME); + let before = fs::read(&path).unwrap(); + store + .set_enabled_with_persist("foo", true, |_, _| { + panic!("repeating the same choice must not rewrite the store") + }) + .unwrap(); + assert_eq!(fs::read(path).unwrap(), before); + } + + // The released v0.10.0 serde shape and exact-set writer semantics at + // 1be1a703b975fc0a6c125886c761141341615a32. This intentionally does not + // interpret markers or use the upgraded loader/writer. + fn released_writer_toggle(path: &Path, name: &str, enabled: bool) -> bool { + #[derive(Deserialize, Serialize)] + struct ReleasedState { + #[serde(default)] + disabled: Vec, + } + let old: ReleasedState = toml::from_str(&fs::read_to_string(path).unwrap()).unwrap(); + let mut next: BTreeSet = old.disabled.into_iter().collect(); + let changed = if enabled { + next.remove(name) + } else { + next.insert(name.to_string()) + }; + if changed { + fs::write( + path, + toml::to_string_pretty(&ReleasedState { + disabled: next.into_iter().collect(), + }) + .unwrap(), + ) + .unwrap(); + } + changed + } + + #[test] + fn legacy_veto_and_exact_choices_survive_released_writer_serialization() { + for legacy in ["skill", "pdf", "demo:skill"] { + let (dir, mut store) = fresh(); + let path = dir.path().join(STATE_FILE_NAME); + let original = format!("disabled = [\"{legacy}\"]\n"); + fs::write(&path, &original).unwrap(); + store.refresh().unwrap(); + let a = format!("{legacy}-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + let b = format!("{legacy}-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"); + assert!(!store.is_enabled_with_legacy(&a, Some(legacy))); + assert!(!store.is_enabled_with_legacy(&b, Some(legacy))); + assert_eq!( + fs::read_to_string(&path).unwrap(), + original, + "reads never migrate" + ); + + store.set_enabled(&a, true).unwrap(); + assert!(released_writer_toggle(&path, "unrelated", false)); + store.refresh().unwrap(); + assert!(store.is_enabled_with_legacy(&a, Some(legacy))); + assert!(!store.is_enabled_with_legacy(&b, Some(legacy))); + // Old serialization cannot erase the history when removing L. + assert!(released_writer_toggle(&path, legacy, true)); + store.refresh().unwrap(); + assert!(!store.is_enabled_with_legacy(&b, Some(legacy))); + assert!(!store.is_enabled(legacy)); + + store.set_enabled(legacy, true).unwrap(); + assert!( + store.is_enabled(legacy), + "literal ASCII identity has its own exception" + ); + assert!(!store.is_enabled_with_legacy(&b, Some(legacy))); + assert!(released_writer_toggle(&path, legacy, false)); + store.refresh().unwrap(); + assert!( + !store.is_enabled(legacy), + "later raw exact disable beats an exception" + ); + // The old lossy/no-op toggle cannot express a new per-skill + // revocation. Preserve this limit rather than claim parity. + assert!(!released_writer_toggle(&path, legacy, false)); + store.refresh().unwrap(); + assert!(store.is_enabled_with_legacy(&a, Some(legacy))); + + store.set_enabled(&a, false).unwrap(); + released_writer_toggle(&path, legacy, true); + store.refresh().unwrap(); + assert!(!store.is_enabled_with_legacy(&a, Some(legacy))); + assert!(!store.is_enabled_with_legacy(&b, Some(legacy))); + } + } + + #[test] + fn malformed_or_unknown_activation_markers_preserve_bytes_and_memory() { + let (dir, mut store) = fresh(); + let path = dir.path().join(STATE_FILE_NAME); + store.set_enabled("kept", false).unwrap(); + for marker in [ + "!codewhale-skill-state:1:enabled:", + "!codewhale-skill-state:2:enabled:kept", + "!codewhale-skill-state:1:unknown:kept", + "!codewhale-skill-state:1:history:!codewhale-skill-state:1:enabled:kept", + ] { + let bytes = format!("disabled = [\"{marker}\"]\n"); + fs::write(&path, &bytes).unwrap(); + assert!(store.refresh().is_err()); + assert!(store.set_enabled("kept", true).is_err()); + assert!(!store.is_enabled("kept")); + assert_eq!(fs::read_to_string(&path).unwrap(), bytes); + } + fs::write(&path, "disabled = [\"unrelated opaque entry\"]\n").unwrap(); + store.refresh().unwrap(); + store.set_enabled("foo", false).unwrap(); + assert!(!store.is_enabled("unrelated opaque entry")); + } + + #[test] + fn failed_exact_enable_does_not_publish_or_replace_legacy_policy() { + let (dir, mut store) = fresh(); + let path = dir.path().join(STATE_FILE_NAME); + let original = b"disabled = [\"skill\"]\n"; + fs::write(&path, original).unwrap(); + store.refresh().unwrap(); + let name = "skill-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let result = store.set_enabled_with_persist(name, true, |_, _| { + anyhow::bail!("injected persistence failure") + }); + assert!(result.is_err()); + assert!(!store.is_enabled_with_legacy(name, Some("skill"))); + assert_eq!(fs::read(path).unwrap(), original); } #[test] diff --git a/crates/tui/src/commands/contract.rs b/crates/tui/src/commands/contract.rs index f28096f151..be7a6b82f0 100644 --- a/crates/tui/src/commands/contract.rs +++ b/crates/tui/src/commands/contract.rs @@ -2527,6 +2527,65 @@ fn installer_settings() -> (NetworkPolicy, u64, String) { (network, max_size, registry_url) } +/// Resolve the cache destination before loading settings or entering the network bridge. +fn sync_registry_to_cache(cache_dir: Option) -> Result { + use crate::skills::install::{SkillSyncOutcome as TuiSyncOutcome, SyncResult}; + let cache_dir = + cache_dir.ok_or_else(|| "global skill mutations require a home directory".to_string())?; + let (network, max_size, registry_url) = installer_settings(); + let result = run_async(async move { + crate::skills::install::sync_registry(&network, ®istry_url, &cache_dir, max_size).await + }); + match result { + Ok(SyncResult::RegistryDenied(host)) => Ok(SkillSyncOutcome::RegistryDenied(host)), + Ok(SyncResult::RegistryNeedsApproval(host)) => { + Ok(SkillSyncOutcome::RegistryNeedsApproval(host)) + } + Ok(SyncResult::Done { outcomes }) => { + let total = outcomes.len(); + let mut downloaded = 0usize; + let mut fresh = 0usize; + let mut failed = 0usize; + let entries = outcomes + .into_iter() + .map(|outcome| match outcome { + TuiSyncOutcome::Downloaded { name, path } => { + downloaded += 1; + SkillSyncEntry::Downloaded { + name, + path: path.display().to_string(), + } + } + TuiSyncOutcome::Fresh { name } => { + fresh += 1; + SkillSyncEntry::Fresh { name } + } + TuiSyncOutcome::Failed { name, reason } => { + failed += 1; + SkillSyncEntry::Failed { name, reason } + } + TuiSyncOutcome::Denied { name, host } => { + failed += 1; + SkillSyncEntry::Denied { name, host } + } + TuiSyncOutcome::NeedsApproval { name, host } => { + failed += 1; + SkillSyncEntry::NeedsApproval { name, host } + } + }) + .collect(); + Ok(SkillSyncOutcome::Done { + total, + downloaded, + fresh, + failed, + entries, + }) + } + Err(err) => Err(format_registry_error("Sync failed", &err)), + } +} + /// Inspect an anyhow chain and surface a one-line hint pointing at the most /// common cause of a registry fetch failure (DNS, refused, TLS, HTTP status, /// timeout). Mirrors `groups/skills/skills.rs::registry_fetch_error_hint`. @@ -2948,61 +3007,7 @@ impl CommandSkillGroupContext for SkillGroupAdapter<'_> { } fn sync_registry(&mut self) -> Result { - use crate::skills::install::{SkillSyncOutcome as TuiSyncOutcome, SyncResult}; - let (network, max_size, registry_url) = installer_settings(); - let cache_dir = crate::skills::install::default_cache_skills_dir(); - let result = run_async(async move { - crate::skills::install::sync_registry(&network, ®istry_url, &cache_dir, max_size) - .await - }); - match result { - Ok(SyncResult::RegistryDenied(host)) => Ok(SkillSyncOutcome::RegistryDenied(host)), - Ok(SyncResult::RegistryNeedsApproval(host)) => { - Ok(SkillSyncOutcome::RegistryNeedsApproval(host)) - } - Ok(SyncResult::Done { outcomes }) => { - let total = outcomes.len(); - let mut downloaded = 0usize; - let mut fresh = 0usize; - let mut failed = 0usize; - let entries = outcomes - .into_iter() - .map(|outcome| match outcome { - TuiSyncOutcome::Downloaded { name, path } => { - downloaded += 1; - SkillSyncEntry::Downloaded { - name, - path: path.display().to_string(), - } - } - TuiSyncOutcome::Fresh { name } => { - fresh += 1; - SkillSyncEntry::Fresh { name } - } - TuiSyncOutcome::Failed { name, reason } => { - failed += 1; - SkillSyncEntry::Failed { name, reason } - } - TuiSyncOutcome::Denied { name, host } => { - failed += 1; - SkillSyncEntry::Denied { name, host } - } - TuiSyncOutcome::NeedsApproval { name, host } => { - failed += 1; - SkillSyncEntry::NeedsApproval { name, host } - } - }) - .collect(); - Ok(SkillSyncOutcome::Done { - total, - downloaded, - fresh, - failed, - entries, - }) - } - Err(err) => Err(format_registry_error("Sync failed", &err)), - } + sync_registry_to_cache(crate::skills::install::default_cache_skills_dir()) } fn run_review(&mut self) -> Result { @@ -4448,6 +4453,17 @@ mod tests { )) } + #[test] + fn skill_registry_sync_without_home_refuses_before_the_network_bridge() { + // No Tokio runtime is present: entering run_async would panic rather + // than downloading a registry into an undiscoverable temporary root. + let result = sync_registry_to_cache(None); + assert_eq!( + result.unwrap_err(), + "global skill mutations require a home directory" + ); + } + /// A 1x1 PNG for media adapter tests. const PNG_1X1: &[u8] = &[ 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00, 0x00, 0x0d, 0x49, 0x48, 0x44, diff --git a/crates/tui/src/plugins/discovery.rs b/crates/tui/src/plugins/discovery.rs index 3bcffbc56d..901051f0dd 100644 --- a/crates/tui/src/plugins/discovery.rs +++ b/crates/tui/src/plugins/discovery.rs @@ -411,6 +411,7 @@ fn parse_skill_snapshots( } skill_snapshots.push(PluginSkillSnapshot { name: skill.name.clone(), + legacy_activation_name: skill.legacy_activation_name.clone(), description: skill.description.clone(), localized_descriptions: skill.localized_descriptions.clone(), invocation: skill.invocation, diff --git a/crates/tui/src/plugins/install/tarball.rs b/crates/tui/src/plugins/install/tarball.rs index 9f4b1ae08b..2a4620d7b2 100644 --- a/crates/tui/src/plugins/install/tarball.rs +++ b/crates/tui/src/plugins/install/tarball.rs @@ -202,13 +202,26 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> if total_size > max_size { return Err(PluginInstallError::OversizedBundle { limit: max_size }.into()); } - let mut out = fs::OpenOptions::new() - .create_new(true) - .write(true) + let mut options = fs::OpenOptions::new(); + options.create_new(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + let mut out = options .open(&target) .with_context(|| format!("failed to create {}", target.display()))?; out.write_all(&buf) .with_context(|| format!("failed to write {}", target.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let executable = header.mode().context("invalid archive file mode")? & 0o111 != 0; + let mode = if executable { 0o700 } else { 0o600 }; + out.set_permissions(fs::Permissions::from_mode(mode)) + .with_context(|| format!("failed to set mode for {}", target.display()))?; + } } } Ok(()) diff --git a/crates/tui/src/plugins/install/tests.rs b/crates/tui/src/plugins/install/tests.rs index 9f716f0565..38d981719c 100644 --- a/crates/tui/src/plugins/install/tests.rs +++ b/crates/tui/src/plugins/install/tests.rs @@ -28,6 +28,44 @@ fn tarball(entries: &[(&str, &[u8])]) -> Vec { encoder.finish().unwrap() } +#[cfg(unix)] +#[test] +fn remote_plugin_stage_preserves_only_owner_executable_intent() { + use std::os::unix::fs::PermissionsExt as _; + + let encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + let mut builder = tar::Builder::new(encoder); + for (path, body, mode) in [ + ( + "repo/plugin.toml", + &b"schema_version = 1\n[plugin]\nname = \"demo\"\nversion = \"1.0.0\"\n"[..], + 0o644, + ), + ("repo/bin/server", &b"#!/bin/sh\nexit 0\n"[..], 0o6755), + ("repo/data.txt", &b"data"[..], 0o666), + ] { + let mut header = tar::Header::new_gnu(); + header.set_size(body.len() as u64); + header.set_mode(mode); + header.set_cksum(); + builder.append_data(&mut header, path, body).unwrap(); + } + let bytes = builder.into_inner().unwrap().finish().unwrap(); + let tmp = tempfile::tempdir().unwrap(); + let staged = stage_tarball(&bytes, tmp.path(), DEFAULT_MAX_SIZE_BYTES, None).unwrap(); + for (path, expected) in [("bin/server", 0o700), ("data.txt", 0o600)] { + assert_eq!( + fs::metadata(staged.staged_path.join(path)) + .unwrap() + .permissions() + .mode() + & 0o7777, + expected, + "{path}" + ); + } +} + fn symlink_tarball(link_path: &str, target: &str, manifest: &str) -> Vec { let encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); let mut builder = tar::Builder::new(encoder); diff --git a/crates/tui/src/plugins/types.rs b/crates/tui/src/plugins/types.rs index 736bef4f94..0ede2ce1af 100644 --- a/crates/tui/src/plugins/types.rs +++ b/crates/tui/src/plugins/types.rs @@ -157,6 +157,7 @@ impl PluginTrustStatus { #[derive(Debug, Clone)] pub struct PluginSkillSnapshot { pub name: String, + pub legacy_activation_name: Option, pub description: String, pub localized_descriptions: HashMap, pub invocation: crate::skills::SkillInvocation, diff --git a/crates/tui/src/runtime_api.rs b/crates/tui/src/runtime_api.rs index de8e8ae3d0..b35aa8ce2d 100644 --- a/crates/tui/src/runtime_api.rs +++ b/crates/tui/src/runtime_api.rs @@ -3823,7 +3823,8 @@ async fn list_skills( plugin_id, plugin_generation, plugin_content_hash, - enabled: skill_state.is_enabled(&skill.name), + enabled: skill_state + .is_enabled_with_legacy(&skill.name, skill.legacy_activation_name.as_deref()), is_bundled: skill_entry_is_bundled(skill, &skills_dir), } }) diff --git a/crates/tui/src/runtime_api/tests.rs b/crates/tui/src/runtime_api/tests.rs index bc06862acc..5b0f2bb03a 100644 --- a/crates/tui/src/runtime_api/tests.rs +++ b/crates/tui/src/runtime_api/tests.rs @@ -11153,6 +11153,142 @@ async fn skills_endpoint_includes_enabled_field() -> Result<()> { Ok(()) } +#[tokio::test] +async fn unicode_skill_activation_matches_api_load_and_owned_lifecycle() -> Result<()> { + use crate::tools::spec::{ToolContext, ToolSpec as _}; + + let _env = lock_test_env(); + let tmp = tempfile::tempdir()?; + let root = tmp.path().join("runtime"); + let workspace = tmp.path().join("workspace"); + let home = tmp.path().join("home"); + fs::create_dir_all(&root)?; + fs::create_dir_all(&home)?; + let _home = EnvVarGuard::set("HOME", &home); + let _userprofile = EnvVarGuard::set("USERPROFILE", &home); + let _state_home = EnvVarGuard::set("CODEWHALE_HOME", &root); + let _config = EnvVarGuard::set("CODEWHALE_CONFIG_PATH", tmp.path().join("config.toml")); + crate::test_support::trust_workspace(&workspace); + let (original_dir, _) = create_managed_skill(&workspace, "技能")?; + create_managed_skill(&workspace, "分析")?; + create_managed_skill(&workspace, "skill")?; + let a = crate::skills::normalize_skill_name_for_lookup("技能"); + let b = crate::skills::normalize_skill_name_for_lookup("分析"); + assert_ne!(a, b); + let state_path = root.join("skills_state.toml"); + let initial = b"disabled = [\"skill\"]\n"; + fs::write(&state_path, initial)?; + let (addr, _threads, handle) = spawn_test_server_with_root_token_mobile_workspace( + root.clone(), + root.join("sessions"), + None, + false, + workspace.clone(), + ) + .await? + .context("isolated skill API fixture requires loopback")?; + let client = crate::tls::reqwest_client(); + let context = + ToolContext::new(&workspace).with_skills_config(workspace.join(".codewhale/skills"), false); + let tool = crate::tools::skill::LoadSkillTool; + + let before: serde_json::Value = client + .get(format!("http://{addr}/v1/skills")) + .send() + .await? + .error_for_status()? + .json() + .await?; + for name in [a.as_str(), b.as_str(), "skill"] { + let entry = before["skills"] + .as_array() + .context("skills")? + .iter() + .find(|skill| skill["name"] == name) + .context("discovered identity")?; + assert_eq!(entry["enabled"], false); + } + assert!( + tool.execute(json!({"name":"技能"}), &context) + .await + .is_err() + ); + assert_eq!( + fs::read(&state_path)?, + initial, + "listing/loading must not migrate state" + ); + + let raw_toggle = client + .post(format!("http://{addr}/v1/skills/技能")) + .json(&json!({"enabled":true})) + .send() + .await?; + assert_eq!( + raw_toggle.status(), + StatusCode::NOT_FOUND, + "toggle accepts exact catalog IDs only" + ); + for name in [a.as_str(), "skill"] { + client + .post(format!("http://{addr}/v1/skills/{name}")) + .json(&json!({"enabled":true})) + .send() + .await? + .error_for_status()?; + } + let after: serde_json::Value = client + .get(format!("http://{addr}/v1/skills")) + .send() + .await? + .error_for_status()? + .json() + .await?; + for (name, enabled) in [(a.as_str(), true), (b.as_str(), false), ("skill", true)] { + let entry = after["skills"] + .as_array() + .context("skills")? + .iter() + .find(|skill| skill["name"] == name) + .context("discovered identity")?; + assert_eq!(entry["enabled"], enabled); + } + for name in [a.as_str(), "技能", "skill"] { + let loaded = tool.execute(json!({"name":name}), &context).await?; + assert!(loaded.success); + } + assert!( + tool.execute(json!({"name":"分析"}), &context) + .await + .is_err() + ); + let audit: serde_json::Value = client + .get(format!("http://{addr}/v1/skills/{a}/audit")) + .send() + .await? + .error_for_status()? + .json() + .await?; + assert_eq!(audit["skills"][0]["name"], a); + assert!(original_dir.join("SKILL.md").is_file()); + assert!( + !workspace.join(".codewhale/skills").join(&a).exists(), + "no directory rename" + ); + client + .delete(format!("http://{addr}/v1/skills/{a}?scope=project")) + .send() + .await? + .error_for_status()?; + assert!( + !original_dir.exists(), + "owned resolver must use the shared canonical identity" + ); + assert!(workspace.join(".codewhale/skills/分析/SKILL.md").is_file()); + handle.abort(); + Ok(()) +} + #[tokio::test] async fn skills_endpoint_exposes_safe_plugin_provenance_and_shared_toggle() -> Result<()> { let tmp = tempfile::tempdir()?; @@ -11391,6 +11527,7 @@ fn skill_entry_is_bundled_requires_configured_bundle_path() { .expect("write override skill"); let bundled_skill = crate::skills::Skill { + legacy_activation_name: None, name: "delegate".to_string(), description: String::new(), localized_descriptions: std::collections::HashMap::new(), @@ -11401,6 +11538,7 @@ fn skill_entry_is_bundled_requires_configured_bundle_path() { source: crate::skills::SkillSource::Native, }; let override_skill = crate::skills::Skill { + legacy_activation_name: None, name: "delegate".to_string(), description: String::new(), localized_descriptions: std::collections::HashMap::new(), diff --git a/crates/tui/src/skills/frontmatter.rs b/crates/tui/src/skills/frontmatter.rs new file mode 100644 index 0000000000..3f072b33b8 --- /dev/null +++ b/crates/tui/src/skills/frontmatter.rs @@ -0,0 +1,218 @@ +//! Shared frontmatter reader for Skills, agent profiles, and installation. +//! This leaf is also included by the installation acceptance harness. + +use std::collections::HashMap; + +/// Parsed frontmatter: lowercased metadata keys and the body after the fence. +pub(crate) type Frontmatter<'a> = (HashMap, &'a str); + +/// Split a Markdown file into its `---` frontmatter metadata and body. +/// +/// Returns `Ok(None)` when the file does not open with a `---` fence. Keys are +/// lowercased; values are unquoted, and YAML block scalars (`>`, `|`, with +/// chomping) are folded the way `SKILL.md` has always read them. This is the +/// one frontmatter reader: skills and Claude Code agent files both use it. +pub(crate) fn parse_frontmatter( + content: &str, +) -> std::result::Result>, String> { + let content = content + .strip_prefix('\u{feff}') + .unwrap_or(content) + .trim_start(); + let opening = content.split_inclusive('\n').next().unwrap_or_default(); + if opening.trim_end() != "---" { + return Ok(None); + } + let rest = &content[opening.len()..]; + let mut offset = 0; + let end = rest + .split_inclusive('\n') + .find_map(|line| { + let start = offset; + offset += line.len(); + (line.trim_end() == "---").then_some(start) + }) + .ok_or_else(|| "missing frontmatter closing delimiter".to_string())?; + let frontmatter = &rest[..end]; + let body = &rest[end + 3..]; + + let mut metadata = HashMap::new(); + let indentation = |line: &str| line.chars().take_while(|ch| ch.is_whitespace()).count(); + let lines: Vec<&str> = frontmatter.lines().collect(); + let mut i = 0; + while i < lines.len() { + let raw = lines[i]; + let line = raw.trim(); + if line.is_empty() || line.starts_with('#') { + i += 1; + continue; + } + if let Some((key, value)) = line.split_once(':') { + let value = value.trim(); + // Check for YAML block scalar indicators: > (folded), | (literal), + // optionally with chomping: >-, >+, |-, |+ + let is_block_scalar = matches!(value, ">" | "|" | ">-" | ">+" | "|-" | "|+"); + if is_block_scalar { + let is_folded = value.starts_with('>'); + let chomp = if value.ends_with('-') { + "strip" + } else if value.ends_with('+') { + "keep" + } else { + "clip" + }; + // Determine the base indentation from the key line + let base_indent = indentation(raw); + let mut block_lines: Vec<&str> = Vec::new(); + let mut content_indent: Option = None; + i += 1; + while i < lines.len() { + let raw_line = lines[i]; + if raw_line.trim().is_empty() { + // Empty lines are part of the block + block_lines.push(""); + i += 1; + continue; + } + let line_indent = indentation(raw_line); + if line_indent > base_indent { + // Track content indent from the first non-empty + // line so we strip only that one level of + // leading whitespace, preserving any deeper + // relative indentation (YAML §8.1.2). + if content_indent.is_none() { + content_indent = Some(line_indent); + } + block_lines.push(raw_line); + i += 1; + } else { + break; + } + } + let content_indent = content_indent.unwrap_or(base_indent); + // Strip only the content indent from each non-empty + // line so nested indentation survives. + let block_lines: Vec<&str> = block_lines + .iter() + .map(|raw| { + if raw.is_empty() { + "" + } else { + let indent = indentation(raw); + let strip = std::cmp::min(indent, content_indent); + let byte = raw.char_indices().nth(strip).map_or(raw.len(), |(i, _)| i); + &raw[byte..] + } + }) + .collect(); + // Apply chomping to trailing empty lines before folding. + // Chomping operates on the raw block_lines (before join), so + // strip / keep / clip behave per the YAML spec. + let block_lines = if matches!(chomp, "strip") { + // strip: remove all trailing empty lines + let mut lines = block_lines; + while lines.last().is_some_and(|s| s.is_empty()) { + lines.pop(); + } + lines + } else if matches!(chomp, "keep") { + // keep: no modification + block_lines + } else { + // clip: keep at most one trailing empty line + let mut lines = block_lines; + while lines.len() >= 2 + && lines[lines.len() - 1].is_empty() + && lines[lines.len() - 2].is_empty() + { + lines.pop(); + } + lines + }; + let description = if is_folded { + // Folded: join non-empty lines with spaces; empty + // lines become paragraph breaks. + let mut result = String::new(); + let mut pending_space = false; + for line in &block_lines { + if line.is_empty() { + result.push('\n'); + pending_space = false; + } else { + if pending_space { + result.push(' '); + } + result.push_str(line); + pending_space = true; + } + } + result + } else { + // Literal: join with newlines. + block_lines.join("\n") + }; + metadata.insert(key.trim().to_ascii_lowercase(), description); + } else if value.is_empty() + && lines + .get(i + 1) + .is_some_and(|next| is_block_sequence_item(next)) + { + // A block sequence (`tools:` then ` - Read` lines) becomes + // one comma-separated value, the same as the flow form + // `tools: Read, Grep`. Dropping it would read as "no list". + let mut items = Vec::new(); + i += 1; + while let Some(next) = lines.get(i).filter(|next| is_block_sequence_item(next)) { + let item = next.trim()[1..].trim(); + let item = item + .strip_prefix('"') + .and_then(|v| v.strip_suffix('"')) + .or_else(|| item.strip_prefix('\'').and_then(|v| v.strip_suffix('\''))) + .unwrap_or(item); + if !item.is_empty() { + items.push(item); + } + i += 1; + } + metadata.insert(key.trim().to_ascii_lowercase(), items.join(", ")); + } else { + let unquoted = match value { + v if (v.starts_with('"') && v.ends_with('"') && v.len() >= 2) + || (v.starts_with('\'') && v.ends_with('\'') && v.len() >= 2) => + { + &v[1..v.len() - 1] + } + _ => value, + }; + i += 1; + let mut text = unquoted.to_string(); + // Wrapped plain scalars continue at a deeper indentation. + // A colon in that continuation belongs to the value, not a + // new metadata key. Quoted/flow values retain their grammar. + if !value.is_empty() && !value.starts_with(['"', '\'', '[', '{']) { + while let Some(next) = lines.get(i) { + if next.trim().is_empty() || indentation(next) <= indentation(raw) { + break; + } + if !next.trim_start().starts_with('#') { + text.push(' '); + text.push_str(next.trim()); + } + i += 1; + } + } + metadata.insert(key.trim().to_ascii_lowercase(), text); + } + } else { + i += 1; + } + } + + Ok(Some((metadata, body))) +} + +/// A YAML block-sequence entry: `- item` (or a bare `-`) on its own line. +fn is_block_sequence_item(line: &str) -> bool { + let line = line.trim(); + line == "-" || line.starts_with("- ") +} diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs index aa342289ab..9ab52ef066 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -29,9 +29,9 @@ //! escape. Multi-skill repository archives may contain unrelated symlinks //! outside that selected subtree; those entries are ignored and never //! extracted. -//! * No `+x` is granted on extracted files. The optional `/skill trust ` -//! command writes a `.trusted` marker; tool-execution gating is a separate -//! concern that lives next to the tool registry. +//! * Archive executable intent is preserved for the owner only; extraction +//! never runs files or accepts an archive's trust/installation markers. +//! `/skill trust ` writes the local `.trusted` receipt separately. //! * Claude Code plugin archives that contain multiple skills are rejected with //! an explicit migration message. Codewhale can install individual //! `SKILL.md` bundles, including `.claude/skills//SKILL.md`, but it @@ -60,11 +60,10 @@ fn reqwest_client() -> reqwest::Client { /// /// Lives at `~/.codewhale/cache/skills/` so it's separate from user-installed /// skills and can be blown away without losing anything irreplaceable. -pub fn default_cache_skills_dir() -> PathBuf { - crate::config::effective_home_dir().map_or_else( - || PathBuf::from("/tmp/codewhale/cache/skills"), - |p| p.join(".codewhale").join("cache").join("skills"), - ) +/// A missing home has no cache destination; callers must refuse the sync. +pub fn default_cache_skills_dir() -> Option { + crate::config::effective_home_dir() + .map(|home| home.join(".codewhale").join("cache").join("skills")) } /// Default registry. Falls back to a community-curated `index.json` hosted on @@ -87,6 +86,21 @@ pub const INSTALLED_FROM_MARKER: &str = ".installed-from"; /// never auto-runs anything) — the runtime tool-invocation gate consults this /// marker before executing scripts that ship with the skill. pub const TRUSTED_MARKER: &str = ".trusted"; +const RESERVED_ROOT_METADATA: [&str; 3] = [ + INSTALLED_FROM_MARKER, + TRUSTED_MARKER, + ".system-installed-version", +]; + +/// Installer-owned root metadata must never arrive from a remote package. +/// Descendants of a reserved root directory are excluded too. +pub(super) fn is_reserved_root_metadata(relative: &Path) -> bool { + let first = relative + .components() + .find(|component| !matches!(component, Component::CurDir)); + matches!(first, Some(Component::Normal(name)) if name.to_str().is_some_and(|name| + RESERVED_ROOT_METADATA.iter().any(|reserved| name.eq_ignore_ascii_case(reserved)))) +} // ───────────────────────────────────────────────────────────────────────────── // Source parsing @@ -481,19 +495,10 @@ pub async fn update_with_registry( // Bytes changed — fall back to the regular install path with `update = true` // so we get the same atomic-replace semantics. Content updates must not - // inherit a previous trust marker. - let trust_path = target.join(TRUSTED_MARKER); - let had_trust = tokio::fs::try_exists(&trust_path).await.unwrap_or(false); + // inherit a previous trust marker. The shared extractor excludes archive + // markers on every install, including an update of an untrusted package. let outcome = install_with_registry(source, skills_dir, max_size, network, true, registry_url).await?; - match &outcome { - InstallOutcome::Installed(installed) => { - if had_trust { - let _ = tokio::fs::remove_file(installed.path.join(TRUSTED_MARKER)).await; - } - } - InstallOutcome::NeedsApproval(_) | InstallOutcome::NetworkDenied(_) => {} - } match outcome { InstallOutcome::Installed(installed) => Ok(UpdateResult::Updated(installed)), InstallOutcome::NeedsApproval(host) => Ok(UpdateResult::NeedsApproval(host)), @@ -1344,9 +1349,8 @@ fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { } } - // Parse frontmatter to extract the skill name. We reuse the same parser - // shape as `SkillRegistry::parse_skill` but inline it here so we don't - // depend on the discovery module's private function. + // Install and discovery share the same frontmatter reader; install adds + // the required description and path-safe destination-name checks. let name = parse_frontmatter_name(&skill_md_bytes)?; Ok(TarballScan { @@ -1469,6 +1473,9 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> if entry_type.is_symlink() || entry_type.is_hard_link() { return Err(InstallError::SymlinkRejected.into()); } + if is_reserved_root_metadata(stripped_path) { + continue; + } let target = dest.join(stripped_path); // Final paranoia check: ensure the resolved target stays under dest. @@ -1500,13 +1507,36 @@ fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> if total_size > max_size { return Err(InstallError::OversizedTarball { limit: max_size }.into()); } - let mut out = fs::OpenOptions::new() - .create_new(true) - .write(true) + let mut options = fs::OpenOptions::new(); + options.create_new(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + let mut out = options .open(&target) .with_context(|| format!("failed to create {}", target.display()))?; out.write_all(&buf) .with_context(|| format!("failed to write {}", target.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let executable = header.mode().context("invalid archive file mode")? & 0o111 != 0; + let mode = if executable { 0o700 } else { 0o600 }; + out.set_permissions(fs::Permissions::from_mode(mode)) + .with_context(|| format!("failed to set mode for {}", target.display()))?; + } + } + } + // Let the host filesystem resolve any platform-specific aliases (for + // example a trailing dot on Windows) before this staged tree is published. + // Remote content can never supply the local install or trust receipt. + for marker in RESERVED_ROOT_METADATA { + match fs::remove_file(dest.join(marker)) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error).context("failed to discard archive metadata"), } } Ok(()) @@ -1598,36 +1628,20 @@ fn strip_prefix<'a>(path: &'a str, prefix: &str) -> std::borrow::Cow<'a, str> { /// Also verifies the leading `---` fence so we reject malformed files early. fn parse_frontmatter_name(bytes: &[u8]) -> Result { let content = std::str::from_utf8(bytes).context("SKILL.md is not valid UTF-8")?; - let trimmed = content.trim_start(); - if !trimmed.starts_with("---") { - bail!("SKILL.md is missing the leading '---' frontmatter fence"); - } - let after_open = &trimmed[3..]; - let close = after_open.find("---").ok_or_else(|| { - anyhow::anyhow!("SKILL.md is missing the closing '---' frontmatter fence") - })?; - let frontmatter = &after_open[..close]; - - let mut name: Option = None; - let mut has_description = false; - for raw in frontmatter.lines() { - let line = raw.trim(); - if line.is_empty() || line.starts_with('#') { - continue; - } - if let Some((key, value)) = line.split_once(':') { - let key = key.trim().to_ascii_lowercase(); - let value = value.trim().to_string(); - match key.as_str() { - "name" if !value.is_empty() => name = Some(value), - "description" if !value.is_empty() => has_description = true, - _ => {} - } - } - } - - let name = name.ok_or(InstallError::MissingFrontmatterField("name"))?; - if !has_description { + let (metadata, _) = super::frontmatter::parse_frontmatter(content) + .map_err(anyhow::Error::msg)? + .ok_or_else(|| { + anyhow::anyhow!("SKILL.md is missing the leading '---' frontmatter fence") + })?; + let name = metadata + .get("name") + .filter(|value| !value.trim().is_empty()) + .cloned() + .ok_or(InstallError::MissingFrontmatterField("name"))?; + if !metadata + .get("description") + .is_some_and(|value| !value.trim().is_empty()) + { return Err(InstallError::MissingFrontmatterField("description").into()); } if validate_skill_name_segment(&name).is_err() { @@ -1674,6 +1688,90 @@ fn hex_bytes(bytes: impl AsRef<[u8]>) -> String { mod tests { use super::*; + fn skill_tarball(entries: &[(&str, &[u8], u32)]) -> Vec { + let encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + let mut builder = tar::Builder::new(encoder); + for (path, body, mode) in entries { + let mut header = tar::Header::new_gnu(); + header.set_size(body.len() as u64); + header.set_mode(*mode); + header.set_cksum(); + builder.append_data(&mut header, path, *body).unwrap(); + } + builder.into_inner().unwrap().finish().unwrap() + } + + #[test] + fn remote_stage_discards_forged_root_receipts_and_keeps_hidden_payloads() { + let tmp = tempfile::tempdir().unwrap(); + let skills = tmp.path().join(".codewhale/skills"); + let body = b"---\nname: demo\ndescription: test\n---\nbody"; + let baseline = tmp.path().join("baseline"); + fs::create_dir_all(baseline.join("nested")).unwrap(); + fs::write(baseline.join("SKILL.md"), body).unwrap(); + fs::write(baseline.join(".hidden"), b"payload").unwrap(); + fs::write(baseline.join("nested/.trusted"), b"nested payload").unwrap(); + let digest = super::super::package_digest::compute_package_digest(&baseline).unwrap(); + let forged = serde_json::json!({"schema_version": 2, "content_digest": digest}).to_string(); + let bytes = skill_tarball(&[ + ("repo-main/SKILL.md", body, 0o644), + ("repo-main/.trusted", forged.as_bytes(), 0o644), + ("repo-main/.TRUSTED", forged.as_bytes(), 0o644), + ("repo-main/./.installed-from", b"forged provenance", 0o644), + ("repo-main/.system-installed-version", b"999", 0o644), + ("repo-main/.hidden", b"payload", 0o644), + ("repo-main/nested/.trusted", b"nested payload", 0o644), + ]); + let staged = stage_tarball(&bytes, &skills, DEFAULT_MAX_SIZE_BYTES).unwrap(); + for marker in RESERVED_ROOT_METADATA { + assert!(!staged.staged_path.join(marker).exists(), "{marker}"); + } + assert!(!staged.staged_path.join(".TRUSTED").exists()); + assert_eq!( + fs::read(staged.staged_path.join(".hidden")).unwrap(), + b"payload" + ); + assert_eq!( + super::super::package_digest::compute_package_digest(&staged.staged_path).unwrap(), + digest + ); + fs::rename(&staged.staged_path, skills.join("demo")).unwrap(); + } + + #[cfg(unix)] + #[test] + fn remote_skill_stage_preserves_only_owner_executable_intent() { + use std::os::unix::fs::PermissionsExt as _; + + let bytes = skill_tarball(&[ + ( + "repo/SKILL.md", + b"---\nname: demo\ndescription: test\n---\nbody", + 0o644, + ), + ("repo/scripts/run.sh", b"#!/bin/sh\nexit 0\n", 0o6755), + ("repo/scripts/group-only.sh", b"#!/bin/sh\nexit 0\n", 0o010), + ("repo/data.txt", b"data", 0o666), + ]); + let tmp = tempfile::tempdir().unwrap(); + let staged = stage_tarball(&bytes, tmp.path(), DEFAULT_MAX_SIZE_BYTES).unwrap(); + for (path, expected) in [ + ("scripts/run.sh", 0o700), + ("scripts/group-only.sh", 0o700), + ("data.txt", 0o600), + ] { + assert_eq!( + fs::metadata(staged.staged_path.join(path)) + .unwrap() + .permissions() + .mode() + & 0o7777, + expected, + "{path}" + ); + } + } + #[tokio::test] async fn registry_sync_refuses_a_key_that_is_not_a_single_segment() { let root = tempfile::tempdir().expect("tempdir"); @@ -1864,6 +1962,17 @@ mod tests { assert_eq!(parse_frontmatter_name(body).unwrap(), "hello"); } + #[test] + fn installer_uses_shared_frontmatter_without_weakening_name_checks() { + let body = "\u{feff}---\r\nname: 'hello'\r\ndescription: Deploy --- safely\r\n with details: here\r\n---\r\nbody"; + assert_eq!(parse_frontmatter_name(body.as_bytes()).unwrap(), "hello"); + assert!(parse_frontmatter_name(body.replace("'hello'", "'../escape'").as_bytes()).is_err()); + assert!(parse_frontmatter_name(b"---\nname: hello\ndescription: ''\n---\n").is_err()); + assert!( + parse_frontmatter_name(b"---\nname: hello\ndescription: missing --- fence").is_err() + ); + } + #[test] fn parse_frontmatter_missing_name_fails() { let body = b"---\ndescription: x\n---\n"; diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 357e936f9d..694440848d 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -4,6 +4,8 @@ pub mod audit; /// Provider-free contract tests for the bundled starter pack (#4698). #[cfg(test)] mod catalog_matrix; +mod frontmatter; +pub(crate) use frontmatter::parse_frontmatter; pub mod install; pub mod mutation; mod package_digest; @@ -192,11 +194,20 @@ pub fn default_skills_dir() -> PathBuf { } } crate::config::effective_home_dir().map_or_else( - || PathBuf::from("/tmp/codewhale/skills"), + || unavailable_home_root().join("skills"), |p| p.join(".codewhale").join("skills"), ) } +// Match plugin discovery's fail-closed fallback: never discover or write to +// a predictable shared temporary path when the user's home is unavailable. +fn unavailable_home_root() -> PathBuf { + std::env::temp_dir().join(format!( + ".codewhale-home-unavailable-{}", + uuid::Uuid::new_v4().simple() + )) +} + /// Global agentskills.io-compatible skills directory (`~/.agents/skills`). #[must_use] pub fn agents_global_skills_dir() -> Option { @@ -241,6 +252,9 @@ impl SkillDiscoveryMode { #[derive(Debug, Clone)] pub struct Skill { pub name: String, + /// Former lossy key, used only to preserve activation vetoes, never as + /// a body lookup alias. Plugin keys carry the declared namespace. + pub legacy_activation_name: Option, /// Default (language-neutral, usually English) description. pub description: String, /// Optional locale-specific descriptions, keyed by lowercased locale tag @@ -581,15 +595,26 @@ impl SkillRegistry { } fn normalize_skill_name(&mut self, skill: &mut Skill, skill_path: &Path) { + let legacy = legacy_skill_name_for_lookup(&skill.name); let normalized = normalize_skill_name_for_lookup(&skill.name); + let supported_unicode_name = !skill.name.is_ascii() + && !skill.name.chars().any(char::is_control) + && is_valid_skill_name(&normalized); + skill.legacy_activation_name = (legacy != normalized).then_some(legacy); if normalized != skill.name || !is_valid_skill_name(&skill.name) { let original = skill.name.clone(); skill.name = normalized; - self.push_warning(format!( - "Skill name `{original}` in {} is not a safe command name; using `{}` instead.", - skill_path.display(), - skill.name - )); + // Unicode names have an intentional, stable command identity. + // Reporting that translation as invalid would make reviewed plugin + // snapshots refuse otherwise valid skills. Malformed names still + // produce the existing warning and fail closed during review. + if !supported_unicode_name { + self.push_warning(format!( + "Skill name `{original}` in {} is not a safe command name; using `{}` instead.", + skill_path.display(), + skill.name + )); + } } } @@ -631,6 +656,7 @@ impl SkillRegistry { return Ok(Skill { name, + legacy_activation_name: None, description, localized_descriptions, invocation, @@ -657,6 +683,7 @@ impl SkillRegistry { Ok(Skill { name, + legacy_activation_name: None, description: String::new(), localized_descriptions: HashMap::new(), invocation: SkillInvocation::ModelAndUser, @@ -684,6 +711,30 @@ impl SkillRegistry { /// Lookup a skill by name. pub fn get(&self, name: &str) -> Option<&Skill> { + let name = name.trim(); + if let Some(skill) = self + .skills + .iter() + .find(|skill| skill.name.eq_ignore_ascii_case(name)) + { + return Some(skill); + } + if let Some((namespace, suffix)) = name.split_once(':') { + if namespace.is_empty() || suffix.is_empty() || suffix.contains(':') { + return None; + } + let suffix = normalize_skill_name_segment(suffix); + // Namespace punctuation is identity, not a slug. An absent dotted + // namespace must never fall back into a dashed plugin's body. + return self.skills.iter().find(|skill| { + skill + .name + .split_once(':') + .is_some_and(|(declared, canonical)| { + declared.eq_ignore_ascii_case(namespace) && canonical == suffix + }) + }); + } let normalized = normalize_skill_name_for_lookup(name); self.skills .iter() @@ -714,7 +765,9 @@ impl SkillRegistry { state: anyhow::Result, ) -> Self { match state { - Ok(state) => self.skills.retain(|skill| state.is_enabled(&skill.name)), + Ok(state) => self.skills.retain(|skill| { + state.is_enabled_with_legacy(&skill.name, skill.legacy_activation_name.as_deref()) + }), Err(error) => { let hidden_plugin_skills = self .skills @@ -762,210 +815,39 @@ fn is_valid_skill_name(name: &str) -> bool { .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '-') } -/// Parsed frontmatter: lowercased metadata keys and the body after the fence. -pub(crate) type Frontmatter<'a> = (HashMap, &'a str); - -/// Split a Markdown file into its `---` frontmatter metadata and body. -/// -/// Returns `Ok(None)` when the file does not open with a `---` fence. Keys are -/// lowercased; values are unquoted, and YAML block scalars (`>`, `|`, with -/// chomping) are folded the way `SKILL.md` has always read them. This is the -/// one frontmatter reader: skills and Claude Code agent files both use it. -pub(crate) fn parse_frontmatter( - content: &str, -) -> std::result::Result>, String> { - if !content.trim_start().starts_with("---") { - return Ok(None); - } - let start = content - .find("---") - .ok_or_else(|| "missing frontmatter opening delimiter".to_string())?; - let rest = &content[start + 3..]; - let end = rest - .find("---") - .ok_or_else(|| "missing frontmatter closing delimiter".to_string())?; - let frontmatter = &rest[..end]; - let body = &rest[end + 3..]; - - let mut metadata = HashMap::new(); - let lines: Vec<&str> = frontmatter.lines().collect(); - let mut i = 0; - while i < lines.len() { - let raw = lines[i]; - let line = raw.trim(); - if line.is_empty() || line.starts_with('#') { - i += 1; - continue; - } - if let Some((key, value)) = line.split_once(':') { - let value = value.trim(); - // Check for YAML block scalar indicators: > (folded), | (literal), - // optionally with chomping: >-, >+, |-, |+ - let is_block_scalar = matches!(value, ">" | "|" | ">-" | ">+" | "|-" | "|+"); - if is_block_scalar { - let is_folded = value.starts_with('>'); - let chomp = if value.ends_with('-') { - "strip" - } else if value.ends_with('+') { - "keep" - } else { - "clip" - }; - // Determine the base indentation from the key line - let base_indent = raw.len() - raw.trim_start().len(); - let mut block_lines: Vec<&str> = Vec::new(); - let mut content_indent: Option = None; - i += 1; - while i < lines.len() { - let raw_line = lines[i]; - if raw_line.trim().is_empty() { - // Empty lines are part of the block - block_lines.push(""); - i += 1; - continue; - } - let line_indent = raw_line.len() - raw_line.trim_start().len(); - if line_indent > base_indent { - // Track content indent from the first non-empty - // line so we strip only that one level of - // leading whitespace, preserving any deeper - // relative indentation (YAML §8.1.2). - if content_indent.is_none() { - content_indent = Some(line_indent); - } - block_lines.push(raw_line); - i += 1; - } else { - break; - } - } - let content_indent = content_indent.unwrap_or(base_indent); - // Strip only the content indent from each non-empty - // line so nested indentation survives. - let block_lines: Vec<&str> = block_lines - .iter() - .map(|raw| { - if raw.is_empty() { - "" - } else { - let indent = raw.len() - raw.trim_start().len(); - let strip = std::cmp::min(indent, content_indent); - &raw[strip..] - } - }) - .collect(); - // Apply chomping to trailing empty lines before folding. - // Chomping operates on the raw block_lines (before join), so - // strip / keep / clip behave per the YAML spec. - let block_lines = if matches!(chomp, "strip") { - // strip: remove all trailing empty lines - let mut lines = block_lines; - while lines.last().is_some_and(|s| s.is_empty()) { - lines.pop(); - } - lines - } else if matches!(chomp, "keep") { - // keep: no modification - block_lines - } else { - // clip: keep at most one trailing empty line - let mut lines = block_lines; - while lines.len() >= 2 - && lines[lines.len() - 1].is_empty() - && lines[lines.len() - 2].is_empty() - { - lines.pop(); - } - lines - }; - let description = if is_folded { - // Folded: join non-empty lines with spaces; empty - // lines become paragraph breaks. - let mut result = String::new(); - let mut pending_space = false; - for line in &block_lines { - if line.is_empty() { - result.push('\n'); - pending_space = false; - } else { - if pending_space { - result.push(' '); - } - result.push_str(line); - pending_space = true; - } - } - result - } else { - // Literal: join with newlines. - block_lines.join("\n") - }; - metadata.insert(key.trim().to_ascii_lowercase(), description); - } else if value.is_empty() - && lines - .get(i + 1) - .is_some_and(|next| is_block_sequence_item(next)) - { - // A block sequence (`tools:` then ` - Read` lines) becomes - // one comma-separated value, the same as the flow form - // `tools: Read, Grep`. Dropping it would read as "no list". - let mut items = Vec::new(); - i += 1; - while let Some(next) = lines.get(i).filter(|next| is_block_sequence_item(next)) { - let item = next.trim()[1..].trim(); - let item = item - .strip_prefix('"') - .and_then(|v| v.strip_suffix('"')) - .or_else(|| item.strip_prefix('\'').and_then(|v| v.strip_suffix('\''))) - .unwrap_or(item); - if !item.is_empty() { - items.push(item); - } - i += 1; - } - metadata.insert(key.trim().to_ascii_lowercase(), items.join(", ")); - } else { - let unquoted = match value { - v if (v.starts_with('"') && v.ends_with('"') && v.len() >= 2) - || (v.starts_with('\'') && v.ends_with('\'') && v.len() >= 2) => - { - &v[1..v.len() - 1] - } - _ => value, - }; - metadata.insert(key.trim().to_ascii_lowercase(), unquoted.to_string()); - i += 1; - } - } else { - i += 1; - } - } - - Ok(Some((metadata, body))) +pub(crate) fn normalize_skill_name_for_lookup(name: &str) -> String { + normalize_qualified_skill_name(name, normalize_skill_name_segment) } -/// A YAML block-sequence entry: `- item` (or a bare `-`) on its own line. -fn is_block_sequence_item(line: &str) -> bool { - let line = line.trim(); - line == "-" || line.starts_with("- ") +fn legacy_skill_name_for_lookup(name: &str) -> String { + normalize_qualified_skill_name(name, legacy_skill_name_segment) } -pub(crate) fn normalize_skill_name_for_lookup(name: &str) -> String { +fn normalize_qualified_skill_name(name: &str, segment: fn(&str) -> String) -> String { if let Some((plugin, skill)) = name.trim().split_once(':') && !plugin.is_empty() && !skill.is_empty() && !skill.contains(':') { - return format!( - "{}:{}", - normalize_skill_name_segment(plugin), - normalize_skill_name_segment(skill) - ); + return format!("{}:{}", segment(plugin), segment(skill)); } - normalize_skill_name_segment(name) + segment(name) } fn normalize_skill_name_segment(name: &str) -> String { + let name = name.trim(); + let legacy = legacy_skill_name_segment(name); + if name.is_ascii() { + return legacy; + } + // Preserve ASCII identities, but distinguish UTF-8 source names the old + // slug folded together. No transliteration or Unicode normalization. + let digest = crate::hashing::sha256_hex(name.to_ascii_lowercase().as_bytes()); + let prefix = legacy[..legacy.len().min(31)].trim_end_matches('-'); + format!("{prefix}-{}", &digest[..32]) +} + +fn legacy_skill_name_segment(name: &str) -> String { let mut out = String::new(); let mut pending_dash = false; @@ -1320,6 +1202,9 @@ fn merge_plugin_skills_from_plugins( } registry.skills.push(Skill { name: qualified_name, + legacy_activation_name: snapshot + .legacy_activation_name + .map(|legacy| format!("{plugin_name}:{legacy}")), description: snapshot.description, localized_descriptions: snapshot.localized_descriptions, invocation: snapshot.invocation, diff --git a/crates/tui/src/skills/package_digest.rs b/crates/tui/src/skills/package_digest.rs index d13d2bfc15..d50412de5e 100644 --- a/crates/tui/src/skills/package_digest.rs +++ b/crates/tui/src/skills/package_digest.rs @@ -5,11 +5,14 @@ use std::collections::HashSet; use std::fs; +use std::io::Read; use std::path::{Path, PathBuf}; use sha2::{Digest, Sha256}; -use super::install::{DEFAULT_MAX_SIZE_BYTES, INSTALLED_FROM_MARKER, TRUSTED_MARKER}; +use super::install::{ + DEFAULT_MAX_SIZE_BYTES, INSTALLED_FROM_MARKER, TRUSTED_MARKER, is_reserved_root_metadata, +}; pub const PACKAGE_DIGEST_MAX_BYTES: u64 = DEFAULT_MAX_SIZE_BYTES; pub const PACKAGE_DIGEST_MAX_FILES: usize = 256; @@ -90,30 +93,41 @@ fn walk( if !canonical.starts_with(package_root) { return Err(PackageDigestError::EscapedRoot); } - if !visited.insert(canonical) { + if !visited.insert(canonical.clone()) { return Err(PackageDigestError::Cycle); } - let entries = fs::read_dir(dir).map_err(|_| PackageDigestError::Unreadable)?; - for entry in entries.flatten() { + let entries = fs::read_dir(&canonical).map_err(|_| PackageDigestError::Unreadable)?; + for entry in entries { + let entry = entry.map_err(|_| PackageDigestError::Unreadable)?; let path = entry.path(); - let Some(name) = path.file_name().and_then(|s| s.to_str()) else { - continue; - }; + let name = path + .file_name() + .and_then(|s| s.to_str()) + .ok_or(PackageDigestError::Unreadable)?; let meta = fs::symlink_metadata(&path).map_err(|_| PackageDigestError::Unreadable)?; if meta.file_type().is_symlink() { return Err(PackageDigestError::SymlinkPresent); } - if name == INSTALLED_FROM_MARKER - || name == TRUSTED_MARKER - || name == ".system-installed-version" - || name.ends_with(".bak") - || name.ends_with(".tmp") - || name.starts_with('.') - { - continue; + // Only local bookkeeping at the package root is outside the receipt. + // Hidden files and backup/temp names can contain executable payloads. + if depth == 0 && is_reserved_root_metadata(Path::new(name)) { + if meta.is_file() + && [ + INSTALLED_FROM_MARKER, + TRUSTED_MARKER, + ".system-installed-version", + ] + .contains(&name) + { + continue; + } + // A variant may be an ordinary payload on a case-sensitive disk + // or alias the receipt itself on another disk. Refuse ambiguity: + // the owner must rename it before the package can be trusted. + return Err(PackageDigestError::Unreadable); } if meta.is_dir() { @@ -121,26 +135,47 @@ fn walk( continue; } if !meta.is_file() { - continue; + return Err(PackageDigestError::Unreadable); } if files.len() >= PACKAGE_DIGEST_MAX_FILES { return Err(PackageDigestError::TooManyFiles); } - let len = meta.len(); - if *total_bytes + len > PACKAGE_DIGEST_MAX_BYTES { + let remaining = PACKAGE_DIGEST_MAX_BYTES.saturating_sub(*total_bytes); + if meta.len() > remaining { return Err(PackageDigestError::Oversized); } - let bytes = fs::read(&path).map_err(|_| PackageDigestError::Unreadable)?; + let mut file = fs::File::open(&path).map_err(|_| PackageDigestError::Unreadable)?; + let bytes = read_package_file(&mut file, remaining)?; *total_bytes += bytes.len() as u64; let rel = path .strip_prefix(package_root) - .map(|p| p.to_string_lossy().replace('\\', "/")) - .unwrap_or_else(|_| name.to_string()); + .map_err(|_| PackageDigestError::EscapedRoot)? + .components() + .map(|component| { + component + .as_os_str() + .to_str() + .ok_or(PackageDigestError::Unreadable) + }) + .collect::, _>>()? + .join("/"); files.push((rel, bytes)); } Ok(()) } +fn read_package_file(file: &mut fs::File, remaining: u64) -> Result, PackageDigestError> { + // Metadata is only an early check: a package file may grow before reading. + let mut bytes = Vec::new(); + file.take(remaining + 1) + .read_to_end(&mut bytes) + .map_err(|_| PackageDigestError::Unreadable)?; + if bytes.len() as u64 > remaining { + return Err(PackageDigestError::Oversized); + } + Ok(bytes) +} + fn hex_digest(bytes: impl AsRef<[u8]>) -> String { let bytes = bytes.as_ref(); let mut out = String::with_capacity(bytes.len() * 2); @@ -150,3 +185,159 @@ fn hex_digest(bytes: impl AsRef<[u8]>) -> String { } out } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn relative_roots_keep_nested_paths_and_match_canonical_roots() { + let cwd = std::env::current_dir().unwrap(); + let tmp = tempfile::tempdir_in(&cwd).unwrap(); + let relative = tmp.path().strip_prefix(&cwd).unwrap(); + assert!(!relative.is_absolute()); + fs::create_dir(relative.join("before")).unwrap(); + fs::create_dir(relative.join("after")).unwrap(); + fs::write(relative.join("before/payload.txt"), b"same bytes").unwrap(); + let canonical = fs::canonicalize(relative).unwrap(); + + let before = compute_package_digest(relative).unwrap(); + assert_eq!(before, compute_package_digest(&canonical).unwrap()); + fs::rename( + relative.join("before/payload.txt"), + relative.join("after/payload.txt"), + ) + .unwrap(); + let after = compute_package_digest(relative).unwrap(); + assert_ne!(before, after); + assert_eq!(after, compute_package_digest(&canonical).unwrap()); + } + + #[cfg(unix)] + #[test] + fn aliased_ancestors_keep_nested_paths_but_symlink_package_roots_are_refused() { + use std::os::unix::fs::symlink; + + let tmp = tempfile::tempdir().unwrap(); + let parent = tmp.path().join("real"); + let package = parent.join("package"); + fs::create_dir_all(package.join("before")).unwrap(); + fs::create_dir(package.join("after")).unwrap(); + fs::write(package.join("before/payload.txt"), b"same bytes").unwrap(); + let alias = tmp.path().join("alias"); + symlink(&parent, &alias).unwrap(); + let aliased_package = alias.join("package"); + let canonical = fs::canonicalize(&package).unwrap(); + + let before = compute_package_digest(&aliased_package).unwrap(); + assert_eq!(before, compute_package_digest(&canonical).unwrap()); + fs::rename( + package.join("before/payload.txt"), + package.join("after/payload.txt"), + ) + .unwrap(); + let after = compute_package_digest(&aliased_package).unwrap(); + assert_ne!(before, after); + assert_eq!(after, compute_package_digest(&canonical).unwrap()); + + let root_link = tmp.path().join("package-link"); + symlink(&package, &root_link).unwrap(); + assert_eq!( + compute_package_digest(&root_link), + Err(PackageDigestError::SymlinkPresent) + ); + } + + #[cfg(unix)] + #[test] + fn literal_backslash_filename_differs_from_nested_path() { + let tmp = tempfile::tempdir().unwrap(); + let package = fs::canonicalize(tmp.path()).unwrap(); + fs::create_dir(package.join("a")).unwrap(); + let literal = package.join(r"a\b"); + fs::write(&literal, b"same bytes").unwrap(); + let before = compute_package_digest(&package).unwrap(); + + fs::rename(literal, package.join("a/b")).unwrap(); + assert_ne!(before, compute_package_digest(&package).unwrap()); + } + + #[test] + fn case_variant_metadata_fails_closed_instead_of_omitting_payload_or_hashing_itself() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".TRUSTED"), b"payload or aliased receipt").unwrap(); + assert_eq!( + compute_package_digest(tmp.path()), + Err(PackageDigestError::Unreadable) + ); + } + + #[test] + fn reserved_metadata_directories_cannot_hide_payload() { + let tmp = tempfile::tempdir().unwrap(); + for marker in [ + INSTALLED_FROM_MARKER, + TRUSTED_MARKER, + ".system-installed-version", + ] { + let path = tmp.path().join(marker); + fs::create_dir(&path).unwrap(); + fs::write(path.join("payload"), b"must not be omitted").unwrap(); + assert_eq!( + compute_package_digest(tmp.path()), + Err(PackageDigestError::Unreadable) + ); + fs::remove_dir_all(path).unwrap(); + } + } + + // Linux permits non-UTF-8 filenames; macOS APFS rejects the fixture itself. + #[cfg(target_os = "linux")] + #[test] + fn non_utf8_payload_names_fail_closed() { + use std::os::unix::ffi::OsStringExt as _; + let tmp = tempfile::tempdir().unwrap(); + let name = std::ffi::OsString::from_vec(b"payload-\xff".to_vec()); + fs::write(tmp.path().join(name), b"payload").unwrap(); + assert_eq!( + compute_package_digest(tmp.path()), + Err(PackageDigestError::Unreadable) + ); + } + + #[cfg(unix)] + #[test] + fn non_regular_package_entries_fail_closed() { + let tmp = tempfile::tempdir().unwrap(); + let _socket = std::os::unix::net::UnixListener::bind(tmp.path().join("socket")).unwrap(); + assert_eq!( + compute_package_digest(tmp.path()), + Err(PackageDigestError::Unreadable) + ); + } + + #[test] + fn bounded_read_rejects_growth_after_metadata_without_reading_the_whole_file() { + use std::io::Seek as _; + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("growing"); + fs::write(&path, b"ok").unwrap(); + let allowed = fs::metadata(&path).unwrap().len(); + fs::write(&path, b"grew beyond the remaining budget").unwrap(); + let mut file = fs::File::open(path).unwrap(); + assert_eq!( + read_package_file(&mut file, allowed), + Err(PackageDigestError::Oversized) + ); + assert_eq!(file.stream_position().unwrap(), allowed + 1); + } + + #[test] + fn bounded_read_accepts_exact_remaining_bytes() { + let mut file = tempfile::tempfile().unwrap(); + use std::io::{Seek as _, Write as _}; + file.write_all(b"exact").unwrap(); + file.rewind().unwrap(); + assert_eq!(read_package_file(&mut file, 5).unwrap(), b"exact"); + } +} diff --git a/crates/tui/src/skills/roots.rs b/crates/tui/src/skills/roots.rs index 576d61a93b..711ca60d03 100644 --- a/crates/tui/src/skills/roots.rs +++ b/crates/tui/src/skills/roots.rs @@ -296,20 +296,6 @@ impl SkillRootCatalog { "registry-cache", false, ); - } else { - // Match legacy fallback when HOME is unavailable. - push_descriptor( - &mut roots, - &mut precedence, - SkillRootKind::CodeWhaleGlobal, - SkillRootAccess::WritableOwned, - SkillScope::Global, - PathBuf::from("/tmp/codewhale/skills"), - true, - true, - "global-codewhale-fallback", - true, - ); } if let Some(configured) = configured_skills_dir { @@ -767,6 +753,22 @@ mod tests { std::fs::create_dir_all(path).unwrap(); } + #[test] + fn unavailable_home_has_no_ambient_global_or_cache_root() { + let tmp = TempDir::new().unwrap(); + let catalog = SkillRootCatalog::build(tmp.path(), None, None); + assert!( + catalog + .roots + .iter() + .all(|root| root.scope == SkillScope::Project) + ); + let owned = catalog.owned_writable_roots(); + assert_eq!(owned.len(), 1); + assert_eq!(owned[0].kind, SkillRootKind::CodeWhaleProject); + assert_eq!(owned[0].path, tmp.path().join(".codewhale/skills")); + } + #[test] fn runtime_compatible_preserves_historical_workspace_order() { let tmp = TempDir::new().unwrap(); diff --git a/crates/tui/src/skills/tests.rs b/crates/tui/src/skills/tests.rs index fd945ab3e5..26db55576a 100644 --- a/crates/tui/src/skills/tests.rs +++ b/crates/tui/src/skills/tests.rs @@ -1,11 +1,219 @@ use tempfile::TempDir; +#[test] +fn frontmatter_handles_bom_exact_fences_and_plain_continuations() { + let content = "\u{feff}---\r\nname: demo\r\ndescription: Deploy apps --- safely\r\n for X or Y: really\r\ninvocation: user\r\n---\r\n# Body\r\n"; + let (metadata, body) = super::parse_frontmatter(content).unwrap().unwrap(); + assert_eq!(metadata["name"], "demo"); + assert_eq!( + metadata["description"], + "Deploy apps --- safely for X or Y: really" + ); + assert_eq!(metadata.len(), 3, "continuation text is not a new key"); + assert_eq!(body.trim(), "# Body"); + assert!( + super::parse_frontmatter("---not a fence\n# Body") + .unwrap() + .is_none() + ); + assert!(super::parse_frontmatter("---\nname: demo\ndescription: inline --- only").is_err()); +} + +#[test] +fn frontmatter_block_indentation_is_character_safe_and_keeps_nested_fences() { + for indicator in ["|", ">"] { + let content = format!( + "---\nname: demo\ndescription: {indicator}\n first\n\u{3000}wide\n \u{a0}mixed\n ---\n nested\n---\nbody" + ); + let (metadata, body) = super::parse_frontmatter(&content).unwrap().unwrap(); + let expected = if indicator == "|" { + "first\nwide\nmixed\n---\n nested" + } else { + "first wide mixed --- nested" + }; + assert_eq!(metadata["description"], expected); + assert_eq!(body.trim(), "body"); + } +} + +#[test] +fn unavailable_home_fallbacks_are_fresh_nonexistent_paths() { + let first = super::unavailable_home_root(); + let second = super::unavailable_home_root(); + assert_ne!(first, second); + assert!(!first.exists()); + assert!(!second.exists()); + assert!(first.starts_with(std::env::temp_dir())); + assert_ne!(first, std::path::Path::new("/tmp/codewhale")); +} + fn create_skill_dir(tmpdir: &TempDir, skill_name: &str, skill_content: &str) { let skill_dir = tmpdir.path().join("skills").join(skill_name); std::fs::create_dir_all(&skill_dir).unwrap(); std::fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap(); } +#[test] +fn unicode_skill_identity_preserves_bodies_and_legacy_activation() { + let tmp = TempDir::new().unwrap(); + let names = [ + "技能", + "分析", + "PDF阅读", + "PDF签名", + "skill", + "pdf", + "Café", + "Cafe\u{301}", + ]; + for (index, raw) in names.iter().enumerate() { + create_skill_dir( + &tmp, + &format!("source-{index}"), + &format!("---\nname: {raw}\ndescription: identity fixture\n---\nbody {index}"), + ); + } + let registry = super::SkillRegistry::discover(&tmp.path().join("skills")); + assert_eq!( + registry.len(), + names.len(), + "colliding old slugs must not hide bodies" + ); + for (index, raw) in names.iter().enumerate() { + let skill = registry.get(raw).unwrap(); + assert_eq!(skill.body, format!("body {index}")); + assert!(super::is_valid_skill_name(&skill.name)); + assert_eq!(registry.get(&skill.name).unwrap().body, skill.body); + assert_eq!( + super::normalize_skill_name_for_lookup(&skill.name), + skill.name + ); + assert!(skill.path.ends_with(format!("source-{index}/SKILL.md"))); + } + assert_eq!(registry.get("skill").unwrap().name, "skill"); + assert_eq!(registry.get("pdf阅读").unwrap().body, "body 2"); + assert_ne!( + registry.get("Café").unwrap().name, + registry.get("Cafe\u{301}").unwrap().name + ); + let long = format!("{}技能", "a".repeat(100)); + assert_eq!(super::normalize_skill_name_for_lookup(&long).len(), 64); + assert!(registry.warnings().is_empty(), "{:?}", registry.warnings()); + let mut malformed = registry.get("技能").unwrap().clone(); + malformed.name = "bad\u{1}技能".to_string(); + let mut validation = super::SkillRegistry::default(); + validation.normalize_skill_name(&mut malformed, std::path::Path::new("SKILL.md")); + assert_eq!( + validation.warnings().len(), + 1, + "control bytes remain invalid" + ); + + let path = tmp.path().join("skills_state.toml"); + let original = b"disabled = [\"skill\", \"pdf\"]\n"; + std::fs::write(&path, original).unwrap(); + let filtered = registry + .clone() + .into_enabled_with_state(crate::skill_state::SkillStateStore::load_from(path.clone())); + assert_eq!(filtered.len(), 2); + assert!(filtered.get("技能").is_none()); + assert_eq!(std::fs::read(&path).unwrap(), original); + let mut state = crate::skill_state::SkillStateStore::load_from(path.clone()).unwrap(); + state + .set_enabled(®istry.get("技能").unwrap().name, true) + .unwrap(); + state.set_enabled("skill", true).unwrap(); + let filtered = registry.into_enabled_with_state(Ok(state)); + assert_eq!(filtered.get("技能").unwrap().body, "body 0"); + assert_eq!(filtered.get("skill").unwrap().body, "body 4"); + for hidden in ["分析", "PDF阅读", "PDF签名", "pdf"] { + assert!(filtered.get(hidden).is_none(), "must not revive {hidden}"); + } + let rendered = super::render_skills_block(&filtered, "en", tmp.path()).unwrap(); + assert!(rendered.contains(&filtered.get("技能").unwrap().name)); + assert!(!rendered.contains("body 1")); +} + +#[test] +fn unicode_plugin_identity_keeps_exact_namespaces_and_reviewed_metadata() { + let tmp = TempDir::new().unwrap(); + let config = crate::plugins::discovery::DiscoveryConfig { + workspace: tmp.path().join("workspace"), + user_plugins_dir: tmp.path().join("plugins"), + workspace_plugins_dir: tmp.path().join("workspace-plugins"), + builtin_plugin_dirs: Vec::new(), + state_path: tmp.path().join("plugin-state/state.json"), + }; + for namespace in ["team.plugin", "team-plugin"] { + let root = config.user_plugins_dir.join(namespace); + std::fs::create_dir_all(&root).unwrap(); + std::fs::write( + root.join("plugin.json"), + serde_json::json!({ + "$schema": "https://agent-plugins.org/schemas/plugin.json", + "name": namespace, + "version": "1.0.0" + }) + .to_string(), + ) + .unwrap(); + for name in ["技能", "分析", "skill"] { + write_skill( + &root.join("skills"), + name, + "reviewed identity", + &format!("{namespace} {name}"), + ); + } + } + let mut plugins = crate::plugins::discovery::discover_with_config(&config); + assert!(plugins.validation_is_clean(), "{:?}", plugins.diagnostics()); + let mut registry = super::SkillRegistry::default(); + super::merge_active_plugin_skills(&mut registry, &plugins); + assert!(registry.is_empty()); + for namespace in ["team.plugin", "team-plugin"] { + plugins.trust(namespace).unwrap(); + plugins.enable(namespace).unwrap_or_else(|error| { + panic!("{error}: {:?}", plugins.get(namespace).unwrap().diagnostics) + }); + let plugin = plugins.get(namespace).unwrap(); + let unicode = plugin + .skill_snapshots + .iter() + .find(|s| s.body == format!("{namespace} 技能")) + .unwrap(); + assert_eq!(unicode.legacy_activation_name.as_deref(), Some("skill")); + assert!(!unicode.source_hash.is_empty()); + } + super::merge_active_plugin_skills(&mut registry, &plugins); + assert_eq!(registry.len(), 6); + let dotted = registry.get("TEAM.PLUGIN:技能").unwrap(); + assert_eq!(dotted.body, "team.plugin 技能"); + assert_eq!( + dotted.legacy_activation_name.as_deref(), + Some("team.plugin:skill") + ); + assert_eq!( + registry.get("TEAM-PLUGIN:技能").unwrap().body, + "team-plugin 技能" + ); + assert!(registry.get("team_plugin:技能").is_none()); + let path = tmp.path().join("skills_state.toml"); + std::fs::write(&path, "disabled = [\"team.plugin:skill\"]\n").unwrap(); + let filtered = registry + .clone() + .into_enabled_with_state(crate::skill_state::SkillStateStore::load_from(path)); + assert!(filtered.get("team.plugin:技能").is_none()); + assert!(filtered.get("team-plugin:技能").is_some()); + registry + .skills + .retain(|skill| !skill.name.starts_with("team.plugin:")); + assert!( + registry.get("team.plugin:技能").is_none(), + "absent dotted namespace must not select dashed namespace" + ); +} + #[test] fn discovery_metrics_reset_and_snapshot_are_exact() { super::reset_discovery_metrics(); @@ -321,6 +529,7 @@ fn render_skills_block_shortens_summary_before_trigger() { let summary = "s".repeat(300); for i in 0..120 { registry.skills.push(super::Skill { + legacy_activation_name: None, name: format!("skill-{i:03}"), description: format!("{summary} Use when: the user asks for widget {i}."), localized_descriptions: std::collections::HashMap::new(), @@ -371,6 +580,7 @@ fn render_skills_block_holds_budget_with_five_digit_omission_counts() { let mut registry = super::SkillRegistry::default(); for i in 0..15_000 { registry.skills.push(super::Skill { + legacy_activation_name: None, name: format!("skill-{i:05}"), description: "x".to_string(), localized_descriptions: std::collections::HashMap::new(), @@ -412,6 +622,7 @@ fn explicit_only_skills_do_not_reduce_ambient_index_capacity() { let mut registry = super::SkillRegistry::default(); for i in 0..6 { registry.skills.push(super::Skill { + legacy_activation_name: None, name: format!("visible-{i:03}"), description: "x".repeat(246), localized_descriptions: std::collections::HashMap::new(), @@ -430,6 +641,7 @@ fn explicit_only_skills_do_not_reduce_ambient_index_capacity() { let mut with_explicit_only = registry.clone(); for i in 0..10_000 { with_explicit_only.skills.push(super::Skill { + legacy_activation_name: None, name: format!("explicit-{i:05}"), description: String::new(), localized_descriptions: std::collections::HashMap::new(), @@ -451,6 +663,7 @@ fn render_skills_block_preserves_registry_precedence_under_prompt_budget() { let tmpdir = TempDir::new().unwrap(); let mut registry = super::SkillRegistry::default(); registry.skills.push(super::Skill { + legacy_activation_name: None, name: "workspace-priority".to_string(), description: "must survive truncation".to_string(), localized_descriptions: std::collections::HashMap::new(), @@ -469,6 +682,7 @@ fn render_skills_block_preserves_registry_precedence_under_prompt_budget() { let big_desc = "y".repeat(super::MAX_SKILL_DESCRIPTION_CHARS - 20); for i in 0..200 { registry.skills.push(super::Skill { + legacy_activation_name: None, name: format!("aaa-global-{i:03}"), description: big_desc.clone(), localized_descriptions: std::collections::HashMap::new(), @@ -588,6 +802,7 @@ fn description_for_locale_matches_exact_then_primary_then_falls_back() { localized.insert("zh".to_string(), "中文描述".to_string()); localized.insert("ja".to_string(), "日本語の説明".to_string()); let skill = super::Skill { + legacy_activation_name: None, name: "demo".to_string(), description: "English description".to_string(), localized_descriptions: localized, @@ -622,6 +837,7 @@ fn description_for_locale_uses_exact_traditional_key_when_authored() { localized.insert("zh".to_string(), "简体描述".to_string()); localized.insert("zh-hant".to_string(), "繁體描述".to_string()); let skill = super::Skill { + legacy_activation_name: None, name: "demo".to_string(), description: "English".to_string(), localized_descriptions: localized, @@ -641,6 +857,7 @@ fn description_for_locale_uses_exact_traditional_key_when_authored() { #[test] fn description_for_locale_uses_default_when_no_localized_variants() { let skill = super::Skill { + legacy_activation_name: None, name: "demo".to_string(), description: "only english".to_string(), localized_descriptions: std::collections::HashMap::new(), @@ -659,6 +876,7 @@ fn render_skills_block_selects_description_by_locale() { let mut localized = std::collections::HashMap::new(); localized.insert("zh".to_string(), "压缩日志的技能".to_string()); registry.skills.push(super::Skill { + legacy_activation_name: None, name: "compress".to_string(), description: "Compress logs to save space".to_string(), localized_descriptions: localized, @@ -1677,6 +1895,7 @@ fn plugin_skills_are_qualified_and_denied_until_trusted_and_enabled() { let mut fail_closed_input = registry.clone(); fail_closed_input.skills.push(super::Skill { + legacy_activation_name: None, name: "native-recovery".to_string(), description: "native recovery skill".to_string(), localized_descriptions: std::collections::HashMap::new(), @@ -2017,3 +2236,63 @@ fn project_skills_require_workspace_trust() { registry.warnings() ); } + +#[test] +fn hidden_and_backup_payload_changes_stale_the_trust_receipt() { + use super::audit::{self, SkillAuditMode, TrustState}; + use super::install::{INSTALLED_FROM_MARKER, TRUSTED_MARKER, write_trust_v2}; + use super::package_digest::compute_package_digest; + use std::fs; + let tmp = tempfile::tempdir().unwrap(); + let package = tmp.path().join(".codewhale/skills/demo"); + fs::create_dir_all(&package).unwrap(); + fs::write( + package.join("SKILL.md"), + "---\nname: demo\ndescription: test\n---\nbody", + ) + .unwrap(); + let payloads = [ + ".hidden", + ".hidden-dir/run.sh", + "script.bak", + "script.tmp", + "nested/.trusted", + ]; + for relative in payloads { + let path = package.join(relative); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, "safe").unwrap(); + } + let digest = compute_package_digest(&package).unwrap(); + write_trust_v2(&package, &digest).unwrap(); + let initial = audit::scan(tmp.path(), None, SkillAuditMode::OwnedOnly, None); + assert_eq!(initial.skills.len(), 1); + assert_eq!( + initial.skills[0].trust, + TrustState::TrustedForDigest(digest.clone()) + ); + for relative in payloads { + fs::write(package.join(relative), "evil").unwrap(); + assert_ne!( + compute_package_digest(&package).unwrap(), + digest, + "{relative}" + ); + let changed = audit::scan(tmp.path(), None, SkillAuditMode::OwnedOnly, None); + assert_eq!( + changed.skills[0].trust, + TrustState::TrustStale, + "{relative}" + ); + fs::write(package.join(relative), "safe").unwrap(); + } + // These root files are local bookkeeping, not executable payload. + for marker in [ + INSTALLED_FROM_MARKER, + TRUSTED_MARKER, + ".system-installed-version", + ] { + fs::write(package.join(marker), "local metadata").unwrap(); + assert_eq!(compute_package_digest(&package).unwrap(), digest); + } +} diff --git a/crates/tui/src/tools/skill.rs b/crates/tui/src/tools/skill.rs index df681c643a..c8358e5948 100644 --- a/crates/tui/src/tools/skill.rs +++ b/crates/tui/src/tools/skill.rs @@ -417,6 +417,7 @@ mod tests { let tmp = tempdir().unwrap(); let missing = tmp.path().join("delegate").join("SKILL.md"); let skill = Skill { + legacy_activation_name: None, name: "delegate".to_string(), description: "delegate work".to_string(), localized_descriptions: std::collections::HashMap::new(), @@ -472,6 +473,7 @@ mod tests { fs::write(&skill_path, "changed on disk").unwrap(); fs::write(tmp.path().join("companion.txt"), "changed companion").unwrap(); let skill = Skill { + legacy_activation_name: None, name: "demo:hello".to_string(), description: "hello".to_string(), localized_descriptions: std::collections::HashMap::new(), diff --git a/crates/tui/tests/integration/main.rs b/crates/tui/tests/integration/main.rs index 5ff4f3b1c1..a2f48a910b 100644 --- a/crates/tui/tests/integration/main.rs +++ b/crates/tui/tests/integration/main.rs @@ -15,6 +15,8 @@ mod config; #[path = "../../src/eval.rs"] mod eval; +#[path = "../../src/skills/frontmatter.rs"] +mod frontmatter; #[path = "../../src/skills/install.rs"] #[allow(dead_code)] mod install; diff --git a/docs/SKILLS.md b/docs/SKILLS.md index 40b021eee5..e51bfff032 100644 --- a/docs/SKILLS.md +++ b/docs/SKILLS.md @@ -141,6 +141,37 @@ behavior. Canonical names win over aliases when a collision exists. Loading a skill reports its canonical invocation and aliases so receipts remain inspectable. +### Non-ASCII names and saved activation + +ASCII names keep their existing command spelling. A name containing non-ASCII +characters gets a stable ASCII ID: a shortened old slug plus 32 hexadecimal +SHA-256 digits, at most 64 characters per skill-name segment. The hash uses +trimmed UTF-8 with ASCII case folding; it does not transliterate or merge Unicode +normalization forms. Unqualified raw names and those IDs select the same body. +Package directories stay in place. Qualified lookup requires the declared canonical +namespace, with ASCII case folding and no punctuation folding: `Team.Plugin:技能` +cannot select a skill in `team-plugin`. + +Previously disabled lossy names such as `skill` or `pdf` continue to suppress +every corresponding renamed skill. Enabling one exact catalog ID enables only +that identity, including a literal ASCII skill named `skill`; it does not enable +its formerly colliding siblings. Toggle requests use the exact ID returned by +`GET /v1/skills`. Plugin bundle trust remains a separate gate. + +Activation still uses one `skills_state.toml` file and its `disabled` array. +Reserved `!codewhale-skill-state:1:*` entries preserve legacy veto history and +exact enable choices through older writers, using the same lock and atomic +write. Listing/discovery do not rewrite the file. Unknown versions or malformed +reserved entries are errors and are left untouched; existing recovery behavior +keeps native skills available but hides reviewed plugin skills when policy +cannot be read. + +This is **not simultaneous-version activation compatibility**. v0.10.0 readers +cannot enforce new per-identity disables, and their lossy or no-op toggles cannot +express every new choice. Upgrade every runtime sharing the state directory +before relying on consistent controls. Retaining marker strings through an old +write does not give that old binary the new identity semantics. + ### Starter-pack parity decisions The v0.9.2 parity audit in [#4698](https://github.com/Hmbown/CodeWhale/issues/4698) diff --git a/docs/zh_hans/SKILLS.md b/docs/zh_hans/SKILLS.md index 1b91d65656..3c6e063287 100644 --- a/docs/zh_hans/SKILLS.md +++ b/docs/zh_hans/SKILLS.md @@ -102,6 +102,16 @@ Codewhale 以两个紧凑层级呈现其随附 skills,这样 agentic 工作流 缺失或未知的 invocation 值保留历史 `model+user` 行为。发生冲突时规范名称优先于别名。加载 skill 会报告其规范调用与别名,让回执保持可检查。 +### 非 ASCII 名称与保存的启用状态 + +ASCII 名称保留现有命令拼写。包含非 ASCII 字符的名称会获得稳定的 ASCII ID:缩短后的旧 slug 加上 32 位十六进制 SHA-256 摘要,每个 skill 名称段最多 64 个字符。摘要使用去掉两端空白、仅对 ASCII 大小写折叠后的 UTF-8;不会音译,也不会合并不同的 Unicode 规范化形式。未限定命名空间的原始名称与 ID 指向同一正文,包目录不重命名。限定名称的查找必须使用已声明的规范命名空间,仅折叠 ASCII 大小写,不折叠标点:`Team.Plugin:技能` 不会选中 `team-plugin` 的 skill。 + +过去通过 `skill`、`pdf` 等有损名称禁用的 skill,在改名后仍保持禁用。显式启用一个目录 ID 只影响该身份;即使它是字面名称为 `skill` 的 ASCII skill,也不会启用原先发生冲突的其他 skill。切换请求必须使用 `GET /v1/skills` 返回的精确 ID。插件包信任仍是独立的门槛。 + +启用状态继续使用同一个 `skills_state.toml` 文件及其 `disabled` 数组。保留的 `!codewhale-skill-state:1:*` 条目在旧版本写入时保留历史禁用和精确启用选择,并复用原有锁与原子写入。列出或发现 skill 不会改写文件。未知版本或格式错误的保留条目会报错,原始内容不会被改写;现有恢复行为在无法读取策略时仍保留原生 skill,但隐藏已审核的插件 skill。 + +这**不代表不同版本同时运行时的启用状态兼容性**。v0.10.0 读取器无法执行新身份的独立禁用,其有损或无操作切换也无法表达所有新选择。在依赖一致的控制前,请升级共享该状态目录的所有运行时。旧写入器保留标记字符串,并不意味着旧程序理解新身份语义。 + ### 入门包对等决策 [#4698](https://github.com/Hmbown/CodeWhale/issues/4698) 中的 v0.9.2 对等审计比较了五个 `xai-grok-memory` / `xai-grok-shell` 参考 skills 与实际的 Codewhale 包。这是一张决策矩阵,不是复制参考文本或宣传不受支持工具的请求: