From 7054cdb08c9fee2f7256edd483b455200f3632ef Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 29 Sep 2026 04:41:59 -0700 Subject: [PATCH 1/4] chore(skills): checkpoint bounded package input repair Scope: shared skill frontmatter boundaries, remote skill metadata and digest coverage, unavailable-home discovery, and archive executable modes in the seven declared paths. Skill slug migration is deferred to preserve exact-name disabled state. No implementation edits or gates run at this checkpoint. From 9cee704e385b60382f729307cbbcf029985bbb57 Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 29 Sep 2026 04:57:36 -0700 Subject: [PATCH 2/4] wip(skills): checkpoint package input repairs before parser extraction Seven declared paths are preserved for review. Formatting and diff whitespace checks passed; production, Rust, npm and web gates have not run. Review identified pending digest fail-closed/read-cap corrections, no-home cache sync guard, and the integration harness include boundary. This is an unqualified intermediate checkpoint, not a ready-to-land change. Unicode slug migration remains deferred. --- crates/tui/src/plugins/install/tarball.rs | 19 +- crates/tui/src/plugins/install/tests.rs | 38 ++++ crates/tui/src/skills/install.rs | 217 +++++++++++++++++----- crates/tui/src/skills/mod.rs | 59 ++++-- crates/tui/src/skills/package_digest.rs | 75 +++++++- crates/tui/src/skills/roots.rs | 30 +-- crates/tui/src/skills/tests.rs | 47 +++++ 7 files changed, 397 insertions(+), 88 deletions(-) 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/skills/install.rs b/crates/tui/src/skills/install.rs index aa342289ab..b7a17918b7 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 @@ -62,7 +62,7 @@ fn reqwest_client() -> reqwest::Client { /// 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"), + || super::unavailable_home_root().join("cache").join("skills"), |p| p.join(".codewhale").join("cache").join("skills"), ) } @@ -87,6 +87,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 +496,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 +1350,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 +1474,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 +1508,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 +1629,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::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 +1689,95 @@ 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() { + use crate::skills::audit::{self, SkillAuditMode, TrustState}; + + 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 = crate::skills::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!( + crate::skills::package_digest::compute_package_digest(&staged.staged_path).unwrap(), + digest + ); + fs::rename(&staged.staged_path, skills.join("demo")).unwrap(); + let audit = audit::scan(tmp.path(), None, SkillAuditMode::OwnedOnly, None); + assert_eq!(audit.skills.len(), 1); + assert_eq!(audit.skills[0].trust, TrustState::Untrusted); + } + + #[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 +1968,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..5b0b1648a6 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -192,11 +192,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 { @@ -774,20 +783,29 @@ pub(crate) type Frontmatter<'a> = (HashMap, &'a str); pub(crate) fn parse_frontmatter( content: &str, ) -> std::result::Result>, String> { - if !content.trim_start().starts_with("---") { + 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 start = content - .find("---") - .ok_or_else(|| "missing frontmatter opening delimiter".to_string())?; - let rest = &content[start + 3..]; + let rest = &content[opening.len()..]; + let mut offset = 0; let end = rest - .find("---") + .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() { @@ -812,7 +830,7 @@ pub(crate) fn parse_frontmatter( "clip" }; // Determine the base indentation from the key line - let base_indent = raw.len() - raw.trim_start().len(); + let base_indent = indentation(raw); let mut block_lines: Vec<&str> = Vec::new(); let mut content_indent: Option = None; i += 1; @@ -824,7 +842,7 @@ pub(crate) fn parse_frontmatter( i += 1; continue; } - let line_indent = raw_line.len() - raw_line.trim_start().len(); + 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 @@ -848,9 +866,10 @@ pub(crate) fn parse_frontmatter( if raw.is_empty() { "" } else { - let indent = raw.len() - raw.trim_start().len(); + let indent = indentation(raw); let strip = std::cmp::min(indent, content_indent); - &raw[strip..] + let byte = raw.char_indices().nth(strip).map_or(raw.len(), |(i, _)| i); + &raw[byte..] } }) .collect(); @@ -933,8 +952,24 @@ pub(crate) fn parse_frontmatter( } _ => value, }; - metadata.insert(key.trim().to_ascii_lowercase(), unquoted.to_string()); 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; diff --git a/crates/tui/src/skills/package_digest.rs b/crates/tui/src/skills/package_digest.rs index d13d2bfc15..0703924225 100644 --- a/crates/tui/src/skills/package_digest.rs +++ b/crates/tui/src/skills/package_digest.rs @@ -9,7 +9,7 @@ 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, is_reserved_root_metadata}; pub const PACKAGE_DIGEST_MAX_BYTES: u64 = DEFAULT_MAX_SIZE_BYTES; pub const PACKAGE_DIGEST_MAX_FILES: usize = 256; @@ -106,13 +106,9 @@ fn walk( 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('.') - { + // 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)) { continue; } @@ -150,3 +146,66 @@ fn hex_digest(bytes: impl AsRef<[u8]>) -> String { } out } + +#[cfg(test)] +mod tests { + use super::*; + use crate::skills::audit::{self, SkillAuditMode, TrustState}; + use crate::skills::install::{INSTALLED_FROM_MARKER, TRUSTED_MARKER, write_trust_v2}; + + #[test] + fn hidden_and_backup_payload_changes_stale_the_trust_receipt() { + 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/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..329d7f8d7b 100644 --- a/crates/tui/src/skills/tests.rs +++ b/crates/tui/src/skills/tests.rs @@ -1,5 +1,52 @@ 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(); From 9d90b9713d4c0ed81021bdc2e6202333c1c43a2e Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 29 Sep 2026 05:30:29 -0700 Subject: [PATCH 3/4] fix(skills): preserve package identity through install and review Reuse one frontmatter reader for discovery, profiles and install. Preserve owner executable intent in archives, keep local receipts out of package input, include all payload files and relative paths in bounded digests, and refuse global cache work when a home directory is unavailable. Validation: production cargo check passed; 330 focused Rust tests and 16 CLI install integration tests passed, 0 failed/ignored; npm test 636 passed, 0 failed; npm run check:web passed. Linux exercises the non-UTF8 filename fixture because APFS refuses creating it. Hosted CI pending. Signed-off-by: Hunter B --- crates/tui/src/commands/contract.rs | 126 ++++++------ crates/tui/src/skills/frontmatter.rs | 218 +++++++++++++++++++++ crates/tui/src/skills/install.rs | 20 +- crates/tui/src/skills/mod.rs | 216 +-------------------- crates/tui/src/skills/package_digest.rs | 248 ++++++++++++++++++------ crates/tui/src/skills/tests.rs | 60 ++++++ crates/tui/tests/integration/main.rs | 2 + 7 files changed, 550 insertions(+), 340 deletions(-) create mode 100644 crates/tui/src/skills/frontmatter.rs 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/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 b7a17918b7..9ab52ef066 100644 --- a/crates/tui/src/skills/install.rs +++ b/crates/tui/src/skills/install.rs @@ -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( - || super::unavailable_home_root().join("cache").join("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 @@ -1629,7 +1628,7 @@ 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 (metadata, _) = super::parse_frontmatter(content) + 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") @@ -1704,8 +1703,6 @@ mod tests { #[test] fn remote_stage_discards_forged_root_receipts_and_keeps_hidden_payloads() { - use crate::skills::audit::{self, SkillAuditMode, TrustState}; - let tmp = tempfile::tempdir().unwrap(); let skills = tmp.path().join(".codewhale/skills"); let body = b"---\nname: demo\ndescription: test\n---\nbody"; @@ -1714,7 +1711,7 @@ mod tests { 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 = crate::skills::package_digest::compute_package_digest(&baseline).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), @@ -1735,13 +1732,10 @@ mod tests { b"payload" ); assert_eq!( - crate::skills::package_digest::compute_package_digest(&staged.staged_path).unwrap(), + super::super::package_digest::compute_package_digest(&staged.staged_path).unwrap(), digest ); fs::rename(&staged.staged_path, skills.join("demo")).unwrap(); - let audit = audit::scan(tmp.path(), None, SkillAuditMode::OwnedOnly, None); - assert_eq!(audit.skills.len(), 1); - assert_eq!(audit.skills[0].trust, TrustState::Untrusted); } #[cfg(unix)] diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 5b0b1648a6..52ec8983f7 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; @@ -771,220 +773,6 @@ 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> { - 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("- ") -} - pub(crate) fn normalize_skill_name_for_lookup(name: &str) -> String { if let Some((plugin, skill)) = name.trim().split_once(':') && !plugin.is_empty() diff --git a/crates/tui/src/skills/package_digest.rs b/crates/tui/src/skills/package_digest.rs index 0703924225..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, is_reserved_root_metadata}; +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,16 +93,18 @@ 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() { @@ -109,7 +114,20 @@ fn walk( // 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)) { - continue; + 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() { @@ -117,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,62 +189,155 @@ fn hex_digest(bytes: impl AsRef<[u8]>) -> String { #[cfg(test)] mod tests { use super::*; - use crate::skills::audit::{self, SkillAuditMode, TrustState}; - use crate::skills::install::{INSTALLED_FROM_MARKER, TRUSTED_MARKER, write_trust_v2}; #[test] - fn hidden_and_backup_payload_changes_stale_the_trust_receipt() { + 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 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", + 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 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); + 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!( - initial.skills[0].trust, - TrustState::TrustedForDigest(digest.clone()) + compute_package_digest(&root_link), + Err(PackageDigestError::SymlinkPresent) ); - 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. + } + + #[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", ] { - fs::write(package.join(marker), "local metadata").unwrap(); - assert_eq!(compute_package_digest(&package).unwrap(), digest); + 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/tests.rs b/crates/tui/src/skills/tests.rs index 329d7f8d7b..8725f3b5de 100644 --- a/crates/tui/src/skills/tests.rs +++ b/crates/tui/src/skills/tests.rs @@ -2064,3 +2064,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/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; From 43452952be39161b438b584623100d480ed61934 Mon Sep 17 00:00:00 2001 From: Hunter B Date: Tue, 29 Sep 2026 07:35:36 -0700 Subject: [PATCH 4/4] fix(skills): preserve Unicode identities and exact activation choices Give non-ASCII skill names stable bounded ASCII identities while retaining legacy disable vetoes and separate plugin trust. Keep exact qualified namespaces distinct, and let supported Unicode names pass reviewed snapshot validation without labeling intentional normalization as invalid. Validation: production cargo check passed; 22 selected Rust tests passed (14 unchanged runtime tests plus 8 TUI tests on the final source), 0 failed or ignored in the qualified groups. npm test 636 passed and check:web passed on unchanged JavaScript/docs. Earlier fixture failures remain recorded; final plugin trust/enable regression passed. Hosted CI pending. Signed-off-by: Hunter B --- crates/runtime/src/skill_state.rs | 219 ++++++++++++++++++++++++++-- crates/tui/src/plugins/discovery.rs | 1 + crates/tui/src/plugins/types.rs | 1 + crates/tui/src/runtime_api.rs | 3 +- crates/tui/src/runtime_api/tests.rs | 138 ++++++++++++++++++ crates/tui/src/skills/mod.rs | 86 +++++++++-- crates/tui/src/skills/tests.rs | 172 ++++++++++++++++++++++ crates/tui/src/tools/skill.rs | 2 + docs/SKILLS.md | 31 ++++ docs/zh_hans/SKILLS.md | 10 ++ 10 files changed, 640 insertions(+), 23 deletions(-) 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/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/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/mod.rs b/crates/tui/src/skills/mod.rs index 52ec8983f7..694440848d 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -252,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 @@ -592,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 + )); + } } } @@ -642,6 +656,7 @@ impl SkillRegistry { return Ok(Skill { name, + legacy_activation_name: None, description, localized_descriptions, invocation, @@ -668,6 +683,7 @@ impl SkillRegistry { Ok(Skill { name, + legacy_activation_name: None, description: String::new(), localized_descriptions: HashMap::new(), invocation: SkillInvocation::ModelAndUser, @@ -695,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() @@ -725,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 @@ -774,21 +816,38 @@ fn is_valid_skill_name(name: &str) -> bool { } pub(crate) fn normalize_skill_name_for_lookup(name: &str) -> String { + normalize_qualified_skill_name(name, normalize_skill_name_segment) +} + +fn legacy_skill_name_for_lookup(name: &str) -> String { + normalize_qualified_skill_name(name, legacy_skill_name_segment) +} + +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; @@ -1143,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/tests.rs b/crates/tui/src/skills/tests.rs index 8725f3b5de..26db55576a 100644 --- a/crates/tui/src/skills/tests.rs +++ b/crates/tui/src/skills/tests.rs @@ -53,6 +53,167 @@ fn create_skill_dir(tmpdir: &TempDir, skill_name: &str, skill_content: &str) { 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(); @@ -368,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(), @@ -418,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(), @@ -459,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(), @@ -477,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(), @@ -498,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(), @@ -516,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(), @@ -635,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, @@ -669,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, @@ -688,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(), @@ -706,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, @@ -1724,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(), 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/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 包。这是一张决策矩阵，不是复制参考文本或宣传不受支持工具的请求：