diff --git a/crates/command-contract/src/facets.rs b/crates/command-contract/src/facets.rs index d075cef9bd..c5efbf61cf 100644 --- a/crates/command-contract/src/facets.rs +++ b/crates/command-contract/src/facets.rs @@ -205,3 +205,274 @@ pub trait CommandProjectContext { /// `/goal` projection: visible and effective goal state. fn goal_state(&self) -> ProjectGoalState; } + +// --------------------------------------------------------------------------- +// Skill group (FEAT-022 D1) +// --------------------------------------------------------------------------- + +/// Source provenance of a discovered skill (native file vs reviewed plugin snapshot). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillSourceKind { + Native, + Plugin { + plugin_name: String, + plugin_id: String, + }, +} + +/// Curated product tier for bundled (shipped) skills. +/// +/// The canonical name→tier classification stays in the TUI host +/// (`crate::skills::system::bundled_skill_tier`); the portable projection +/// carries the resolved tier so the handler can render the curated listing +/// without duplicating the canonical bundle list. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillBundledTier { + CoreAgentic, + FormatTooling, +} + +impl SkillBundledTier { + /// Product-facing tier heading used by the `/skills` listing. + #[must_use] + pub fn heading(self) -> &'static str { + match self { + Self::CoreAgentic => "Core agentic", + Self::FormatTooling => "Format & tooling", + } + } +} + +/// One discovered skill entry (portable). +/// +/// The body is intentionally excluded: activation and review receive body +/// text through their own delegates (`SkillActivationOutcome`/`ReviewOutcome`); +/// listing and inspect render name, description, source, and path only (D1 +/// exact-minimum). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillEntry { + pub name: String, + pub description: String, + pub source: SkillSourceKind, + /// Native skills carry their on-disk path (inspect output). + pub path: Option, + /// Bundled catalog tier; `None` for user/compatible skills. + pub bundled_tier: Option, +} + +/// Portable projection of the host skill registry (discovery, D1). +/// +/// Carries every value the `/skills` and `/skill` handlers render: workspace +/// and configured skills dir displays, discovery mode label, searched +/// directories, entries, warnings, and the enabled-skill total. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillRegistryProjection { + pub workspace: String, + pub skills_dir: String, + pub mode_label: String, + pub dirs: Vec, + pub entries: Vec, + pub warnings: Vec, + pub total: usize, +} + +/// Target scope for skill mutations (`/skill install|update|uninstall|trust`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillTargetScope { + Project, + Global, +} + +/// Portable mutation outcome mirroring the host receipt variants. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillMutationOutcome { + Installed, + Updated, + NoChange, + Removed, + Trusted, + Imported, + AlreadyPresent, + NeedsApproval(String), + NetworkDenied(String), +} + +/// Synchronous portable receipt for a skill mutation (FEAT-020 D11 mirror): +/// the host owns the async network bridge; the handler renders the receipt +/// byte-identically from these values. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillMutationReceipt { + pub name: String, + pub safe_target_path: String, + pub outcome: SkillMutationOutcome, +} + +/// One curated remote registry entry (`/skills --remote`). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RemoteSkillEntry { + pub name: String, + pub description: Option, + pub source: String, +} + +/// Remote registry fetch outcome (`/skills --remote`, suggest source). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RemoteRegistryOutcome { + Loaded { entries: Vec }, + NeedsApproval(String), + Denied(String), +} + +/// Remote recommendation for `/skills suggest `. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillRecommendation { + pub name: String, + pub description: Option, + pub matched_terms: Vec, +} + +/// Per-skill outcome of `/skills sync`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillSyncEntry { + Downloaded { name: String, path: String }, + Fresh { name: String }, + Failed { name: String, reason: String }, + Denied { name: String, host: String }, + NeedsApproval { name: String, host: String }, +} + +/// Aggregate `/skills sync` outcome. +/// +/// Registry-level network-policy outcomes are carried as variants so the +/// portable handler composes the exact `needs_approval` / `denied` messages. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillSyncOutcome { + Done { + total: usize, + downloaded: usize, + fresh: usize, + failed: usize, + entries: Vec, + }, + RegistryNeedsApproval(String), + RegistryDenied(String), +} + +/// Successful skill activation data (host performs the side effects). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillActivationOutcome { + pub name: String, + pub description: String, +} + +/// Activation failures with the exact data the handler renders. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillActivationError { + NotFound { + requested: String, + available: Vec, + warnings: Vec, + }, + PluginRejected { + name: String, + reason: String, + }, +} + +/// `/review` outcome data (host performs the side effects). +/// +/// On success the baseline `/review` renders no message — it only emits the +/// `SendMessage` action — so `Ready` carries no payload (D1 exact-minimum). +/// Warnings are only rendered on the not-found path. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ReviewOutcome { + Ready, + NotFound { + skills_dir: String, + global_dir: String, + warnings: Vec, + }, +} + +/// One snapshot entry for `/restore` listings. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SnapshotEntry { + pub id: String, + pub label: String, + pub timestamp: i64, +} + +/// Host approval posture for the `/restore` trust gate (D4: no MODE_POLICY). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CommandApprovalState { + pub yolo: bool, + pub trust_mode: bool, +} + +/// Host skill data for the skills command group (FEAT-022 D1). +/// +/// Exposes the typed, exact-minimum operations the live skills handlers +/// consume: discovery (`/skills`), activation (`/skill`), synchronous +/// mutation receipts (`/skill install|update|uninstall|trust`), remote +/// registry + sync (`/skills --remote|sync|suggest`), review (`/review`), +/// and snapshot list/restore plus approval state (`/restore`). The host +/// adapter is the only place that touches `App`, `crate::plugins`, +/// `SnapshotRepo`, `crate::skills` services, config/network policy, and the +/// async runtime bridge. The shared FEAT-015 `CommandSkillsContext` is never +/// widened; active-skill reads use that facet, mutations flow through the +/// delegates here (D2). All results are contract-owned portable values; +/// implementation errors cross as safe text. `/skill` declares this facet +/// plus `CommandSkillsContext` for the baseline cache-refresh policy; +/// `/skills`, `/review`, and `/restore` declare exactly this facet. +pub trait CommandSkillGroupContext { + /// `/skills` discovery projection (workspace, skills dir, scan mode, + /// searched directories, plugin-provided skills, warnings). + fn skill_registry_projection(&self) -> SkillRegistryProjection; + /// `/skill` activation: host lookup, plugin-authority verification, and + /// active-skill/history side effects. `SendMessage` task composition is + /// handler-side. + fn activate_skill( + &mut self, + name: &str, + ) -> Result; + /// `/skill install` — synchronous portable receipt; host owns network/async. + fn install_skill( + &mut self, + scope: Option, + spec: &str, + ) -> Result; + /// `/skill update` — synchronous portable receipt; host owns network/async. + fn update_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result; + /// `/skill uninstall` — synchronous portable receipt. + fn uninstall_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result; + /// `/skill trust` — synchronous portable receipt. + fn trust_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result; + /// `/skills --remote` registry fetch (network policy host-side). + fn fetch_remote_registry(&mut self) -> Result; + /// `/skills suggest ` — host fetch + recommendation computation. + fn recommend_skills(&mut self, task: &str) -> Result, String>; + /// `/skills sync` — host registry sync (async bridge host-side). + fn sync_registry(&mut self) -> Result; + /// `/review` activation: host discovery + side effects (empty-target + /// validation and `SendMessage` composition are handler-side). + fn run_review(&mut self) -> Result; + /// `/restore` snapshot listing. + fn snapshot_list(&mut self, limit: usize) -> Result, String>; + /// `/restore `: host restores by snapshot id; handler composes the + /// exact success message from its list entry. + fn restore_snapshot(&mut self, id: &str) -> Result<(), String>; + /// `/restore` trust gate posture (yolo / trust_mode). + fn approval_state(&self) -> CommandApprovalState; +} diff --git a/crates/command-contract/src/handler.rs b/crates/command-contract/src/handler.rs index 7f2cd30b4a..592a297368 100644 --- a/crates/command-contract/src/handler.rs +++ b/crates/command-contract/src/handler.rs @@ -6,8 +6,9 @@ use crate::facets::{ CommandCostContext, CommandMediaContext, CommandModePolicyContext, CommandModelContext, - CommandPresentationContext, CommandProjectContext, CommandSessionContext, CommandSkillsContext, - CommandSystemPromptContext, CommandWorkspaceContext, + CommandPresentationContext, CommandProjectContext, CommandSessionContext, + CommandSkillGroupContext, CommandSkillsContext, CommandSystemPromptContext, + CommandWorkspaceContext, }; /// A command handler that is either argument-only or capability-scoped. #[derive(Clone, Copy)] @@ -28,6 +29,7 @@ pub struct CommandContexts<'a> { presentation: Option<&'a mut dyn CommandPresentationContext>, media: Option<&'a mut dyn CommandMediaContext>, project: Option<&'a mut dyn CommandProjectContext>, + skill_group: Option<&'a mut dyn CommandSkillGroupContext>, } /// Consumed envelope used when one handler needs several independent facets. @@ -42,6 +44,7 @@ pub struct ContextParts<'a> { pub presentation: Option<&'a mut dyn CommandPresentationContext>, pub media: Option<&'a mut dyn CommandMediaContext>, pub project: Option<&'a mut dyn CommandProjectContext>, + pub skill_group: Option<&'a mut dyn CommandSkillGroupContext>, } impl<'a> CommandContexts<'a> { @@ -57,6 +60,7 @@ impl<'a> CommandContexts<'a> { presentation: None, media: None, project: None, + skill_group: None, } } @@ -72,6 +76,7 @@ impl<'a> CommandContexts<'a> { presentation: self.presentation, media: self.media, project: self.project, + skill_group: self.skill_group, } } @@ -151,6 +156,14 @@ impl<'a> CommandContexts<'a> { ); self } + + pub fn with_skill_group(mut self, value: &'a mut dyn CommandSkillGroupContext) -> Self { + assert!( + self.skill_group.replace(value).is_none(), + "skill_group facet already set" + ); + self + } } impl Default for CommandContexts<'_> { diff --git a/crates/command-contract/src/tests.rs b/crates/command-contract/src/tests.rs index aae164a3ff..918f9b66f7 100644 --- a/crates/command-contract/src/tests.rs +++ b/crates/command-contract/src/tests.rs @@ -498,3 +498,453 @@ fn envelope_rejects_duplicate_project_slot_deterministically() { })); assert!(result.is_err(), "duplicate project slot must assert"); } + +// --------------------------------------------------------------------------- +// FEAT-022: skill-group facet (CommandSkillGroupContext) +// --------------------------------------------------------------------------- + +struct FakeSkillGroup { + projection: SkillRegistryProjection, + activation_result: Result, + receipt: SkillMutationReceipt, + remote: Result, + sync: Result, + review: Result, + snapshots: Vec, + restore_ok: bool, + approval: CommandApprovalState, +} + +impl FakeSkillGroup { + fn new() -> Self { + Self { + projection: SkillRegistryProjection { + workspace: "/ws".into(), + skills_dir: "/ws/.codewhale/skills".into(), + mode_label: "compatible".into(), + dirs: vec!["/ws/.codewhale/skills".into()], + entries: vec![SkillEntry { + name: "demo".into(), + description: "Demo skill".into(), + source: SkillSourceKind::Native, + path: Some("/ws/.codewhale/skills/demo/SKILL.md".into()), + bundled_tier: None, + }], + warnings: vec!["one warning".into()], + total: 1, + }, + activation_result: Ok(SkillActivationOutcome { + name: "demo".into(), + description: "Demo skill".into(), + }), + receipt: SkillMutationReceipt { + name: "demo".into(), + safe_target_path: "/ws/.codewhale/skills/demo".into(), + outcome: SkillMutationOutcome::Installed, + }, + remote: Ok(RemoteRegistryOutcome::Loaded { + entries: vec![RemoteSkillEntry { + name: "demo".into(), + description: Some("Remote demo".into()), + source: "github.com/acme/skills".into(), + }], + }), + sync: Ok(SkillSyncOutcome::Done { + total: 1, + downloaded: 1, + fresh: 0, + failed: 0, + entries: vec![SkillSyncEntry::Downloaded { + name: "demo".into(), + path: "/cache/demo".into(), + }], + }), + review: Ok(ReviewOutcome::Ready), + snapshots: vec![SnapshotEntry { + id: "abcdef123456".into(), + label: "pre-turn:1".into(), + timestamp: 1_700_000_000, + }], + restore_ok: true, + approval: CommandApprovalState { + yolo: true, + trust_mode: false, + }, + } + } +} + +impl CommandSkillGroupContext for FakeSkillGroup { + fn skill_registry_projection(&self) -> SkillRegistryProjection { + self.projection.clone() + } + + fn activate_skill( + &mut self, + _name: &str, + ) -> Result { + self.activation_result.clone() + } + + fn install_skill( + &mut self, + _scope: Option, + _spec: &str, + ) -> Result { + Ok(self.receipt.clone()) + } + + fn update_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + Ok(self.receipt.clone()) + } + + fn uninstall_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + Ok(self.receipt.clone()) + } + + fn trust_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + Ok(self.receipt.clone()) + } + + fn fetch_remote_registry(&mut self) -> Result { + self.remote.clone() + } + + fn recommend_skills(&mut self, task: &str) -> Result, String> { + Ok(vec![SkillRecommendation { + name: format!("rec-{task}"), + description: Some("Recommended".into()), + matched_terms: vec!["term".into()], + }]) + } + + fn sync_registry(&mut self) -> Result { + self.sync.clone() + } + + fn run_review(&mut self) -> Result { + self.review.clone() + } + + fn snapshot_list(&mut self, _limit: usize) -> Result, String> { + Ok(self.snapshots.clone()) + } + + fn restore_snapshot(&mut self, _id: &str) -> Result<(), String> { + if self.restore_ok { + Ok(()) + } else { + Err("Restore failed: boom".into()) + } + } + + fn approval_state(&self) -> CommandApprovalState { + self.approval + } +} + +#[test] +fn skill_group_facet_is_object_safe_and_typed() { + fn project(_: &dyn CommandSkillGroupContext) {} + project(&FakeSkillGroup::new()); + + let group = FakeSkillGroup::new(); + let projection = group.skill_registry_projection(); + assert_eq!(projection.total, 1); + assert_eq!(projection.entries[0].name, "demo"); + assert!(group.approval_state().yolo); +} + +#[test] +fn skill_registry_projection_preserves_semantic_values() { + let group = FakeSkillGroup::new(); + let projection = group.skill_registry_projection(); + assert_eq!(projection.workspace, "/ws"); + assert_eq!(projection.skills_dir, "/ws/.codewhale/skills"); + assert_eq!(projection.mode_label, "compatible"); + assert_eq!(projection.dirs, vec!["/ws/.codewhale/skills"]); + assert_eq!(projection.warnings, vec!["one warning"]); + assert_eq!(projection.entries.len(), 1); + let entry = &projection.entries[0]; + assert_eq!(entry.name, "demo"); + assert_eq!(entry.description, "Demo skill"); + assert_eq!(entry.source, SkillSourceKind::Native); + assert_eq!( + entry.path.as_deref(), + Some("/ws/.codewhale/skills/demo/SKILL.md") + ); + assert_eq!(entry.bundled_tier, None); +} + +#[test] +fn skill_bundled_tier_headings_are_stable() { + assert_eq!(SkillBundledTier::CoreAgentic.heading(), "Core agentic"); + assert_eq!( + SkillBundledTier::FormatTooling.heading(), + "Format & tooling" + ); +} + +#[test] +fn skill_mutation_receipt_preserves_outcome_variants() { + let installed = FakeSkillGroup::new().receipt; + assert_eq!(installed.name, "demo"); + assert_eq!(installed.outcome, SkillMutationOutcome::Installed); + + let denied = SkillMutationReceipt { + outcome: SkillMutationOutcome::NetworkDenied("acme.com".into()), + ..installed.clone() + }; + assert_eq!( + denied.outcome, + SkillMutationOutcome::NetworkDenied("acme.com".into()) + ); + + let approval = SkillMutationReceipt { + outcome: SkillMutationOutcome::NeedsApproval("acme.com".into()), + ..installed.clone() + }; + assert_eq!( + approval.outcome, + SkillMutationOutcome::NeedsApproval("acme.com".into()) + ); + + assert_ne!(installed.outcome, denied.outcome); + assert_ne!(installed.outcome, approval.outcome); + assert_ne!(denied.outcome, approval.outcome); +} + +#[test] +fn skill_source_kind_variants_are_distinguishable() { + let native = SkillSourceKind::Native; + let plugin = SkillSourceKind::Plugin { + plugin_name: "acme".into(), + plugin_id: "acme-1".into(), + }; + assert_ne!(native, plugin); + assert_eq!( + plugin, + SkillSourceKind::Plugin { + plugin_name: "acme".into(), + plugin_id: "acme-1".into(), + } + ); +} + +#[test] +fn remote_registry_outcome_variants_are_distinguishable() { + let loaded = RemoteRegistryOutcome::Loaded { + entries: vec![RemoteSkillEntry { + name: "demo".into(), + description: None, + source: "acme".into(), + }], + }; + let approval = RemoteRegistryOutcome::NeedsApproval("acme.com".into()); + let denied = RemoteRegistryOutcome::Denied("acme.com".into()); + assert_ne!(loaded, approval); + assert_ne!(loaded, denied); + assert_ne!(approval, denied); +} + +#[test] +fn skill_sync_outcome_preserves_all_entry_variants() { + let outcome = SkillSyncOutcome::Done { + total: 4, + downloaded: 1, + fresh: 1, + failed: 2, + entries: vec![ + SkillSyncEntry::Downloaded { + name: "a".into(), + path: "/cache/a".into(), + }, + SkillSyncEntry::Fresh { name: "b".into() }, + SkillSyncEntry::Failed { + name: "c".into(), + reason: "boom".into(), + }, + SkillSyncEntry::Denied { + name: "d".into(), + host: "acme.com".into(), + }, + SkillSyncEntry::NeedsApproval { + name: "e".into(), + host: "acme.com".into(), + }, + ], + }; + let SkillSyncOutcome::Done { + total, + downloaded, + fresh, + failed, + entries, + } = &outcome + else { + panic!("expected Done"); + }; + assert_eq!(*total, 4); + assert_eq!(*downloaded, 1); + assert_eq!(*fresh, 1); + assert_eq!(*failed, 2); + assert_eq!(entries.len(), 5); + assert!(matches!(entries[0], SkillSyncEntry::Downloaded { .. })); + assert!(matches!(entries[1], SkillSyncEntry::Fresh { .. })); + assert!(matches!(entries[2], SkillSyncEntry::Failed { .. })); + assert!(matches!(entries[3], SkillSyncEntry::Denied { .. })); + assert!(matches!(entries[4], SkillSyncEntry::NeedsApproval { .. })); +} + +#[test] +fn skill_sync_registry_policy_variants_are_distinguishable() { + let approval = SkillSyncOutcome::RegistryNeedsApproval("acme.com".into()); + let denied = SkillSyncOutcome::RegistryDenied("acme.com".into()); + assert_ne!(approval, denied); + assert!(matches!( + approval, + SkillSyncOutcome::RegistryNeedsApproval(host) if host == "acme.com" + )); + assert!(matches!( + denied, + SkillSyncOutcome::RegistryDenied(host) if host == "acme.com" + )); +} + +#[test] +fn skill_activation_error_variants_are_distinguishable() { + let mut group = FakeSkillGroup::new(); + group.activation_result = Err(SkillActivationError::NotFound { + requested: "missing".into(), + available: vec!["demo".into()], + warnings: vec![], + }); + let not_found = group.activate_skill("missing").unwrap_err(); + match ¬_found { + SkillActivationError::NotFound { + requested, + available, + .. + } => { + assert_eq!(requested, "missing"); + assert_eq!(available, &vec!["demo".to_string()]); + } + _ => panic!("expected NotFound"), + } + + let mut group = FakeSkillGroup::new(); + group.activation_result = Err(SkillActivationError::PluginRejected { + name: "plug".into(), + reason: "authority revoked".into(), + }); + let rejected = group.activate_skill("plug").unwrap_err(); + match rejected { + SkillActivationError::PluginRejected { name, reason } => { + assert_eq!(name, "plug"); + assert_eq!(reason, "authority revoked"); + } + _ => panic!("expected PluginRejected"), + } +} + +#[test] +fn review_outcome_variants_are_distinguishable() { + let mut group = FakeSkillGroup::new(); + group.review = Ok(ReviewOutcome::NotFound { + skills_dir: "/ws/skills".into(), + global_dir: "/home/u/.codewhale/skills".into(), + warnings: vec!["w".into()], + }); + let outcome = group.run_review().unwrap(); + match outcome { + ReviewOutcome::NotFound { + skills_dir, + global_dir, + warnings, + } => { + assert_eq!(skills_dir, "/ws/skills"); + assert_eq!(global_dir, "/home/u/.codewhale/skills"); + assert_eq!(warnings, vec!["w".to_string()]); + } + _ => panic!("expected NotFound"), + } +} + +#[test] +fn snapshot_and_approval_values_preserve_semantics() { + let mut group = FakeSkillGroup::new(); + let snapshots = group.snapshot_list(20).unwrap(); + assert_eq!(snapshots.len(), 1); + assert_eq!(snapshots[0].id, "abcdef123456"); + assert_eq!(snapshots[0].label, "pre-turn:1"); + assert_eq!(snapshots[0].timestamp, 1_700_000_000); + + let approval = group.approval_state(); + assert!(approval.yolo); + assert!(!approval.trust_mode); +} + +#[test] +fn skill_group_facet_transports_through_envelope_when_declared() { + let mut group = FakeSkillGroup::new(); + let parts = CommandContexts::empty() + .with_skill_group(&mut group) + .into_parts(); + assert!(parts.skill_group.is_some()); + assert!(parts.session.is_none()); + assert!(parts.project.is_none()); + + // /skill combines skill_group with SKILLS for baseline cache refreshes. + let mut skills = Skills; + let parts = CommandContexts::empty() + .with_skill_group(&mut group) + .with_skills(&mut skills) + .into_parts(); + assert!(parts.skill_group.is_some()); + assert!(parts.skills.is_some()); + assert!(parts.workspace.is_none()); +} + +#[test] +fn envelope_rejects_duplicate_skill_group_slot_deterministically() { + let mut a = FakeSkillGroup::new(); + let mut b = FakeSkillGroup::new(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + CommandContexts::empty() + .with_skill_group(&mut a) + .with_skill_group(&mut b); + })); + assert!(result.is_err(), "duplicate skill_group slot must assert"); +} + +/// Regression: the shared FEAT-015 `CommandSkillsContext` surface is unchanged +/// (getters + cache refresh only, no setter) and still transports through the +/// envelope alongside the new skill-group facet (D2). +#[test] +fn shared_skills_facet_surface_remains_read_only_and_transportable() { + let mut skills = Skills; + let active = skills.active_skill(); + assert_eq!(active, None); + assert_eq!(skills.active_skill_provenance(), None); + skills.refresh_skill_cache(); + + let mut group = FakeSkillGroup::new(); + let parts = CommandContexts::empty() + .with_skills(&mut skills) + .with_skill_group(&mut group) + .into_parts(); + assert!(parts.skills.is_some()); + assert!(parts.skill_group.is_some()); +} diff --git a/crates/tui/src/commands/contract.rs b/crates/tui/src/commands/contract.rs index 7ab4ad03ce..054c6c840d 100644 --- a/crates/tui/src/commands/contract.rs +++ b/crates/tui/src/commands/contract.rs @@ -32,10 +32,14 @@ use std::path::{Path, PathBuf}; use std::rc::Rc; use codewhale_command_contract::facets::{ - CommandCostContext, CommandMediaContext, CommandModePolicyContext, CommandModelContext, - CommandPresentationContext, CommandProjectContext, CommandSessionContext, CommandSkillsContext, - CommandSystemPromptContext, CommandWorkspaceContext, MediaAttachmentReceipt, ProjectGoalState, - ProjectGoalStatus, ProjectShareProjection, + CommandApprovalState, CommandCostContext, CommandMediaContext, CommandModePolicyContext, + CommandModelContext, CommandPresentationContext, CommandProjectContext, CommandSessionContext, + CommandSkillGroupContext, CommandSkillsContext, CommandSystemPromptContext, + CommandWorkspaceContext, MediaAttachmentReceipt, ProjectGoalState, ProjectGoalStatus, + ProjectShareProjection, RemoteRegistryOutcome, RemoteSkillEntry, ReviewOutcome, + SkillActivationError, SkillActivationOutcome, SkillBundledTier, SkillEntry, + SkillMutationOutcome, SkillMutationReceipt, SkillRecommendation, SkillRegistryProjection, + SkillSourceKind, SkillSyncEntry, SkillSyncOutcome, SkillTargetScope, SnapshotEntry, }; use codewhale_command_contract::handler::CommandContexts; #[cfg(test)] @@ -48,8 +52,10 @@ use codewhale_core::request::{Message, SystemPrompt}; use codewhale_execpolicy::ApprovalMode; use crate::localization::{MessageId, tr}; +use crate::network_policy::NetworkPolicy; use crate::pricing::CostCurrency; use crate::tui::app::{App, ReasoningEffort}; +use crate::tui::history::HistoryCell; // --------------------------------------------------------------------------- // Pending frontier projection (D4) @@ -64,9 +70,8 @@ use crate::tui::app::{App, ReasoningEffort}; /// (`scripts/check-command-migration-manifest.py`) reads this exact /// declaration by source regex and the Rust frontier tests assert it. #[allow(dead_code)] -pub(crate) const PENDING_GROUPS: &[&str] = &[ - "config", "core", "debug", "memory", "plugins", "session", "skills", -]; +pub(crate) const PENDING_GROUPS: &[&str] = + &["config", "core", "debug", "memory", "plugins", "session"]; // --------------------------------------------------------------------------- // Boundary-value mappings (D8) @@ -731,17 +736,639 @@ impl CommandProjectContext for ProjectAdapter<'_> { } } +// --------------------------------------------------------------------------- +// Skill group adapter (FEAT-022 D1/D3) +// --------------------------------------------------------------------------- + +/// The single new skills-specific host adapter. +/// +/// Owns every concrete skills touch: `App` skill state, `crate::skills` +/// discovery/mutation/install/recommend services, `crate::plugins` authority +/// verification, `SnapshotRepo`, config/network policy, and the async bridge +/// (`tokio::task::block_in_place`). Portable handlers never name these +/// subsystems (D3); every method returns portable contract values or safe +/// error text (D1). +pub(crate) struct SkillGroupAdapter<'a> { + host: SharedCommandHost<'a>, +} + +/// Bridge a sync slash-command handler back into the async ecosystem. +/// +/// We are on the TUI's thread, which is part of the multi-threaded runtime; +/// `block_in_place` + `Handle::current().block_on` bridges sync handlers back +/// into the async ecosystem. Mirrors `groups/skills/skills.rs::run_async`; +/// the legacy copy is removed in Phase 4 when the handlers are ported. +fn run_async(future: F) -> T +where + F: std::future::Future, +{ + tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(future)) +} + +/// Read the active config knobs for the installer (network policy, max size, +/// registry URL). `Config::load` is cheap and `App` does not carry a `Config`; +/// on parse failure we fall back to defaults so the user still gets a +/// network-gated install rather than a silent crash. Mirrors +/// `groups/skills/skills.rs::installer_settings`. +fn installer_settings() -> (NetworkPolicy, u64, String) { + let cfg = crate::config::Config::load(None, None).unwrap_or_default(); + let network = cfg + .network + .clone() + .map(|policy| policy.into_runtime()) + .unwrap_or_default(); + let skills_cfg = cfg.skills.as_ref(); + let max_size = skills_cfg + .and_then(|s| s.max_install_size_bytes) + .unwrap_or(crate::skills::install::DEFAULT_MAX_SIZE_BYTES); + let registry_url = skills_cfg + .and_then(|s| s.registry_url.clone()) + .unwrap_or_else(|| crate::skills::install::DEFAULT_REGISTRY_URL.to_string()); + (network, max_size, registry_url) +} + +/// 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`. +fn registry_fetch_error_hint(err: &anyhow::Error) -> Option<&'static str> { + let msg = format!("{err:#}").to_lowercase(); + if msg.contains("dns") + || msg.contains("name resolution") + || msg.contains("getaddrinfo") + || msg.contains("nodename nor servname") + { + Some( + "Hint: DNS lookup failed. Check internet/DNS connectivity, or override the registry URL in [skills] of ~/.codewhale/config.toml.", + ) + } else if msg.contains("connection refused") + || msg.contains("connection reset") + || msg.contains("connection aborted") + { + Some( + "Hint: connection refused/reset. The registry host may be unreachable from this network (corporate proxy, firewall, offline).", + ) + } else if msg.contains("tls") + || msg.contains("certificate") + || msg.contains("ssl") + || msg.contains("handshake") + { + Some( + "Hint: TLS handshake failed. The system trust store may be missing the registry's CA, or a TLS-intercepting proxy is rewriting the certificate.", + ) + } else if msg.contains(" 404") || msg.contains("not found") { + Some( + "Hint: registry URL returned 404. Verify the registry URL in [skills] of ~/.codewhale/config.toml.", + ) + } else if msg.contains(" 401") || msg.contains(" 403") || msg.contains("forbidden") { + Some( + "Hint: registry returned an auth error. The registry may require credentials or have been moved.", + ) + } else if msg.contains(" 429") || msg.contains("rate limit") || msg.contains("too many") { + Some("Hint: rate-limited by the registry. Try again in a moment.") + } else if msg.contains("timed out") || msg.contains("timeout") { + Some("Hint: request timed out. Network may be slow or the registry host may be down.") + } else { + None + } +} + +/// Append the actionable hint to a registry fetch error. Mirrors +/// `groups/skills/skills.rs::format_registry_error`. +fn format_registry_error(prefix: &str, err: &anyhow::Error) -> String { + let mut out = format!("{prefix}: {err:#}"); + if let Some(hint) = registry_fetch_error_hint(err) { + out.push_str("\n\n"); + out.push_str(hint); + } + out +} + +/// Discover the enabled visible skills for the current App state. +fn discover_visible(app: &App) -> crate::skills::SkillRegistry { + crate::skills::discover_for_workspace_and_dir_with_mode_and_plugins( + &app.workspace, + &app.skills_dir, + crate::skills::SkillDiscoveryMode::from_codewhale_only(app.skills_scan_codewhale_only), + Some(app.plugin_registry.as_ref()), + ) + .into_enabled() +} + +/// Map a TUI skill to its portable projection entry. +fn portable_skill_entry(skill: &crate::skills::Skill) -> SkillEntry { + let source = match &skill.source { + crate::skills::SkillSource::Native => SkillSourceKind::Native, + crate::skills::SkillSource::Plugin { + plugin_id, + plugin_name, + .. + } => SkillSourceKind::Plugin { + plugin_name: plugin_name.clone(), + plugin_id: plugin_id.clone(), + }, + }; + let path = match &skill.source { + crate::skills::SkillSource::Native => Some(skill.path.display().to_string()), + crate::skills::SkillSource::Plugin { .. } => None, + }; + let bundled_tier = crate::skills::bundled_skill_tier(&skill.name).map(|tier| match tier { + crate::skills::BundledSkillTier::CoreAgentic => SkillBundledTier::CoreAgentic, + crate::skills::BundledSkillTier::FormatTooling => SkillBundledTier::FormatTooling, + }); + SkillEntry { + name: skill.name.clone(), + description: skill.description.clone(), + source, + path, + bundled_tier, + } +} + +/// Map a TUI mutation receipt to its portable receipt. +fn portable_mutation_receipt( + receipt: &crate::skills::mutation::SkillMutationReceipt, +) -> SkillMutationReceipt { + use crate::skills::mutation::SkillMutationOutcome as TuiOutcome; + let outcome = match &receipt.outcome { + TuiOutcome::Installed => SkillMutationOutcome::Installed, + TuiOutcome::Updated => SkillMutationOutcome::Updated, + TuiOutcome::NoChange => SkillMutationOutcome::NoChange, + TuiOutcome::Removed => SkillMutationOutcome::Removed, + TuiOutcome::Trusted => SkillMutationOutcome::Trusted, + TuiOutcome::Imported => SkillMutationOutcome::Imported, + TuiOutcome::AlreadyPresent => SkillMutationOutcome::AlreadyPresent, + TuiOutcome::NeedsApproval(host) => SkillMutationOutcome::NeedsApproval(host.clone()), + TuiOutcome::NetworkDenied(host) => SkillMutationOutcome::NetworkDenied(host.clone()), + }; + SkillMutationReceipt { + name: receipt.name.clone(), + safe_target_path: receipt.safe_target_path.clone(), + outcome, + } +} + +/// Map a portable target scope to the TUI scope. +fn portable_scope( + scope: Option, +) -> Option { + use crate::skills::mutation::SkillTargetScope as TuiScope; + scope.map(|s| match s { + SkillTargetScope::Project => TuiScope::Project, + SkillTargetScope::Global => TuiScope::Global, + }) +} + +/// Map a curated registry document to portable entries. +fn portable_registry_entries( + doc: &crate::skills::install::RegistryDocument, +) -> Vec { + doc.skills + .iter() + .map(|(name, entry)| RemoteSkillEntry { + name: name.clone(), + description: entry.description.clone(), + source: entry.source.clone(), + }) + .collect() +} + +/// Message shown when a network-policy host requires approval. Moved +/// verbatim from `groups/skills/skills.rs`; the legacy copy is removed in +/// Phase 4. Rendered by the portable handler from the typed outcome. +fn needs_approval_message(host: &str) -> String { + format!( + "Network policy requires approval for {host}.\n\ + Add it to your allow list with `/network allow {host}` (or set [network].default = \"allow\" in ~/.codewhale/config.toml), then retry." + ) +} + +/// Message shown when a network-policy host is denied. Moved verbatim from +/// `groups/skills/skills.rs`; the legacy copy is removed in Phase 4. +fn network_denied_message(host: &str) -> String { + format!( + "Network policy denied access to {host}.\n\ + Remove the deny entry from ~/.codewhale/config.toml under [network] or contact your administrator." + ) +} + +impl CommandSkillGroupContext for SkillGroupAdapter<'_> { + fn skill_registry_projection(&self) -> SkillRegistryProjection { + let app = self.host.app.borrow(); + let mode = + crate::skills::SkillDiscoveryMode::from_codewhale_only(app.skills_scan_codewhale_only); + let dirs = crate::skills::skill_directories_for_workspace_and_dir( + &app.workspace, + &app.skills_dir, + mode, + ); + let registry = discover_visible(&app); + let mode_label = match mode { + crate::skills::SkillDiscoveryMode::Compatible => "compatible", + crate::skills::SkillDiscoveryMode::CodeWhaleOnly => "codewhale-only", + }; + SkillRegistryProjection { + workspace: app.workspace.display().to_string(), + skills_dir: app.skills_dir.display().to_string(), + mode_label: mode_label.to_string(), + dirs: dirs.iter().map(|dir| dir.display().to_string()).collect(), + entries: registry.list().iter().map(portable_skill_entry).collect(), + warnings: registry.warnings().to_vec(), + total: registry.len(), + } + } + + fn activate_skill( + &mut self, + name: &str, + ) -> Result { + let registry = { + let app = self.host.app.borrow(); + discover_visible(&app) + }; + if let Some(skill) = registry.get(name) { + let plugin_provenance = match &skill.source { + crate::skills::SkillSource::Native => None, + crate::skills::SkillSource::Plugin { authority, .. } => { + if let Err(reason) = crate::plugins::registry::verify_plugin_component_authority( + authority, + crate::plugins::activation::PluginActivationCapability::Skills, + ) { + return Err(SkillActivationError::PluginRejected { + name: skill.name.clone(), + reason, + }); + } + Some(authority.as_ref().clone()) + } + }; + let skill = skill.clone(); + let instruction = format!( + "You are now using a skill. Follow these instructions:\n\n# Skill: {}\n\n{}\n\n---\n\nNow respond to the user's request following the above skill instructions.", + skill.name, skill.body + ); + let mut app = self.host.app.borrow_mut(); + app.add_message(HistoryCell::System { + content: format!("Activated skill: {}\n\n{}", skill.name, skill.description), + }); + app.active_skill = Some(instruction); + app.active_skill_provenance = plugin_provenance; + Ok(SkillActivationOutcome { + name: skill.name, + description: skill.description, + }) + } else { + let available: Vec = registry.list().iter().map(|s| s.name.clone()).collect(); + Err(SkillActivationError::NotFound { + requested: name.to_string(), + available, + warnings: registry.warnings().to_vec(), + }) + } + } + + fn install_skill( + &mut self, + scope: Option, + spec: &str, + ) -> Result { + use crate::skills::mutation::{MutationContext, SkillMutationRequest}; + let source = match crate::skills::install::InstallSource::parse(spec) { + Ok(source) => source, + Err(err) => return Err(format!("Invalid install source: {err}")), + }; + let target = + portable_scope(scope).unwrap_or(crate::skills::mutation::SkillTargetScope::Global); + let workspace = self.host.app.borrow().workspace.clone(); + let home = crate::config::effective_home_dir(); + let (network, max_size, registry_url) = installer_settings(); + let outcome = run_async(async move { + let ctx = MutationContext { + workspace: &workspace, + home: home.as_deref(), + configured_skills_dir: None, + network: &network, + max_size, + registry_url: ®istry_url, + }; + crate::skills::mutation::execute( + SkillMutationRequest::InstallRemote { source, target }, + &ctx, + ) + .await + }); + match outcome { + Ok(receipt) => Ok(portable_mutation_receipt(&receipt)), + Err(err) => Err(format!("Install failed: {err:#}")), + } + } + + fn update_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result { + use crate::skills::mutation::{MutationContext, SkillMutationRequest}; + let workspace = self.host.app.borrow().workspace.clone(); + let home = crate::config::effective_home_dir(); + let (network, max_size, registry_url) = installer_settings(); + let owned_name = name.to_string(); + let scope = portable_scope(scope); + let outcome = run_async(async move { + let ctx = MutationContext { + workspace: &workspace, + home: home.as_deref(), + configured_skills_dir: None, + network: &network, + max_size, + registry_url: ®istry_url, + }; + crate::skills::mutation::execute( + SkillMutationRequest::UpdateByName { + name: owned_name, + scope, + expected_digest: None, + }, + &ctx, + ) + .await + }); + match outcome { + Ok(receipt) => Ok(portable_mutation_receipt(&receipt)), + Err(err) => Err(format!("Update failed: {err:#}")), + } + } + + fn uninstall_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result { + use crate::skills::mutation::{MutationContext, SkillMutationRequest}; + let workspace = self.host.app.borrow().workspace.clone(); + let home = crate::config::effective_home_dir(); + let (network, max_size, registry_url) = installer_settings(); + let ctx = MutationContext { + workspace: &workspace, + home: home.as_deref(), + configured_skills_dir: None, + network: &network, + max_size, + registry_url: ®istry_url, + }; + match crate::skills::mutation::execute_sync( + SkillMutationRequest::RemoveByName { + name: name.to_string(), + scope: portable_scope(scope), + expected_digest: None, + }, + &ctx, + ) { + Ok(receipt) => Ok(portable_mutation_receipt(&receipt)), + Err(err) => Err(format!("Uninstall failed: {err:#}")), + } + } + + fn trust_skill( + &mut self, + scope: Option, + name: &str, + ) -> Result { + use crate::skills::mutation::{MutationContext, SkillMutationRequest}; + let workspace = self.host.app.borrow().workspace.clone(); + let home = crate::config::effective_home_dir(); + let (network, max_size, registry_url) = installer_settings(); + let ctx = MutationContext { + workspace: &workspace, + home: home.as_deref(), + configured_skills_dir: None, + network: &network, + max_size, + registry_url: ®istry_url, + }; + match crate::skills::mutation::execute_sync( + SkillMutationRequest::TrustByName { + name: name.to_string(), + scope: portable_scope(scope), + expected_digest: None, + }, + &ctx, + ) { + Ok(receipt) => Ok(portable_mutation_receipt(&receipt)), + Err(err) => Err(format!("Trust failed: {err:#}")), + } + } + + fn fetch_remote_registry(&mut self) -> Result { + let (network, _max_size, registry_url) = installer_settings(); + let registry = run_async(async move { + crate::skills::install::fetch_registry(&network, ®istry_url).await + }); + match registry { + Ok(crate::skills::install::RegistryFetchResult::Loaded(doc)) => { + Ok(RemoteRegistryOutcome::Loaded { + entries: portable_registry_entries(&doc), + }) + } + Ok(crate::skills::install::RegistryFetchResult::NeedsApproval(host)) => { + Ok(RemoteRegistryOutcome::NeedsApproval(host)) + } + Ok(crate::skills::install::RegistryFetchResult::Denied(host)) => { + Ok(RemoteRegistryOutcome::Denied(host)) + } + Err(err) => Err(format_registry_error("Failed to fetch registry", &err)), + } + } + + fn recommend_skills(&mut self, task: &str) -> Result, String> { + let (network, _max_size, registry_url) = installer_settings(); + let registry = run_async(async move { + crate::skills::install::fetch_registry(&network, ®istry_url).await + }); + match registry { + Ok(crate::skills::install::RegistryFetchResult::Loaded(doc)) => { + let recommendations = + crate::skills::recommend::recommend_remote_skills(task, &doc, 3); + Ok(recommendations + .into_iter() + .map(|recommendation| SkillRecommendation { + name: recommendation.name.to_string(), + description: recommendation.entry.description.clone(), + matched_terms: recommendation.matched_terms.clone(), + }) + .collect()) + } + Ok(crate::skills::install::RegistryFetchResult::NeedsApproval(host)) => { + Err(needs_approval_message(&host)) + } + Ok(crate::skills::install::RegistryFetchResult::Denied(host)) => { + Err(network_denied_message(&host)) + } + Err(err) => Err(format_registry_error("Failed to fetch registry", &err)), + } + } + + 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)), + } + } + + fn run_review(&mut self) -> Result { + let skills_dir = self.host.app.borrow().skills_dir.clone(); + let registry = crate::skills::SkillRegistry::discover(&skills_dir).into_enabled(); + let mut warnings: Vec = registry.warnings().to_vec(); + let mut skill = registry.get("review").cloned(); + + let global_dir = crate::skills::default_skills_dir(); + if skill.is_none() && global_dir != skills_dir { + let registry = crate::skills::SkillRegistry::discover(&global_dir).into_enabled(); + if warnings.is_empty() { + warnings = registry.warnings().to_vec(); + } else if !registry.warnings().is_empty() { + warnings.extend(registry.warnings().iter().cloned()); + } + skill = registry.get("review").cloned(); + } + + match skill { + Some(skill) => { + // Host-side side effects (D2): session-message insertion and + // active-skill mutation are authoritative App operations; the + // portable handler renders no success message (baseline emits + // only the SendMessage action) and never touches App. + let instruction = format!( + "You are now using a skill. Follow these instructions:\n\n# Skill: {}\n\n{}\n\n---\n\nNow respond to the user's request following the above skill instructions.", + skill.name, skill.body + ); + let mut app = self.host.app.borrow_mut(); + app.add_message(HistoryCell::System { + content: format!("Activated skill: {}\n\n{}", skill.name, skill.description), + }); + app.active_skill = Some(instruction); + app.active_skill_provenance = None; + Ok(ReviewOutcome::Ready) + } + None => Ok(ReviewOutcome::NotFound { + skills_dir: skills_dir.display().to_string(), + global_dir: global_dir.display().to_string(), + warnings, + }), + } + } + + fn snapshot_list(&mut self, limit: usize) -> Result, String> { + let workspace = self.host.app.borrow().workspace.clone(); + let repo = match crate::snapshot::SnapshotRepo::open_or_init(&workspace) { + Ok(repo) => repo, + Err(err) => { + return Err(format!( + "Snapshot repo unavailable for {}: {err}", + workspace.display(), + )); + } + }; + let snapshots = match repo.list(limit) { + Ok(snapshots) => snapshots, + Err(err) => return Err(format!("Failed to list snapshots: {err}")), + }; + Ok(snapshots + .into_iter() + .map(|snapshot| SnapshotEntry { + id: snapshot.id.0, + label: snapshot.label, + timestamp: snapshot.timestamp, + }) + .collect()) + } + + fn restore_snapshot(&mut self, id: &str) -> Result<(), String> { + let workspace = self.host.app.borrow().workspace.clone(); + let repo = match crate::snapshot::SnapshotRepo::open_or_init(&workspace) { + Ok(repo) => repo, + Err(err) => { + return Err(format!( + "Snapshot repo unavailable for {}: {err}", + workspace.display(), + )); + } + }; + repo.restore(&crate::snapshot::SnapshotId(id.to_string())) + .map_err(|err| format!("Restore failed: {err}")) + } + + fn approval_state(&self) -> CommandApprovalState { + let app = self.host.app.borrow(); + CommandApprovalState { + yolo: app.yolo, + trust_mode: app.trust_mode, + } + } +} + // --------------------------------------------------------------------------- // Envelope construction (D1) // --------------------------------------------------------------------------- -/// Owns ten facet objects sharing one synchronous TUI host proxy. +/// Owns eleven facet objects sharing one synchronous TUI host proxy. /// /// Handlers borrow only these adapters. Every method delegates to the real App /// authority and releases its `RefCell` borrow before returning, so facets can /// be called sequentially without exposing TUI types across the boundary. pub(crate) struct CommandContextBundle<'a> { session: SessionAdapter<'a>, + skill_group: SkillGroupAdapter<'a>, model: ModelAdapter<'a>, cost: CostAdapter<'a>, mode_policy: ModePolicyAdapter<'a>, @@ -766,6 +1393,7 @@ impl<'a> CommandContextBundle<'a> { .with_presentation(&mut self.presentation) .with_media(&mut self.media) .with_project(&mut self.project) + .with_skill_group(&mut self.skill_group) } /// Test-only: consume the bundle into independent facet parts. @@ -792,7 +1420,8 @@ impl App { workspace: WorkspaceAdapter { host: host.clone() }, presentation: PresentationAdapter { host: host.clone() }, project: ProjectAdapter { host: host.clone() }, - media: MediaAdapter { host }, + media: MediaAdapter { host: host.clone() }, + skill_group: SkillGroupAdapter { host }, } } } @@ -802,6 +1431,7 @@ mod tests { use super::*; use crate::localization::Locale; use crate::models::Role; + use tempfile::TempDir; fn test_app() -> App { crate::test_support::test_app_with_options(crate::test_support::test_tui_options( @@ -1450,4 +2080,367 @@ mod tests { assert!(parts.workspace.is_some()); assert!(parts.presentation.is_some()); } + + // ─── FEAT-022 skill-group adapter tests ─────────────────────────────────── + + /// Pins HOME to a tempdir for the duration of the test under the + /// crate-wide env mutex (keeps global skill/snapshot discovery hermetic). + struct ScopedHome { + prev: Option, + _home: TempDir, + _guard: crate::test_support::TestEnvLock, + } + impl Drop for ScopedHome { + fn drop(&mut self) { + // SAFETY: process-wide lock still held. + unsafe { + match self.prev.take() { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + } + } + } + fn scoped_home(_workspace: &TempDir) -> ScopedHome { + let guard = crate::test_support::lock_test_env(); + let prev = std::env::var_os("HOME"); + let home = TempDir::new().expect("home tempdir"); + // SAFETY: serialised by the global env lock. + unsafe { + std::env::set_var("HOME", home.path()); + } + ScopedHome { + prev, + _home: home, + _guard: guard, + } + } + + fn skill_test_app(tmp: &TempDir, skills_dir: &Path) -> App { + let mut options = crate::test_support::test_tui_options(tmp.path()); + options.skills_dir = skills_dir.to_path_buf(); + crate::test_support::test_app_with_options(options) + } + + fn write_skill(dir: &Path, name: &str) { + let skill_dir = dir.join(name); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {name} skill\n---\n{name} instructions"), + ) + .unwrap(); + } + + #[test] + fn skill_group_projection_maps_native_skills_and_dirs() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + write_skill(&skills_dir, "demo"); + let mut app = skill_test_app(&tmp, &skills_dir); + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let projection = group.skill_registry_projection(); + assert_eq!(projection.total, 1); + assert_eq!(projection.entries.len(), 1); + assert_eq!(projection.entries[0].name, "demo"); + assert_eq!(projection.entries[0].description, "demo skill"); + assert_eq!(projection.entries[0].source, SkillSourceKind::Native); + assert!(projection.entries[0].path.is_some()); + assert_eq!(projection.skills_dir, skills_dir.display().to_string()); + assert!(!projection.dirs.is_empty()); + assert!(projection.warnings.is_empty()); + } + + #[test] + fn skill_group_projection_reports_empty_registry() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + std::fs::create_dir_all(&skills_dir).unwrap(); + let mut app = skill_test_app(&tmp, &skills_dir); + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let projection = group.skill_registry_projection(); + assert_eq!(projection.total, 0); + assert!(projection.entries.is_empty()); + } + + #[test] + fn skill_group_activation_sets_active_skill_and_history() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + write_skill(&skills_dir, "demo"); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let outcome = group.activate_skill("demo").unwrap(); + assert_eq!(outcome.name, "demo"); + assert_eq!(outcome.description, "demo skill"); + } + assert!(app.active_skill.is_some()); + assert!( + app.active_skill + .as_deref() + .unwrap() + .contains("# Skill: demo") + ); + assert!(app.active_skill_provenance.is_none()); + assert!(!app.history.is_empty()); + } + + #[test] + fn skill_group_activation_looks_up_exact_name() { + // The `/skill new` -> skill-creator alias is handler-side parsing + // (Phase 4); the delegate performs an exact host lookup. + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + write_skill(&skills_dir, "skill-creator"); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let outcome = group.activate_skill("skill-creator").unwrap(); + assert_eq!(outcome.name, "skill-creator"); + } + assert!(app.active_skill.is_some()); + } + + #[test] + fn skill_group_activation_not_found_lists_available() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + write_skill(&skills_dir, "demo"); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let err = group.activate_skill("missing").unwrap_err(); + match err { + SkillActivationError::NotFound { + requested, + available, + .. + } => { + assert_eq!(requested, "missing"); + assert!(available.contains(&"demo".to_string())); + } + _ => panic!("expected NotFound"), + } + } + assert!(app.active_skill.is_none()); + } + + #[test] + fn skill_group_install_invalid_source_returns_safe_error() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + std::fs::create_dir_all(&skills_dir).unwrap(); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let err = group.install_skill(None, " ").unwrap_err(); + assert!(err.contains("Invalid install source"), "{err}"); + } + } + + #[test] + fn skill_group_review_ready_sets_side_effects() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + write_skill(&skills_dir, "review"); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let outcome = group.run_review().unwrap(); + assert_eq!(outcome, ReviewOutcome::Ready); + } + assert!(app.active_skill.is_some()); + assert!(app.active_skill_provenance.is_none()); + assert!(!app.history.is_empty()); + } + + #[test] + fn skill_group_review_not_found_reports_searched_dirs() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + std::fs::create_dir_all(&skills_dir).unwrap(); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let outcome = group.run_review().unwrap(); + match outcome { + ReviewOutcome::NotFound { + skills_dir: found_dir, + global_dir, + warnings, + } => { + assert_eq!(found_dir, skills_dir.display().to_string()); + assert_eq!( + global_dir, + crate::skills::default_skills_dir().display().to_string() + ); + assert!(warnings.is_empty()); + } + _ => panic!("expected NotFound"), + } + } + assert!(app.active_skill.is_none()); + } + + #[test] + fn skill_group_snapshot_list_and_restore_roundtrip() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + let file = tmp.path().join("a.txt"); + let repo = crate::snapshot::SnapshotRepo::open_or_init(tmp.path()).unwrap(); + std::fs::write(&file, b"v1").unwrap(); + repo.snapshot("pre-turn:1").unwrap(); + std::fs::write(&file, b"v2").unwrap(); + let mut app = skill_test_app(&tmp, &skills_dir); + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let entries = group.snapshot_list(20).unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].label, "pre-turn:1"); + assert!(!entries[0].id.is_empty()); + group.restore_snapshot(&entries[0].id).unwrap(); + } + assert_eq!(std::fs::read_to_string(&file).unwrap(), "v1"); + } + + #[test] + fn skill_group_approval_state_reflects_app_posture() { + let tmp = TempDir::new().unwrap(); + let _home = scoped_home(&tmp); + let skills_dir = tmp.path().join("skills"); + let mut app = skill_test_app(&tmp, &skills_dir); + app.yolo = true; + app.trust_mode = false; + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let state = group.approval_state(); + assert!(state.yolo); + assert!(!state.trust_mode); + } + app.yolo = false; + app.trust_mode = true; + { + let mut bundle = app.command_contexts(); + let group = bundle + .parts() + .skill_group + .expect("skill_group facet must be present"); + let state = group.approval_state(); + assert!(!state.yolo); + assert!(state.trust_mode); + } + } + + #[test] + fn portable_scope_maps_both_scopes_and_none() { + use crate::skills::mutation::SkillTargetScope as TuiScope; + assert_eq!( + portable_scope(Some(SkillTargetScope::Project)), + Some(TuiScope::Project) + ); + assert_eq!( + portable_scope(Some(SkillTargetScope::Global)), + Some(TuiScope::Global) + ); + assert_eq!(portable_scope(None), None); + } + + #[test] + fn portable_mutation_receipt_maps_distinct_outcomes() { + use crate::skills::audit::SkillActionKind; + use crate::skills::mutation::{ + SkillMutationOutcome as TuiOutcome, SkillMutationReceipt as TuiReceipt, + }; + use crate::skills::roots::SkillScope; + let make = |outcome: TuiOutcome| TuiReceipt { + action: SkillActionKind::Install, + name: "demo".to_string(), + scope: SkillScope::Global, + safe_target_path: "/tmp/demo".to_string(), + before_digest: None, + after_digest: None, + outcome, + }; + let installed = portable_mutation_receipt(&make(TuiOutcome::Installed)); + assert_eq!(installed.outcome, SkillMutationOutcome::Installed); + assert_eq!(installed.name, "demo"); + assert_eq!(installed.safe_target_path, "/tmp/demo"); + + let approval = + portable_mutation_receipt(&make(TuiOutcome::NeedsApproval("acme.com".to_string()))); + assert_eq!( + approval.outcome, + SkillMutationOutcome::NeedsApproval("acme.com".to_string()) + ); + + let denied = + portable_mutation_receipt(&make(TuiOutcome::NetworkDenied("acme.com".to_string()))); + assert_eq!( + denied.outcome, + SkillMutationOutcome::NetworkDenied("acme.com".to_string()) + ); + assert_ne!(installed.outcome, denied.outcome); + } + + #[test] + fn skill_group_adapter_exposure_matches_main_envelope_model() { + // The envelope populates the skill_group slot alongside the other + // adapters; handlers destructure only their declared facets (D4). + let mut app = test_app(); + let mut bundle = app.command_contexts(); + let parts = bundle.parts(); + assert!(parts.skill_group.is_some()); + assert!(parts.project.is_some()); + assert!(parts.skills.is_some()); + } } diff --git a/crates/tui/src/commands/groups/skills/mod.rs b/crates/tui/src/commands/groups/skills/mod.rs index cb7f34a4a0..bf73cffe56 100644 --- a/crates/tui/src/commands/groups/skills/mod.rs +++ b/crates/tui/src/commands/groups/skills/mod.rs @@ -10,29 +10,28 @@ mod skills; pub(in crate::commands) use self::skills::run_skill_by_name; -use crate::commands::traits::{Command, CommandGroup, FunctionCommand, RegisterCommand}; +use crate::commands::traits::{Command, CommandGroup, ContextualCommand}; pub struct SkillsCommands; impl CommandGroup for SkillsCommands { fn commands(&self) -> &'static [Box] { cached_command_list!(vec![ - Box::new(FunctionCommand::new( - skills::SkillsCmd::info(), - skills::SkillsCmd::execute, - )), - Box::new(FunctionCommand::new( - skills::SkillCmd::info(), - skills::SkillCmd::execute, - )), - Box::new(FunctionCommand::new( - review::ReviewCmd::info(), - review::ReviewCmd::execute, - )), - Box::new(FunctionCommand::new( - restore::RestoreCmd::info(), - restore::RestoreCmd::execute, - )), + Box::new( + ContextualCommand::from_contract::() + .expect("skills registration") + ), + Box::new( + ContextualCommand::from_contract::().expect("skill registration") + ), + Box::new( + ContextualCommand::from_contract::() + .expect("review registration") + ), + Box::new( + ContextualCommand::from_contract::() + .expect("restore registration") + ), ]) } } diff --git a/crates/tui/src/commands/groups/skills/restore.rs b/crates/tui/src/commands/groups/skills/restore.rs index 0a70023146..b41c84827e 100644 --- a/crates/tui/src/commands/groups/skills/restore.rs +++ b/crates/tui/src/commands/groups/skills/restore.rs @@ -7,33 +7,61 @@ //! the user has explicitly trusted the workspace (`/trust on` or Full Access) — //! the user can always view the list, just not one-shot revert without a //! safety net. +//! +//! FEAT-022 Phase 4: portable contextual dispatch. `SnapshotRepo` and the +//! approval state stay host-side (`CommandSkillGroupContext` delegates); the +//! portable handler owns all parsing, formatting, and the trust gate. -use crate::commands::CommandResult; -use crate::snapshot::{Snapshot, SnapshotRepo}; -use crate::tui::app::App; use chrono::TimeZone; +use codewhale_command_contract::facets::{CommandSkillGroupContext, SnapshotEntry}; +use codewhale_command_contract::handler::{CommandContexts, CommandHandler}; +use codewhale_command_contract::metadata::{CommandInfo, RegisterCommand}; + +use crate::commands::CommandResult; + const DEFAULT_LIST_LIMIT: usize = 20; const MAX_LIST_LIMIT: usize = 100; const MAX_RESTORE_INDEX: usize = 1000; -/// Entry point for `/restore [N|list [N]]`. -fn restore(app: &mut App, arg: Option<&str>) -> CommandResult { - let workspace = app.workspace.clone(); - let repo = match SnapshotRepo::open_or_init(&workspace) { - Ok(r) => r, - Err(e) => { - return CommandResult::error(format!( - "Snapshot repo unavailable for {}: {e}", - workspace.display(), - )); - } +pub(in crate::commands) const COMMAND_INFO: CommandInfo = CommandInfo { + name: "restore", + aliases: &[], + usage: "/restore [N|list [N]]", + description_key: "cmd_restore_description", +}; + +pub(in crate::commands) struct RestoreCmd; + +impl RegisterCommand for RestoreCmd { + fn info() -> &'static CommandInfo { + &COMMAND_INFO + } + + fn handler() -> CommandHandler { + CommandHandler::Contextual(restore_contextual) + } +} + +/// Contextual `/restore` dispatch (FEAT-022 D4): exactly the skill-group facet +/// (snapshot list/restore + approval state — no `MODE_POLICY` declaration). +fn restore_contextual(contexts: CommandContexts<'_>, arg: Option<&str>) -> CommandResult { + let mut parts = contexts.into_parts(); + let Some(skill_group) = parts.skill_group.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: skill_group"); }; + restore(skill_group, arg) +} +/// Portable `/restore` dispatch — byte-identical to the baseline handler. +/// +/// The host owns `SnapshotRepo` open/list/restore and the yolo/trust posture; +/// the handler composes every message, error, listing, and the trust gate. +fn restore(group: &mut dyn CommandSkillGroupContext, arg: Option<&str>) -> CommandResult { let Some(arg) = arg.map(str::trim).filter(|s| !s.is_empty()) else { - let snapshots = match repo.list(DEFAULT_LIST_LIMIT) { + let snapshots = match group.snapshot_list(DEFAULT_LIST_LIMIT) { Ok(s) => s, - Err(e) => return CommandResult::error(format!("Failed to list snapshots: {e}")), + Err(err) => return CommandResult::error(err), }; if snapshots.is_empty() { return no_snapshots_message(); @@ -45,9 +73,9 @@ fn restore(app: &mut App, arg: Option<&str>) -> CommandResult { Ok(limit) => limit, Err(message) => return CommandResult::error(message), } { - let snapshots = match repo.list(limit) { + let snapshots = match group.snapshot_list(limit) { Ok(s) => s, - Err(e) => return CommandResult::error(format!("Failed to list snapshots: {e}")), + Err(err) => return CommandResult::error(err), }; if snapshots.is_empty() { return no_snapshots_message(); @@ -68,9 +96,9 @@ fn restore(app: &mut App, arg: Option<&str>) -> CommandResult { )); } }; - let snapshots = match repo.list(n.max(DEFAULT_LIST_LIMIT)) { + let snapshots = match group.snapshot_list(n.max(DEFAULT_LIST_LIMIT)) { Ok(s) => s, - Err(e) => return CommandResult::error(format!("Failed to list snapshots: {e}")), + Err(err) => return CommandResult::error(err), }; if snapshots.is_empty() { return no_snapshots_message(); @@ -87,7 +115,8 @@ fn restore(app: &mut App, arg: Option<&str>) -> CommandResult { // modal-confirmation path inside slash commands today, so the gate // is "require trust mode" — `/trust on` or Full Access. Users in plain // Agent mode get a clear message explaining how to proceed. - if !(app.yolo || app.trust_mode) { + let approval = group.approval_state(); + if !(approval.yolo || approval.trust_mode) { return CommandResult::message(format!( "Refusing to restore snapshot #{n} ('{}') outside trusted mode.\n\ Run `/trust on` or select Full Access with Shift+Tab, then re-run `/restore {n}`.", @@ -96,8 +125,8 @@ fn restore(app: &mut App, arg: Option<&str>) -> CommandResult { } let target = &snapshots[n - 1]; - if let Err(e) = repo.restore(&target.id) { - return CommandResult::error(format!("Restore failed: {e}")); + if let Err(err) = group.restore_snapshot(&target.id) { + return CommandResult::error(err); } CommandResult::message(format!( @@ -141,7 +170,7 @@ fn no_snapshots_message() -> CommandResult { ) } -fn format_listing(snapshots: &[Snapshot]) -> String { +fn format_listing(snapshots: &[SnapshotEntry]) -> String { let mut out = String::from( "Recent snapshots (newest first; pass /restore to revert; /restore list 50 shows more):\n", ); @@ -168,180 +197,154 @@ fn short_sha(sha: &str) -> &str { &sha[..sha.len().min(8)] } -pub(in crate::commands) const COMMAND_INFO: crate::commands::traits::CommandInfo = - crate::commands::traits::CommandInfo { - name: "restore", - aliases: &[], - usage: "/restore [N|list [N]]", - description_id: crate::localization::MessageId::CmdRestoreDescription, - }; - -pub(in crate::commands) struct RestoreCmd; - -impl crate::commands::traits::RegisterCommand for RestoreCmd { - fn info() -> &'static crate::commands::traits::CommandInfo { - &COMMAND_INFO - } - - fn execute( - app: &mut crate::tui::app::App, - arg: Option<&str>, - ) -> crate::commands::CommandResult { - restore(app, arg) - } -} - #[cfg(test)] mod tests { use super::*; - use crate::config::Config; - use crate::test_support::lock_test_env; - use crate::tui::app::TuiOptions; - use tempfile::TempDir; - - fn make_app(tmp: &TempDir, yolo: bool) -> App { - let workspace = tmp.path().to_path_buf(); - let options = TuiOptions { - skills_dir: tmp.path().join("skills"), - memory_path: tmp.path().join("memory.md"), - notes_path: tmp.path().join("notes.txt"), - mcp_config_path: tmp.path().join("mcp.json"), - yolo, - ..crate::test_support::test_tui_options(workspace) - }; - App::new(options, &Config::default()) - } + use codewhale_command_contract::facets::{ + CommandApprovalState, RemoteRegistryOutcome, ReviewOutcome, SkillActivationError, + SkillMutationReceipt, SkillRecommendation, SkillSyncOutcome, SkillTargetScope, + }; - /// Pins HOME to a tempdir for the duration of the test under the - /// crate-wide env mutex. - struct ScopedHome { - prev: Option, - _home: TempDir, - _guard: crate::test_support::TestEnvLock, + struct FakeSkillGroup { + snapshots: Result, String>, + restore: Result<(), String>, + approval: CommandApprovalState, } - impl Drop for ScopedHome { - fn drop(&mut self) { - // SAFETY: process-wide lock still held. - unsafe { - match self.prev.take() { - Some(v) => std::env::set_var("HOME", v), - None => std::env::remove_var("HOME"), - } + impl FakeSkillGroup { + fn new(snapshots: Vec) -> Self { + Self { + snapshots: Ok(snapshots), + restore: Ok(()), + approval: CommandApprovalState { + yolo: true, + trust_mode: false, + }, } } } - fn scoped_home(_workspace: &TempDir) -> ScopedHome { - let guard = lock_test_env(); - let prev = std::env::var_os("HOME"); - let home = TempDir::new().expect("home tempdir"); - // SAFETY: serialised by the global env lock. - unsafe { - std::env::set_var("HOME", home.path()); + impl CommandSkillGroupContext for FakeSkillGroup { + fn skill_registry_projection( + &self, + ) -> codewhale_command_contract::facets::SkillRegistryProjection { + unimplemented!("not used by restore tests") } - ScopedHome { - prev, - _home: home, - _guard: guard, + fn activate_skill( + &mut self, + _name: &str, + ) -> Result + { + unimplemented!("not used by restore tests") + } + fn install_skill( + &mut self, + _scope: Option, + _spec: &str, + ) -> Result { + unimplemented!("not used by restore tests") + } + fn update_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by restore tests") + } + fn uninstall_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by restore tests") + } + fn trust_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by restore tests") + } + fn fetch_remote_registry(&mut self) -> Result { + unimplemented!("not used by restore tests") + } + fn recommend_skills(&mut self, _task: &str) -> Result, String> { + unimplemented!("not used by restore tests") + } + fn sync_registry(&mut self) -> Result { + unimplemented!("not used by restore tests") + } + fn run_review(&mut self) -> Result { + unimplemented!("not used by restore tests") + } + fn snapshot_list(&mut self, limit: usize) -> Result, String> { + match &self.snapshots { + Ok(snapshots) => Ok(snapshots.iter().take(limit).cloned().collect()), + Err(err) => Err(err.clone()), + } + } + fn restore_snapshot(&mut self, _id: &str) -> Result<(), String> { + self.restore.clone() + } + fn approval_state(&self) -> CommandApprovalState { + self.approval + } + } + + fn snap(label: &str, id: &str, timestamp: i64) -> SnapshotEntry { + SnapshotEntry { + id: id.to_string(), + label: label.to_string(), + timestamp, } } #[test] fn restore_with_no_snapshots_shows_empty_message() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let result = restore(&mut app, None); + let mut group = FakeSkillGroup::new(vec![]); + let result = restore(&mut group, None); let msg = result.message.expect("expected message"); assert!(msg.contains("No snapshots")); } #[test] fn restore_lists_when_no_arg_provided() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - std::fs::write(app.workspace.join("a.txt"), b"v1").unwrap(); - repo.snapshot("pre-turn:1").unwrap(); - std::fs::write(app.workspace.join("a.txt"), b"v2").unwrap(); - repo.snapshot("post-turn:1").unwrap(); - - let result = restore(&mut app, None); + let mut group = FakeSkillGroup::new(vec![ + snap("post-turn:1", "11111111", 1_700_000_000), + snap("pre-turn:1", "22222222", 1_699_000_000), + ]); + let result = restore(&mut group, None); let msg = result.message.expect("expected message"); assert!(msg.contains("post-turn:1")); assert!(msg.contains("pre-turn:1")); assert!(msg.contains("#1")); assert!(msg.contains("#2")); - } - - #[test] - fn restore_lists_more_than_ten_snapshots_by_default() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - for i in 0..12 { - std::fs::write(app.workspace.join("a.txt"), format!("v{i}")).unwrap(); - repo.snapshot(&format!("turn:{i}")).unwrap(); - } - - let result = restore(&mut app, None); - let msg = result.message.expect("expected message"); - assert!(msg.contains("#12"), "{msg}"); - assert!(msg.contains("turn:0"), "{msg}"); - } - - #[test] - fn restore_listing_includes_snapshot_utc_time() { - let snapshots = [Snapshot { - id: crate::snapshot::SnapshotId("abcdef123456".to_string()), - label: "turn:demo".to_string(), - timestamp: 1_700_000_000, - session_id: None, - }]; - - let msg = format_listing(&snapshots); - assert!(msg.contains("2023-11-14 22:13 UTC"), "{msg}"); - assert!(msg.contains("abcdef12"), "{msg}"); - assert!(msg.contains("turn:demo"), "{msg}"); } #[test] fn restore_list_subcommand_accepts_explicit_limit() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - for i in 0..15 { - std::fs::write(app.workspace.join("a.txt"), format!("v{i}")).unwrap(); - repo.snapshot(&format!("turn:{i}")).unwrap(); - } - - let result = restore(&mut app, Some("list 12")); + let mut group = FakeSkillGroup::new(vec![ + snap("turn:1", "11111111", 1_700_000_000), + snap("turn:2", "22222222", 1_699_000_000), + snap("turn:3", "33333333", 1_698_000_000), + ]); + let result = restore(&mut group, Some("list 2")); let msg = result.message.expect("expected message"); - assert!(msg.contains("#12"), "{msg}"); - assert!(!msg.contains("#13"), "{msg}"); + assert!(msg.contains("#2"), "{msg}"); + assert!(!msg.contains("#3"), "{msg}"); } #[test] fn restore_list_subcommand_rejects_invalid_limit() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - - let result = restore(&mut app, Some("list nope")); + let mut group = FakeSkillGroup::new(vec![]); + let result = restore(&mut group, Some("list nope")); assert!(result.is_error); assert!(result.message.unwrap().contains("Usage: /restore list [N]")); } #[test] fn restore_list_subcommand_rejects_limit_above_cap() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - - let result = restore(&mut app, Some("list 101")); + let mut group = FakeSkillGroup::new(vec![]); + let result = restore(&mut group, Some("list 101")); assert!(result.is_error); assert!( result @@ -351,32 +354,10 @@ mod tests { ); } - #[test] - fn restore_numeric_index_can_target_beyond_default_listing() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - let f = app.workspace.join("a.txt"); - for i in 0..12 { - std::fs::write(&f, format!("v{i}")).unwrap(); - repo.snapshot(&format!("turn:{i}")).unwrap(); - } - std::fs::write(&f, "changed").unwrap(); - - let result = restore(&mut app, Some("12")); - assert!(result.message.unwrap().contains("Restored")); - assert_eq!(std::fs::read_to_string(&f).unwrap(), "v0"); - } - #[test] fn restore_numeric_index_rejects_unbounded_query() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - - let result = restore(&mut app, Some("1001")); - + let mut group = FakeSkillGroup::new(vec![]); + let result = restore(&mut group, Some("1001")); assert!(result.is_error); assert!( result @@ -388,33 +369,23 @@ mod tests { #[test] fn restore_in_yolo_reverts_workspace() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - let f = app.workspace.join("a.txt"); - - std::fs::write(&f, b"original").unwrap(); - repo.snapshot("pre-turn:1").unwrap(); - std::fs::write(&f, b"clobbered").unwrap(); - repo.snapshot("post-turn:1").unwrap(); - - let result = restore(&mut app, Some("2")); - assert!(result.message.unwrap().contains("Restored")); - let after = std::fs::read_to_string(&f).unwrap(); - assert_eq!(after, "original"); + let mut group = FakeSkillGroup::new(vec![ + snap("post-turn:1", "22222222", 1_700_000_000), + snap("pre-turn:1", "11111111", 1_699_000_000), + ]); + let result = restore(&mut group, Some("2")); + assert!(!result.is_error); + assert!(result.message.unwrap().contains("Restored snapshot #2")); } #[test] fn restore_outside_trust_mode_refuses() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, false); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - std::fs::write(app.workspace.join("a.txt"), b"v1").unwrap(); - repo.snapshot("pre-turn:1").unwrap(); - - let result = restore(&mut app, Some("1")); + let mut group = FakeSkillGroup::new(vec![snap("pre-turn:1", "11111111", 1_700_000_000)]); + group.approval = CommandApprovalState { + yolo: false, + trust_mode: false, + }; + let result = restore(&mut group, Some("1")); let msg = result.message.expect("expected message"); assert!(msg.contains("Refusing")); assert!(msg.contains("/trust on")); @@ -422,31 +393,39 @@ mod tests { #[test] fn restore_invalid_index_returns_error() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - std::fs::write(app.workspace.join("a.txt"), b"v1").unwrap(); - repo.snapshot("pre-turn:1").unwrap(); - - let result = restore(&mut app, Some("99")); + let mut group = FakeSkillGroup::new(vec![snap("pre-turn:1", "11111111", 1_700_000_000)]); + let result = restore(&mut group, Some("99")); let msg = result.message.expect("expected message"); assert!(msg.contains("Only 1 snapshot")); } #[test] fn restore_zero_index_returns_error() { - let tmp = TempDir::new().unwrap(); - let _home = scoped_home(&tmp); - let mut app = make_app(&tmp, true); - // Need at least one snapshot so we exercise the parse-index - // branch instead of the "no snapshots" early return. - let repo = SnapshotRepo::open_or_init(&app.workspace).unwrap(); - std::fs::write(app.workspace.join("a.txt"), b"v1").unwrap(); - repo.snapshot("pre-turn:1").unwrap(); - - let result = restore(&mut app, Some("0")); - let msg = result.message.expect("expected message"); - assert!(msg.contains("Usage:")); + let mut group = FakeSkillGroup::new(vec![snap("pre-turn:1", "11111111", 1_700_000_000)]); + let result = restore(&mut group, Some("0")); + assert!(result.is_error); + assert!(result.message.unwrap().contains("Usage:")); + } + + #[test] + fn restore_host_error_reaches_boundary() { + let mut group = FakeSkillGroup::new(vec![]); + group.snapshots = Err("Snapshot repo unavailable for /ws: boom".to_string()); + let result = restore(&mut group, None); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Snapshot repo unavailable for /ws: boom" + ); + } + + #[test] + fn restore_missing_facet_errors_are_safe() { + let result = restore_contextual(CommandContexts::empty(), Some("1")); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Command capability unavailable: skill_group" + ); } } diff --git a/crates/tui/src/commands/groups/skills/review.rs b/crates/tui/src/commands/groups/skills/review.rs index ab97cccf0c..9b0adf8319 100644 --- a/crates/tui/src/commands/groups/skills/review.rs +++ b/crates/tui/src/commands/groups/skills/review.rs @@ -1,148 +1,220 @@ //! Review command: activate review skill and send a target immediately. +//! +//! FEAT-022 Phase 4: portable contextual dispatch. The host performs the +//! discovery + side effects (`CommandSkillGroupContext::run_review`); the +//! portable handler composes the exact error text and the `SendMessage` action. -use crate::skills::{SkillRegistry, default_skills_dir}; -use crate::tui::app::{App, AppAction}; -use crate::tui::history::HistoryCell; +use codewhale_command_contract::facets::{CommandSkillGroupContext, ReviewOutcome}; +use codewhale_command_contract::handler::{CommandContexts, CommandHandler}; +use codewhale_command_contract::metadata::{CommandInfo, RegisterCommand}; use crate::commands::CommandResult; +use crate::tui::app::AppAction; -fn warnings_suffix(registry: &SkillRegistry) -> String { - if registry.warnings().is_empty() { +/// Render the review warnings suffix (baseline `warnings_suffix`). +fn warnings_suffix(warnings: &[String]) -> String { + if warnings.is_empty() { return String::new(); } - format!("\n\nWarnings:\n- {}", registry.warnings().join("\n- ")) + format!("\n\nWarnings:\n- {}", warnings.join("\n- ")) } -fn review(app: &mut App, args: Option<&str>) -> CommandResult { - let target = args.unwrap_or("").trim(); - if target.is_empty() { - return CommandResult::error("Usage: /review "); - } +pub(in crate::commands) const COMMAND_INFO: CommandInfo = CommandInfo { + name: "review", + aliases: &["shencha"], + usage: "/review ", + description_key: "cmd_review_description", +}; - let skills_dir = app.skills_dir.clone(); - let registry = SkillRegistry::discover(&skills_dir).into_enabled(); - let mut warnings = warnings_suffix(®istry); - let mut skill = registry.get("review").cloned(); +pub(in crate::commands) struct ReviewCmd; - let global_dir = default_skills_dir(); - if skill.is_none() && global_dir != skills_dir { - let registry = SkillRegistry::discover(&global_dir).into_enabled(); - if warnings.is_empty() { - warnings = warnings_suffix(®istry); - } else if !registry.warnings().is_empty() { - warnings.push_str(&format!("\n- {}", registry.warnings().join("\n- "))); - } - skill = registry.get("review").cloned(); +impl RegisterCommand for ReviewCmd { + fn info() -> &'static CommandInfo { + &COMMAND_INFO } - let skill = match skill { - Some(skill) => skill, - None => { - let global_display = global_dir.display(); - return CommandResult::error(format!( - "Review skill not found in {} or {}. Create ~/.codewhale/skills/review/SKILL.md.{}", - skills_dir.display(), - global_display, - warnings - )); - } - }; - - let instruction = format!( - "You are now using a skill. Follow these instructions:\n\n# Skill: {}\n\n{}\n\n---\n\nNow respond to the user's request following the above skill instructions.", - skill.name, skill.body - ); - - app.add_message(HistoryCell::System { - content: format!("Activated skill: {}\n\n{}", skill.name, skill.description), - }); - app.active_skill = Some(instruction); - app.active_skill_provenance = None; - - CommandResult::action(AppAction::SendMessage(target.to_string())) + fn handler() -> CommandHandler { + CommandHandler::Contextual(review_contextual) + } } -pub(in crate::commands) const COMMAND_INFO: crate::commands::traits::CommandInfo = - crate::commands::traits::CommandInfo { - name: "review", - aliases: &["shencha"], - usage: "/review ", - description_id: crate::localization::MessageId::CmdReviewDescription, +/// Contextual `/review` dispatch: exactly the skill-group facet. The baseline +/// command never refreshed the shared skill cache, so `/review` must not +/// request the unrelated SKILLS facet. +fn review_contextual(contexts: CommandContexts<'_>, arg: Option<&str>) -> CommandResult { + let mut parts = contexts.into_parts(); + let Some(skill_group) = parts.skill_group.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: skill_group"); }; + review(skill_group, arg) +} -pub(in crate::commands) struct ReviewCmd; - -impl crate::commands::traits::RegisterCommand for ReviewCmd { - fn info() -> &'static crate::commands::traits::CommandInfo { - &COMMAND_INFO +/// Portable `/review` dispatch — byte-identical to the baseline handler. +/// +/// The host performs discovery, warning merge, session-message insertion, and +/// active-skill mutation (`run_review`); the handler validates the target, +/// renders the not-found error, and emits the `SendMessage` action. The +/// baseline success path renders no message and does not refresh the cache. +fn review(group: &mut dyn CommandSkillGroupContext, arg: Option<&str>) -> CommandResult { + let target = arg.unwrap_or("").trim(); + if target.is_empty() { + return CommandResult::error("Usage: /review "); } - fn execute( - app: &mut crate::tui::app::App, - arg: Option<&str>, - ) -> crate::commands::CommandResult { - review(app, arg) + match group.run_review() { + Ok(ReviewOutcome::Ready) => { + CommandResult::action(AppAction::SendMessage(target.to_string())) + } + Ok(ReviewOutcome::NotFound { + skills_dir, + global_dir, + warnings, + }) => { + let warnings = warnings_suffix(&warnings); + CommandResult::error(format!( + "Review skill not found in {} or {}. Create ~/.codewhale/skills/review/SKILL.md.{}", + skills_dir, global_dir, warnings + )) + } + Err(err) => CommandResult::error(err), } } #[cfg(test)] mod tests { use super::*; - use crate::config::Config; - use crate::tui::app::{App, TuiOptions}; - use tempfile::TempDir; - - fn create_test_app_with_tmpdir(tmpdir: &TempDir) -> App { - let options = TuiOptions { - skills_dir: tmpdir.path().join("skills"), - memory_path: tmpdir.path().join("memory.md"), - notes_path: tmpdir.path().join("notes.txt"), - mcp_config_path: tmpdir.path().join("mcp.json"), - ..crate::test_support::test_tui_options(tmpdir.path()) - }; - App::new(options, &Config::default()) - } + use codewhale_command_contract::facets::{ + CommandApprovalState, RemoteRegistryOutcome, SkillActivationError, SkillMutationReceipt, + SkillRecommendation, SkillSyncOutcome, SkillTargetScope, SnapshotEntry, + }; - fn create_review_skill_dir(tmpdir: &TempDir) { - let skill_dir = tmpdir.path().join("skills").join("review"); - std::fs::create_dir_all(&skill_dir).unwrap(); - std::fs::write( - skill_dir.join("SKILL.md"), - "---\nname: review\ndescription: Code review skill\n---\nReview the code", - ) - .unwrap(); + struct FakeSkillGroup { + review: Result, + approval: CommandApprovalState, + } + impl FakeSkillGroup { + fn ready() -> Self { + Self { + review: Ok(ReviewOutcome::Ready), + approval: CommandApprovalState { + yolo: true, + trust_mode: false, + }, + } + } + } + impl CommandSkillGroupContext for FakeSkillGroup { + fn skill_registry_projection( + &self, + ) -> codewhale_command_contract::facets::SkillRegistryProjection { + unimplemented!("not used by review tests") + } + fn activate_skill( + &mut self, + _name: &str, + ) -> Result + { + unimplemented!("not used by review tests") + } + fn install_skill( + &mut self, + _scope: Option, + _spec: &str, + ) -> Result { + unimplemented!("not used by review tests") + } + fn update_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by review tests") + } + fn uninstall_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by review tests") + } + fn trust_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + unimplemented!("not used by review tests") + } + fn fetch_remote_registry(&mut self) -> Result { + unimplemented!("not used by review tests") + } + fn recommend_skills(&mut self, _task: &str) -> Result, String> { + unimplemented!("not used by review tests") + } + fn sync_registry(&mut self) -> Result { + unimplemented!("not used by review tests") + } + fn run_review(&mut self) -> Result { + self.review.clone() + } + fn snapshot_list(&mut self, _limit: usize) -> Result, String> { + unimplemented!("not used by review tests") + } + fn restore_snapshot(&mut self, _id: &str) -> Result<(), String> { + unimplemented!("not used by review tests") + } + fn approval_state(&self) -> CommandApprovalState { + self.approval + } } #[test] - fn test_review_without_target() { - let tmpdir = TempDir::new().unwrap(); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = review(&mut app, None); - assert!(result.message.is_some()); + fn review_without_target_prints_usage() { + let mut group = FakeSkillGroup::ready(); + let result = review(&mut group, None); + assert!(result.is_error); assert!(result.message.unwrap().contains("Usage: /review")); } #[test] - fn test_review_without_skill_installed() { - let tmpdir = TempDir::new().unwrap(); - let mut app = create_test_app_with_tmpdir(&tmpdir); - // Set skills dir to empty temp dir - app.skills_dir = tmpdir.path().join("nonexistent_skills"); - let result = review(&mut app, Some("file.rs")); - // The command should either error about missing skill or work if global skill exists - assert!(result.message.is_some() || result.action.is_some()); + fn review_ready_sends_target_without_skills_context() { + let mut group = FakeSkillGroup::ready(); + let contexts = CommandContexts::empty().with_skill_group(&mut group); + let result = review_contextual(contexts, Some("file.rs")); + assert!(result.message.is_none()); + assert!(matches!( + result.action, + Some(AppAction::SendMessage(ref t)) if t == "file.rs" + )); } #[test] - fn test_review_with_skill_activates_and_sends() { - let tmpdir = TempDir::new().unwrap(); - create_review_skill_dir(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = review(&mut app, Some("file.rs")); - assert!(result.message.is_none()); - assert!(matches!(result.action, Some(AppAction::SendMessage(_)))); - assert!(app.active_skill.is_some()); - assert!(!app.history.is_empty()); + fn review_not_found_renders_exact_error_with_warnings() { + let mut group = FakeSkillGroup::ready(); + group.review = Ok(ReviewOutcome::NotFound { + skills_dir: "/ws/skills".to_string(), + global_dir: "/home/u/.codewhale/skills".to_string(), + warnings: vec!["one warning".to_string()], + }); + let result = review(&mut group, Some("file.rs")); + assert!(result.is_error); + let msg = result.message.unwrap(); + assert!( + msg.contains( + "Review skill not found in /ws/skills or /home/u/.codewhale/skills. Create ~/.codewhale/skills/review/SKILL.md." + ), + "{msg}" + ); + assert!(msg.contains("Warnings:\n- one warning"), "{msg}"); + } + + #[test] + fn review_missing_facet_errors_are_safe() { + let result = review_contextual(CommandContexts::empty(), Some("file.rs")); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Command capability unavailable: skill_group" + ); } } diff --git a/crates/tui/src/commands/groups/skills/skills.rs b/crates/tui/src/commands/groups/skills/skills.rs index ec8d3ec35b..f7ffa0dc97 100644 --- a/crates/tui/src/commands/groups/skills/skills.rs +++ b/crates/tui/src/commands/groups/skills/skills.rs @@ -1,26 +1,34 @@ //! Skills commands: skills, skill +//! +//! FEAT-022 Phase 4: portable contextual dispatch over +//! [`CommandSkillGroupContext`]; the legacy `RegisterCommand::execute` is a +//! transitional shell that builds the capability envelope and delegates (Phase +//! 6 replaces it with the contract bridge). The dispatcher-only +//! `run_skill_by_name` path and its shared host machinery +//! ([`discover_visible_skills`], [`activate_skill_with_task`]) stay +//! App-carrying and co-located for FEAT-042 extraction. use std::fmt::Write; -use crate::network_policy::NetworkPolicy; -use crate::skills::install::{ - self, DEFAULT_MAX_SIZE_BYTES, DEFAULT_REGISTRY_URL, InstallSource, RegistryFetchResult, - SkillSyncOutcome, SyncResult, +use codewhale_command_contract::facets::{ + CommandSkillGroupContext, CommandSkillsContext, RemoteRegistryOutcome, SkillActivationError, + SkillBundledTier, SkillEntry, SkillMutationOutcome, SkillMutationReceipt, SkillSourceKind, + SkillSyncEntry, SkillSyncOutcome, SkillTargetScope, }; -use crate::skills::{SkillRegistry, SkillSource}; -use crate::tui::app::{App, AppAction}; -use crate::tui::history::HistoryCell; +use codewhale_command_contract::handler::{CommandContexts, CommandHandler}; +use codewhale_command_contract::metadata::{CommandInfo, RegisterCommand}; use crate::commands::CommandResult; +use crate::tui::app::AppAction; -#[cfg(test)] -thread_local! { - static TEST_HOME_DIR: std::cell::RefCell> = - const { std::cell::RefCell::new(None) }; -} +// --------------------------------------------------------------------------- +// Host-side dispatcher machinery (FEAT-042 handoff — stays App-carrying) +// --------------------------------------------------------------------------- -#[cfg(not(test))] -fn discover_visible_skills(app: &App) -> SkillRegistry { +/// Discover the enabled visible skills for the current App state. Shared by the +/// dispatcher fallback (`run_skill_by_name`) and the host activation helper; +/// kept co-located for FEAT-042. +fn discover_visible_skills(app: &crate::tui::app::App) -> crate::skills::SkillRegistry { crate::skills::discover_for_workspace_and_dir_with_mode_and_plugins( &app.workspace, &app.skills_dir, @@ -30,144 +38,248 @@ fn discover_visible_skills(app: &App) -> SkillRegistry { .into_enabled() } -#[cfg(test)] -fn discover_visible_skills(app: &App) -> SkillRegistry { - let mode = - crate::skills::SkillDiscoveryMode::from_codewhale_only(app.skills_scan_codewhale_only); - TEST_HOME_DIR - .with(|home| { - if let Some(home) = home.borrow().as_deref() { - crate::skills::discover_for_workspace_and_dir_with_home_and_mode_and_plugins( - &app.workspace, - &app.skills_dir, - Some(home), - mode, - Some(app.plugin_registry.as_ref()), - ) - } else { - crate::skills::discover_for_workspace_and_dir_with_mode_and_plugins( - &app.workspace, - &app.skills_dir, - mode, - Some(app.plugin_registry.as_ref()), - ) - } - }) - .into_enabled() +/// Run a specific skill — activates skill for next user message, or +/// dispatches a sub-command (`install`, `update`, `uninstall`, `trust`). +/// Try to run a skill by exact name (used for unified slash-command namespace, #435). +/// Returns None when no skill with that name exists, so the caller can try other sources. +pub(in crate::commands) fn run_skill_by_name( + app: &mut crate::tui::app::App, + name: &str, + arg: Option<&str>, +) -> Option { + let registry = discover_visible_skills(app); + let lookup_name = if name == "new" { "skill-creator" } else { name }; + if registry.get(lookup_name).is_some() { + Some(activate_skill_with_task(app, name, arg)) + } else { + None + } } -fn render_skill_warnings(registry: &SkillRegistry) -> String { - if registry.warnings().is_empty() { - return String::new(); +/// Host-side activation helper shared with the dispatcher fallback. The +/// portable `/skill` path uses the `CommandSkillGroupContext` delegate instead +/// (D2); this App-carrying copy is retained for `run_skill_by_name` (FEAT-042). +fn activate_skill_with_task( + app: &mut crate::tui::app::App, + name: &str, + task: Option<&str>, +) -> CommandResult { + let mut result = activate_skill(app, name); + if !result.is_error + && let Some(task) = task.map(str::trim).filter(|task| !task.is_empty()) + { + result.action = Some(AppAction::SendMessage(task.to_string())); } + result +} - let mut out = String::new(); - let _ = writeln!(out, "\nWarnings ({}):", registry.warnings().len()); - for warning in registry.warnings() { - let _ = writeln!(out, " - {warning}"); +/// Host-side `/skill ` activation (FEAT-042 dispatcher machinery). +fn activate_skill(app: &mut crate::tui::app::App, name: &str) -> CommandResult { + // `/skill new` is a friendly alias for `/skill skill-creator`. + let name = if name == "new" { "skill-creator" } else { name }; + + let registry = discover_visible_skills(app); + + if let Some(skill) = registry.get(name) { + let plugin_provenance = match &skill.source { + crate::skills::SkillSource::Native => None, + crate::skills::SkillSource::Plugin { authority, .. } => { + if let Err(reason) = crate::plugins::registry::verify_plugin_component_authority( + authority, + crate::plugins::activation::PluginActivationCapability::Skills, + ) { + return CommandResult::error(format!( + "Plugin skill '{}' is no longer active: {reason}", + skill.name + )); + } + Some(authority.as_ref().clone()) + } + }; + let instruction = format!( + "You are now using a skill. Follow these instructions:\n\n# Skill: {}\n\n{}\n\n---\n\nNow respond to the user's request following the above skill instructions.", + skill.name, skill.body + ); + + app.add_message(crate::tui::history::HistoryCell::System { + content: format!("Activated skill: {}\n\n{}", skill.name, skill.description), + }); + + app.active_skill = Some(instruction); + app.active_skill_provenance = plugin_provenance; + + CommandResult::message(format!( + "Skill '{}' activated.\n\nDescription: {}\n\nType your request and the skill instructions will be applied.", + skill.name, skill.description + )) + } else { + let available: Vec = registry.list().iter().map(|s| s.name.clone()).collect(); + let warnings = render_skill_warnings(registry.warnings()); + + if available.is_empty() { + CommandResult::error(format!( + "Skill '{name}' not found. No skills installed.\n\nUse /skills to see how to add skills.{warnings}" + )) + } else { + CommandResult::error(format!( + "Skill '{}' not found.\n\nAvailable skills: {}{}", + name, + available.join(", "), + warnings + )) + } } - out } -fn skill_discovery_mode(app: &App) -> crate::skills::SkillDiscoveryMode { - crate::skills::SkillDiscoveryMode::from_codewhale_only(app.skills_scan_codewhale_only) -} +// --------------------------------------------------------------------------- +// Portable rendering helpers (byte-identical to the pre-migration handlers) +// --------------------------------------------------------------------------- -fn skill_discovery_mode_label(mode: crate::skills::SkillDiscoveryMode) -> &'static str { - match mode { - crate::skills::SkillDiscoveryMode::Compatible => "compatible", - crate::skills::SkillDiscoveryMode::CodeWhaleOnly => "codewhale-only", +/// Render registry warnings as the baseline suffix block. +fn render_skill_warnings(warnings: &[String]) -> String { + if warnings.is_empty() { + return String::new(); } -} -fn visible_skill_directories(app: &App) -> Vec { - crate::skills::skill_directories_for_workspace_and_dir( - &app.workspace, - &app.skills_dir, - skill_discovery_mode(app), - ) + let mut out = String::new(); + let _ = writeln!(out, "\nWarnings ({}):", warnings.len()); + for warning in warnings { + let _ = writeln!(out, " - {warning}"); + } + out } -fn skill_source_label(source: &SkillSource) -> String { +/// Source label used by `/skills inspect` (baseline `skill_source_label`). +fn skill_source_label(source: &SkillSourceKind) -> String { match source { - SkillSource::Native => "native".to_string(), - SkillSource::Plugin { - plugin_id, + SkillSourceKind::Native => "native".to_string(), + SkillSourceKind::Plugin { plugin_name, - .. + plugin_id, } => format!("reviewed plugin snapshot {plugin_name} ({plugin_id})"), } } -fn inspect_skills(app: &mut App) -> CommandResult { - let mode = skill_discovery_mode(app); - let dirs = visible_skill_directories(app); - let registry = discover_visible_skills(app); - let warnings = render_skill_warnings(®istry); +/// Network-policy approval message (baseline `needs_approval_message`). +fn needs_approval_message(host: &str) -> String { + format!( + "Network policy requires approval for {host}.\n\ + Add it to your allow list with `/network allow {host}` (or set [network].default = \"allow\" in ~/.codewhale/config.toml), then retry." + ) +} - let mut output = String::from("Skills Inspect\n"); - output.push_str("─────────────────────────────\n"); - let _ = writeln!( - output, - "Discovery mode: {}", - skill_discovery_mode_label(mode) - ); - let _ = writeln!(output, "Workspace: {}", app.workspace.display()); - let _ = writeln!( - output, - "Configured skills dir: {}", - app.skills_dir.display() - ); +/// Network-policy denial message (baseline `network_denied_message`). +fn network_denied_message(host: &str) -> String { + format!( + "Network policy denied access to {host}.\n\ + Remove the deny entry from ~/.codewhale/config.toml under [network] or contact your administrator." + ) +} - if dirs.is_empty() { - output.push_str("\nSearched directories: none found\n"); - } else { - let _ = writeln!(output, "\nSearched directories ({}):", dirs.len()); - for (idx, dir) in dirs.iter().enumerate() { - let _ = writeln!(output, " {}. {}", idx + 1, dir.display()); +/// Render a mutation receipt byte-identically (baseline `format_mutation_receipt`). +fn format_mutation_receipt(receipt: &SkillMutationReceipt) -> String { + match &receipt.outcome { + SkillMutationOutcome::Installed => format!( + "Installed skill '{}'.\nLocation: {}\n\nManage skills with /skills.", + receipt.name, receipt.safe_target_path + ), + SkillMutationOutcome::Updated => format!( + "Skill '{}' updated.\nLocation: {}", + receipt.name, receipt.safe_target_path + ), + SkillMutationOutcome::NoChange => { + format!("Skill '{}': no upstream change.", receipt.name) } + SkillMutationOutcome::Removed => format!("Removed skill '{}'.", receipt.name), + SkillMutationOutcome::Trusted => format!( + "Marked skill '{}' as trusted. The .trusted marker is advisory and digest-bound; it records your review intent but does not sandbox or auto-authorize scripts.", + receipt.name + ), + SkillMutationOutcome::Imported => format!( + "Imported skill '{}'.\nLocation: {}", + receipt.name, receipt.safe_target_path + ), + SkillMutationOutcome::AlreadyPresent => format!( + "Skill '{}' is already present at {} (exact duplicate).", + receipt.name, receipt.safe_target_path + ), + SkillMutationOutcome::NeedsApproval(host) => needs_approval_message(host), + SkillMutationOutcome::NetworkDenied(host) => network_denied_message(host), } +} - let _ = writeln!(output, "\nAvailable skills ({}):", registry.len()); - if registry.is_empty() { - output.push_str(" (none)\n"); - } else { - for skill in registry.list() { - if skill.description.trim().is_empty() { - let _ = writeln!(output, " - {}", skill.name); - } else { - let _ = writeln!(output, " - {} — {}", skill.name, skill.description); +/// Parse an optional `--project` / `--global` scope prefix (baseline +/// `parse_scope_args`, portable scope enum). +fn parse_scope_args(args: &str) -> Result<(Option, &str), String> { + let mut scope = None; + let mut rest = args.trim(); + loop { + if let Some(next) = rest.strip_prefix("--project") { + if scope.is_some() { + return Err("specify at most one of --project / --global".into()); } - let _ = writeln!(output, " source: {}", skill_source_label(&skill.source)); - if matches!(skill.source, SkillSource::Native) { - let _ = writeln!(output, " path: {}", skill.path.display()); + scope = Some(SkillTargetScope::Project); + rest = next.trim_start(); + continue; + } + if let Some(next) = rest.strip_prefix("--global") { + if scope.is_some() { + return Err("specify at most one of --project / --global".into()); } + scope = Some(SkillTargetScope::Global); + rest = next.trim_start(); + continue; } + break; } + Ok((scope, rest.trim())) +} - output.push_str(&warnings); - CommandResult::message(output) +// --------------------------------------------------------------------------- +// /skills — portable contextual dispatch +// --------------------------------------------------------------------------- + +pub(in crate::commands) const SKILLS_INFO: CommandInfo = CommandInfo { + name: "skills", + aliases: &["jinengliebiao"], + usage: "/skills [--remote|sync|inspect|suggest |] (bare opens manager)", + description_key: "cmd_skills_description", +}; + +pub(in crate::commands) struct SkillsCmd; + +impl RegisterCommand for SkillsCmd { + fn info() -> &'static CommandInfo { + &SKILLS_INFO + } + + fn handler() -> CommandHandler { + CommandHandler::Contextual(skills_contextual) + } +} + +/// Contextual `/skills` dispatch (FEAT-022 D4): exactly the skill-group facet. +fn skills_contextual(contexts: CommandContexts<'_>, arg: Option<&str>) -> CommandResult { + let mut parts = contexts.into_parts(); + let Some(skill_group) = parts.skill_group.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: skill_group"); + }; + list_skills(skill_group, arg) } -/// List all available skills. Pass `--remote` (or `remote`) to fetch the -/// curated registry instead of scanning the local skills directory. Pass -/// `suggest ` to rank remote catalog entries for a task without -/// installing anything. -/// Pass `sync` to pull the registry index and download all skills to the -/// local cache (`~/.codewhale/cache/skills/`). Pass `inspect` to show local -/// discovery mode, searched directories, and skill source paths. -fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { +/// Portable `/skills` dispatch — byte-identical to the baseline handler. +fn list_skills(group: &mut dyn CommandSkillGroupContext, arg: Option<&str>) -> CommandResult { let mut prefix: Option = None; if let Some(arg) = arg { let trimmed = arg.trim(); if trimmed == "--remote" || trimmed == "remote" { - return list_remote_skills(app); + return list_remote_skills(group); } if trimmed == "sync" || trimmed == "--sync" { - return sync_skills(app); + return sync_skills(group); } if trimmed == "inspect" || trimmed == "--inspect" { - return inspect_skills(app); + return inspect_skills(group); } if trimmed == "suggest" || trimmed == "recommend" { return CommandResult::error("Usage: /skills suggest "); @@ -176,7 +288,7 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { .strip_prefix("suggest ") .or_else(|| trimmed.strip_prefix("recommend ")) { - return suggest_remote_skills(app, task); + return suggest_remote_skills(group, task); } if !trimmed.is_empty() { // Anything else is treated as a name-prefix filter (#1318). @@ -195,11 +307,12 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { // Bare `/skills` opens the unified manager (owned-only, zero network). return CommandResult::action(AppAction::OpenSkillsManager); } - let skills_dir = app.skills_dir.clone(); - let registry = discover_visible_skills(app); - let warnings = render_skill_warnings(®istry); - if registry.is_empty() { + let projection = group.skill_registry_projection(); + let warnings = render_skill_warnings(&projection.warnings); + let skills_dir = projection.skills_dir.clone(); + + if projection.entries.is_empty() { let msg = format!( "No skills found.\n\n\ Skills location: {}\n\n\ @@ -211,20 +324,19 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { description: What this skill does\n \ ---\n\n \ {warnings}", - skills_dir.display(), - skills_dir.display() + skills_dir, skills_dir ); return CommandResult::message(msg); } - let filtered: Vec<&crate::skills::Skill> = if let Some(p) = prefix.as_deref() { - registry - .list() + let filtered: Vec<&SkillEntry> = if let Some(p) = prefix.as_deref() { + projection + .entries .iter() .filter(|s| s.name.to_ascii_lowercase().starts_with(p)) .collect() } else { - registry.list().iter().collect() + projection.entries.iter().collect() }; if filtered.is_empty() { @@ -234,7 +346,7 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { let p = prefix.as_deref().unwrap_or(""); return CommandResult::message(format!( "No skills match prefix `{p}` (out of {} available).\n\nRun /skills to see them all.{warnings}", - registry.len() + projection.total )); } @@ -242,10 +354,10 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { format!( "Available skills matching `{p}` ({} of {}):\n", filtered.len(), - registry.len() + projection.total ) } else { - format!("Available skills ({}):\n", registry.len()) + format!("Available skills ({}):\n", projection.total) }; output.push_str("─────────────────────────────\n"); @@ -259,13 +371,11 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { } } else { // Unfiltered view: keep user-created skills prominent, then split the - // shipped catalog into its two curated product tiers. - let (user_skills, bundled_skills): ( - Vec<&&crate::skills::Skill>, - Vec<&&crate::skills::Skill>, - ) = filtered - .iter() - .partition(|s| !crate::skills::is_bundled_skill_name(&s.name)); + // shipped catalog into its two curated product tiers. The tier + // classification is resolved host-side into `bundled_tier` so the + // canonical bundle-name list is never duplicated here. + let (user_skills, bundled_skills): (Vec<&SkillEntry>, Vec<&SkillEntry>) = + filtered.iter().partition(|s| s.bundled_tier.is_none()); if !user_skills.is_empty() { let _ = writeln!(output, "Your skills ({}):", user_skills.len()); @@ -278,15 +388,12 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { } if !bundled_skills.is_empty() { - use crate::skills::{BundledSkillTier, bundled_skill_tier}; - - let (core, tooling): (Vec<&&crate::skills::Skill>, Vec<&&crate::skills::Skill>) = - bundled_skills.into_iter().partition(|skill| { - bundled_skill_tier(&skill.name) == Some(BundledSkillTier::CoreAgentic) - }); + let (core, tooling): (Vec<&SkillEntry>, Vec<&SkillEntry>) = bundled_skills + .into_iter() + .partition(|skill| skill.bundled_tier == Some(SkillBundledTier::CoreAgentic)); for (group_idx, (tier, skills)) in [ - (BundledSkillTier::CoreAgentic, core), - (BundledSkillTier::FormatTooling, tooling), + (SkillBundledTier::CoreAgentic, core), + (SkillBundledTier::FormatTooling, tooling), ] .into_iter() .enumerate() @@ -319,404 +426,75 @@ fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { let _ = write!( output, "\nUse /skill to run a skill\nSkills location: {}{}", - skills_dir.display(), - warnings + skills_dir, warnings ); CommandResult::message(output) } -/// Run a specific skill — activates skill for next user message, or -/// dispatches a sub-command (`install`, `update`, `uninstall`, `trust`). -/// Try to run a skill by exact name (used for unified slash-command namespace, #435). -/// Returns None when no skill with that name exists, so the caller can try other sources. -pub(in crate::commands) fn run_skill_by_name( - app: &mut App, - name: &str, - arg: Option<&str>, -) -> Option { - let registry = discover_visible_skills(app); - let lookup_name = if name == "new" { "skill-creator" } else { name }; - if registry.get(lookup_name).is_some() { - Some(activate_skill_with_task(app, name, arg)) - } else { - None - } -} +/// `/skills inspect` — byte-identical discovery diagnostics. +fn inspect_skills(group: &mut dyn CommandSkillGroupContext) -> CommandResult { + let projection = group.skill_registry_projection(); + let warnings = render_skill_warnings(&projection.warnings); -fn run_skill(app: &mut App, name: Option<&str>) -> CommandResult { - let raw = match name { - Some(n) => n.trim(), - None => { - return CommandResult::error( - "Usage: /skill \n\nSubcommands:\n /skill install [--project|--global] >\n /skill update [--project|--global] \n /skill uninstall [--project|--global] \n /skill trust [--project|--global] ", - ); - } - }; + let mut output = String::from("Skills Inspect\n"); + output.push_str("─────────────────────────────\n"); + let _ = writeln!(output, "Discovery mode: {}", projection.mode_label); + let _ = writeln!(output, "Workspace: {}", projection.workspace); + let _ = writeln!(output, "Configured skills dir: {}", projection.skills_dir); - // Sub-command dispatch happens before the activation path so users can't - // accidentally activate a skill literally named "install". - let mut iter = raw.splitn(2, char::is_whitespace); - let head = iter.next().unwrap_or("").trim(); - let rest = iter.next().unwrap_or("").trim(); - match head { - "install" => return install_skill(app, rest), - "update" => return update_skill(app, rest), - "uninstall" => return uninstall_skill(app, rest), - "trust" => return trust_skill(app, rest), - _ => {} + if projection.dirs.is_empty() { + output.push_str("\nSearched directories: none found\n"); + } else { + let _ = writeln!( + output, + "\nSearched directories ({}):", + projection.dirs.len() + ); + for (idx, dir) in projection.dirs.iter().enumerate() { + let _ = writeln!(output, " {}. {}", idx + 1, dir); + } } - let task = (!rest.is_empty()).then_some(rest); - activate_skill_with_task(app, head, task) -} - -/// Parse optional `--project` / `--global` scope prefix from a skill subcommand. -fn parse_scope_args( - args: &str, -) -> Result<(Option, &str), String> { - use crate::skills::mutation::SkillTargetScope; - let mut scope = None; - let mut rest = args.trim(); - loop { - if let Some(next) = rest.strip_prefix("--project") { - if scope.is_some() { - return Err("specify at most one of --project / --global".into()); + let _ = writeln!(output, "\nAvailable skills ({}):", projection.total); + if projection.entries.is_empty() { + output.push_str(" (none)\n"); + } else { + for skill in &projection.entries { + if skill.description.trim().is_empty() { + let _ = writeln!(output, " - {}", skill.name); + } else { + let _ = writeln!(output, " - {} — {}", skill.name, skill.description); } - scope = Some(SkillTargetScope::Project); - rest = next.trim_start(); - continue; - } - if let Some(next) = rest.strip_prefix("--global") { - if scope.is_some() { - return Err("specify at most one of --project / --global".into()); + let _ = writeln!(output, " source: {}", skill_source_label(&skill.source)); + if let Some(path) = skill + .path + .as_ref() + .filter(|_| matches!(skill.source, SkillSourceKind::Native)) + { + let _ = writeln!(output, " path: {}", path); } - scope = Some(SkillTargetScope::Global); - rest = next.trim_start(); - continue; } - break; } - Ok((scope, rest.trim())) + + output.push_str(&warnings); + CommandResult::message(output) } -fn format_mutation_receipt(receipt: &crate::skills::mutation::SkillMutationReceipt) -> String { - use crate::skills::mutation::SkillMutationOutcome; - match &receipt.outcome { - SkillMutationOutcome::Installed => format!( - "Installed skill '{}'.\nLocation: {}\n\nManage skills with /skills.", - receipt.name, receipt.safe_target_path - ), - SkillMutationOutcome::Updated => format!( - "Skill '{}' updated.\nLocation: {}", - receipt.name, receipt.safe_target_path - ), - SkillMutationOutcome::NoChange => { - format!("Skill '{}': no upstream change.", receipt.name) - } - SkillMutationOutcome::Removed => format!("Removed skill '{}'.", receipt.name), - SkillMutationOutcome::Trusted => format!( - "Marked skill '{}' as trusted. The .trusted marker is advisory and digest-bound; it records your review intent but does not sandbox or auto-authorize scripts.", - receipt.name - ), - SkillMutationOutcome::Imported => format!( - "Imported skill '{}'.\nLocation: {}", - receipt.name, receipt.safe_target_path - ), - SkillMutationOutcome::AlreadyPresent => format!( - "Skill '{}' is already present at {} (exact duplicate).", - receipt.name, receipt.safe_target_path - ), - SkillMutationOutcome::NeedsApproval(host) => needs_approval_message(host), - SkillMutationOutcome::NetworkDenied(host) => network_denied_message(host), - } -} - -/// Activate a skill and, when the invocation includes a task, send that task -/// immediately. `AppAction::SendMessage` is converted into a `QueuedMessage` -/// by the UI, where `app.active_skill` is consumed and attached to this turn. -fn activate_skill_with_task(app: &mut App, name: &str, task: Option<&str>) -> CommandResult { - let mut result = activate_skill(app, name); - if !result.is_error - && let Some(task) = task.map(str::trim).filter(|task| !task.is_empty()) - { - result.action = Some(AppAction::SendMessage(task.to_string())); - } - result -} - -fn activate_skill(app: &mut App, name: &str) -> CommandResult { - // `/skill new` is a friendly alias for `/skill skill-creator`. - let name = if name == "new" { "skill-creator" } else { name }; - - let registry = discover_visible_skills(app); - - if let Some(skill) = registry.get(name) { - let plugin_provenance = match &skill.source { - SkillSource::Native => None, - SkillSource::Plugin { authority, .. } => { - if let Err(reason) = crate::plugins::registry::verify_plugin_component_authority( - authority, - crate::plugins::activation::PluginActivationCapability::Skills, - ) { - return CommandResult::error(format!( - "Plugin skill '{}' is no longer active: {reason}", - skill.name - )); - } - Some(authority.as_ref().clone()) - } - }; - let instruction = format!( - "You are now using a skill. Follow these instructions:\n\n# Skill: {}\n\n{}\n\n---\n\nNow respond to the user's request following the above skill instructions.", - skill.name, skill.body - ); - - app.add_message(HistoryCell::System { - content: format!("Activated skill: {}\n\n{}", skill.name, skill.description), - }); - - app.active_skill = Some(instruction); - app.active_skill_provenance = plugin_provenance; - - CommandResult::message(format!( - "Skill '{}' activated.\n\nDescription: {}\n\nType your request and the skill instructions will be applied.", - skill.name, skill.description - )) - } else { - let available: Vec = registry.list().iter().map(|s| s.name.clone()).collect(); - let warnings = render_skill_warnings(®istry); - - if available.is_empty() { - CommandResult::error(format!( - "Skill '{name}' not found. No skills installed.\n\nUse /skills to see how to add skills.{warnings}" - )) - } else { - CommandResult::error(format!( - "Skill '{}' not found.\n\nAvailable skills: {}{}", - name, - available.join(", "), - warnings - )) - } - } -} - -// ─── /skill install ──────────────────────────────────────────────────────── - -fn install_skill(app: &mut App, args: &str) -> CommandResult { - use crate::skills::mutation::{MutationContext, SkillMutationRequest, SkillTargetScope}; - - let (scope, spec) = match parse_scope_args(args) { - Ok(v) => v, - Err(err) => return CommandResult::error(err), - }; - if spec.is_empty() { - return CommandResult::error( - "Usage: /skill install [--project|--global] >", - ); - } - let source = match InstallSource::parse(spec) { - Ok(s) => s, - Err(err) => return CommandResult::error(format!("Invalid install source: {err}")), - }; - // Legacy no-scope install maps to the CodeWhale global owned root. - let target = scope.unwrap_or(SkillTargetScope::Global); - let workspace = app.workspace.clone(); - let home = crate::config::effective_home_dir(); - let (network, max_size, registry_url) = installer_settings(app); - - let outcome = run_async(async move { - let ctx = MutationContext { - workspace: &workspace, - home: home.as_deref(), - configured_skills_dir: None, - network: &network, - max_size, - registry_url: ®istry_url, - }; - crate::skills::mutation::execute( - SkillMutationRequest::InstallRemote { source, target }, - &ctx, - ) - .await - }); - - match outcome { - Ok(receipt) => { - if matches!( - receipt.outcome, - crate::skills::mutation::SkillMutationOutcome::Installed - ) { - app.refresh_skill_cache(); - } - let message = format_mutation_receipt(&receipt); - if matches!( - receipt.outcome, - crate::skills::mutation::SkillMutationOutcome::NeedsApproval(_) - | crate::skills::mutation::SkillMutationOutcome::NetworkDenied(_) - ) { - CommandResult::error(message) - } else { - CommandResult::message(message) - } - } - Err(err) => CommandResult::error(format!("Install failed: {err:#}")), - } -} - -// ─── /skill update ───────────────────────────────────────────────────────── - -fn update_skill(app: &mut App, args: &str) -> CommandResult { - use crate::skills::mutation::{MutationContext, SkillMutationRequest}; - - let (scope, name) = match parse_scope_args(args) { - Ok(v) => v, - Err(err) => return CommandResult::error(err), - }; - if name.is_empty() { - return CommandResult::error("Usage: /skill update [--project|--global] "); - } - let workspace = app.workspace.clone(); - let home = crate::config::effective_home_dir(); - let (network, max_size, registry_url) = installer_settings(app); - let owned_name = name.to_string(); - - let outcome = run_async(async move { - let ctx = MutationContext { - workspace: &workspace, - home: home.as_deref(), - configured_skills_dir: None, - network: &network, - max_size, - registry_url: ®istry_url, - }; - crate::skills::mutation::execute( - SkillMutationRequest::UpdateByName { - name: owned_name, - scope, - expected_digest: None, - }, - &ctx, - ) - .await - }); - - match outcome { - Ok(receipt) => { - if matches!( - receipt.outcome, - crate::skills::mutation::SkillMutationOutcome::Updated - ) { - app.refresh_skill_cache(); - } - let message = format_mutation_receipt(&receipt); - if matches!( - receipt.outcome, - crate::skills::mutation::SkillMutationOutcome::NeedsApproval(_) - | crate::skills::mutation::SkillMutationOutcome::NetworkDenied(_) - ) { - CommandResult::error(message) - } else { - CommandResult::message(message) - } - } - Err(err) => CommandResult::error(format!("Update failed: {err:#}")), - } -} - -// ─── /skill uninstall ────────────────────────────────────────────────────── - -fn uninstall_skill(app: &mut App, args: &str) -> CommandResult { - use crate::skills::mutation::{MutationContext, SkillMutationRequest}; - - let (scope, name) = match parse_scope_args(args) { - Ok(v) => v, - Err(err) => return CommandResult::error(err), - }; - if name.is_empty() { - return CommandResult::error("Usage: /skill uninstall [--project|--global] "); - } - let home = crate::config::effective_home_dir(); - let (network, max_size, registry_url) = installer_settings(app); - let ctx = MutationContext { - workspace: &app.workspace, - home: home.as_deref(), - configured_skills_dir: None, - network: &network, - max_size, - registry_url: ®istry_url, - }; - - match crate::skills::mutation::execute_sync( - SkillMutationRequest::RemoveByName { - name: name.to_string(), - scope, - expected_digest: None, - }, - &ctx, - ) { - Ok(receipt) => { - app.refresh_skill_cache(); - CommandResult::message(format_mutation_receipt(&receipt)) - } - Err(err) => CommandResult::error(format!("Uninstall failed: {err:#}")), - } -} - -// ─── /skill trust ────────────────────────────────────────────────────────── - -fn trust_skill(app: &mut App, args: &str) -> CommandResult { - use crate::skills::mutation::{MutationContext, SkillMutationRequest}; - - let (scope, name) = match parse_scope_args(args) { - Ok(v) => v, - Err(err) => return CommandResult::error(err), - }; - if name.is_empty() { - return CommandResult::error("Usage: /skill trust [--project|--global] "); - } - let home = crate::config::effective_home_dir(); - let (network, max_size, registry_url) = installer_settings(app); - let ctx = MutationContext { - workspace: &app.workspace, - home: home.as_deref(), - configured_skills_dir: None, - network: &network, - max_size, - registry_url: ®istry_url, - }; - - match crate::skills::mutation::execute_sync( - SkillMutationRequest::TrustByName { - name: name.to_string(), - scope, - expected_digest: None, - }, - &ctx, - ) { - Ok(receipt) => CommandResult::message(format_mutation_receipt(&receipt)), - Err(err) => CommandResult::error(format!("Trust failed: {err:#}")), - } -} - -// ─── /skills --remote ────────────────────────────────────────────────────── - -/// List skills available in the configured curated registry. -fn list_remote_skills(app: &mut App) -> CommandResult { - let (network, _max_size, registry_url) = installer_settings(app); - let registry = run_async(async move { install::fetch_registry(&network, ®istry_url).await }); - match registry { - Ok(RegistryFetchResult::Loaded(doc)) => { - if doc.skills.is_empty() { +/// `/skills --remote` — curated registry listing. +fn list_remote_skills(group: &mut dyn CommandSkillGroupContext) -> CommandResult { + match group.fetch_remote_registry() { + Ok(RemoteRegistryOutcome::Loaded { entries }) => { + if entries.is_empty() { return CommandResult::message("Registry is empty."); } - let mut out = format!("Available remote skills ({}):\n", doc.skills.len()); + let mut out = format!("Available remote skills ({}):\n", entries.len()); out.push_str("─────────────────────────────\n"); - for (name, entry) in &doc.skills { + for entry in &entries { let _ = writeln!( out, - " {name} — {} (source: {})", + " {} — {} (source: {})", + entry.name, entry.description.clone().unwrap_or_default(), entry.source ); @@ -724,32 +502,25 @@ fn list_remote_skills(app: &mut App) -> CommandResult { let _ = write!(out, "\nInstall with: /skill install "); CommandResult::message(out) } - Ok(RegistryFetchResult::NeedsApproval(host)) => { + Ok(RemoteRegistryOutcome::NeedsApproval(host)) => { CommandResult::error(needs_approval_message(&host)) } - Ok(RegistryFetchResult::Denied(host)) => { + Ok(RemoteRegistryOutcome::Denied(host)) => { CommandResult::error(network_denied_message(&host)) } - Err(err) => CommandResult::error(format_registry_error("Failed to fetch registry", &err)), + Err(err) => CommandResult::error(err), } } -// ─── /skills suggest ────────────────────────────────────────────────────── - -/// Recommend a small set of remote skills for a task. This performs the same -/// network-policy-gated registry read as `/skills --remote`, but it cannot -/// download, trust, enable, or activate a skill. -fn suggest_remote_skills(app: &mut App, task: &str) -> CommandResult { +/// `/skills suggest ` — ranked remote recommendations. +fn suggest_remote_skills(group: &mut dyn CommandSkillGroupContext, task: &str) -> CommandResult { let task = task.trim(); if task.chars().count() < 3 { return CommandResult::error("Usage: /skills suggest "); } - let (network, _max_size, registry_url) = installer_settings(app); - let registry = run_async(async move { install::fetch_registry(&network, ®istry_url).await }); - match registry { - Ok(RegistryFetchResult::Loaded(doc)) => { - let recommendations = crate::skills::recommend::recommend_remote_skills(task, &doc, 3); + match group.recommend_skills(task) { + Ok(recommendations) => { if recommendations.is_empty() { return CommandResult::message(format!( "No curated remote skills matched `{task}`.\n\nBrowse the catalog with /skills --remote. Nothing was installed, trusted, or enabled." @@ -758,9 +529,8 @@ fn suggest_remote_skills(app: &mut App, task: &str) -> CommandResult { let mut out = format!("Suggested remote skills for `{task}`:\n"); out.push_str("─────────────────────────────\n"); - for recommendation in recommendations { + for recommendation in &recommendations { let description = recommendation - .entry .description .as_deref() .filter(|description| !description.trim().is_empty()) @@ -776,63 +546,37 @@ fn suggest_remote_skills(app: &mut App, task: &str) -> CommandResult { out.push_str("\nNothing was installed, trusted, or enabled."); CommandResult::message(out) } - Ok(RegistryFetchResult::NeedsApproval(host)) => { - CommandResult::error(needs_approval_message(&host)) - } - Ok(RegistryFetchResult::Denied(host)) => { - CommandResult::error(network_denied_message(&host)) - } - Err(err) => CommandResult::error(format_registry_error("Failed to fetch registry", &err)), + Err(err) => CommandResult::error(err), } } -// ─── /skills sync ────────────────────────────────────────────────────────── - -/// Fetch the remote registry index and download every listed skill into the -/// local cache (`~/.codewhale/cache/skills//`). -/// -/// For each skill the sync checks the cached ETag / SHA-256 before -/// downloading so unchanged skills are skipped in O(1) network round-trips. -fn sync_skills(app: &mut App) -> CommandResult { - let (network, max_size, registry_url) = installer_settings(app); - let cache_dir = install::default_cache_skills_dir(); - - let result = run_async(async move { - install::sync_registry(&network, ®istry_url, &cache_dir, max_size).await - }); - - match result { - Ok(SyncResult::RegistryDenied(host)) => CommandResult::error(network_denied_message(&host)), - Ok(SyncResult::RegistryNeedsApproval(host)) => { - CommandResult::error(needs_approval_message(&host)) - } - Ok(SyncResult::Done { outcomes }) => { - let total = outcomes.len(); - let mut downloaded = 0usize; - let mut fresh = 0usize; - let mut failed = 0usize; +/// `/skills sync` — registry sync report. +fn sync_skills(group: &mut dyn CommandSkillGroupContext) -> CommandResult { + match group.sync_registry() { + Ok(SkillSyncOutcome::Done { + total, + downloaded, + fresh, + failed, + entries, + }) => { let mut out = String::from("Registry sync complete.\n\n"); - for outcome in &outcomes { + for outcome in &entries { match outcome { - SkillSyncOutcome::Downloaded { name, path } => { - downloaded += 1; - let _ = writeln!(out, " [+] {name} — downloaded to {}", path.display()); + SkillSyncEntry::Downloaded { name, path } => { + let _ = writeln!(out, " [+] {name} — downloaded to {path}"); } - SkillSyncOutcome::Fresh { name } => { - fresh += 1; + SkillSyncEntry::Fresh { name } => { let _ = writeln!(out, " [=] {name} — already up to date"); } - SkillSyncOutcome::Failed { name, reason } => { - failed += 1; + SkillSyncEntry::Failed { name, reason } => { let _ = writeln!(out, " [!] {name} — failed: {reason}"); } - SkillSyncOutcome::Denied { name, host } => { - failed += 1; + SkillSyncEntry::Denied { name, host } => { let _ = writeln!(out, " [!] {name} — network denied ({host})"); } - SkillSyncOutcome::NeedsApproval { name, host } => { - failed += 1; + SkillSyncEntry::NeedsApproval { name, host } => { let _ = writeln!( out, " [?] {name} — needs approval for {host} (run `/network allow {host}` then retry)" @@ -848,826 +592,822 @@ fn sync_skills(app: &mut App) -> CommandResult { CommandResult::message(out) } - Err(err) => CommandResult::error(format_registry_error("Sync failed", &err)), + Ok(SkillSyncOutcome::RegistryNeedsApproval(host)) => { + CommandResult::error(needs_approval_message(&host)) + } + Ok(SkillSyncOutcome::RegistryDenied(host)) => { + CommandResult::error(network_denied_message(&host)) + } + Err(err) => CommandResult::error(err), } } -// ─── helpers ─────────────────────────────────────────────────────────────── - -/// Read the active config knobs for the installer. -/// -/// We load `Config::load` on demand because [`App`] does not carry a `Config` -/// field — and loading is cheap (small TOML file) compared to the network -/// round-trip the install/update operation will incur next. If the config -/// fails to parse, we fall back to defaults so the user still gets a -/// network-gated install rather than a silent crash. -fn installer_settings(_app: &App) -> (NetworkPolicy, u64, String) { - let cfg = crate::config::Config::load(None, None).unwrap_or_default(); - let network = cfg - .network - .clone() - .map(|policy| policy.into_runtime()) - .unwrap_or_default(); - let skills_cfg = cfg.skills.as_ref(); - let max_size = skills_cfg - .and_then(|s| s.max_install_size_bytes) - .unwrap_or(DEFAULT_MAX_SIZE_BYTES); - let registry_url = skills_cfg - .and_then(|s| s.registry_url.clone()) - .unwrap_or_else(|| DEFAULT_REGISTRY_URL.to_string()); - (network, max_size, registry_url) -} +// --------------------------------------------------------------------------- +// /skill — portable contextual dispatch +// --------------------------------------------------------------------------- -fn run_async(future: F) -> T -where - F: std::future::Future, -{ - // We're on the TUI's thread, which is part of the multi-threaded runtime. - // `block_in_place` + `Handle::current().block_on` bridges sync - // slash-command handlers back into the async ecosystem. - tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(future)) -} +pub(in crate::commands) const SKILL_INFO: CommandInfo = CommandInfo { + name: "skill", + aliases: &["jineng"], + usage: "/skill |update |uninstall |trust >", + description_key: "cmd_skill_description", +}; -fn needs_approval_message(host: &str) -> String { - format!( - "Network policy requires approval for {host}.\n\ - Add it to your allow list with `/network allow {host}` (or set [network].default = \"allow\" in ~/.codewhale/config.toml), then retry." - ) +pub(in crate::commands) struct SkillCmd; + +impl RegisterCommand for SkillCmd { + fn info() -> &'static CommandInfo { + &SKILL_INFO + } + + fn handler() -> CommandHandler { + CommandHandler::Contextual(skill_contextual) + } } -fn network_denied_message(host: &str) -> String { - format!( - "Network policy denied access to {host}.\n\ - Remove the deny entry from ~/.codewhale/config.toml under [network] or contact your administrator." - ) +/// Contextual `/skill` dispatch (FEAT-022 D4): exactly the skill-group facet +/// plus the shared SKILLS facet (active-skill reads + cache refresh; D2). +fn skill_contextual(contexts: CommandContexts<'_>, arg: Option<&str>) -> CommandResult { + let mut parts = contexts.into_parts(); + let Some(skill_group) = parts.skill_group.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: skill_group"); + }; + let Some(skills) = parts.skills.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: skills"); + }; + run_skill(skill_group, skills, arg) } -/// 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). The chain itself is still rendered with `{err:#}`; this hint is -/// appended below it so users on `/skills --remote` and `/skills sync` get an -/// actionable next step instead of an opaque reqwest error. -fn registry_fetch_error_hint(err: &anyhow::Error) -> Option<&'static str> { - let msg = format!("{err:#}").to_lowercase(); - if msg.contains("dns") - || msg.contains("name resolution") - || msg.contains("getaddrinfo") - || msg.contains("nodename nor servname") - { - Some( - "Hint: DNS lookup failed. Check internet/DNS connectivity, or override the registry URL in [skills] of ~/.codewhale/config.toml.", - ) - } else if msg.contains("connection refused") - || msg.contains("connection reset") - || msg.contains("connection aborted") - { - Some( - "Hint: connection refused/reset. The registry host may be unreachable from this network (corporate proxy, firewall, offline).", - ) - } else if msg.contains("tls") - || msg.contains("certificate") - || msg.contains("ssl") - || msg.contains("handshake") - { - Some( - "Hint: TLS handshake failed. The system trust store may be missing the registry's CA, or a TLS-intercepting proxy is rewriting the certificate.", - ) - } else if msg.contains(" 404") || msg.contains("not found") { - Some( - "Hint: registry URL returned 404. Verify the registry URL in [skills] of ~/.codewhale/config.toml.", - ) - } else if msg.contains(" 401") || msg.contains(" 403") || msg.contains("forbidden") { - Some( - "Hint: registry returned an auth error. The registry may require credentials or have been moved.", - ) - } else if msg.contains(" 429") || msg.contains("rate limit") || msg.contains("too many") { - Some("Hint: rate-limited by the registry. Try again in a moment.") - } else if msg.contains("timed out") || msg.contains("timeout") { - Some("Hint: request timed out. Network may be slow or the registry host may be down.") - } else { - None +/// Portable `/skill` dispatch — byte-identical to the baseline handler. +fn run_skill( + group: &mut dyn CommandSkillGroupContext, + skills: &mut dyn CommandSkillsContext, + arg: Option<&str>, +) -> CommandResult { + let raw = match arg { + Some(n) => n.trim(), + None => { + return CommandResult::error( + "Usage: /skill \n\nSubcommands:\n /skill install [--project|--global] >\n /skill update [--project|--global] \n /skill uninstall [--project|--global] \n /skill trust [--project|--global] ", + ); + } + }; + + // Sub-command dispatch happens before the activation path so users can't + // accidentally activate a skill literally named "install". + let mut iter = raw.splitn(2, char::is_whitespace); + let head = iter.next().unwrap_or("").trim(); + let rest = iter.next().unwrap_or("").trim(); + match head { + "install" => return install_skill(group, skills, rest), + "update" => return update_skill(group, skills, rest), + "uninstall" => return uninstall_skill(group, skills, rest), + "trust" => return trust_skill(group, rest), + _ => {} } + + let task = (!rest.is_empty()).then_some(rest); + activate_skill_portable(group, head, task) } -fn format_registry_error(prefix: &str, err: &anyhow::Error) -> String { - let mut out = format!("{prefix}: {err:#}"); - if let Some(hint) = registry_fetch_error_hint(err) { - out.push_str("\n\n"); - out.push_str(hint); +/// Portable activation — the host performs lookup, authority verification, and +/// side effects; the handler composes the byte-identical messages/actions. +fn activate_skill_portable( + group: &mut dyn CommandSkillGroupContext, + name: &str, + task: Option<&str>, +) -> CommandResult { + // `/skill new` is a friendly alias for `/skill skill-creator`; the alias is + // resolved here (parsing stays portable) so the not-found message uses the + // mapped name exactly like the baseline. + let name = if name == "new" { "skill-creator" } else { name }; + + match group.activate_skill(name) { + Ok(outcome) => { + let mut result = CommandResult::message(format!( + "Skill '{}' activated.\n\nDescription: {}\n\nType your request and the skill instructions will be applied.", + outcome.name, outcome.description + )); + if let Some(task) = task.map(str::trim).filter(|task| !task.is_empty()) { + result.action = Some(AppAction::SendMessage(task.to_string())); + } + result + } + Err(SkillActivationError::NotFound { + requested, + available, + warnings, + }) => { + let warnings = render_skill_warnings(&warnings); + if available.is_empty() { + CommandResult::error(format!( + "Skill '{requested}' not found. No skills installed.\n\nUse /skills to see how to add skills.{warnings}" + )) + } else { + CommandResult::error(format!( + "Skill '{}' not found.\n\nAvailable skills: {}{}", + requested, + available.join(", "), + warnings + )) + } + } + Err(SkillActivationError::PluginRejected { name, reason }) => CommandResult::error( + format!("Plugin skill '{}' is no longer active: {reason}", name), + ), } - out } -pub(in crate::commands) const SKILLS_INFO: crate::commands::traits::CommandInfo = - crate::commands::traits::CommandInfo { - name: "skills", - aliases: &["jinengliebiao"], - usage: "/skills [--remote|sync|inspect|suggest |] (bare opens manager)", - description_id: crate::localization::MessageId::CmdSkillsDescription, +// ─── /skill install ──────────────────────────────────────────────────────── + +fn install_skill( + group: &mut dyn CommandSkillGroupContext, + skills: &mut dyn CommandSkillsContext, + args: &str, +) -> CommandResult { + let (scope, spec) = match parse_scope_args(args) { + Ok(v) => v, + Err(err) => return CommandResult::error(err), }; + if spec.is_empty() { + return CommandResult::error( + "Usage: /skill install [--project|--global] >", + ); + } + match group.install_skill(scope, spec) { + Ok(receipt) => { + // Cache refresh is a D2 shared-SKILLS operation: the host returns + // the receipt; the portable handler owns the refresh policy. + if matches!(receipt.outcome, SkillMutationOutcome::Installed) { + skills.refresh_skill_cache(); + } + let message = format_mutation_receipt(&receipt); + if matches!( + receipt.outcome, + SkillMutationOutcome::NeedsApproval(_) | SkillMutationOutcome::NetworkDenied(_) + ) { + CommandResult::error(message) + } else { + CommandResult::message(message) + } + } + Err(err) => CommandResult::error(err), + } +} -pub(in crate::commands) struct SkillsCmd; +// ─── /skill update ───────────────────────────────────────────────────────── -impl crate::commands::traits::RegisterCommand for SkillsCmd { - fn info() -> &'static crate::commands::traits::CommandInfo { - &SKILLS_INFO +fn update_skill( + group: &mut dyn CommandSkillGroupContext, + skills: &mut dyn CommandSkillsContext, + args: &str, +) -> CommandResult { + let (scope, name) = match parse_scope_args(args) { + Ok(v) => v, + Err(err) => return CommandResult::error(err), + }; + if name.is_empty() { + return CommandResult::error("Usage: /skill update [--project|--global] "); } - - fn execute( - app: &mut crate::tui::app::App, - arg: Option<&str>, - ) -> crate::commands::CommandResult { - list_skills(app, arg) + match group.update_skill(scope, name) { + Ok(receipt) => { + if matches!(receipt.outcome, SkillMutationOutcome::Updated) { + skills.refresh_skill_cache(); + } + let message = format_mutation_receipt(&receipt); + if matches!( + receipt.outcome, + SkillMutationOutcome::NeedsApproval(_) | SkillMutationOutcome::NetworkDenied(_) + ) { + CommandResult::error(message) + } else { + CommandResult::message(message) + } + } + Err(err) => CommandResult::error(err), } } -pub(in crate::commands) const SKILL_INFO: crate::commands::traits::CommandInfo = - crate::commands::traits::CommandInfo { - name: "skill", - aliases: &["jineng"], - usage: "/skill |update |uninstall |trust >", - description_id: crate::localization::MessageId::CmdSkillDescription, +// ─── /skill uninstall ────────────────────────────────────────────────────── + +fn uninstall_skill( + group: &mut dyn CommandSkillGroupContext, + skills: &mut dyn CommandSkillsContext, + args: &str, +) -> CommandResult { + let (scope, name) = match parse_scope_args(args) { + Ok(v) => v, + Err(err) => return CommandResult::error(err), }; + if name.is_empty() { + return CommandResult::error("Usage: /skill uninstall [--project|--global] "); + } + match group.uninstall_skill(scope, name) { + Ok(receipt) => { + skills.refresh_skill_cache(); + CommandResult::message(format_mutation_receipt(&receipt)) + } + Err(err) => CommandResult::error(err), + } +} -pub(in crate::commands) struct SkillCmd; +// ─── /skill trust ────────────────────────────────────────────────────────── -impl crate::commands::traits::RegisterCommand for SkillCmd { - fn info() -> &'static crate::commands::traits::CommandInfo { - &SKILL_INFO +fn trust_skill(group: &mut dyn CommandSkillGroupContext, args: &str) -> CommandResult { + let (scope, name) = match parse_scope_args(args) { + Ok(v) => v, + Err(err) => return CommandResult::error(err), + }; + if name.is_empty() { + return CommandResult::error("Usage: /skill trust [--project|--global] "); } - - fn execute( - app: &mut crate::tui::app::App, - arg: Option<&str>, - ) -> crate::commands::CommandResult { - run_skill(app, arg) + match group.trust_skill(scope, name) { + Ok(receipt) => CommandResult::message(format_mutation_receipt(&receipt)), + Err(err) => CommandResult::error(err), } } #[cfg(test)] mod tests { use super::*; - use crate::config::Config; - use crate::tui::app::{App, TuiOptions}; - use std::ffi::OsString; - use tempfile::TempDir; - - struct IsolatedHome { - _lock: crate::test_support::TestEnvLock, - home_prev: Option, - userprofile_prev: Option, - test_home_prev: Option, - } - - impl IsolatedHome { - fn new(tmpdir: &TempDir) -> Self { - let lock = crate::test_support::lock_test_env(); - let home = tmpdir.path().join("home"); - std::fs::create_dir_all(&home).unwrap(); - let home_prev = std::env::var_os("HOME"); - let userprofile_prev = std::env::var_os("USERPROFILE"); - // SAFETY: tests that mutate process env hold the shared test env - // mutex for the full lifetime of this guard. - unsafe { - std::env::set_var("HOME", &home); - std::env::set_var("USERPROFILE", &home); - } - let test_home_prev = TEST_HOME_DIR.with(|slot| slot.replace(Some(home))); - Self { - _lock: lock, - home_prev, - userprofile_prev, - test_home_prev, - } + use codewhale_command_contract::facets::{ + CommandApprovalState, RemoteRegistryOutcome, RemoteSkillEntry, ReviewOutcome, + SkillActivationError, SkillActivationOutcome, SkillRecommendation, SkillRegistryProjection, + SkillSourceKind, SnapshotEntry, + }; + + /// Shared SKILLS fake: read-only getters + cache refresh (D2 surface). + struct FakeSkills { + refreshed: bool, + } + impl CommandSkillsContext for FakeSkills { + fn active_skill(&self) -> Option { + None + } + fn active_skill_provenance(&self) -> Option { + None + } + fn refresh_skill_cache(&mut self) { + self.refreshed = true; } + } - unsafe fn restore_var(key: &str, value: Option) { - if let Some(value) = value { - unsafe { std::env::set_var(key, value) }; - } else { - unsafe { std::env::remove_var(key) }; - } + /// Counting fake for preserving the baseline's exact cache-refresh policy. + #[derive(Default)] + struct CountingSkills { + refresh_count: usize, + } + impl CommandSkillsContext for CountingSkills { + fn active_skill(&self) -> Option { + None + } + fn active_skill_provenance(&self) -> Option { + None + } + fn refresh_skill_cache(&mut self) { + self.refresh_count += 1; } } - impl Drop for IsolatedHome { - fn drop(&mut self) { - TEST_HOME_DIR.with(|slot| { - *slot.borrow_mut() = self.test_home_prev.take(); - }); - // SAFETY: the shared test env mutex is still held while Drop runs. - unsafe { - Self::restore_var("HOME", self.home_prev.take()); - Self::restore_var("USERPROFILE", self.userprofile_prev.take()); + /// Deterministic fake skill-group facet over portable values only. + struct FakeSkillGroup { + projection: SkillRegistryProjection, + activation: Result, + install: Result, + update: Result, + uninstall: Result, + trust: Result, + remote: Result, + recommend: Result, String>, + sync: Result, + review: Result, + snapshots: Result, String>, + restore: Result<(), String>, + approval: CommandApprovalState, + } + + impl FakeSkillGroup { + fn new(entries: Vec) -> Self { + let total = entries.len(); + Self { + projection: SkillRegistryProjection { + workspace: "/ws".to_string(), + skills_dir: "/ws/.codewhale/skills".to_string(), + mode_label: "compatible".to_string(), + dirs: vec!["/ws/.codewhale/skills".to_string()], + entries, + warnings: vec![], + total, + }, + activation: Ok(SkillActivationOutcome { + name: "demo".to_string(), + description: "Demo skill".to_string(), + }), + install: Ok(SkillMutationReceipt { + name: "demo".to_string(), + safe_target_path: "/ws/.codewhale/skills/demo".to_string(), + outcome: SkillMutationOutcome::Installed, + }), + update: Ok(SkillMutationReceipt { + name: "demo".to_string(), + safe_target_path: "/ws/.codewhale/skills/demo".to_string(), + outcome: SkillMutationOutcome::Updated, + }), + uninstall: Ok(SkillMutationReceipt { + name: "demo".to_string(), + safe_target_path: "/ws/.codewhale/skills/demo".to_string(), + outcome: SkillMutationOutcome::Removed, + }), + trust: Ok(SkillMutationReceipt { + name: "demo".to_string(), + safe_target_path: "/ws/.codewhale/skills/demo".to_string(), + outcome: SkillMutationOutcome::Trusted, + }), + remote: Ok(RemoteRegistryOutcome::Loaded { + entries: vec![RemoteSkillEntry { + name: "remote-demo".to_string(), + description: Some("Remote demo".to_string()), + source: "github.com/acme/skills".to_string(), + }], + }), + recommend: Ok(vec![SkillRecommendation { + name: "remote-demo".to_string(), + description: Some("Remote demo".to_string()), + matched_terms: vec!["demo".to_string()], + }]), + sync: Ok(SkillSyncOutcome::Done { + total: 1, + downloaded: 1, + fresh: 0, + failed: 0, + entries: vec![SkillSyncEntry::Downloaded { + name: "demo".to_string(), + path: "/cache/demo".to_string(), + }], + }), + review: Ok(ReviewOutcome::Ready), + snapshots: Ok(vec![SnapshotEntry { + id: "abcdef123456".to_string(), + label: "pre-turn:1".to_string(), + timestamp: 1_700_000_000, + }]), + restore: Ok(()), + approval: CommandApprovalState { + yolo: true, + trust_mode: false, + }, } } } - fn create_test_app_with_tmpdir(tmpdir: &TempDir) -> App { - let options = TuiOptions { - skills_dir: tmpdir.path().join("skills"), - memory_path: tmpdir.path().join("memory.md"), - notes_path: tmpdir.path().join("notes.txt"), - mcp_config_path: tmpdir.path().join("mcp.json"), - ..crate::test_support::test_tui_options(tmpdir.path()) - }; - let mut app = App::new(options, &Config::default()); - app.skills_dir = tmpdir.path().join("skills"); - app + impl CommandSkillGroupContext for FakeSkillGroup { + fn skill_registry_projection(&self) -> SkillRegistryProjection { + self.projection.clone() + } + fn activate_skill( + &mut self, + _name: &str, + ) -> Result { + self.activation.clone() + } + fn install_skill( + &mut self, + _scope: Option, + _spec: &str, + ) -> Result { + self.install.clone() + } + fn update_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + self.update.clone() + } + fn uninstall_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + self.uninstall.clone() + } + fn trust_skill( + &mut self, + _scope: Option, + _name: &str, + ) -> Result { + self.trust.clone() + } + fn fetch_remote_registry(&mut self) -> Result { + self.remote.clone() + } + fn recommend_skills(&mut self, _task: &str) -> Result, String> { + self.recommend.clone() + } + fn sync_registry(&mut self) -> Result { + self.sync.clone() + } + fn run_review(&mut self) -> Result { + self.review.clone() + } + fn snapshot_list(&mut self, _limit: usize) -> Result, String> { + self.snapshots.clone() + } + fn restore_snapshot(&mut self, _id: &str) -> Result<(), String> { + self.restore.clone() + } + fn approval_state(&self) -> CommandApprovalState { + self.approval + } } - fn create_skill_dir(tmpdir: &TempDir, skill_name: &str, skill_content: &str) { - let skill_dir = tmpdir.path().join("skills").join(skill_name); - std::fs::create_dir_all(&skill_dir).unwrap(); - std::fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap(); + fn demo_entry() -> SkillEntry { + SkillEntry { + name: "demo".to_string(), + description: "Demo skill".to_string(), + source: SkillSourceKind::Native, + path: Some("/ws/.codewhale/skills/demo".to_string()), + bundled_tier: None, + } } - #[test] - fn registry_fetch_error_hint_recognises_dns_failures() { - let err = anyhow::Error::msg("error sending request: dns error: failed to lookup") - .context("failed to fetch registry https://example.com/registry.json"); - let hint = registry_fetch_error_hint(&err).expect("dns hint"); - assert!(hint.contains("DNS"), "got: {hint}"); + fn bundled_entry(name: &str, tier: SkillBundledTier) -> SkillEntry { + SkillEntry { + name: name.to_string(), + description: format!("{name} skill"), + source: SkillSourceKind::Native, + path: None, + bundled_tier: Some(tier), + } } - #[test] - fn registry_fetch_error_hint_recognises_connection_refused() { - let err = anyhow::Error::msg("error sending request: tcp connect: connection refused"); - let hint = registry_fetch_error_hint(&err).expect("refused hint"); - assert!(hint.contains("refused"), "got: {hint}"); - } + // ── /skills parity ──────────────────────────────────────────────────── #[test] - fn registry_fetch_error_hint_recognises_tls_failures() { - let err = anyhow::Error::msg("invalid peer certificate: UnknownIssuer (TLS handshake)"); - let hint = registry_fetch_error_hint(&err).expect("tls hint"); - assert!(hint.contains("TLS"), "got: {hint}"); + fn bare_skills_opens_manager_action() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, None); + assert!(result.message.is_none()); + assert!(matches!(result.action, Some(AppAction::OpenSkillsManager))); } #[test] - fn registry_fetch_error_hint_recognises_http_status_codes() { - let err_404 = anyhow::Error::msg("registry returned an error status: 404 Not Found"); - assert!( - registry_fetch_error_hint(&err_404) - .map(|h| h.contains("404")) - .unwrap_or(false) - ); - let err_429 = - anyhow::Error::msg("registry returned an error status: 429 Too Many Requests"); + fn skills_empty_registry_message_is_exact() { + let mut group = FakeSkillGroup::new(vec![]); + let result = list_skills(&mut group, Some("")); + let msg = result.message.expect("expected message"); assert!( - registry_fetch_error_hint(&err_429) - .map(|h| h.contains("rate")) - .unwrap_or(false) + msg.starts_with("No skills found.\n\nSkills location: /ws/.codewhale/skills\n"), + "{msg}" ); + assert!(msg.contains("/ws/.codewhale/skills/my-skill/SKILL.md")); } #[test] - fn registry_fetch_error_hint_returns_none_for_unrecognised_errors() { - let err = anyhow::Error::msg("a totally novel error nobody anticipated"); - assert!(registry_fetch_error_hint(&err).is_none()); + fn skills_prefix_listing_flat_format_is_exact() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("de")); + let msg = result.message.expect("expected message"); + assert!( + msg.starts_with("Available skills matching `de` (1 of 1):\n"), + "{msg}" + ); + assert!(msg.contains(" /demo - Demo skill")); } #[test] - fn format_registry_error_appends_hint_when_pattern_matches() { - let err = anyhow::Error::msg("dns error: nodename nor servname provided"); - let formatted = format_registry_error("Failed to fetch registry", &err); - assert!(formatted.starts_with("Failed to fetch registry: ")); + fn skills_no_match_reports_prefix_and_total() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("zzz")); + let msg = result.message.expect("expected message"); assert!( - formatted.contains("Hint: DNS"), - "expected hint, got: {formatted}" + msg.starts_with("No skills match prefix `zzz` (out of 1 available)."), + "{msg}" ); } #[test] - fn format_registry_error_omits_hint_when_no_pattern_matches() { - let err = anyhow::Error::msg("inscrutable opaque failure"); - let formatted = format_registry_error("Sync failed", &err); - assert_eq!(formatted, "Sync failed: inscrutable opaque failure"); + fn skills_unfiltered_splits_user_and_bundled_tiers() { + let mut group = FakeSkillGroup::new(vec![ + demo_entry(), + bundled_entry("skill-creator", SkillBundledTier::FormatTooling), + bundled_entry("help", SkillBundledTier::CoreAgentic), + ]); + let result = list_skills(&mut group, Some("")); + let msg = result.message.expect("expected message"); + assert!(msg.contains("Your skills (1):"), "{msg}"); + assert!(msg.contains("Core agentic (1):"), "{msg}"); + assert!(msg.contains(" /help"), "{msg}"); + assert!(msg.contains("Format & tooling (1):"), "{msg}"); + assert!(msg.contains(" /skill-creator"), "{msg}"); + assert!( + msg.contains("(run /skills for details on a built-in)"), + "{msg}" + ); } #[test] - fn test_bare_skills_opens_manager() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, None); - assert!(matches!(result.action, Some(AppAction::OpenSkillsManager))); + fn skills_rejects_flag_like_and_multiword_prefixes() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("-x")); + assert!(result.is_error); + assert!( + result + .message + .unwrap() + .contains("Usage: /skills [--remote|sync|inspect|suggest |]") + ); + let result = list_skills(&mut group, Some("two words")); + assert!(result.is_error); } #[test] - fn test_list_skills_empty_directory() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - // Empty arg still uses the legacy text inventory (prefix path). - let result = list_skills(&mut app, Some("")); - assert!(result.message.is_some()); - let msg = result.message.unwrap(); - assert!(msg.contains("No skills found")); - assert!(msg.contains("Skills location:")); + fn skills_suggest_requires_meaningful_task() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("suggest")); + assert!(result.is_error); assert!( - !msg.contains("allowed-tools"), - "empty-state template must not advertise unenforced tool restrictions: {msg}" + result + .message + .unwrap() + .contains("Usage: /skills suggest ") ); + let result = list_skills(&mut group, Some("suggest ab")); + assert!(result.is_error); + assert!(result.message.unwrap().contains("at least 3 characters")); } #[test] - fn test_list_skills_with_skills() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "test-skill", - "---\nname: test-skill\ndescription: A test skill\n---\nDo something", - ); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("")); - assert!(result.message.is_some()); - let msg = result.message.unwrap(); - assert!(msg.contains("Available skills")); - assert!(msg.contains("/test-skill")); + fn skills_inspect_reports_discovery_details() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("inspect")); + let msg = result.message.expect("expected message"); + assert!(msg.starts_with("Skills Inspect\n"), "{msg}"); + assert!(msg.contains("Discovery mode: compatible")); + assert!(msg.contains("Workspace: /ws")); + assert!(msg.contains("Configured skills dir: /ws/.codewhale/skills")); + assert!(msg.contains("Searched directories (1):")); + assert!(msg.contains("Available skills (1):")); + assert!(msg.contains("source: native")); + assert!(msg.contains("path: /ws/.codewhale/skills/demo")); } #[test] - fn test_list_skills_filters_by_name_prefix() { - // #1318: a `/skills ` argument should narrow the list to - // skills whose names start with the prefix. The header reflects - // both the matched count and the registry total so the user - // knows what they're looking at. - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "alpha-skill", - "---\nname: alpha-skill\ndescription: First\n---\nbody", - ); - create_skill_dir( - &tmpdir, - "alphabet-helper", - "---\nname: alphabet-helper\ndescription: Helper\n---\nbody", - ); - create_skill_dir( - &tmpdir, - "beta-skill", - "---\nname: beta-skill\ndescription: Second\n---\nbody", - ); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("alph")); - let msg = result.message.expect("filter result has message"); - - assert!(msg.contains("/alpha-skill")); - assert!(msg.contains("/alphabet-helper")); + fn skills_remote_lists_entries_and_policy_errors() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("--remote")); + let msg = result.message.expect("expected message"); + assert!(msg.contains("Available remote skills (1):"), "{msg}"); + assert!(msg.contains("remote-demo — Remote demo (source: github.com/acme/skills)")); + assert!(msg.contains("\nInstall with: /skill install ")); + + group.remote = Ok(RemoteRegistryOutcome::NeedsApproval("acme.com".to_string())); + let result = list_skills(&mut group, Some("remote")); + assert!(result.is_error); assert!( - !msg.contains("/beta-skill"), - "beta-skill must be filtered out" + result + .message + .unwrap() + .contains("Network policy requires approval for acme.com") ); + + group.remote = Ok(RemoteRegistryOutcome::Denied("acme.com".to_string())); + let result = list_skills(&mut group, Some("remote")); + assert!(result.is_error); assert!( - msg.contains("matching `alph`") && msg.contains("2 of 3"), - "header should show count + total, got: {msg}" + result + .message + .unwrap() + .contains("Network policy denied access to acme.com") ); - } - #[test] - fn test_list_skills_filter_is_case_insensitive() { - // Prefix matching is case-insensitive — typing `Alph` finds - // `alpha-skill` the same as `alph` does. - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "alpha-skill", - "---\nname: alpha-skill\ndescription: First\n---\nbody", + group.remote = Err("Failed to fetch registry: boom".to_string()); + let result = list_skills(&mut group, Some("--remote")); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Failed to fetch registry: boom" ); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("ALPH")); - let msg = result.message.expect("case-insensitive filter has message"); - assert!(msg.contains("/alpha-skill")); } #[test] - fn test_list_skills_filter_with_zero_matches_says_so() { - // When the prefix matches nothing, the message must say so - // explicitly (rather than printing an empty list) and point - // the user back at the unfiltered command. - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "alpha-skill", - "---\nname: alpha-skill\ndescription: First\n---\nbody", - ); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("nonexistent")); - let msg = result.message.expect("zero-match filter still has message"); - assert!(msg.contains("No skills match prefix `nonexistent`")); - assert!(msg.contains("Run /skills")); + fn skills_suggest_renders_recommendations() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("suggest demo")); + let msg = result.message.expect("expected message"); + assert!(msg.contains("Suggested remote skills for `demo`:"), "{msg}"); + assert!(msg.contains(" remote-demo — Remote demo")); + assert!(msg.contains(" Why: demo")); + assert!(msg.contains(" Install if you want it: /skill install remote-demo")); + assert!(msg.contains("\nNothing was installed, trusted, or enabled.")); } #[test] - fn test_list_skills_rejects_flag_like_prefix() { - // `--remote` and `sync` stay reserved as subcommands; any other - // dash-prefixed argument is rejected so we don't silently turn - // a future flag into a no-match filter. - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("--bogus")); - assert!( - result.is_error, - "expected usage error for --bogus, got: {result:?}" - ); + fn skills_sync_renders_per_skill_report() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let result = list_skills(&mut group, Some("sync")); + let msg = result.message.expect("expected message"); + assert!(msg.starts_with("Registry sync complete.\n"), "{msg}"); + assert!(msg.contains(" [+] demo — downloaded to /cache/demo")); + assert!(msg.contains("\n1 skill(s) processed: 1 downloaded, 0 up-to-date, 0 failed.")); + + group.sync = Ok(SkillSyncOutcome::RegistryNeedsApproval( + "acme.com".to_string(), + )); + let result = list_skills(&mut group, Some("sync")); + assert!(result.is_error); assert!( result .message - .as_deref() - .is_some_and(|m| m.contains("name-prefix")), - "expected --bogus error message to mention name-prefix, got: {result:?}" + .unwrap() + .contains("requires approval for acme.com") ); } + // ── /skill parity ───────────────────────────────────────────────────── + #[test] - fn test_list_skills_suggest_requires_a_meaningful_task_before_network_access() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - - for arg in ["suggest", "recommend", "suggest go"] { - let result = list_skills(&mut app, Some(arg)); - assert!( - result.is_error, - "expected usage error for {arg}: {result:?}" - ); - assert!( - result - .message - .as_deref() - .is_some_and(|message| message.contains("/skills suggest ")); } #[test] - fn test_list_skills_renders_user_skills_under_your_skills_section() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "alpha-skill", - "---\nname: alpha-skill\ndescription: First skill\n---\nDo alpha work", - ); - create_skill_dir( - &tmpdir, - "beta-skill", - "---\nname: beta-skill\ndescription: Second skill\n---\nDo beta work", + fn skill_activation_success_composes_message_and_task_action() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("demo")); + assert!(!result.is_error); + let msg = result.message.expect("expected message"); + assert!( + msg.starts_with("Skill 'demo' activated.\n\nDescription: Demo skill"), + "{msg}" ); + assert!(result.action.is_none()); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("")); - let msg = result.message.unwrap(); - - // User-created skills must appear in their own section so they - // stay visible even when many bundled skills are installed. - let section = msg - .find("Your skills") - .expect("user skills section header missing"); - let alpha = msg.find("/alpha-skill").expect("alpha skill should render"); - let beta = msg.find("/beta-skill").expect("beta skill should render"); + let result = run_skill(&mut group, &mut skills, Some("demo do the thing")); assert!( - alpha > section, - "alpha-skill should follow the header: {msg}" + matches!(result.action, Some(AppAction::SendMessage(ref t)) if t == "do the thing") ); - assert!(beta > section, "beta-skill should follow the header: {msg}"); - // Each entry on its own line with the description inline. - assert!(msg.contains("/alpha-skill - First skill"), "got: {msg}"); - assert!(msg.contains("/beta-skill - Second skill"), "got: {msg}"); } #[test] - fn test_list_skills_tiers_bundled_catalog_and_omits_false_image_capability() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - crate::skills::install_system_skills(&app.skills_dir).unwrap(); - - let result = list_skills(&mut app, Some("")); - let msg = result.message.unwrap(); - let core = msg.find("Core agentic").expect("core tier"); - let best = msg.find("/best-of-n").expect("best-of-n skill"); - let tooling = msg.find("Format & tooling").expect("tooling tier"); - let pdf = msg.find("/pdf").expect("pdf skill"); - - assert!(core < best && best < tooling && tooling < pdf, "got: {msg}"); + fn skill_new_aliases_skill_creator_in_not_found_message() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + group.activation = Err(SkillActivationError::NotFound { + requested: "skill-creator".to_string(), + available: vec!["demo".to_string()], + warnings: vec![], + }); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("new")); + assert!(result.is_error); assert!( - !msg.contains("/imagine"), - "catalog must not advertise an unavailable image-generation tool: {msg}" + result + .message + .unwrap() + .contains("Skill 'skill-creator' not found.") ); } #[test] - fn test_list_skills_merges_workspace_and_configured_dirs() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let workspace_skill_dir = tmpdir - .path() - .join(".agents") - .join("skills") - .join("workspace-skill"); - std::fs::create_dir_all(&workspace_skill_dir).unwrap(); - std::fs::write( - workspace_skill_dir.join("SKILL.md"), - "---\nname: workspace-skill\ndescription: Workspace skill\n---\nDo workspace work", - ) - .unwrap(); - create_skill_dir( - &tmpdir, - "configured-skill", - "---\nname: configured-skill\ndescription: Configured skill\n---\nDo configured work", - ); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("")); + fn skill_not_found_lists_available_and_warnings() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + group.activation = Err(SkillActivationError::NotFound { + requested: "missing".to_string(), + available: vec!["demo".to_string()], + warnings: vec!["one warning".to_string()], + }); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("missing")); + assert!(result.is_error); let msg = result.message.unwrap(); - - assert!(msg.contains("/workspace-skill"), "got: {msg}"); - assert!(msg.contains("/configured-skill"), "got: {msg}"); + assert!(msg.contains("Skill 'missing' not found."), "{msg}"); + assert!(msg.contains("Available skills: demo"), "{msg}"); + assert!(msg.contains("Warnings (1):"), "{msg}"); + assert!(msg.contains(" - one warning"), "{msg}"); } #[test] - fn test_skills_inspect_reports_discovery_details_and_source_paths() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let workspace_skill_dir = tmpdir - .path() - .join(".agents") - .join("skills") - .join("workspace-skill"); - std::fs::create_dir_all(&workspace_skill_dir).unwrap(); - std::fs::write( - workspace_skill_dir.join("SKILL.md"), - "---\nname: workspace-skill\ndescription: Workspace skill\n---\nDo workspace work", - ) - .unwrap(); - create_skill_dir( - &tmpdir, - "configured-skill", - "---\nname: configured-skill\ndescription: Configured skill\n---\nDo configured work", - ); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app, Some("inspect")); - let msg = result.message.expect("inspect should return a message"); - - let normalized = msg.replace('\\', "/"); - assert!(normalized.contains("Skills Inspect"), "got: {msg}"); + fn skill_not_found_with_no_skills_uses_install_hint() { + let mut group = FakeSkillGroup::new(vec![]); + group.activation = Err(SkillActivationError::NotFound { + requested: "missing".to_string(), + available: vec![], + warnings: vec![], + }); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("missing")); + assert!(result.is_error); assert!( - normalized.contains("Discovery mode: compatible"), - "got: {msg}" + result + .message + .unwrap() + .contains("No skills installed.\n\nUse /skills to see how to add skills.") ); - assert!(normalized.contains("Searched directories"), "got: {msg}"); - assert!(normalized.contains(".agents/skills"), "got: {msg}"); - assert!(normalized.contains("skills"), "got: {msg}"); - assert!(normalized.contains("Available skills (2):"), "got: {msg}"); - assert!(normalized.contains("workspace-skill"), "got: {msg}"); - assert!(normalized.contains("configured-skill"), "got: {msg}"); - assert!(normalized.contains("path:"), "got: {msg}"); } #[test] - fn test_list_skills_respects_codewhale_only_scan() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let claude_skill_dir = tmpdir - .path() - .join(".claude") - .join("skills") - .join("claude-skill"); - std::fs::create_dir_all(&claude_skill_dir).unwrap(); - std::fs::write( - claude_skill_dir.join("SKILL.md"), - "---\nname: claude-skill\ndescription: Claude skill\n---\nbody", - ) - .unwrap(); - let codewhale_skill_dir = tmpdir - .path() - .join(".codewhale") - .join("skills") - .join("codewhale-skill"); - std::fs::create_dir_all(&codewhale_skill_dir).unwrap(); - std::fs::write( - codewhale_skill_dir.join("SKILL.md"), - "---\nname: codewhale-skill\ndescription: CodeWhale skill\n---\nbody", - ) - .unwrap(); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - app.skills_dir = tmpdir.path().join(".codewhale").join("skills"); - app.skills_scan_codewhale_only = true; - let result = list_skills(&mut app, Some("")); - let msg = result.message.unwrap(); - - assert!(msg.contains("/codewhale-skill"), "got: {msg}"); - assert!(!msg.contains("/claude-skill"), "got: {msg}"); + fn skill_plugin_rejected_renders_exact_error() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + group.activation = Err(SkillActivationError::PluginRejected { + name: "plug".to_string(), + reason: "authority revoked".to_string(), + }); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("plug")); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Plugin skill 'plug' is no longer active: authority revoked" + ); } #[test] - fn test_skills_inspect_reports_codewhale_only_scan_mode() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let claude_skill_dir = tmpdir - .path() - .join(".claude") - .join("skills") - .join("claude-skill"); - std::fs::create_dir_all(&claude_skill_dir).unwrap(); - std::fs::write( - claude_skill_dir.join("SKILL.md"), - "---\nname: claude-skill\ndescription: Claude skill\n---\nbody", - ) - .unwrap(); - let codewhale_skill_dir = tmpdir - .path() - .join(".codewhale") - .join("skills") - .join("codewhale-skill"); - std::fs::create_dir_all(&codewhale_skill_dir).unwrap(); - std::fs::write( - codewhale_skill_dir.join("SKILL.md"), - "---\nname: codewhale-skill\ndescription: CodeWhale skill\n---\nbody", - ) - .unwrap(); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - app.skills_dir = tmpdir.path().join(".codewhale").join("skills"); - app.skills_scan_codewhale_only = true; - let result = list_skills(&mut app, Some("--inspect")); - let msg = result.message.expect("inspect should return a message"); - - let normalized = msg.replace('\\', "/"); + fn skill_install_receipt_refreshes_cache_exactly_once() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = CountingSkills::default(); + let result = run_skill(&mut group, &mut skills, Some("install github:acme/demo")); + assert!(!result.is_error); assert!( - normalized.contains("Discovery mode: codewhale-only"), - "got: {msg}" + result + .message + .unwrap() + .starts_with("Installed skill 'demo'.\nLocation: /ws/.codewhale/skills/demo"), + ); + assert_eq!( + skills.refresh_count, 1, + "Installed receipt must refresh the skill cache exactly once" ); - assert!(normalized.contains("codewhale-skill"), "got: {msg}"); - assert!(!normalized.contains("claude-skill"), "got: {msg}"); - assert!(!normalized.contains(".claude/skills"), "got: {msg}"); } #[test] - fn test_skill_subcommand_dispatch_install_usage() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - // Empty install spec → usage hint, not invalid-source error. - let result = run_skill(&mut app, Some("install")); - let msg = result.message.unwrap(); - assert!(msg.contains("/skill install"), "got: {msg}"); + fn skill_update_and_uninstall_refresh_cache_exactly_once_each() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = CountingSkills::default(); + let result = run_skill(&mut group, &mut skills, Some("update demo")); + assert!(!result.is_error); + assert_eq!(skills.refresh_count, 1, "update refresh count"); + + skills.refresh_count = 0; + let result = run_skill(&mut group, &mut skills, Some("uninstall --global demo")); + assert!(!result.is_error); + assert_eq!(skills.refresh_count, 1, "uninstall refresh count"); + assert!(result.message.unwrap().contains("Removed skill 'demo'.")); } #[test] - fn test_skill_subcommand_dispatch_uninstall_missing() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = run_skill(&mut app, Some("uninstall absent-skill")); - let msg = result.message.unwrap(); + fn skill_trust_does_not_refresh_cache() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = CountingSkills::default(); + let result = run_skill(&mut group, &mut skills, Some("trust demo")); + assert!(!result.is_error); + assert_eq!(skills.refresh_count, 0, "trust must not refresh the cache"); assert!( - msg.contains("not found") || msg.contains("not installed"), - "got: {msg}" + result + .message + .unwrap() + .contains("Marked skill 'demo' as trusted.") ); } #[test] - fn test_skill_trust_message_marks_marker_advisory() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - // Mutations only touch CodeWhale-owned roots; place under project scope. - let skill_dir = tmpdir - .path() - .join(".codewhale") - .join("skills") - .join("trusted-skill"); - std::fs::create_dir_all(&skill_dir).unwrap(); - std::fs::write( - skill_dir.join("SKILL.md"), - "---\nname: trusted-skill\ndescription: Trust copy\n---\nbody", - ) - .unwrap(); - install::write_installed_from_v2( - &skill_dir, - "github:owner/repo", - None, - "src", - "placeholder", - "trusted-skill", - ) - .unwrap(); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = run_skill(&mut app, Some("trust --project trusted-skill")); - assert!(!result.is_error, "got: {:?}", result.message); - let msg = result.message.expect("trust result"); - assert!(msg.contains("advisory"), "got: {msg}"); - assert!(!msg.contains("may now invoke"), "got: {msg}"); + fn skill_install_empty_spec_prints_usage() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill(&mut group, &mut skills, Some("install")); + assert!(result.is_error); + assert!(result.message.unwrap().contains("Usage: /skill install")); } #[test] - fn parse_scope_args_and_default_install_target_is_global() { - use crate::skills::mutation::SkillTargetScope; - - let (scope, rest) = parse_scope_args("github:o/r").unwrap(); - assert_eq!(scope, None); - assert_eq!(rest, "github:o/r"); - // Bare install (no --project/--global) maps to the CodeWhale global root. - assert_eq!( - scope.unwrap_or(SkillTargetScope::Global), - SkillTargetScope::Global + fn skill_scope_conflict_errors() { + let mut group = FakeSkillGroup::new(vec![demo_entry()]); + let mut skills = FakeSkills { refreshed: false }; + let result = run_skill( + &mut group, + &mut skills, + Some("install --project --global x"), ); - - let (scope, rest) = parse_scope_args("--project my-skill").unwrap(); - assert_eq!(scope, Some(SkillTargetScope::Project)); - assert_eq!(rest, "my-skill"); - - let (scope, rest) = parse_scope_args("--global my-skill").unwrap(); - assert_eq!(scope, Some(SkillTargetScope::Global)); - assert_eq!(rest, "my-skill"); - - assert!(parse_scope_args("--project --global x").is_err()); - } - - #[test] - fn uninstall_external_only_skill_refuses_write() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let ext = tmpdir - .path() - .join(".claude") - .join("skills") - .join("ext-only"); - std::fs::create_dir_all(&ext).unwrap(); - std::fs::write( - ext.join("SKILL.md"), - "---\nname: ext-only\ndescription: d\n---\nbody\n", - ) - .unwrap(); - let sentinel = tmpdir - .path() - .join(".claude") - .join("skills") - .join("SENTINEL"); - std::fs::write(&sentinel, "keep").unwrap(); - - let mut app = create_test_app_with_tmpdir(&tmpdir); - app.workspace = tmpdir.path().to_path_buf(); - let result = run_skill(&mut app, Some("uninstall ext-only")); - assert!(result.is_error, "got: {:?}", result.message); - let msg = result.message.unwrap_or_default(); + assert!(result.is_error); assert!( - msg.contains("compatible external") || msg.contains("not found"), - "got: {msg}" + result + .message + .unwrap() + .contains("specify at most one of --project / --global") ); - assert_eq!(std::fs::read_to_string(&sentinel).unwrap(), "keep"); - assert!(ext.join("SKILL.md").is_file()); - } - - #[test] - fn test_run_skill_without_name() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = run_skill(&mut app, None); - assert!(result.message.is_some()); - assert!(result.message.unwrap().contains("Usage: /skill")); } #[test] - fn test_run_skill_not_found() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = run_skill(&mut app, Some("nonexistent")); - assert!(result.message.is_some()); - let msg = result.message.unwrap(); - assert!(msg.contains("not found")); - } - - #[test] - fn test_run_skill_activates() { - let tmpdir = TempDir::new().unwrap(); - let _home = IsolatedHome::new(&tmpdir); - create_skill_dir( - &tmpdir, - "test-skill", - "---\nname: test-skill\ndescription: A test skill\n---\nDo something special", + fn skill_missing_facet_errors_are_safe() { + let result = skills_contextual(CommandContexts::empty(), Some("demo")); + assert!(result.is_error); + assert_eq!( + result.message.unwrap(), + "Error: Command capability unavailable: skill_group" ); - let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = run_skill(&mut app, Some("test-skill")); - assert!(result.message.is_some()); - let msg = result.message.unwrap(); - assert!(msg.contains("Skill 'test-skill' activated")); - assert!(msg.contains("A test skill")); - assert!(app.active_skill.is_some()); - assert!(!app.history.is_empty()); } } diff --git a/crates/tui/src/commands/mod.rs b/crates/tui/src/commands/mod.rs index 129899bbfb..a0a6d09601 100644 --- a/crates/tui/src/commands/mod.rs +++ b/crates/tui/src/commands/mod.rs @@ -1941,6 +1941,11 @@ mod tests { "lsp", "share", "goal", + // FEAT-022 skills group. + "skills", + "skill", + "review", + "restore", ]; for info in command_infos() { if info.name == "feat015ctx" || MIGRATED_GROUPS.contains(&info.name) { @@ -2152,4 +2157,162 @@ mod tests { ); } } + // --------------------------------------------------------------------- + // FEAT-022: skills group registration + public dispatch (Task 6.2). + // All four commands register through the portable bridge; frontier state + // is asserted by the migration fixtures and live gate. + // --------------------------------------------------------------------- + + /// Pins HOME to a tempdir so global skill discovery stays hermetic. + struct Feat022ScopedHome { + prev: Option, + _home: tempfile::TempDir, + _guard: crate::test_support::TestEnvLock, + } + impl Drop for Feat022ScopedHome { + fn drop(&mut self) { + // SAFETY: process-wide lock still held. + unsafe { + match self.prev.take() { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + } + } + } + fn feat022_scoped_home(_tmp: &tempfile::TempDir) -> Feat022ScopedHome { + let guard = crate::test_support::lock_test_env(); + let prev = std::env::var_os("HOME"); + let home = tempfile::TempDir::new().expect("home tempdir"); + // SAFETY: serialised by the global env lock. + unsafe { + std::env::set_var("HOME", home.path()); + } + Feat022ScopedHome { + prev, + _home: home, + _guard: guard, + } + } + + fn feat022_test_app(tmp: &tempfile::TempDir) -> App { + let mut options = crate::test_support::test_tui_options(tmp.path()); + options.skills_dir = tmp.path().join("skills"); + crate::test_support::test_app_with_options(options) + } + + fn feat022_write_skill(dir: &std::path::Path, name: &str) { + let skill_dir = dir.join(name); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {name} skill\n---\n{name} instructions"), + ) + .unwrap(); + } + + #[test] + fn feat022_all_four_skills_entries_are_registered_with_portable_handlers() { + for (name, alias) in [ + ("skills", Some("jinengliebiao")), + ("skill", Some("jineng")), + ("review", Some("shencha")), + ("restore", None), + ] { + let info = registry() + .get_info(name) + .unwrap_or_else(|| panic!("/{name} must be registered")); + assert_eq!(info.name, name, "canonical name"); + assert!( + registry().has_contextual_handler(name), + "/{name} must carry a portable handler" + ); + if let Some(alias) = alias { + assert!( + registry().get_info(alias).is_some(), + "/{name} alias {alias} must resolve" + ); + } + } + } + + #[test] + fn feat022_skills_commands_dispatch_through_public_seam() { + let tmp = tempfile::TempDir::new().unwrap(); + let _home = feat022_scoped_home(&tmp); + let mut app = feat022_test_app(&tmp); + std::fs::create_dir_all(tmp.path().join("skills")).unwrap(); + feat022_write_skill(&tmp.path().join("skills"), "demo"); + + // Bare /skills opens the unified manager (zero network). + let result = execute("/skills", &mut app); + assert!(!result.is_error, "{result:?}"); + assert!( + matches!( + result.action, + Some(crate::tui::app::AppAction::OpenSkillsManager) + ), + "{result:?}" + ); + + // /skill activates the demo skill and sets active_skill. + let result = execute("/skill demo", &mut app); + assert!(!result.is_error, "{result:?}"); + assert!(result.message.unwrap().contains("Skill 'demo' activated.")); + assert!(app.active_skill.is_some()); + + // /restore with no snapshots shows the empty message. + let result = execute("/restore", &mut app); + assert!(!result.is_error, "{result:?}"); + assert!(result.message.unwrap().contains("No snapshots")); + + // /review without a target prints usage. + let result = execute("/review", &mut app); + assert!(result.is_error, "{result:?}"); + assert!(result.message.unwrap().contains("Usage: /review")); + } + + #[test] + fn feat022_aliases_dispatch_through_public_seam() { + // All four aliases (jinengliebiao, jineng, shencha) resolve through the + // registry to the same portable handlers as the canonical names. + let tmp = tempfile::TempDir::new().unwrap(); + let _home = feat022_scoped_home(&tmp); + let mut app = feat022_test_app(&tmp); + std::fs::create_dir_all(tmp.path().join("skills")).unwrap(); + feat022_write_skill(&tmp.path().join("skills"), "demo"); + + let result = execute("/jinengliebiao", &mut app); + assert!( + matches!( + result.action, + Some(crate::tui::app::AppAction::OpenSkillsManager) + ), + "{result:?}" + ); + + let result = execute("/jineng demo", &mut app); + assert!(!result.is_error, "{result:?}"); + assert!(result.message.unwrap().contains("Skill 'demo' activated.")); + + let result = execute("/shencha", &mut app); + assert!(result.is_error, "{result:?}"); + assert!(result.message.unwrap().contains("Usage: /review")); + } + + #[test] + fn feat022_context_exposure_is_exact_per_d4() { + // The envelope exposes every adapter (main's model); the handlers + // consume exactly their required facets. skills/review/restore consume + // only skill_group; skill also consumes skills for cache refreshes. + let tmp = tempfile::TempDir::new().unwrap(); + let _home = feat022_scoped_home(&tmp); + let mut app = feat022_test_app(&tmp); + let mut bundle = app.command_contexts(); + let parts = bundle.parts(); + assert!(parts.skill_group.is_some()); + assert!(parts.skills.is_some()); + // Missing-facet safety through the public seam is covered by the + // handler-level tests; here we assert the envelope carries both. + } } diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index d8ebf3c5f7..1674ce69ef 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -2583,6 +2583,7 @@ fn merge_usage_totals(into: &mut UsageTotals, from: &UsageTotals) { into.turns = into.turns.saturating_add(from.turns); } +#[allow(clippy::too_many_arguments)] // pre-existing baseline signature; FEAT-022 gate repair fn accumulate_runtime_cost_coverage( audit: Option<&crate::pricing::TurnCostAudit>, priced_turns: &mut u64, diff --git a/crates/tui/src/skills/system.rs b/crates/tui/src/skills/system.rs index 5f94535b95..fdfb437ff2 100644 --- a/crates/tui/src/skills/system.rs +++ b/crates/tui/src/skills/system.rs @@ -273,14 +273,6 @@ impl BundledSkillTier { Self::FormatTooling => "tools", } } - - #[must_use] - pub const fn heading(self) -> &'static str { - match self { - Self::CoreAgentic => "Core agentic", - Self::FormatTooling => "Format & tooling", - } - } } /// Return the curated tier for a bundled skill name. diff --git a/crates/tui/src/tui/underwater.rs b/crates/tui/src/tui/underwater.rs index 6732904592..af6a78ea04 100644 --- a/crates/tui/src/tui/underwater.rs +++ b/crates/tui/src/tui/underwater.rs @@ -1222,6 +1222,7 @@ impl<'a> LaunchComposerDisplay<'a> { /// beneath. This is the same composer state the conversation view edits, /// not a second input system; only the geometry is the startup stage's /// dock. +#[allow(clippy::too_many_arguments)] // pre-existing baseline signature; FEAT-022 gate repair fn render_launch_composer( area: Rect, buf: &mut Buffer, diff --git a/scripts/check-command-migration-manifest.py b/scripts/check-command-migration-manifest.py index c79f5703ed..6003ed6cc3 100644 --- a/scripts/check-command-migration-manifest.py +++ b/scripts/check-command-migration-manifest.py @@ -751,6 +751,27 @@ def _first_param_type(fn_sig: str) -> str | None: return None +# --------------------------------------------------------------------------- +# Retained host machinery (FEAT-042 tracking) +# --------------------------------------------------------------------------- +# +# The migration topology is immutable, so the dispatcher-only host helpers that +# intentionally keep `&mut App` after a group migrates are declared here — the +# gate's own enforcement home. Each entry maps a migrated group to selectors +# that must keep their concrete-App signature until FEAT-042 extracts them to a +# host-side module; a missing or refactored-away signature fails the gate, so +# the tracking cannot silently go stale. FEAT-022: the skills group retains the +# unified slash-command fallback and its activation helpers co-located with the +# portable handlers (D7). +RETAINED_HOST_MACHINERY: dict[str, list[dict]] = { + "skills": [ + {"kind": "free", "item": ["crate", "commands", "groups", "skills", "skills", "run_skill_by_name"]}, + {"kind": "free", "item": ["crate", "commands", "groups", "skills", "skills", "activate_skill_with_task"]}, + {"kind": "free", "item": ["crate", "commands", "groups", "skills", "skills", "activate_skill"]}, + ], +} + + def _is_concrete_app_type(param_type: str | None) -> bool: if param_type is None: return False @@ -951,6 +972,24 @@ def _self_type_qual(self_type: str, module_path: str) -> str: return f"{module_path}::{base}" +def _selector_matches(selector: dict, item: RustItem) -> bool: + """Match one RustItem against a checked-in selector (shared by + `resolve_selector` and the retained-host source scan).""" + kind = selector["kind"] + if kind == "free": + target = "::".join(selector["item"]) + return item.kind == "free" and item.qual_path == target + if kind == "inherent": + self_qual = _selector_type_to_text(selector["self_type"]) + return item.kind == "inherent" and item.name == selector["method"] \ + and item.qual_path.startswith(f"{self_qual}::") + self_qual = _selector_type_to_text(selector["self_type"]) + trait_qual = _selector_type_to_text(selector["trait_path"]) + return item.kind == "trait_impl" and item.name == selector["method"] \ + and item.qual_path.startswith(f"{self_qual}::") \ + and f"[{trait_qual}]" in item.qual_path + + def resolve_selector(selector: dict, items: list[RustItem]) -> list[SourceScanViolation]: """Resolve one checked-in handler selector against parsed items. @@ -1084,6 +1123,25 @@ def check_source_frontier(topology: dict, frontier: list[str], root: Path = REPO violations.extend(resolve_selector(selector, group_items)) continue + # Validate retained host machinery declarations first so the tracking + # stays fail-closed even when the group has no other concrete-App + # handlers (e.g. every retained helper lost its signature at once). + retained_names: set[str] = set() + for selector in RETAINED_HOST_MACHINERY.get(group_name, []): + matches = [it for it in group_items if _selector_matches(selector, it)] + if not matches: + violations.append(SourceScanViolation( + "retained-host", json.dumps(selector, sort_keys=True), + f"retained host machinery selector resolves to no source item in {group_name!r}", + )) + for match in matches: + if not match.is_concrete_app: + violations.append(SourceScanViolation( + "retained-host", match.qual_path, + "retained host machinery must keep its concrete-App signature until FEAT-042 extracts it", + )) + retained_names.add(match.qual_path) + if not handlers: continue @@ -1116,16 +1174,18 @@ def check_source_frontier(topology: dict, frontier: list[str], root: Path = REPO )) continue - # Not pending and not split: every remaining handler is a stale removal. - for handler in handlers[:5]: + # Not pending and not split: every remaining handler is a stale removal, + # except the retained host machinery resolved above. + stale = [h for h in handlers if h.qual_path not in retained_names] + for handler in stale[:5]: violations.append(SourceScanViolation( "stale-removal", handler.qual_path, f"handler still uses concrete App but group {group_name!r} is not pending", )) - if len(handlers) > 5: + if len(stale) > 5: violations.append(SourceScanViolation( "stale-removal", group_name, - f"... and {len(handlers) - 5} more concrete-App handlers in this group", + f"... and {len(stale) - 5} more concrete-App handlers in this group", )) return violations diff --git a/scripts/command-migration-topology.json b/scripts/command-migration-topology.json index 45f4150391..327ef17063 100644 --- a/scripts/command-migration-topology.json +++ b/scripts/command-migration-topology.json @@ -284,7 +284,6 @@ "debug", "memory", "plugins", - "session", - "skills" + "session" ] -} +} \ No newline at end of file diff --git a/scripts/test_check_command_migration_manifest.py b/scripts/test_check_command_migration_manifest.py index 43a96a1f85..acb0a917ca 100644 --- a/scripts/test_check_command_migration_manifest.py +++ b/scripts/test_check_command_migration_manifest.py @@ -367,7 +367,7 @@ def test_topology_artifact_is_sorted_unique(self) -> None: # groups stay pending. self.assertEqual( set(frontier), - {"memory", "plugins", "skills", "session", "config", "debug", "core"}, + {"memory", "plugins", "session", "config", "debug", "core"}, ) @@ -514,6 +514,75 @@ def test_stale_frontier_entry_fails(self) -> None: violations = mod.check_source_frontier(doc["topology"], ["session", "utility"], root) self.assertTrue(any("stale-entry" in str(v) for v in violations)) + def test_retained_host_exempts_declared_machinery_from_stale_removal(self) -> None: + """A migrated group may declare dispatcher host machinery (FEAT-042) + that keeps `&mut App`; the gate exempts it and flags the rest.""" + topology = { + "alpha": { + "kind": "group", + "scope": ["alpha/mod.rs"], + "slices": [], + } + } + frontier: list[str] = [] + import tempfile + with tempfile.TemporaryDirectory() as d: + root = Path(d) + (root / "alpha").mkdir(parents=True, exist_ok=True) + (root / "alpha" / "mod.rs").write_text( + "use crate::tui::app::App;\n" + "fn retained(app: &mut App, arg: Option<&str>) {} \n" + "fn stale(app: &mut App, arg: Option<&str>) {} \n", + encoding="utf-8", + ) + # stub RETAINED_HOST_MACHINERY for the hermetic group + original = mod.RETAINED_HOST_MACHINERY + try: + mod.RETAINED_HOST_MACHINERY = { + "alpha": [ + {"kind": "free", "item": ["crate", "commands", "groups", "alpha", "retained"]} + ] + } + violations = mod.check_source_frontier(topology, frontier, root) + finally: + mod.RETAINED_HOST_MACHINERY = original + kinds = [v.category for v in violations] + self.assertNotIn("retained-host", kinds) + self.assertEqual(kinds.count("stale-removal"), 1, violations) + + def test_retained_host_fails_closed_when_signature_lost(self) -> None: + """If retained machinery loses its concrete-App signature, the gate fails.""" + topology = { + "alpha": { + "kind": "group", + "scope": ["alpha/mod.rs"], + "slices": [], + } + } + frontier: list[str] = [] + import tempfile + with tempfile.TemporaryDirectory() as d: + root = Path(d) + (root / "alpha").mkdir(parents=True, exist_ok=True) + (root / "alpha" / "mod.rs").write_text( + "fn retained(arg: Option<&str>) {} \n", + encoding="utf-8", + ) + original = mod.RETAINED_HOST_MACHINERY + try: + mod.RETAINED_HOST_MACHINERY = { + "alpha": [ + {"kind": "free", "item": ["crate", "commands", "groups", "alpha", "retained"]} + ] + } + violations = mod.check_source_frontier(topology, frontier, root) + finally: + mod.RETAINED_HOST_MACHINERY = original + self.assertTrue( + any(v.category == "retained-host" for v in violations), + f"expected retained-host violation, got {violations}", + ) + def test_live_source_gate_passes(self) -> None: doc = mod.load_topology() violations = mod.check_source_frontier(doc["topology"], doc["frontier"])