diff --git a/crates/command-contract/src/facets.rs b/crates/command-contract/src/facets.rs index b67f433ff7..4e384a6f41 100644 --- a/crates/command-contract/src/facets.rs +++ b/crates/command-contract/src/facets.rs @@ -186,16 +186,6 @@ pub struct ProjectGoalState { pub goal_continuation_waiting: bool, } -/// Host project data for the project command group (FEAT-021 D1). -/// -/// Exposes the typed, exact-minimum operations the live project handlers -/// consume: `/lsp` status/set state, `/share` session payload data, and -/// `/goal` goal state including the session-derived effective values. -/// `/init` host data flows through the existing `WORKSPACE` facet (D2), so -/// `/init` destructures exactly `WORKSPACE` (D4) and consumes no -/// project-facet method. All results are contract-owned portable values; implementation -/// errors cross as safe text. The TUI adapter is the only place that touches -/// `App`, `config::config`, the goal service, or the session manager. /// Host project data for the project command group (FEAT-021 D1). /// /// Exposes the typed, exact-minimum operations the live project handlers @@ -338,3 +328,274 @@ pub trait CommandMemoryContext { /// Delete the given workspace scope; workspace path is the first argument. fn delete_workspace(&self, workspace: &Path) -> Result; } + +// --------------------------------------------------------------------------- +// 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 9717d802ee..36cf62d36e 100644 --- a/crates/command-contract/src/handler.rs +++ b/crates/command-contract/src/handler.rs @@ -7,7 +7,8 @@ use crate::facets::{ CommandCostContext, CommandMediaContext, CommandMemoryContext, CommandModePolicyContext, CommandModelContext, CommandPresentationContext, CommandProjectContext, CommandSessionContext, - CommandSkillsContext, CommandSystemPromptContext, CommandWorkspaceContext, + CommandSkillGroupContext, CommandSkillsContext, CommandSystemPromptContext, + CommandWorkspaceContext, }; /// Exact host capabilities exposed to one contextual command handler. @@ -33,13 +34,15 @@ impl CommandCapabilities { pub const MEMORY: Self = Self(1 << 9); /// Project-group host data (FEAT-021 D1). pub const PROJECT: Self = Self(1 << 10); + /// Skills-group host data (FEAT-022 D1). + pub const SKILL_GROUP: Self = Self(1 << 11); pub const fn union(self, other: Self) -> Self { Self(self.0 | other.0) } pub const fn contains(self, capability: Self) -> bool { - self.0 & capability.0 == capability.0 + !capability.is_empty() && self.0 & capability.0 == capability.0 } pub const fn is_empty(self) -> bool { @@ -78,6 +81,7 @@ pub struct CommandContexts<'a> { media: Option<&'a mut dyn CommandMediaContext>, memory: Option<&'a mut dyn CommandMemoryContext>, project: Option<&'a mut dyn CommandProjectContext>, + skill_group: Option<&'a mut dyn CommandSkillGroupContext>, } /// Consumed envelope used when one handler needs several independent facets. @@ -93,6 +97,7 @@ pub struct ContextParts<'a> { pub media: Option<&'a mut dyn CommandMediaContext>, pub memory: Option<&'a mut dyn CommandMemoryContext>, pub project: Option<&'a mut dyn CommandProjectContext>, + pub skill_group: Option<&'a mut dyn CommandSkillGroupContext>, } impl<'a> CommandContexts<'a> { @@ -109,6 +114,7 @@ impl<'a> CommandContexts<'a> { media: None, memory: None, project: None, + skill_group: None, } } @@ -125,6 +131,7 @@ impl<'a> CommandContexts<'a> { media: self.media, memory: self.memory, project: self.project, + skill_group: self.skill_group, } } @@ -212,6 +219,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 920f1b52c0..52546f3ab8 100644 --- a/crates/command-contract/src/tests.rs +++ b/crates/command-contract/src/tests.rs @@ -853,6 +853,8 @@ fn capabilities_declare_exact_memory_authority() { assert!(!workspace.contains(CommandCapabilities::MEMORY)); assert!(!memory.contains(CommandCapabilities::WORKSPACE)); assert!(CommandCapabilities::NONE.is_empty()); + assert!(!workspace_memory.contains(CommandCapabilities::NONE)); + assert!(!CommandCapabilities::NONE.contains(CommandCapabilities::NONE)); // No presentation or media authority is declared for the memory group. assert!(!workspace_memory.contains(CommandCapabilities::PRESENTATION)); assert!(!workspace_memory.contains(CommandCapabilities::MEDIA)); @@ -893,3 +895,452 @@ fn envelope_rejects_duplicate_memory_slot_deterministically() { })); assert!(result.is_err(), "duplicate memory 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/config/src/lib.rs b/crates/config/src/lib.rs index 8f85f47811..703f81107b 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -882,6 +882,11 @@ pub struct ConfigToml { /// empty `path` disables the feature and leaves behavior unchanged. #[serde(default)] pub lifecycle_outbox: Option, + /// Per-session control socket (`[control_socket]`). Opt-in: an absent + /// table or `enabled = false` (the default) leaves the feature off and + /// behavior unchanged. + #[serde(default)] + pub control_socket: Option, /// Agent Fleet trust and security policy (#3165). When absent, fleet /// workers inherit conservative Sandbox defaults. #[serde(default)] @@ -1621,6 +1626,21 @@ pub struct LifecycleOutboxToml { pub webhook_token: Option, } +/// On-disk schema for the `[control_socket]` table. +/// +/// Opt-in per-session control surface: when `enabled`, the interactive TUI +/// binds a unix domain socket at `//control.sock` +/// for the running session. The socket speaks newline-framed JSON-RPC with +/// the verbs `message`, `interrupt`, `relaunch`, and `status`. An absent +/// table, or `enabled = false` (the default), disables the feature entirely — +/// behavior is unchanged from a release without the table. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct ControlSocketToml { + /// Bind the per-session control socket. Default: false (OFF). + #[serde(default)] + pub enabled: bool, +} + /// On-disk schema for the `[skills]` table (#140). See `config.example.toml` /// for documentation. #[derive(Debug, Clone, Serialize, Deserialize, Default)] diff --git a/crates/config/src/tests.rs b/crates/config/src/tests.rs index c4675a075a..266de73b1b 100644 --- a/crates/config/src/tests.rs +++ b/crates/config/src/tests.rs @@ -131,6 +131,48 @@ fn lifecycle_outbox_toml_webhook_is_optional() { assert!(outbox.webhook_token.is_none()); } +#[test] +fn control_socket_toml_is_off_by_default_and_parses_when_enabled() { + // Unset = feature OFF: the table is absent and the field is None. + let absent: ConfigToml = toml::from_str("model = \"demo\"\n").expect("minimal config"); + assert!( + absent.control_socket.is_none(), + "unset [control_socket] must leave the feature off" + ); + + // An empty table is also off: enabled defaults to false. + let empty: ConfigToml = + toml::from_str("[control_socket]\n").expect("empty control_socket table"); + let socket = empty.control_socket.expect("table should parse"); + assert!(!socket.enabled, "empty table must leave the socket off"); + + // Explicit enable. + let enabled: ConfigToml = toml::from_str( + r#" + [control_socket] + enabled = true + "#, + ) + .expect("enabled control_socket table"); + assert!( + enabled.control_socket.expect("table should parse").enabled, + "enabled = true must turn the socket on" + ); + + // Explicit disable stays off. + let disabled: ConfigToml = toml::from_str( + r#" + [control_socket] + enabled = false + "#, + ) + .expect("disabled control_socket table"); + assert!( + !disabled.control_socket.expect("table should parse").enabled, + "enabled = false must keep the socket off" + ); +} + #[test] fn permissions_toml_deserializes_typed_ask_rules() { let permissions: PermissionsToml = toml::from_str( diff --git a/crates/tui/src/client.rs b/crates/tui/src/client.rs index 633c91c53d..534a0f2808 100644 --- a/crates/tui/src/client.rs +++ b/crates/tui/src/client.rs @@ -4215,6 +4215,12 @@ pub(crate) use prepared::{ }; pub(crate) use provider_native_search::{ProviderNativeSearchClient, ProviderNativeSearchRequest}; +/// Whether a route speaks ordinary `/chat/completions` (not Messages or Responses). +#[must_use] +pub(crate) fn provider_speaks_chat_completions(api_provider: ApiProvider) -> bool { + provider_default_wire_format(api_provider) == WireFormat::ChatCompletions +} + pub(crate) fn inspect_prompt_for_request(request: &MessageRequest) -> PromptInspection { chat::inspect_prompt_for_request(request) } diff --git a/crates/tui/src/commands/contract.rs b/crates/tui/src/commands/contract.rs index 27b1ec6f2f..777cf82941 100644 --- a/crates/tui/src/commands/contract.rs +++ b/crates/tui/src/commands/contract.rs @@ -14,7 +14,7 @@ //! //! ## Authoritative host-proxy design (D1) //! -//! `CommandContexts` holds ten independently borrowed facet objects, while +//! `CommandContexts` holds eleven independently borrowed facet objects, while //! important behavior (mode transitions, model invalidation, cost accounting, //! skill refresh) is authoritative on `App`. The adapters therefore share a //! synchronous TUI-owned host proxy. Each trait call borrows `App` only for the @@ -32,12 +32,16 @@ use std::path::{Path, PathBuf}; use std::rc::Rc; use codewhale_command_contract::facets::{ - CommandCostContext, CommandMediaContext, CommandMemoryContext, CommandModePolicyContext, - CommandModelContext, CommandPresentationContext, CommandProjectContext, CommandSessionContext, - CommandSkillsContext, CommandSystemPromptContext, CommandWorkspaceContext, - MediaAttachmentReceipt, MemoryDelete, MemoryDeleteScope, MemoryExport, MemoryGetOutcome, - MemoryHit, MemoryImportOutcome, MemoryReindex, MemoryRememberTarget, MemoryRemembered, - MemoryStatus, ProjectGoalState, ProjectGoalStatus, ProjectShareProjection, + CommandApprovalState, CommandCostContext, CommandMediaContext, CommandMemoryContext, + CommandModePolicyContext, CommandModelContext, CommandPresentationContext, + CommandProjectContext, CommandSessionContext, CommandSkillGroupContext, CommandSkillsContext, + CommandSystemPromptContext, CommandWorkspaceContext, MediaAttachmentReceipt, MemoryDelete, + MemoryDeleteScope, MemoryExport, MemoryGetOutcome, MemoryHit, MemoryImportOutcome, + MemoryReindex, MemoryRememberTarget, MemoryRemembered, MemoryStatus, ProjectGoalState, + ProjectGoalStatus, ProjectShareProjection, RemoteRegistryOutcome, RemoteSkillEntry, + ReviewOutcome, SkillActivationError, SkillActivationOutcome, SkillBundledTier, SkillEntry, + SkillMutationOutcome, SkillMutationReceipt, SkillRecommendation, SkillRegistryProjection, + SkillSourceKind, SkillSyncEntry, SkillSyncOutcome, SkillTargetScope, SnapshotEntry, }; #[cfg(test)] use codewhale_command_contract::handler::ContextParts; @@ -50,8 +54,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) @@ -66,8 +72,7 @@ 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", "plugins", "session", "skills"]; +pub(crate) const PENDING_GROUPS: &[&str] = &["config", "core", "debug", "plugins", "session"]; // --------------------------------------------------------------------------- // Boundary-value mappings (D8) @@ -917,11 +922,632 @@ 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 @@ -938,6 +1564,7 @@ pub(crate) struct CommandContextBundle<'a> { media: MediaAdapter<'a>, project: ProjectAdapter<'a>, memory: MemoryAdapter<'a>, + skill_group: SkillGroupAdapter<'a>, } impl<'a> CommandContextBundle<'a> { @@ -977,6 +1604,9 @@ impl<'a> CommandContextBundle<'a> { if capabilities.contains(CommandCapabilities::PROJECT) { contexts = contexts.with_project(&mut self.project); } + if capabilities.contains(CommandCapabilities::SKILL_GROUP) { + contexts = contexts.with_skill_group(&mut self.skill_group); + } contexts } @@ -993,7 +1623,8 @@ impl<'a> CommandContextBundle<'a> { .union(CommandCapabilities::PRESENTATION) .union(CommandCapabilities::MEDIA) .union(CommandCapabilities::MEMORY) - .union(CommandCapabilities::PROJECT); + .union(CommandCapabilities::PROJECT) + .union(CommandCapabilities::SKILL_GROUP); self.contexts(all_test_capabilities).into_parts() } } @@ -1016,7 +1647,8 @@ impl App { presentation: PresentationAdapter { host: host.clone() }, media: MediaAdapter { host: host.clone() }, project: ProjectAdapter { host: host.clone() }, - memory: MemoryAdapter { host }, + memory: MemoryAdapter { host: host.clone() }, + skill_group: SkillGroupAdapter { host }, } } } @@ -1970,4 +2602,367 @@ mod tests { assert!(parts.session.is_some()); assert!(parts.memory.is_none()); } + + // ─── 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/debug/balance.rs b/crates/tui/src/commands/groups/debug/balance.rs index 45d941c9ac..9924f3c1d1 100644 --- a/crates/tui/src/commands/groups/debug/balance.rs +++ b/crates/tui/src/commands/groups/debug/balance.rs @@ -1,28 +1,18 @@ -//! Balance: query the active provider's account balance or credit status. -//! -//! Provider-specific network dispatch is still pending. Until that lands, keep -//! this command explicit about being a scaffold so users do not mistake it for -//! a live balance lookup. +//! Balance: query the active provider's remaining prepaid credit. -use crate::config::ApiProvider; -use crate::tui::app::App; +use crate::config::provider_has_balance_api; +use crate::tui::app::{App, AppAction}; use super::CommandResult; /// Query provider account balance / credits. pub fn balance(app: &mut App) -> CommandResult { let provider = app.api_provider; - match provider { - ApiProvider::Deepseek - | ApiProvider::DeepseekCN - | ApiProvider::Openrouter - | ApiProvider::Novita => CommandResult::message(format!( - "Balance check for {} is planned, but provider balance network dispatch is not wired in this build yet.", - provider.display_name() - )), - _ => CommandResult::message(format!( + if !provider_has_balance_api(provider) { + return CommandResult::message(format!( "Balance check is not supported for {} yet. Check the provider dashboard for account balance details.", provider.display_name() - )), + )); } + CommandResult::action(AppAction::FetchBalance) } diff --git a/crates/tui/src/commands/groups/project/goal.rs b/crates/tui/src/commands/groups/project/goal.rs index 4c52f939c2..ff0ae84f6b 100644 --- a/crates/tui/src/commands/groups/project/goal.rs +++ b/crates/tui/src/commands/groups/project/goal.rs @@ -291,8 +291,7 @@ impl RegisterCommand for GoalCmd { fn handler() -> CommandHandler { CommandHandler::Contextual { capabilities: codewhale_command_contract::handler::CommandCapabilities::PROJECT - .union(codewhale_command_contract::handler::CommandCapabilities::PRESENTATION) - .union(codewhale_command_contract::handler::CommandCapabilities::WORKSPACE), + .union(codewhale_command_contract::handler::CommandCapabilities::PRESENTATION), handler: goal_contextual, } } 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..e77223987d 100644 --- a/crates/tui/src/commands/groups/skills/restore.rs +++ b/crates/tui/src/commands/groups/skills/restore.rs @@ -7,33 +7,64 @@ //! 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 { + capabilities: codewhale_command_contract::handler::CommandCapabilities::SKILL_GROUP, + handler: 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 +76,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 +99,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 +118,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 +128,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 +173,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 +200,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") + } + 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 } - ScopedHome { - prev, - _home: home, - _guard: guard, + } + + 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 +357,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 +372,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 +396,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..62543d3f74 100644 --- a/crates/tui/src/commands/groups/skills/review.rs +++ b/crates/tui/src/commands/groups/skills/review.rs @@ -1,148 +1,223 @@ //! 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 - )); + fn handler() -> CommandHandler { + CommandHandler::Contextual { + capabilities: codewhale_command_contract::handler::CommandCapabilities::SKILL_GROUP, + handler: review_contextual, } - }; - - 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())) + } } -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..80c95074dc 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,251 @@ 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()), - ) +/// 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 + } +} + +/// 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 +} + +/// 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()) } - }) - .into_enabled() + }; + 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 + )) + } + } } -fn render_skill_warnings(registry: &SkillRegistry) -> String { - if registry.warnings().is_empty() { +// --------------------------------------------------------------------------- +// Portable rendering helpers (byte-identical to the pre-migration handlers) +// --------------------------------------------------------------------------- + +/// Render registry warnings as the baseline suffix block. +fn render_skill_warnings(warnings: &[String]) -> String { + if warnings.is_empty() { return String::new(); } let mut out = String::new(); - let _ = writeln!(out, "\nWarnings ({}):", registry.warnings().len()); - for warning in registry.warnings() { + let _ = writeln!(out, "\nWarnings ({}):", warnings.len()); + for warning in warnings { let _ = writeln!(out, " - {warning}"); } out } -fn skill_discovery_mode(app: &App) -> crate::skills::SkillDiscoveryMode { - crate::skills::SkillDiscoveryMode::from_codewhale_only(app.skills_scan_codewhale_only) +/// Source label used by `/skills inspect` (baseline `skill_source_label`). +fn skill_source_label(source: &SkillSourceKind) -> String { + match source { + SkillSourceKind::Native => "native".to_string(), + SkillSourceKind::Plugin { + plugin_name, + plugin_id, + } => format!("reviewed plugin snapshot {plugin_name} ({plugin_id})"), + } } -fn skill_discovery_mode_label(mode: crate::skills::SkillDiscoveryMode) -> &'static str { - match mode { - crate::skills::SkillDiscoveryMode::Compatible => "compatible", - crate::skills::SkillDiscoveryMode::CodeWhaleOnly => "codewhale-only", - } +/// 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." + ) } -fn visible_skill_directories(app: &App) -> Vec { - crate::skills::skill_directories_for_workspace_and_dir( - &app.workspace, - &app.skills_dir, - skill_discovery_mode(app), +/// 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." ) } -fn skill_source_label(source: &SkillSource) -> String { - match source { - SkillSource::Native => "native".to_string(), - SkillSource::Plugin { - plugin_id, - plugin_name, - .. - } => format!("reviewed plugin snapshot {plugin_name} ({plugin_id})"), +/// 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), } } -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); +/// 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()); + } + 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())) +} - 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() - ); +// --------------------------------------------------------------------------- +// /skills — portable contextual dispatch +// --------------------------------------------------------------------------- - 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()); - } +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 } - 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); - } - let _ = writeln!(output, " source: {}", skill_source_label(&skill.source)); - if matches!(skill.source, SkillSource::Native) { - let _ = writeln!(output, " path: {}", skill.path.display()); - } + fn handler() -> CommandHandler { + CommandHandler::Contextual { + capabilities: codewhale_command_contract::handler::CommandCapabilities::SKILL_GROUP, + handler: skills_contextual, } } +} - output.push_str(&warnings); - CommandResult::message(output) +/// 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 +291,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 +310,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 +327,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 +349,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 +357,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 +374,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 +391,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 +429,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 +505,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 +532,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 +549,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 +595,826 @@ 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 { + capabilities: codewhale_command_contract::handler::CommandCapabilities::SKILL_GROUP + .union(codewhale_command_contract::handler::CommandCapabilities::SKILLS), + handler: 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/groups/utility/dispatch.rs b/crates/tui/src/commands/groups/utility/dispatch.rs index cb9c65e8e3..d7e531372c 100644 --- a/crates/tui/src/commands/groups/utility/dispatch.rs +++ b/crates/tui/src/commands/groups/utility/dispatch.rs @@ -36,7 +36,9 @@ impl RegisterCommand for DispatchCmd { fn dispatch_contextual(contexts: CommandContexts<'_>, arg: Option<&str>) -> CommandResult { let mut parts = contexts.into_parts(); - let workspace = parts.workspace.as_deref_mut().expect("workspace facet"); + let Some(workspace) = parts.workspace.as_deref_mut() else { + return CommandResult::error("Command capability unavailable: workspace"); + }; dispatch(workspace, arg) } @@ -229,6 +231,17 @@ mod tests { assert!(DispatchCmd::info().usage.starts_with("/dispatch")); } + #[test] + fn missing_workspace_facet_fails_safely() { + let result = dispatch_contextual(CommandContexts::empty(), None); + assert!(result.is_error, "{result:?}"); + assert_eq!( + result.message.as_deref(), + Some("Error: Command capability unavailable: workspace") + ); + assert!(result.action.is_none()); + } + #[test] fn bare_dispatch_is_a_status_card_not_a_silent_launch() { let mut workspace = FakeWorkspace(PathBuf::from(".")); diff --git a/crates/tui/src/commands/mod.rs b/crates/tui/src/commands/mod.rs index 34cad75679..051cfcf8d0 100644 --- a/crates/tui/src/commands/mod.rs +++ b/crates/tui/src/commands/mod.rs @@ -152,9 +152,18 @@ fn feat015_contextual( ) -> CommandResult { use codewhale_command_contract::handler::ContextParts; let parts: ContextParts<'_> = contexts.into_parts(); - let workspace = parts.workspace.expect("workspace facet").workspace(); - let mode = parts.mode_policy.expect("mode-policy facet").mode(); - let currency = parts.cost.expect("cost facet").display_currency(); + let Some(workspace) = parts.workspace else { + return CommandResult::error("Command capability unavailable: workspace"); + }; + let Some(mode_policy) = parts.mode_policy else { + return CommandResult::error("Command capability unavailable: mode-policy"); + }; + let Some(cost) = parts.cost else { + return CommandResult::error("Command capability unavailable: cost"); + }; + let workspace = workspace.workspace(); + let mode = mode_policy.mode(); + let currency = cost.display_currency(); let normalized = arg.unwrap_or(""); CommandResult::message(format!( "feat015ctx workspace={} mode={:?} currency={:?} arg={}", @@ -1733,19 +1742,21 @@ mod tests { } #[test] - fn balance_command_reports_scaffold_without_claiming_dispatch() { + fn balance_command_dispatches_live_fetch_for_prepaid_providers() { let mut app = create_test_app(); - app.api_provider = ApiProvider::Deepseek; - - let result = execute("/balance", &mut app); - let msg = result - .message - .expect("balance scaffold should explain current state"); - - assert!(!result.is_error); - assert!(msg.contains("DeepSeek")); - assert!(msg.contains("not wired")); - assert!(!msg.contains("sent")); + for provider in [ + ApiProvider::Deepseek, + ApiProvider::Openrouter, + ApiProvider::Siliconflow, + ] { + app.api_provider = provider; + let result = execute("/balance", &mut app); + assert!(!result.is_error, "{provider:?}"); + assert!( + matches!(result.action, Some(AppAction::FetchBalance)), + "{provider:?} should dispatch a live remaining-credit fetch" + ); + } } #[test] @@ -1924,6 +1935,20 @@ mod tests { assert!(result.action.is_none()); } + #[test] + fn feat015_contextual_command_fails_safely_without_declared_facets() { + let result = feat015_contextual( + codewhale_command_contract::handler::CommandContexts::empty(), + None, + ); + assert!(result.is_error, "{result:?}"); + assert_eq!( + result.message.as_deref(), + Some("Error: Command capability unavailable: workspace") + ); + assert!(result.action.is_none()); + } + #[test] fn feat015_contextual_command_is_registered_only_in_test_builds() { // The fixture entry is present in the test-build registry with a @@ -1961,6 +1986,11 @@ mod tests { // FEAT-019 memory group. "note", "memory", + // FEAT-022 skills group. + "skills", + "skill", + "review", + "restore", ]; for info in command_infos() { if info.name == "feat015ctx" || MIGRATED_GROUPS.contains(&info.name) { @@ -2077,12 +2107,17 @@ mod tests { #[test] fn feat021_project_entries_register_through_portable_bridge() { - // main's model carries no capability bitmask: each project command - // must register through the portable bridge as a contextual handler - // and dispatch safely through the public seam. Exact facet - // destructuring (D4) is proven by the handler tests and the adapter - // exposure test. - for name in ["init", "lsp", "share", "goal"] { + use codewhale_command_contract::handler::{CommandCapabilities, CommandHandler}; + + for (name, expected) in [ + ("init", CommandCapabilities::WORKSPACE), + ("lsp", CommandCapabilities::PROJECT), + ("share", CommandCapabilities::PROJECT), + ( + "goal", + CommandCapabilities::PROJECT.union(CommandCapabilities::PRESENTATION), + ), + ] { assert!( registry().has_contextual_handler(name), "/{name} must register through the portable bridge" @@ -2092,13 +2127,10 @@ mod tests { .expect("entry") .contextual_handler() .expect("contextual handler"); - assert!( - matches!( - handler, - codewhale_command_contract::handler::CommandHandler::Contextual { .. } - ), - "/{name} must be contextual" - ); + let CommandHandler::Contextual { capabilities, .. } = handler else { + panic!("/{name} must be contextual"); + }; + assert_eq!(capabilities, expected, "/{name} exact capability set"); } } @@ -2229,6 +2261,100 @@ 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() { + use codewhale_command_contract::handler::{CommandCapabilities, CommandHandler}; + + for (name, alias, expected) in [ + ( + "skills", + Some("jinengliebiao"), + CommandCapabilities::SKILL_GROUP, + ), + ( + "skill", + Some("jineng"), + CommandCapabilities::SKILL_GROUP.union(CommandCapabilities::SKILLS), + ), + ("review", Some("shencha"), CommandCapabilities::SKILL_GROUP), + ("restore", None, CommandCapabilities::SKILL_GROUP), + ] { + let info = registry() + .get_info(name) + .unwrap_or_else(|| panic!("/{name} must be registered")); + assert_eq!(info.name, name, "canonical name"); + let handler = registry() + .get(name) + .expect("entry") + .contextual_handler() + .expect("contextual handler"); + let CommandHandler::Contextual { capabilities, .. } = handler else { + panic!("/{name} must be contextual"); + }; + assert_eq!(capabilities, expected, "/{name} exact capability set"); + if let Some(alias) = alias { + assert!( + registry().get_info(alias).is_some(), + "/{name} alias {alias} must resolve" + ); + } + } + } + #[test] fn feat019_note_dispatches_through_public_seam() { let tmpdir = tempfile::TempDir::new().unwrap(); @@ -2297,4 +2423,85 @@ mod tests { assert!(result.message.is_some(), "{command}: {result:?}"); } } + + #[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 test-only full envelope exposes every adapter; production + // dispatch exposes only each handler's declared 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/config.rs b/crates/tui/src/config.rs index 1ab73e3a71..c9f5e2236e 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -2351,7 +2351,7 @@ pub enum StatusItem { RateLimit, /// Session token usage: input / cache-hit / output. Tokens, - /// DeepSeek account balance, refreshed once per turn completion. + /// Prepaid remaining credit, refreshed once per turn completion. Balance, /// Session metrics strip: turns · steps │ LLM · tools │ TTFT · tok/s │ /// cache │ in — sourced from engine timings and provider usage. @@ -2466,7 +2466,7 @@ impl StatusItem { StatusItem::LastToolElapsed => "ms of the most recent tool call (reserved)", StatusItem::RateLimit => "remaining requests in the budget (reserved)", StatusItem::Tokens => "input / cache-hit / output token totals", - StatusItem::Balance => "topped-up + granted balance from DeepSeek", + StatusItem::Balance => "remaining prepaid credit from the active provider", StatusItem::SessionMetrics => "turns · steps · LLM/tool time · TTFT · tok/s · input", } } @@ -2499,14 +2499,26 @@ impl StatusItem { #[must_use] pub fn is_available_for(self, provider: ApiProvider) -> bool { match self { - StatusItem::Balance => { - matches!(provider, ApiProvider::Deepseek | ApiProvider::DeepseekCN) - } + StatusItem::Balance => provider_has_balance_api(provider), _ => true, } } } +/// Prepaid providers that publish a remaining-credit endpoint Codewhale +/// can fetch. Local runtimes and invoice-only vendors stay out. +#[must_use] +pub fn provider_has_balance_api(provider: ApiProvider) -> bool { + matches!( + provider, + ApiProvider::Deepseek + | ApiProvider::DeepseekCN + | ApiProvider::Openrouter + | ApiProvider::Siliconflow + | ApiProvider::SiliconflowCn + ) +} + /// One configurable header item #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Hash)] @@ -3107,6 +3119,13 @@ pub struct Config { #[serde(default)] pub lifecycle_outbox: Option, + /// Per-session control socket (`[control_socket]`). Opt-in: an absent + /// table or `enabled = false` (the default) leaves the feature off. + /// When enabled, the interactive TUI binds a unix socket per running + /// session (see `crate::tui::control_socket`). + #[serde(default)] + pub control_socket: Option, + /// Provider-specific credentials and defaults shared with the `codewhale` facade. #[serde(default)] pub providers: Option, @@ -10493,6 +10512,7 @@ fn merge_config(base: Config, override_cfg: Config) -> Config { transcript: override_cfg.transcript.or(base.transcript), hooks: override_cfg.hooks.or(base.hooks), lifecycle_outbox: override_cfg.lifecycle_outbox.or(base.lifecycle_outbox), + control_socket: override_cfg.control_socket.or(base.control_socket), providers: merge_providers(base.providers, override_cfg.providers), features: merge_features(base.features, override_cfg.features), notifications: override_cfg.notifications.or(base.notifications), diff --git a/crates/tui/src/config/tests.rs b/crates/tui/src/config/tests.rs index 6eb78bd9e8..9f5bfb7475 100644 --- a/crates/tui/src/config/tests.rs +++ b/crates/tui/src/config/tests.rs @@ -774,6 +774,38 @@ webhook_token = "secret-token" assert!(absent.base.lifecycle_outbox.is_none()); } +#[test] +fn tui_config_parses_control_socket_table() { + let raw = r#" +[control_socket] +enabled = true +"#; + let parsed: ConfigFile = toml::from_str(raw).expect("parse control_socket config"); + + let socket = parsed + .base + .control_socket + .expect("control_socket table should parse"); + assert!(socket.enabled); + + // Off by default: a config without the table leaves the feature off. + let absent: ConfigFile = + toml::from_str("model = \"demo\"").expect("parse config without control_socket table"); + assert!(absent.base.control_socket.is_none()); + + // An empty table stays off. + let empty: ConfigFile = + toml::from_str("[control_socket]").expect("parse empty control_socket table"); + assert!( + !empty + .base + .control_socket + .expect("table should parse") + .enabled, + "empty table must leave the socket off" + ); +} + #[test] fn tui_config_parses_hotbar_bindings() { let raw = r#" @@ -11226,12 +11258,13 @@ fn provider_capability_roundtrip_serialization() { } #[test] -fn status_item_balance_available_only_for_deepseek_providers() { - // Balance item should only be offered for DeepSeek / DeepSeekCN. +fn status_item_balance_available_for_prepaid_providers() { assert!(StatusItem::Balance.is_available_for(ApiProvider::Deepseek)); assert!(StatusItem::Balance.is_available_for(ApiProvider::DeepseekCN)); - // Sanity: all other known providers should hide the Balance toggle. - assert!(!StatusItem::Balance.is_available_for(ApiProvider::Openrouter)); + assert!(StatusItem::Balance.is_available_for(ApiProvider::Openrouter)); + assert!(StatusItem::Balance.is_available_for(ApiProvider::Siliconflow)); + assert!(StatusItem::Balance.is_available_for(ApiProvider::SiliconflowCn)); + // Invoice-only, local, and unimplemented prepaid vendors stay hidden. assert!(!StatusItem::Balance.is_available_for(ApiProvider::Novita)); assert!(!StatusItem::Balance.is_available_for(ApiProvider::NvidiaNim)); assert!(!StatusItem::Balance.is_available_for(ApiProvider::Fireworks)); diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 995cc7bcfb..0057cd59b5 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -6068,6 +6068,10 @@ fn print_doctor_setup_report( " · runtime posture: {}", doctor_runtime_posture_line(config, workspace) ); + println!( + " · control socket: {}", + doctor_control_socket_posture_line(config) + ); let consistency = doctor_setup_consistency(state, source); if consistency["status"] == "inconsistent" { let issues = consistency["issues"] @@ -6285,6 +6289,22 @@ fn doctor_runtime_posture_line(config: &Config, workspace: &Path) -> String { ) } +/// Doctor posture for the per-session control socket, enabled via +/// `[control_socket].enabled` (false = off, the default). Report the +/// resolved state and, when enabled, where the socket appears for the +/// running session. +fn doctor_control_socket_posture_line(config: &Config) -> String { + let enabled = config + .control_socket + .as_ref() + .is_some_and(|socket| socket.enabled); + if enabled { + "control_socket=on (sessions//control.sock per running session)".to_string() + } else { + "control_socket=off (default)".to_string() + } +} + /// Resolved telemetry consent and where it came from (#5441). /// /// Telemetry ships ON by default, and no posture surface reported that — a diff --git a/crates/tui/src/mcp_server.rs b/crates/tui/src/mcp_server.rs index 480abfebd3..e49781c9df 100644 --- a/crates/tui/src/mcp_server.rs +++ b/crates/tui/src/mcp_server.rs @@ -71,6 +71,16 @@ struct ExposedTool { internal: String, } +fn mcp_request_model(arguments: &Value, default_model: &str) -> String { + arguments + .get("model") + .and_then(Value::as_str) + .map(str::trim) + .filter(|model| !model.is_empty()) + .unwrap_or(default_model) + .to_string() +} + pub fn run_mcp_server(workspace: PathBuf) -> Result<()> { let settings = McpServerSettings::load()?; let mut server = McpServer::new(workspace, settings)?; @@ -188,7 +198,7 @@ impl McpServer { }, "model": { "type": "string", - "description": "Optional model identifier (default: deepseek-v4-pro)" + "description": "Optional model identifier. When omitted, uses the active provider's default model." }, "cwd": { "type": "string", @@ -345,10 +355,14 @@ impl McpServer { message: "Missing required argument: prompt".to_string(), })?; - let model = arguments - .get("model") - .and_then(Value::as_str) - .unwrap_or("deepseek-v4-pro"); + // Load config first so an omitted model follows the active provider + // default instead of a hardcoded DeepSeek id. + let config = Config::load(None, None).map_err(|e| RpcError { + code: -32000, + message: format!("Failed to load config: {e}"), + })?; + let default_model = config.default_model(); + let model = mcp_request_model(arguments, &default_model); // Resolve thread_id let thread_id = if internal_name == "deepseek" { @@ -365,11 +379,6 @@ impl McpServer { .to_string() }; - // Load config and create client - let config = Config::load(None, None).map_err(|e| RpcError { - code: -32000, - message: format!("Failed to load config: {e}"), - })?; let client = DeepSeekClient::new(&config).map_err(model_client_init_error)?; // Build message list @@ -395,9 +404,9 @@ impl McpServer { // Send the API request (non-streaming for the basic version). Internal // chat uses the same resolved output policy as an ordinary turn. - let request_route = client.effective_route_envelope(model, chrono::Utc::now()); + let request_route = client.effective_route_envelope(&model, chrono::Utc::now()); let request = MessageRequest { - model: model.to_string(), + model: model.clone(), messages: messages.clone(), max_tokens: client.effective_max_output_tokens(&request_route.model), system: None, @@ -721,4 +730,35 @@ mod tests { assert!(!init.message.contains("DeepSeek")); assert!(!request.message.contains("DeepSeek")); } + + #[test] + fn omitted_mcp_model_follows_the_active_provider_default() { + assert_eq!(mcp_request_model(&json!({}), "gpt-5.6"), "gpt-5.6"); + assert_eq!( + mcp_request_model(&json!({ "model": " " }), "GLM-5.3"), + "GLM-5.3" + ); + assert_eq!( + mcp_request_model(&json!({ "model": "kimi-k2.5" }), "gpt-5.6"), + "kimi-k2.5" + ); + } + + #[test] + fn list_tools_does_not_hardcode_a_deepseek_default_model() { + let settings = McpServerSettings { + expose_tools: vec!["deepseek".to_string()], + require_approval: false, + }; + let server = McpServer::new(PathBuf::from("."), settings).expect("build server"); + let tools = server.list_tools_response(); + let description = tools["tools"][0]["inputSchema"]["properties"]["model"]["description"] + .as_str() + .expect("model description"); + assert!( + !description.to_ascii_lowercase().contains("deepseek"), + "{description}" + ); + assert!(description.contains("active provider"), "{description}"); + } } diff --git a/crates/tui/src/pricing.rs b/crates/tui/src/pricing.rs index 0292b42fe7..cbf928762c 100644 --- a/crates/tui/src/pricing.rs +++ b/crates/tui/src/pricing.rs @@ -107,9 +107,10 @@ impl CostEstimate { } } -// === DeepSeek Account Balance === +// === Provider Account Balance === -/// Response from `GET https://api.deepseek.com/user/balance`. +/// Response from DeepSeek `GET /user/balance`. Other prepaid providers are +/// mapped onto [`BalanceInfo`] at the fetch seam. #[derive(Debug, Clone, Default, serde::Deserialize)] pub struct BalanceResponse { #[allow(dead_code)] @@ -117,28 +118,64 @@ pub struct BalanceResponse { pub balance_infos: Vec, } -/// Per-currency balance entry from the balance API. +/// Per-currency remaining-credit entry shown by `/balance` and the status chip. #[derive(Debug, Clone, Default, serde::Deserialize)] pub struct BalanceInfo { - // Wire fields of the live `GET /user/balance` response deserialized in - // `tui::ui::fetch_deepseek_balance` and parked in `App::balance_cell`. - // Nothing renders them today — the footer balance chip went with the - // legacy FooterWidget — so `dead_code` cannot see the producer. Kept - // because they are the API contract, matching the sibling fields below. - #[allow(dead_code)] pub currency: String, #[serde(default)] - #[allow(dead_code)] pub total_balance: String, #[serde(default)] - #[allow(dead_code)] pub topped_up_balance: String, #[serde(default)] - #[allow(dead_code)] pub granted_balance: String, } -impl BalanceInfo {} +impl BalanceInfo { + /// Compact ledger chip, e.g. `$12.50` or `¥123.45`. + #[must_use] + pub fn chip_label(&self) -> Option { + let amount = self.total_balance.trim(); + if amount.is_empty() { + return None; + } + Some(format_balance_amount(amount, &self.currency)) + } + + /// Full `/balance` report for one prepaid provider. + #[must_use] + pub fn report(&self, provider_name: &str) -> String { + let amount = self + .chip_label() + .unwrap_or_else(|| self.total_balance.trim().to_string()); + let mut report = if amount.is_empty() { + format!("{provider_name} account balance is unknown") + } else { + format!("{provider_name} account balance: {amount}") + }; + let topped = self.topped_up_balance.trim(); + let granted = self.granted_balance.trim(); + if !topped.is_empty() || !granted.is_empty() { + let mut parts = Vec::new(); + if !topped.is_empty() { + parts.push(format!("topped up {topped}")); + } + if !granted.is_empty() { + parts.push(format!("granted {granted}")); + } + report.push_str(&format!(" ({})", parts.join(", "))); + } + report + } +} + +fn format_balance_amount(amount: &str, currency: &str) -> String { + match currency.trim().to_ascii_uppercase().as_str() { + "CNY" | "RMB" | "¥" => format!("¥{amount}"), + "USD" | "US$" | "$" => format!("${amount}"), + "" => amount.to_string(), + other => format!("{amount} {other}"), + } +} /// How a hand-sourced row bills cache-creation (cache-write) tokens. /// @@ -4745,5 +4782,33 @@ mod tests { assert!(resp.balance_infos.is_empty()); } - // ── BalanceInfo::total_balance_f64 ───────────────────────────── + #[test] + fn balance_info_chip_label_uses_currency_prefix() { + let cny = BalanceInfo { + currency: "CNY".to_string(), + total_balance: "123.45".to_string(), + ..BalanceInfo::default() + }; + assert_eq!(cny.chip_label().as_deref(), Some("¥123.45")); + let usd = BalanceInfo { + currency: "USD".to_string(), + total_balance: "12.50".to_string(), + ..BalanceInfo::default() + }; + assert_eq!(usd.chip_label().as_deref(), Some("$12.50")); + assert_eq!( + usd.report("OpenRouter"), + "OpenRouter account balance: $12.50" + ); + let deepseek = BalanceInfo { + currency: "CNY".to_string(), + total_balance: "123.45".to_string(), + topped_up_balance: "100.00".to_string(), + granted_balance: "23.45".to_string(), + }; + assert_eq!( + deepseek.report("DeepSeek"), + "DeepSeek account balance: ¥123.45 (topped up 100.00, granted 23.45)" + ); + } } 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/app/types.rs b/crates/tui/src/tui/app/types.rs index e6bee42681..248d50cd7a 100644 --- a/crates/tui/src/tui/app/types.rs +++ b/crates/tui/src/tui/app/types.rs @@ -1108,6 +1108,8 @@ pub enum AppAction { title: String, content: String, }, + /// Live remaining-credit lookup for prepaid providers (`/balance`). + FetchBalance, FetchModels, /// Force a Models.dev live-catalog refresh into ProviderLake (#4187). RefreshModelsDevCatalog, diff --git a/crates/tui/src/tui/control_socket.rs b/crates/tui/src/tui/control_socket.rs new file mode 100644 index 0000000000..35eab11087 --- /dev/null +++ b/crates/tui/src/tui/control_socket.rs @@ -0,0 +1,1254 @@ +//! Per-session control socket — the supervised-operation control surface. +//! +//! This module is the codewhale side of "session control/communication API +//! for supervised operation" (#5533). When the +//! `[control_socket]` config table sets `enabled = true`, the interactive +//! TUI binds one unix domain socket per *running* session at +//! +//! ```text +//! //control.sock (mode 0600) +//! ``` +//! +//! where `` is the same directory the session store uses +//! (`SessionManager::sessions_dir`, typically `~/.codewhale/sessions`) and +//! `` is the session the TUI currently owns. The socket lives +//! inside the per-session artifact directory, so `delete_session` and the +//! orphan-reclaim sweep remove it together with the rest of the session's +//! artifacts, and a crashed process leaves at most a stale socket file that +//! the next bind takes over (connect-probe + unlink, a known-good +//! socket-ownership pattern). +//! +//! # Transport +//! +//! Newline-framed JSON-RPC, one request per connection: connect, write one +//! request line, read one response line, close. Requests: +//! +//! ```json +//! {"id":"1","method":"message","params":{"text":"hello"}} +//! {"id":"2","method":"interrupt","params":{}} +//! {"id":"3","method":"relaunch","params":{}} +//! {"id":"4","method":"status","params":{}} +//! ``` +//! +//! Success responses echo the id and carry a `type`-tagged result: +//! +//! ```json +//! {"id":"1","result":{"type":"message_sent","delivery":"dispatched"}} +//! {"id":"2","result":{"type":"interrupted","cancelled":true}} +//! {"id":"3","result":{"type":"relaunching"}} +//! {"id":"4","result":{"type":"status","turn_state":"idle","goal":{"objective":null,"status":"active","paused":false}}} +//! ``` +//! +//! Failures are `{"id":…,"error":{"code":…,"message":…}}` with codes +//! `invalid_request`, `command_error`, `timeout`, and `server_unavailable`. +//! +//! # Verbs +//! +//! - `message` — delivers `text` as a structured user message through the +//! ordinary composer dispatch path (`dispatch_composer_message`): dispatched +//! immediately when the app is idle, queued when a turn is in flight +//! (queued delivery is the default under load, matching the supervisor +//! contract). The response's `delivery` field reports which happened. +//! - `interrupt` — the exact Esc-shaped "cancel the active turn" body +//! (`escape_cancel_request`), shared with the Esc key path so the two +//! cannot drift. `cancelled` reports whether active work was in flight. +//! - `relaunch` — routed through the slash-command path +//! (`crate::commands::execute("/relaunch", app)`): **no relaunch logic +//! lives here**. The `/relaunch` command is built on the +//! `pr/relaunch-command` branch; this verb is the seam that calls the same +//! command the user's `/relaunch` would. Until that command lands, the +//! verb reports the command's own "unknown command" error verbatim, and +//! once it lands the verb inherits its save-and-quit handoff with no +//! changes here. +//! - `status` — answered by the socket thread directly from a snapshot the +//! event loop republishes every iteration: `turn_state` +//! (`idle | in_progress | waiting`) and `goal` +//! (`objective`, `status`, `paused`). +//! +//! # Wiring (insertion points) +//! +//! 1. `run_event_loop` (crates/tui/src/tui/ui/event_loop.rs) constructs a +//! [`SessionControl`] once and, at the top of the frame loop, calls +//! [`SessionControl::reconcile`] (bind/rebind/unbind when the owned +//! session id changes), [`SessionControl::update_status`] (publish the +//! snapshot for `status`), and [`SessionControl::drain`] (execute queued +//! verbs on the UI thread; a `true` return asks the loop to quit, which +//! is how `relaunch` reuses the ordinary `/exit` teardown). +//! 2. The socket runs on background threads; verbs that touch UI state cross +//! to the event loop over an mpsc channel and answer over a response +//! channel with a 5 s timeout (a dispatch-to-app pattern). +//! +//! The feature is off unless `[control_socket] enabled = true`; an unset +//! table changes nothing. Unix-only: on non-unix platforms the config key +//! parses but binding is refused at runtime. + +use std::io; +#[cfg(unix)] +use std::io::{BufRead, BufReader, Read, Write}; +use std::path::{Path, PathBuf}; +#[cfg(unix)] +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex, mpsc}; +use std::time::{Duration, Instant}; + +#[cfg(unix)] +use std::fs; +#[cfg(unix)] +use std::os::unix::fs::{FileTypeExt, PermissionsExt}; +#[cfg(unix)] +use std::os::unix::net::{UnixListener, UnixStream}; +#[cfg(unix)] +use std::thread; + +use serde::{Deserialize, Serialize}; + +use crate::tui::app::{App, AppAction, ComposerSubmitAction, QueuedMessage, SubmitDisposition}; +use crate::tui::streaming::StreamDisplayClock; +use crate::tui::ui::{DispatchRecovery, dispatch_composer_message, escape_cancel_request}; + +/// Socket file name inside the per-session artifact directory. +pub(crate) const SOCKET_FILE_NAME: &str = "control.sock"; + +/// Hard cap on one request line (initial-request bound). +#[cfg(unix)] +const MAX_REQUEST_BYTES: usize = 1024 * 1024; + +/// Accept-loop poll interval while the listener is idle. +#[cfg(unix)] +const CONNECTION_POLL_INTERVAL: Duration = Duration::from_millis(100); + +/// A client that connects and never sends is dropped after this long. +#[cfg(unix)] +const REQUEST_READ_TIMEOUT: Duration = Duration::from_secs(5); + +/// Response writes give up after this long rather than blocking forever. +#[cfg(unix)] +const RESPONSE_WRITE_TIMEOUT: Duration = Duration::from_secs(5); + +/// How long a verb may wait for the event loop to handle it +/// (`APP_RESPONSE_TIMEOUT`). +#[cfg(unix)] +const DISPATCH_RESPONSE_TIMEOUT: Duration = Duration::from_secs(5); + +/// Minimum pause between bind retries after a refused takeover, so another +/// live process holding the socket cannot turn the per-frame reconcile into +/// a connect-probe and warn-log flood. Shortened under `#[cfg(test)]` so the +/// backoff itself is testable without sleeping for seconds. +#[cfg(not(test))] +const BIND_RETRY_BACKOFF: Duration = Duration::from_secs(5); +#[cfg(test)] +const BIND_RETRY_BACKOFF: Duration = Duration::from_millis(200); + +// ── Protocol ──────────────────────────────────────────────────────────────── + +/// One request line: `{"id": … , "method": … , "params": …}`. +/// Windows builds construct this type only in the portable protocol tests; +/// the plain Windows lib build leaves it unreachable, so the lint allowance +/// below is scoped to exactly that case (unix builds use it via the socket +/// runtime, and CI denies dead code on the MSVC test gate). +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Deserialize)] +struct Request { + id: String, + #[serde(flatten)] + method: Method, +} + +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Deserialize)] +#[serde(tag = "method", content = "params", rename_all = "snake_case")] +enum Method { + Message(MessageParams), + Interrupt(EmptyParams), + Relaunch(EmptyParams), + Status(EmptyParams), +} + +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Deserialize)] +struct MessageParams { + text: String, +} + +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Deserialize)] +struct EmptyParams {} + +/// A verb the socket thread hands to the event loop, plus the way back. +#[derive(Debug)] +pub(crate) struct PendingCommand { + pub(crate) id: String, + pub(crate) command: ControlCommand, + pub(crate) respond_to: mpsc::Sender, +} + +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug)] +pub(crate) enum ControlCommand { + Message { text: String }, + Interrupt, + Relaunch, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub(crate) enum TurnState { + Idle, + InProgress, + Waiting, +} + +/// Goal state visible to supervisors over the `status` verb. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub(crate) struct GoalSnapshot { + pub(crate) objective: Option, + pub(crate) status: String, + pub(crate) paused: bool, +} + +/// The `status` answer, republished by the event loop every iteration. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct StatusSnapshot { + pub(crate) turn_state: TurnState, + pub(crate) goal: GoalSnapshot, +} + +/// Success envelope (`SuccessResponse` shape). +#[derive(Debug, Serialize)] +struct SuccessResponse { + id: String, + result: ResponseResult, +} + +#[cfg_attr(not(unix), allow(dead_code))] +#[derive(Debug, Serialize)] +#[serde(tag = "type", rename_all = "snake_case")] +enum ResponseResult { + MessageSent { + delivery: &'static str, + }, + Interrupted { + cancelled: bool, + }, + Relaunching, + Status { + turn_state: TurnState, + goal: GoalSnapshot, + }, +} + +/// Error envelope (`ErrorResponse` shape). +#[derive(Debug, Serialize)] +struct ErrorResponse { + id: String, + error: ErrorBody, +} + +#[derive(Debug, Serialize)] +struct ErrorBody { + code: &'static str, + message: String, +} + +fn response_ok(id: String, result: ResponseResult) -> String { + serde_json::to_string(&SuccessResponse { id, result }).unwrap_or_else(|_| { + r#"{"id":"","error":{"code":"internal_error","message":"failed to encode response"}}"# + .to_string() + }) +} + +fn response_error(id: &str, code: &'static str, message: String) -> String { + serde_json::to_string(&ErrorResponse { + id: id.to_string(), + error: ErrorBody { code, message }, + }) + .unwrap_or_else(|_| { + r#"{"id":"","error":{"code":"internal_error","message":"failed to encode response"}}"# + .to_string() + }) +} + +// ── UI-side handle ────────────────────────────────────────────────────────── + +/// The event-loop side of the control surface. Cheap to poll every frame: +/// reconcile/update/drain are all no-ops (or near no-ops) when disabled. +pub(crate) struct SessionControl { + enabled: bool, + sessions_dir: Option, + bound_session: Option, + socket: Option, + commands_tx: Option>, + commands_rx: mpsc::Receiver, + status: Arc>, + /// When the last bind attempt failed (e.g. another live process owns the + /// socket), retries for *that session* back off so a refused takeover + /// cannot become a per-frame connect-probe and log flood. + last_bind_failure: Option<(String, Instant)>, +} + +impl SessionControl { + pub(crate) fn new(enabled: bool) -> Self { + Self::new_with_sessions_dir(enabled, None) + } + + /// Test seam: `sessions_dir` bypasses `SessionManager::default_location()` + /// so tests never touch the real `~/.codewhale/sessions`. + fn new_with_sessions_dir(enabled: bool, sessions_dir: Option) -> Self { + let (commands_tx, commands_rx) = mpsc::channel(); + Self { + enabled, + sessions_dir, + bound_session: None, + socket: None, + commands_tx: enabled.then_some(commands_tx), + commands_rx, + status: Arc::new(Mutex::new(StatusSnapshot { + turn_state: TurnState::Idle, + goal: GoalSnapshot { + objective: None, + status: "active".to_string(), + paused: false, + }, + })), + last_bind_failure: None, + } + } + + /// Bind/rebind the socket when the owned session id appears or changes, + /// and unbind when it disappears (session switch or teardown). Runs on + /// the event-loop thread but only spawns a thread on an actual change. + pub(crate) fn reconcile(&mut self, current_session_id: Option<&str>) { + if !self.enabled { + return; + } + let Some(id) = current_session_id + .map(str::trim) + .filter(|id| !id.is_empty()) + else { + // No session yet (fresh session before the first snapshot) or + // the id went away: release whatever we hold. + self.socket = None; + self.bound_session = None; + return; + }; + if self.bound_session.as_deref() == Some(id) { + return; + } + // A refused takeover must not retry every frame: back off so the + // connect probe and its warning log run at most every few seconds. + // Keyed on the session id so switching sessions is never delayed by + // another session's refusal. + if let Some((failed_id, failed_at)) = &self.last_bind_failure + && failed_id == id + && failed_at.elapsed() < BIND_RETRY_BACKOFF + { + return; + } + // Session id changed: drop the old listener first so the socket file + // is unlinked before the new one binds. + self.socket = None; + self.bound_session = None; + + let sessions_dir = match self.sessions_dir.clone() { + Some(dir) => dir, + None => { + let manager = match crate::session_manager::SessionManager::default_location() { + Ok(manager) => manager, + Err(error) => { + tracing::warn!(%error, "control socket: cannot resolve the sessions directory"); + return; + } + }; + let dir = manager.sessions_dir().to_path_buf(); + self.sessions_dir = Some(dir.clone()); + dir + } + }; + let Some(commands_tx) = self.commands_tx.clone() else { + return; + }; + match bind_control_socket(&sessions_dir, id, commands_tx, Arc::clone(&self.status)) { + Ok(handle) => { + tracing::info!( + session = id, + path = %sessions_dir.join(id).join(SOCKET_FILE_NAME).display(), + "control socket listening" + ); + self.bound_session = Some(id.to_string()); + self.socket = Some(handle); + self.last_bind_failure = None; + } + Err(error) => { + tracing::warn!(session = id, %error, "control socket: bind failed; session control unavailable"); + self.last_bind_failure = Some((id.to_string(), Instant::now())); + } + } + } + + /// Republish the `status` snapshot from the current app state. Runs every + /// frame; the mutex write happens only when something actually changed. + pub(crate) fn update_status(&self, app: &App) { + if !self.enabled { + return; + } + let snapshot = snapshot_from_app(app); + let Ok(mut guard) = self.status.try_lock() else { + return; // the socket thread is answering a `status` request; skip a frame + }; + if *guard != snapshot { + *guard = snapshot; + } + } + + /// Execute verbs queued by the socket thread on the UI thread and answer + /// their clients. Returns `true` when a verb requested app quit (the + /// `relaunch` seam) — the caller returns from the event loop and reuses + /// the ordinary `/exit` teardown path. + pub(crate) async fn drain( + &mut self, + app: &mut App, + config: &crate::config::Config, + engine_handle: &crate::core::engine::EngineHandle, + current_streaming_text: &mut String, + stream_display_clock: &mut StreamDisplayClock, + ) -> bool { + if !self.enabled { + return false; + } + let mut quit = false; + while let Ok(pending) = self.commands_rx.try_recv() { + let (do_quit, response) = execute_command( + app, + config, + engine_handle, + current_streaming_text, + stream_display_clock, + pending.id.clone(), + pending.command, + ) + .await; + // The client may have disconnected while we worked; that must + // never fail the loop. + let _ = pending.respond_to.send(response); + quit |= do_quit; + } + quit + } +} + +fn snapshot_from_app(app: &App) -> StatusSnapshot { + let turn_state = + if app.is_loading || matches!(app.runtime_turn_status.as_deref(), Some("in_progress")) { + TurnState::InProgress + } else if app.goal_continuation_waiting { + TurnState::Waiting + } else { + TurnState::Idle + }; + // A paused goal parks its objective in `paused_goal_objective`, so the + // snapshot surfaces the objective that is actually in flight. + let objective = app + .goal + .objective + .clone() + .or_else(|| app.paused_goal_objective.clone()); + StatusSnapshot { + turn_state, + goal: GoalSnapshot { + objective, + status: app.goal.status.as_str().to_string(), + paused: app.paused || app.paused_goal_objective.is_some(), + }, + } +} + +/// Execute one verb on the UI thread. Returns `(quit, response_json)`. +async fn execute_command( + app: &mut App, + config: &crate::config::Config, + engine_handle: &crate::core::engine::EngineHandle, + current_streaming_text: &mut String, + stream_display_clock: &mut StreamDisplayClock, + id: String, + command: ControlCommand, +) -> (bool, String) { + match command { + ControlCommand::Message { text } => { + if text.trim().is_empty() { + return ( + false, + response_error( + &id, + "invalid_request", + "message text must not be empty".to_string(), + ), + ); + } + // Queued delivery is the default under load: while a turn is in + // flight the message waits like any queued follow-up; an idle + // app dispatches immediately. + let busy = + app.is_loading || matches!(app.runtime_turn_status.as_deref(), Some("in_progress")); + let disposition = if busy { + SubmitDisposition::Queue + } else { + SubmitDisposition::Immediate + }; + let message = QueuedMessage::new(text, None); + // Delivery failures surface through the app's own status/toast + // and recovery paths; the verb still answers with what it asked + // for (dispatched vs queued). + let _ = dispatch_composer_message( + app, + config, + engine_handle, + message, + DispatchRecovery::Immediate, + ComposerSubmitAction::Submit(disposition), + ) + .await; + app.needs_redraw = true; + let delivery = if busy { "queued" } else { "dispatched" }; + ( + false, + response_ok(id, ResponseResult::MessageSent { delivery }), + ) + } + ControlCommand::Interrupt => { + let had_active_work = app.is_loading + || app.is_compacting + || app.manual_compaction_queued + || app.goal_continuation_waiting + || app.paused + || app.paused_goal_objective.is_some() + || matches!(app.runtime_turn_status.as_deref(), Some("in_progress")); + if !had_active_work { + // Nothing Esc-cancel would cancel: quiet no-op, like an Esc + // on an idle app that has nothing else to act on. + return ( + false, + response_ok(id, ResponseResult::Interrupted { cancelled: false }), + ); + } + let _ = escape_cancel_request( + app, + engine_handle, + current_streaming_text, + stream_display_clock, + ); + app.needs_redraw = true; + ( + false, + response_ok(id, ResponseResult::Interrupted { cancelled: true }), + ) + } + ControlCommand::Relaunch => { + // Seam: the exact same command path `/relaunch` uses. When the + // /relaunch command lands (pr/relaunch-command), this returns its + // save-and-quit action and the quit flag below reuses the /exit + // teardown; until then the command's own error is reported. + let result = crate::commands::execute("/relaunch", app); + if result.is_error { + return ( + false, + response_error( + &id, + "command_error", + result + .message + .unwrap_or_else(|| "relaunch failed".to_string()), + ), + ); + } + let quit = matches!(result.action, Some(AppAction::Quit)); + app.needs_redraw = true; + (quit, response_ok(id, ResponseResult::Relaunching)) + } + } +} + +// ── Socket server (unix only) ─────────────────────────────────────────────── + +/// Bound listener + its accept thread. Dropping unbinds: the accept thread +/// stops within one poll interval, the socket file is unlinked if this +/// process still owns it, and in-flight connections finish on their own. +#[cfg(unix)] +pub(crate) struct ControlSocketHandle { + stop: Arc, + thread: Option>, +} + +#[cfg(unix)] +impl Drop for ControlSocketHandle { + fn drop(&mut self) { + self.stop.store(true, Ordering::Release); + if let Some(thread) = self.thread.take() { + // The accept loop polls at CONNECTION_POLL_INTERVAL and never + // blocks on a connection (each connection has its own thread), + // so this join is bounded and cannot deadlock. + let _ = thread.join(); + } + } +} + +#[cfg(unix)] +impl std::fmt::Debug for ControlSocketHandle { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ControlSocketHandle") + .field("stopped", &self.stop.load(Ordering::Relaxed)) + .finish_non_exhaustive() + } +} + +#[cfg(not(unix))] +#[derive(Debug)] +#[allow(dead_code)] // kept so the SessionControl field type is portable +pub(crate) struct ControlSocketHandle; + +/// Bind `//control.sock` (0600) and serve it. +/// Refused when another live process already serves that path; a stale file +/// (crash leftover, nothing answering) is taken over. +#[cfg(unix)] +pub(crate) fn bind_control_socket( + sessions_dir: &Path, + session_id: &str, + commands_tx: mpsc::Sender, + status: Arc>, +) -> io::Result { + let session_dir = sessions_dir.join(session_id); + fs::create_dir_all(&session_dir)?; + let path = session_dir.join(SOCKET_FILE_NAME); + prepare_socket_path(&path)?; + + let listener = UnixListener::bind(&path)?; + let identity = socket_file_identity(&path); + fs::set_permissions(&path, fs::Permissions::from_mode(0o600))?; + listener.set_nonblocking(true)?; + + let stop = Arc::new(AtomicBool::new(false)); + let thread_stop = Arc::clone(&stop); + let thread = thread::Builder::new() + .name(format!("codewhale-control-{session_id}")) + .spawn(move || serve(listener, path, identity, thread_stop, commands_tx, status))?; + + Ok(ControlSocketHandle { + stop, + thread: Some(thread), + }) +} + +#[cfg(not(unix))] +pub(crate) fn bind_control_socket( + _sessions_dir: &Path, + _session_id: &str, + _commands_tx: mpsc::Sender, + _status: Arc>, +) -> io::Result { + Err(io::Error::new( + io::ErrorKind::Unsupported, + "the per-session control socket is unix-only", + )) +} + +/// Take over the socket path, or refuse when a live server already holds it. +#[cfg(unix)] +fn prepare_socket_path(path: &Path) -> io::Result<()> { + match fs::symlink_metadata(path) { + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + Ok(metadata) => { + if !metadata.file_type().is_socket() { + // A plain file (or directory) in the way: not ours to keep. + fs::remove_file(path)?; + return Ok(()); + } + match UnixStream::connect(path) { + // Someone answers: a live process owns this session's socket. + // Do not steal it (a "socket busy" refusal). + Ok(_) => Err(io::Error::new( + io::ErrorKind::AddrInUse, + format!("control socket already live at {}", path.display()), + )), + // Stale: the file exists but nothing listens. Take over. + Err(_) => { + fs::remove_file(path)?; + Ok(()) + } + } + } + } +} + +/// (device, inode) so an unlink never removes a file this process did not bind. +#[cfg(unix)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct SocketFileIdentity { + dev: u64, + ino: u64, +} + +#[cfg(unix)] +fn socket_file_identity(path: &Path) -> Option { + let metadata = fs::metadata(path).ok()?; + use std::os::unix::fs::MetadataExt; + Some(SocketFileIdentity { + dev: metadata.dev(), + ino: metadata.ino(), + }) +} + +#[cfg(unix)] +fn serve( + listener: UnixListener, + path: PathBuf, + identity: Option, + stop: Arc, + commands_tx: mpsc::Sender, + status: Arc>, +) { + while !stop.load(Ordering::Acquire) { + match listener.accept() { + Ok((stream, _)) => { + // The listener is nonblocking, and on BSD-family platforms + // (macOS, FreeBSD) an accepted socket *inherits* O_NONBLOCK + // from the listener — Linux does not. The per-connection + // handler expects blocking reads/writes (bounded by request + // caps and timeouts), so make that explicit: without it, a + // read on macOS returns EAGAIN mid-frame on a large request + // and the connection dies with a broken pipe on the client. + let _ = stream.set_nonblocking(false); + // One thread per connection: a silent client + // must not stall other clients or the stop check. + let tx = commands_tx.clone(); + let status = Arc::clone(&status); + let _ = thread::Builder::new() + .name("codewhale-control-conn".to_string()) + .spawn(move || handle_connection(stream, &tx, &status)); + } + Err(error) if error.kind() == io::ErrorKind::WouldBlock => { + thread::sleep(CONNECTION_POLL_INTERVAL); + } + Err(error) if error.kind() == io::ErrorKind::Interrupted => {} + Err(error) => { + // Listener gone (e.g. the session dir was removed out from + // under us) — stop serving; connections fail to connect from + // here on, which is the honest state. + tracing::debug!(%error, "control socket listener closed"); + break; + } + } + } + if let Some(identity) = identity + && socket_file_identity(&path) == Some(identity) + { + let _ = fs::remove_file(&path); + } +} + +/// Serve exactly one request: read one bounded line, answer, close. +#[cfg(unix)] +fn handle_connection( + stream: UnixStream, + commands_tx: &mpsc::Sender, + status: &Arc>, +) { + let _ = stream.set_read_timeout(Some(REQUEST_READ_TIMEOUT)); + let _ = stream.set_write_timeout(Some(RESPONSE_WRITE_TIMEOUT)); + let mut stream = BufReader::new(stream); + + let line = match read_request_line(&mut stream) { + Ok(Some(line)) => line, + Ok(None) => return, // EOF, empty frame, or timeout: close silently + Err(error) if error.kind() == io::ErrorKind::InvalidData => { + // Oversized frame: the reader drained it, so the client can + // finish writing and read this rejection. + let response = response_error("", "invalid_request", error.to_string()); + write_response_line(stream.get_mut(), &response); + return; + } + Err(_) => return, + }; + let trimmed = line.trim(); + if trimmed.is_empty() { + return; + } + + let request: Request = match serde_json::from_str(trimmed) { + Ok(request) => request, + Err(error) => { + let response = + response_error("", "invalid_request", format!("invalid request: {error}")); + write_response_line(stream.get_mut(), &response); + return; + } + }; + + let response = match request.method { + Method::Status(_) => { + let snapshot = status + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .clone(); + response_ok( + request.id, + ResponseResult::Status { + turn_state: snapshot.turn_state, + goal: snapshot.goal, + }, + ) + } + Method::Message(params) => dispatch_to_app( + request.id, + ControlCommand::Message { text: params.text }, + commands_tx, + ), + Method::Interrupt(_) => dispatch_to_app(request.id, ControlCommand::Interrupt, commands_tx), + Method::Relaunch(_) => dispatch_to_app(request.id, ControlCommand::Relaunch, commands_tx), + }; + write_response_line(stream.get_mut(), &response); +} + +/// Hand a verb to the event loop and wait (bounded) for its answer. +#[cfg(unix)] +fn dispatch_to_app( + id: String, + command: ControlCommand, + commands_tx: &mpsc::Sender, +) -> String { + let (respond_to, rx) = mpsc::channel(); + if let Err(error) = commands_tx.send(PendingCommand { + id: id.clone(), + command, + respond_to, + }) { + return response_error( + &id, + "server_unavailable", + format!("failed to dispatch request: {error}"), + ); + } + match rx.recv_timeout(DISPATCH_RESPONSE_TIMEOUT) { + Ok(response) => response, + Err(mpsc::RecvTimeoutError::Timeout) => response_error( + &id, + "timeout", + format!( + "timed out waiting for the app to handle the request after {} ms", + DISPATCH_RESPONSE_TIMEOUT.as_millis() + ), + ), + Err(mpsc::RecvTimeoutError::Disconnected) => response_error( + &id, + "server_unavailable", + "request handling failed: app response channel closed".to_string(), + ), + } +} + +/// One newline-terminated line, bounded. `Ok(None)` = EOF before any content. +/// The cap is enforced *while* reading (a hostile peer cannot make us buffer +/// an unbounded line), and on oversize the remainder of the frame is +/// discarded through a fixed-size buffer — memory stays bounded, and a client +/// that wrote the whole request can still receive the rejection. +#[cfg(unix)] +fn read_request_line(stream: &mut BufReader) -> io::Result> { + let mut capped = stream.by_ref().take(MAX_REQUEST_BYTES as u64 + 1); + let mut line = Vec::new(); + let read = capped.read_until(b'\n', &mut line)?; + if read == 0 { + return Ok(None); // EOF before any content + } + if line.last() != Some(&b'\n') { + // The frame exceeded the cap (or was torn mid-line). Discard the + // remainder so a well-behaved client finishes its write and reads + // the rejection; a torn frame's write lands nowhere and is ignored. + let mut buf = [0u8; 8192]; + loop { + match stream.read(&mut buf) { + Ok(0) | Err(_) => break, + Ok(n) => { + if buf[..n].contains(&b'\n') { + break; + } + } + } + } + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("request exceeds {MAX_REQUEST_BYTES} bytes"), + )); + } + Ok(Some(String::from_utf8_lossy(&line).into_owned())) +} + +#[cfg(unix)] +fn write_response_line(stream: &mut UnixStream, value: &str) { + let _ = writeln!(stream, "{value}"); + let _ = stream.flush(); +} + +#[cfg(test)] +mod tests { + use super::*; + + // ── Verb parsing ────────────────────────────────────────────────────── + + #[test] + fn parses_each_verb_request() { + let message: Request = + serde_json::from_str(r#"{"id":"1","method":"message","params":{"text":"hi"}}"#) + .expect("message request"); + assert_eq!(message.id, "1"); + assert!(matches!(message.method, Method::Message(p) if p.text == "hi")); + + for (raw, want) in [ + ( + r#"{"id":"2","method":"interrupt","params":{}}"#, + "interrupt", + ), + (r#"{"id":"3","method":"relaunch","params":{}}"#, "relaunch"), + (r#"{"id":"4","method":"status","params":{}}"#, "status"), + ] { + let request: Request = serde_json::from_str(raw).expect("verb request"); + let got = match request.method { + Method::Message(_) => "message", + Method::Interrupt(_) => "interrupt", + Method::Relaunch(_) => "relaunch", + Method::Status(_) => "status", + }; + assert_eq!(got, want); + } + } + + #[test] + fn rejects_unknown_verb() { + let error = serde_json::from_str::(r#"{"id":"1","method":"dance","params":{}}"#) + .expect_err("unknown verb must not parse"); + let message = error.to_string(); + assert!(message.contains("unknown variant"), "{message}"); + } + + #[test] + fn rejects_missing_or_wrong_params() { + // message without `text` + let error = serde_json::from_str::(r#"{"id":"1","method":"message","params":{}}"#) + .expect_err("message without text must not parse"); + assert!(error.to_string().contains("missing field"), "{error}"); + + // missing params entirely + let error = serde_json::from_str::(r#"{"id":"1","method":"status"}"#) + .expect_err("missing params must not parse"); + assert!(!error.to_string().is_empty()); + + // non-string text + let error = + serde_json::from_str::(r#"{"id":"1","method":"message","params":{"text":7}}"#) + .expect_err("numeric text must not parse"); + assert!(!error.to_string().is_empty()); + + // non-string id + let error = serde_json::from_str::(r#"{"id":7,"method":"status","params":{}}"#) + .expect_err("numeric id must not parse"); + assert!(!error.to_string().is_empty()); + } + + #[test] + fn serializes_responses_in_the_response_envelope_shape() { + let sent = response_ok( + "1".into(), + ResponseResult::MessageSent { delivery: "queued" }, + ); + assert_eq!( + sent, + r#"{"id":"1","result":{"type":"message_sent","delivery":"queued"}}"# + ); + + let interrupted = response_ok("2".into(), ResponseResult::Interrupted { cancelled: true }); + assert_eq!( + interrupted, + r#"{"id":"2","result":{"type":"interrupted","cancelled":true}}"# + ); + + let relaunching = response_ok("3".into(), ResponseResult::Relaunching); + assert_eq!(relaunching, r#"{"id":"3","result":{"type":"relaunching"}}"#); + + let status = response_ok( + "4".into(), + ResponseResult::Status { + turn_state: TurnState::Idle, + goal: GoalSnapshot { + objective: Some("ship it".to_string()), + status: "active".to_string(), + paused: false, + }, + }, + ); + assert_eq!( + status, + r#"{"id":"4","result":{"type":"status","turn_state":"idle","goal":{"objective":"ship it","status":"active","paused":false}}}"# + ); + + let error = response_error("9", "invalid_request", "nope".to_string()); + assert_eq!( + error, + r#"{"id":"9","error":{"code":"invalid_request","message":"nope"}}"# + ); + } + + // ── Socket framing (unix) ───────────────────────────────────────────── + + #[cfg(unix)] + fn test_endpoint() -> ( + tempfile::TempDir, + PathBuf, + mpsc::Receiver, + ControlSocketHandle, + ) { + let temp = tempfile::TempDir::new().expect("temp dir"); + let sessions_dir = temp.path().join("sessions"); + let (tx, rx) = mpsc::channel(); + let status = Arc::new(Mutex::new(StatusSnapshot { + turn_state: TurnState::Idle, + goal: GoalSnapshot { + objective: Some("goal".to_string()), + status: "active".to_string(), + paused: false, + }, + })); + let handle = bind_control_socket(&sessions_dir, "test-session", tx, status).expect("bind"); + ( + temp, + sessions_dir.join("test-session").join(SOCKET_FILE_NAME), + rx, + handle, + ) + } + + #[cfg(unix)] + fn request_response(path: &Path, request: &str) -> String { + let mut stream = UnixStream::connect(path).expect("connect"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("read timeout"); + writeln!(stream, "{request}").expect("write request"); + let mut response = String::new(); + BufReader::new(stream) + .read_line(&mut response) + .expect("read response"); + response + } + + #[cfg(unix)] + #[test] + fn status_verb_answers_over_the_socket() { + let (_temp, path, _rx, _handle) = test_endpoint(); + let response = request_response(&path, r#"{"id":"4","method":"status","params":{}}"#); + let value: serde_json::Value = serde_json::from_str(&response).expect("response is json"); + assert_eq!(value["id"], "4"); + assert_eq!(value["result"]["type"], "status"); + assert_eq!(value["result"]["turn_state"], "idle"); + assert_eq!(value["result"]["goal"]["objective"], "goal"); + assert_eq!(value["result"]["goal"]["paused"], false); + } + + #[cfg(unix)] + #[test] + fn message_verb_reaches_the_app_channel_and_answers() { + let (_temp, path, rx, _handle) = test_endpoint(); + + // The test stands in for the event loop on the other end of the + // channel: it receives the verb and answers like `drain` would. + let server = std::thread::spawn(move || { + let pending = rx + .recv_timeout(Duration::from_secs(5)) + .expect("verb queued"); + assert_eq!(pending.id, "1"); + match pending.command { + ControlCommand::Message { text } => assert_eq!(text, "hello"), + other => panic!("expected Message, got {other:?}"), + } + pending + .respond_to + .send(response_ok( + "1".into(), + ResponseResult::MessageSent { + delivery: "dispatched", + }, + )) + .expect("answer"); + }); + + let response = request_response( + &path, + r#"{"id":"1","method":"message","params":{"text":"hello"}}"#, + ); + server.join().expect("server thread"); + let value: serde_json::Value = serde_json::from_str(&response).expect("response is json"); + assert_eq!(value["id"], "1"); + assert_eq!(value["result"]["type"], "message_sent"); + assert_eq!(value["result"]["delivery"], "dispatched"); + } + + #[cfg(unix)] + #[test] + fn malformed_json_gets_invalid_request_error() { + let (_temp, path, _rx, _handle) = test_endpoint(); + let response = request_response(&path, "{not json"); + let value: serde_json::Value = serde_json::from_str(&response).expect("response is json"); + assert_eq!(value["id"], ""); + assert_eq!(value["error"]["code"], "invalid_request"); + } + + #[cfg(unix)] + #[test] + fn unknown_verb_gets_invalid_request_error() { + let (_temp, path, _rx, _handle) = test_endpoint(); + let response = request_response(&path, r#"{"id":"7","method":"dance","params":{}}"#); + let value: serde_json::Value = serde_json::from_str(&response).expect("response is json"); + assert_eq!(value["id"], ""); + assert_eq!(value["error"]["code"], "invalid_request"); + } + + #[cfg(unix)] + #[test] + fn empty_line_closes_without_a_response() { + let (_temp, path, _rx, _handle) = test_endpoint(); + let mut stream = UnixStream::connect(&path).expect("connect"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("read timeout"); + writeln!(stream).expect("write empty line"); + let mut response = String::new(); + let read = BufReader::new(stream) + .read_line(&mut response) + .expect("read"); + assert_eq!(read, 0, "empty line must close the connection silently"); + assert!(response.is_empty()); + } + + #[cfg(unix)] + #[test] + fn oversized_request_is_rejected_with_an_error() { + let (_temp, path, _rx, _handle) = test_endpoint(); + let mut stream = UnixStream::connect(&path).expect("connect"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("read timeout"); + let blob = "x".repeat(MAX_REQUEST_BYTES + 16); + let request = format!(r#"{{"id":"1","method":"message","params":{{"text":"{blob}"}}}}"#); + writeln!(stream, "{request}").expect("write oversized request"); + let mut response = String::new(); + BufReader::new(stream) + .read_line(&mut response) + .expect("read rejection"); + let value: serde_json::Value = serde_json::from_str(&response).expect("response is json"); + assert_eq!(value["error"]["code"], "invalid_request"); + } + + #[cfg(unix)] + #[test] + fn bind_refuses_a_live_socket_and_takes_over_a_stale_file() { + let temp = tempfile::TempDir::new().expect("temp dir"); + let sessions_dir = temp.path().join("sessions"); + let socket_path = sessions_dir.join("test-session").join(SOCKET_FILE_NAME); + let status = Arc::new(Mutex::new(StatusSnapshot { + turn_state: TurnState::Idle, + goal: GoalSnapshot { + objective: None, + status: "active".to_string(), + paused: false, + }, + })); + let (tx, _rx) = mpsc::channel(); + + // A stale plain file is taken over. + fs::create_dir_all(socket_path.parent().expect("parent")).expect("mkdir"); + fs::write(&socket_path, b"stale").expect("write stale file"); + let handle = bind_control_socket( + &sessions_dir, + "test-session", + tx.clone(), + Arc::clone(&status), + ) + .expect("bind over a stale file"); + drop(handle); + + // A live listener is refused. + let _ = fs::remove_file(&socket_path); + let live = UnixListener::bind(&socket_path).expect("bind live listener"); + let error = bind_control_socket(&sessions_dir, "test-session", tx, status) + .expect_err("must refuse a live socket"); + assert_eq!(error.kind(), io::ErrorKind::AddrInUse); + drop(live); + let _ = fs::remove_file(&socket_path); + } + + #[cfg(unix)] + #[test] + fn drop_unbinds_and_unlinks_the_socket() { + let temp = tempfile::TempDir::new().expect("temp dir"); + let sessions_dir = temp.path().join("sessions"); + let socket_path = sessions_dir.join("test-session").join(SOCKET_FILE_NAME); + let (tx, _rx) = mpsc::channel(); + let status = Arc::new(Mutex::new(StatusSnapshot { + turn_state: TurnState::Idle, + goal: GoalSnapshot { + objective: None, + status: "active".to_string(), + paused: false, + }, + })); + let handle = bind_control_socket(&sessions_dir, "test-session", tx, status).expect("bind"); + assert!(socket_path.exists(), "socket file exists while bound"); + drop(handle); + // The accept thread unlinks within one poll interval. + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while socket_path.exists() && std::time::Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(20)); + } + assert!( + !socket_path.exists(), + "socket file must be unlinked after drop" + ); + } + + #[cfg(unix)] + #[test] + fn reconcile_backs_off_after_a_refused_takeover() { + let temp = tempfile::TempDir::new().expect("temp dir"); + let sessions_dir = temp.path().join("sessions"); + let socket_path = sessions_dir.join("sess").join(SOCKET_FILE_NAME); + fs::create_dir_all(socket_path.parent().expect("parent")).expect("mkdir"); + + let mut control = SessionControl::new_with_sessions_dir(true, Some(sessions_dir.clone())); + + // A live listener occupies the path: the takeover is refused. + let live = UnixListener::bind(&socket_path).expect("bind live listener"); + control.reconcile(Some("sess")); + assert!( + control.bound_session.is_none(), + "refused bind must not claim" + ); + + // The other process goes away, but the backoff still holds. + drop(live); + let _ = fs::remove_file(&socket_path); + control.reconcile(Some("sess")); + assert!( + control.bound_session.is_none(), + "backoff must suppress an immediate rebind" + ); + + // After the backoff window, the same session binds successfully. + std::thread::sleep(BIND_RETRY_BACKOFF + Duration::from_millis(50)); + control.reconcile(Some("sess")); + assert_eq!(control.bound_session.as_deref(), Some("sess")); + + // Reconcile with the same id is a no-op; a different id rebinds. + control.reconcile(Some("sess")); + assert_eq!(control.bound_session.as_deref(), Some("sess")); + control.reconcile(Some("other")); + assert_eq!(control.bound_session.as_deref(), Some("other")); + drop(control); + } +} diff --git a/crates/tui/src/tui/mod.rs b/crates/tui/src/tui/mod.rs index 7190a3b4b5..0873573900 100644 --- a/crates/tui/src/tui/mod.rs +++ b/crates/tui/src/tui/mod.rs @@ -32,6 +32,7 @@ pub mod composer_chrome; pub mod composer_ui; pub mod context_inspector; pub mod context_menu; +pub(crate) mod control_socket; pub(crate) mod coordination_detail; pub(crate) mod cursor_accent; pub mod diff_render; diff --git a/crates/tui/src/tui/phase_strip.rs b/crates/tui/src/tui/phase_strip.rs index ff35491168..1f73301cb1 100644 --- a/crates/tui/src/tui/phase_strip.rs +++ b/crates/tui/src/tui/phase_strip.rs @@ -977,3 +977,18 @@ pub(crate) fn tideline_footer_from_app(app: &mut App, width: u16) -> TidelineFoo #[cfg(test)] mod tideline_tests; + +#[cfg(test)] +mod neutrality_tests { + #[test] + fn session_metrics_strip_is_on_by_default() { + assert!( + crate::config::StatusItem::default_footer() + .contains(&crate::config::StatusItem::SessionMetrics) + ); + assert_eq!( + crate::config::StatusItem::from_key("session_metrics"), + Some(crate::config::StatusItem::SessionMetrics) + ); + } +} diff --git a/crates/tui/src/tui/prompt_suggestion.rs b/crates/tui/src/tui/prompt_suggestion.rs index 4204e27f28..0dfe5343b0 100644 --- a/crates/tui/src/tui/prompt_suggestion.rs +++ b/crates/tui/src/tui/prompt_suggestion.rs @@ -149,15 +149,14 @@ impl fmt::Debug for SuggestionLaunch { } } -/// Whether a provider speaks the exact DeepSeek OpenAI-compatible +/// Whether a provider speaks the ordinary OpenAI-compatible /// `/chat/completions` shape [`generate_suggestion`] hardcodes. /// -/// This is deliberately narrow: `DeepseekAnthropic` is a different wire -/// protocol, and every other provider is out of scope. Widening this set is a -/// feature change, not a bug fix. +/// Gate on wire protocol, not a vendor enum: Anthropic Messages and the +/// OpenAI Responses API are different request shapes and stay out. #[must_use] pub fn route_is_supported_suggestion_provider(provider: ApiProvider) -> bool { - matches!(provider, ApiProvider::Deepseek | ApiProvider::DeepseekCN) + crate::client::provider_speaks_chat_completions(provider) } /// Resolve credentials for exactly one configured route identity. @@ -181,8 +180,8 @@ fn resolve_credentials_for_identity( provider_identity: &str, model: &str, ) -> Option { - // Belt and braces: callers already gated, but this function must never be - // the thing that reads a non-DeepSeek route's credentials. + // Belt and braces: callers already gated, but this function must never + // read credentials for a wire this helper does not speak. if !route_is_supported_suggestion_provider(provider) { return None; } @@ -225,9 +224,9 @@ fn resolve_credentials_for_identity( /// whatever route config describes *now*, not the route the turn is running on. /// The receipt was minted from the installed client itself, so it cannot drift. /// -/// Unsupported providers — every non-DeepSeek route, including -/// `DeepseekAnthropic` — return `None`, and no credential material of any -/// provider is inspected on this path at all. +/// Unsupported providers — Anthropic Messages, Responses, and any other +/// non-Chat-Completions wire — return `None`, and no credential material +/// of any provider is inspected on this path at all. #[must_use] pub fn capture_route_authority(route: &TurnRoute) -> Option { if !route_is_supported_suggestion_provider(route.provider) { @@ -261,8 +260,8 @@ pub fn capture_route_authority(route: &TurnRoute) -> Option r, Err(_) => return None, }; @@ -689,9 +694,9 @@ mod tests { } #[test] - fn non_deepseek_completion_never_touches_deepseek_credentials() { - // A DeepSeek key exists and would resolve fine — the gate must run - // before the resolver is ever consulted. + fn non_chat_completions_completion_never_touches_foreign_credentials() { + // A Chat Completions key exists and would resolve fine — the gate must + // run before the resolver is ever consulted. let resolver = RecordingResolver::new(vec![( ApiProvider::Deepseek, "deepseek", @@ -699,9 +704,7 @@ mod tests { )]); for (provider, identity, model) in [ (ApiProvider::Anthropic, "anthropic", "claude-sonnet-4"), - (ApiProvider::Openai, "openai", "gpt-4.1"), - (ApiProvider::Openrouter, "openrouter", "some/model"), - (ApiProvider::Custom, "lm-studio", "local-model"), + (ApiProvider::OpenaiCodex, "openai-codex", "gpt-5.4"), ( ApiProvider::DeepseekAnthropic, "deepseek-anthropic", @@ -728,6 +731,65 @@ mod tests { ); } + #[test] + fn chat_completions_routes_launch_with_their_own_credentials() { + for (provider, identity, model, base, key) in [ + ( + ApiProvider::Deepseek, + "deepseek", + "deepseek-chat", + DEEPSEEK_BASE, + DEEPSEEK_KEY, + ), + ( + ApiProvider::Openai, + "openai", + "gpt-5.6", + "https://api.openai.com/v1", + "sk-openai", + ), + ( + ApiProvider::Openrouter, + "openrouter", + "some/model", + "https://openrouter.ai/api/v1", + "sk-or", + ), + ( + ApiProvider::Custom, + "lm-studio", + "local-model", + "http://127.0.0.1:1234/v1", + "lm-key", + ), + ( + ApiProvider::Zai, + "zai", + "GLM-5.3", + "https://api.z.ai/api/paas/v4", + "zai-key", + ), + ] { + let resolver = + RecordingResolver::new(vec![(provider, identity, credentials(key, base, model))]); + let authority = route_authority(provider, identity, model, base, key); + let route = SuggestionRouteSnapshot { + provider, + provider_identity: identity, + model, + authority: &authority, + actual_base_url: Some(base), + }; + let launch = + plan_suggestion_launch(true, true, 2, Some(route), |route| resolver.resolve(route)) + .unwrap_or_else(|| panic!("{provider:?} Chat Completions route must launch")); + assert_eq!(launch.api_key, key, "{provider:?}"); + assert_eq!(launch.base_url, base, "{provider:?}"); + assert_eq!(launch.model, model, "{provider:?}"); + assert_eq!(resolver.asked(), vec![(provider, identity.to_string())]); + } + } + #[test] fn missing_credentials_fail_closed() { let authority = deepseek_authority("deepseek-chat"); @@ -1552,16 +1614,14 @@ mod tests { #[test] fn config_unsupported_providers_capture_no_authority() { let _env = seal_deepseek_env(); - // A usable DeepSeek credential exists in this config, and each route - // below is even handed a DeepSeek-shaped receipt. An unsupported + // A usable Chat Completions credential exists in this config, and + // each route below is even handed a receipt. A Messages/Responses // completed route must still capture nothing. let config = deepseek_config(DEEPSEEK_KEY, DEEPSEEK_BASE); for (provider, identity, model) in [ (ApiProvider::Anthropic, "anthropic", "claude-sonnet-4"), - (ApiProvider::Openai, "openai", "gpt-4.1"), - (ApiProvider::Openrouter, "openrouter", "some/model"), - (ApiProvider::Custom, "lm-studio", "local-model"), + (ApiProvider::OpenaiCodex, "openai-codex", "gpt-5.4"), ( ApiProvider::DeepseekAnthropic, "deepseek-anthropic", diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index b079b2373e..16516bc65a 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -1055,6 +1055,52 @@ fn mark_active_turn_cancelled_locally(app: &mut App) { crate::tui::notifications::stop_title_animation_quietly(); } +/// The Esc-shaped "cancel the active turn" body, extracted verbatim from the +/// event loop's `EscapeAction::CancelRequest` arm so the session control +/// socket's `interrupt` verb and the Esc key cannot drift apart. Returns +/// `true` when the caller should stop handling the event (compaction cancel +/// or goal-continuation stop consumed it), `false` otherwise. The caller +/// keeps its own Esc-specific state (backtrack reset) outside this body. +pub(crate) fn escape_cancel_request( + app: &mut App, + engine_handle: &EngineHandle, + current_streaming_text: &mut String, + stream_display_clock: &mut StreamDisplayClock, +) -> bool { + if try_cancel_compaction(app, engine_handle) { + return true; + } + if app.paused || app.paused_goal_objective.is_some() { + clear_paused_command_state(app, engine_handle); + if app.is_loading || matches!(app.runtime_turn_status.as_deref(), Some("in_progress")) { + engine_handle.cancel(); + mark_active_turn_cancelled_locally(app); + current_streaming_text.clear(); + stream_display_clock.reset(); + } + app.active_allowed_tools = None; + app.goal.objective = None; + app.goal.tokens_used = 0; + app.goal.time_used_seconds = 0; + app.goal.continuation_count = 0; + app.status_message = Some(parent_stop_status(app, "Paused command cancelled")); + false + } else { + let was_waiting = app.goal_continuation_waiting; + engine_handle.cancel(); + if was_waiting { + app.goal_continuation_waiting = false; + app.status_message = Some(app.tr(MessageId::GoalContinuationStopped).to_string()); + return true; + } + mark_active_turn_cancelled_locally(app); + current_streaming_text.clear(); + stream_display_clock.reset(); + app.status_message = Some(parent_stop_status(app, "Request cancelled")); + false + } +} + fn suppress_engine_event_after_local_cancel(event: &EngineEvent) -> bool { matches!( event, diff --git a/crates/tui/src/tui/ui/apply.rs b/crates/tui/src/tui/ui/apply.rs index 96557913b6..343153b78f 100644 --- a/crates/tui/src/tui/ui/apply.rs +++ b/crates/tui/src/tui/ui/apply.rs @@ -1505,6 +1505,63 @@ pub(crate) async fn apply_command_result( app.status_message = Some(format!("Could not cancel {agent_id}")); } } + AppAction::FetchBalance => { + let provider = app.api_provider; + if !crate::config::provider_has_balance_api(provider) { + app.add_message(HistoryCell::System { + content: format!( + "Balance check is not supported for {} yet. Check the provider dashboard for account balance details.", + provider.display_name() + ), + }); + } else { + let api_key = config.deepseek_api_key().unwrap_or_default(); + if api_key.trim().is_empty() { + app.add_message(HistoryCell::System { + content: format!( + "No API key configured for {}.", + provider.display_name() + ), + }); + } else { + let base_url = config.deepseek_base_url(); + match fetch_provider_balance(provider, &api_key, &base_url).await { + Some(info) => { + if let Ok(mut guard) = app.balance_cell.lock() { + *guard = Some(info.clone()); + } + app.last_balance_fetch = Some(Instant::now()); + app.add_message(HistoryCell::System { + content: info.report(provider.display_name()), + }); + } + None => { + let fallback = app + .balance_cell + .lock() + .ok() + .and_then(|guard| guard.clone()) + .and_then(|info| { + info.chip_label().map(|amount| { + format!( + "Could not refresh {} balance; last known: {amount}", + provider.display_name() + ) + }) + }); + app.add_message(HistoryCell::System { + content: fallback.unwrap_or_else(|| { + format!( + "Could not fetch {} account balance. Check the provider dashboard.", + provider.display_name() + ) + }), + }); + } + } + } + } + } AppAction::FetchModels => { app.status_message = Some("Fetching models...".to_string()); match fetch_available_models(config).await { @@ -1603,30 +1660,9 @@ pub(crate) async fn apply_command_result( } AppAction::SwitchProvider { provider, model } => { switch_provider(app, engine_handle, config, provider, model).await; - // Refresh balance after provider switch. - let balance_cooldown_expired = app - .last_balance_fetch - .is_none_or(|t| t.elapsed() >= BALANCE_FETCH_COOLDOWN); - if balance_cooldown_expired && should_fetch_deepseek_balance(app) { - let cell = app.balance_cell.clone(); - let api_key = config.deepseek_api_key().unwrap_or_default(); - let base_url = config.deepseek_base_url(); - if !api_key.is_empty() { - app.last_balance_fetch = Some(Instant::now()); - tokio::spawn(async move { - if let Some(info) = fetch_deepseek_balance(&api_key, &base_url).await - && let Ok(mut guard) = cell.lock() - { - *guard = Some(info); - } - }); - } - } else { - // Clear balance when switching to a non-DeepSeek provider. - if let Ok(mut guard) = app.balance_cell.lock() { - *guard = None; - } - } + let api_key = config.deepseek_api_key().unwrap_or_default(); + let base_url = config.deepseek_base_url(); + schedule_balance_fetch(app, &api_key, &base_url, false); } AppAction::SwitchModelRoute { provider, model } => { let previous_model = if app.auto_model { diff --git a/crates/tui/src/tui/ui/event_loop.rs b/crates/tui/src/tui/ui/event_loop.rs index 7f400fe3ca..e843f765f3 100644 --- a/crates/tui/src/tui/ui/event_loop.rs +++ b/crates/tui/src/tui/ui/event_loop.rs @@ -17,6 +17,8 @@ use super::*; use crate::models::Role; use crate::tui::shell_key_routing::ShellBindingId; +use crate::tui::control_socket::SessionControl; + pub(super) fn event_owner_is_active( current_session_id: Option<&str>, owner_session_id: &str, @@ -1160,6 +1162,15 @@ pub(crate) async fn run_event_loop( // Widgets request future animation frames here; the poll loop remains the // sole `terminal.draw` emitter (no competing animation loop). let mut frame_requester = FrameRequester::new(); + // Per-session control socket (`[control_socket]`): disabled unless the + // config enables it; even then, nothing binds until the owned session id + // appears (see the per-iteration reconcile below). + let mut session_control = SessionControl::new( + config + .control_socket + .as_ref() + .is_some_and(|socket| socket.enabled), + ); let mut web_config_session: Option = None; let mut prev_input_snapshot = String::new(); let mut terminal_paused_at: Option = None; @@ -1212,23 +1223,13 @@ pub(crate) async fn run_event_loop( } } - // Fire a one-shot initial balance fetch for DeepSeek providers - // so the footer chip shows balance on the first frame without + // Fire a one-shot initial remaining-credit fetch for prepaid + // providers so the footer chip can show on the first frame without // waiting for a turn to complete. - if !app.balance_initiated && should_fetch_deepseek_balance(app) { - let cell = app.balance_cell.clone(); + if !app.balance_initiated { let api_key = config.deepseek_api_key().unwrap_or_default(); let base_url = config.deepseek_base_url(); - if !api_key.is_empty() { - app.last_balance_fetch = Some(Instant::now()); - tokio::spawn(async move { - if let Some(info) = fetch_deepseek_balance(&api_key, &base_url).await - && let Ok(mut guard) = cell.lock() - { - *guard = Some(info); - } - }); - } + schedule_balance_fetch(app, &api_key, &base_url, false); app.balance_initiated = true; } @@ -1257,6 +1258,25 @@ pub(crate) async fn run_event_loop( // block keyboard input or silently drop the accepted control. flush_pending_goal_controls(app, &engine_handle); + // Per-session control socket: rebind when the owned session id + // changes, republish the `status` snapshot, and execute queued + // verbs on the UI thread. A verb that asks for quit (the `relaunch` + // seam) exits the loop through the ordinary `/exit` teardown. + session_control.reconcile(app.current_session_id.as_deref()); + session_control.update_status(app); + if session_control + .drain( + app, + config, + &engine_handle, + &mut current_streaming_text, + &mut stream_display_clock, + ) + .await + { + return Ok(()); + } + while let Some(completion) = app.clipboard.poll_write_completion() { if let Err(err) = completion { tracing::warn!(error = %err, "background terminal clipboard write failed"); @@ -2419,28 +2439,12 @@ pub(crate) async fn run_event_loop( // could not be built or queued, the in-flight // checkpoint survives for startup recovery review. - // Refresh DeepSeek account balance after each completed + // Refresh prepaid remaining credit after each completed // turn so the footer balance chip stays current without // adding latency to any request path. - let balance_cooldown_expired = app - .last_balance_fetch - .is_none_or(|t| t.elapsed() >= BALANCE_FETCH_COOLDOWN); - if balance_cooldown_expired && should_fetch_deepseek_balance(app) { - let cell = app.balance_cell.clone(); - let api_key = config.deepseek_api_key().unwrap_or_default(); - let base_url = config.deepseek_base_url(); - if !api_key.is_empty() { - app.last_balance_fetch = Some(Instant::now()); - tokio::spawn(async move { - if let Some(info) = - fetch_deepseek_balance(&api_key, &base_url).await - && let Ok(mut guard) = cell.lock() - { - *guard = Some(info); - } - }); - } - } + let api_key = config.deepseek_api_key().unwrap_or_default(); + let base_url = config.deepseek_base_url(); + schedule_balance_fetch(app, &api_key, &base_url, false); // Legacy pending-steer recovery. Current keyboard // handling keeps Esc as cancel-only, but older saved @@ -5522,45 +5526,14 @@ pub(crate) async fn run_event_loop( } EscapeAction::CancelRequest => { app.backtrack.reset(); - if try_cancel_compaction(app, &engine_handle) { + if escape_cancel_request( + app, + &engine_handle, + &mut current_streaming_text, + &mut stream_display_clock, + ) { continue; } - if app.paused || app.paused_goal_objective.is_some() { - clear_paused_command_state(app, &engine_handle); - if app.is_loading - || matches!( - app.runtime_turn_status.as_deref(), - Some("in_progress") - ) - { - engine_handle.cancel(); - mark_active_turn_cancelled_locally(app); - current_streaming_text.clear(); - stream_display_clock.reset(); - } - app.active_allowed_tools = None; - app.goal.objective = None; - app.goal.tokens_used = 0; - app.goal.time_used_seconds = 0; - app.goal.continuation_count = 0; - app.status_message = - Some(parent_stop_status(app, "Paused command cancelled")); - } else { - let was_waiting = app.goal_continuation_waiting; - engine_handle.cancel(); - if was_waiting { - app.goal_continuation_waiting = false; - app.status_message = Some( - app.tr(MessageId::GoalContinuationStopped).to_string(), - ); - continue; - } - mark_active_turn_cancelled_locally(app); - current_streaming_text.clear(); - stream_display_clock.reset(); - app.status_message = - Some(parent_stop_status(app, "Request cancelled")); - } } EscapeAction::PauseCommand => { app.backtrack.reset(); diff --git a/crates/tui/src/tui/ui/provider_routes.rs b/crates/tui/src/tui/ui/provider_routes.rs index fa364ddabd..4a67c9f15f 100644 --- a/crates/tui/src/tui/ui/provider_routes.rs +++ b/crates/tui/src/tui/ui/provider_routes.rs @@ -157,15 +157,131 @@ pub(crate) fn complete_provider_picker_onboarding_if_switched( } } -/// Fetch the DeepSeek account balance from the balance API. +/// How one prepaid provider publishes remaining credit. Each variant is a +/// distinct wire contract — do not send DeepSeek `/user/balance` to a +/// provider that does not speak it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum BalanceApi { + DeepSeekUserBalance, + OpenRouterCredits, + SiliconFlowUserInfo, +} + +fn balance_api_for(provider: ApiProvider) -> Option { + match provider { + ApiProvider::Deepseek | ApiProvider::DeepseekCN => Some(BalanceApi::DeepSeekUserBalance), + ApiProvider::Openrouter => Some(BalanceApi::OpenRouterCredits), + ApiProvider::Siliconflow | ApiProvider::SiliconflowCn => { + Some(BalanceApi::SiliconFlowUserInfo) + } + _ => None, + } +} + +/// Fetch remaining credit for the active prepaid provider. /// -/// Returns `None` on any error (network, auth, parse) — callers should treat -/// a `None` return as "balance unknown" and keep the previous value. -pub(crate) async fn fetch_deepseek_balance( +/// Returns `None` on any error (network, auth, parse) — callers treat that +/// as "balance unknown" and keep the previous value. +pub(crate) async fn fetch_provider_balance( + provider: ApiProvider, + api_key: &str, + base_url: &str, +) -> Option { + let api_key = api_key.trim(); + if api_key.is_empty() { + return None; + } + match balance_api_for(provider)? { + BalanceApi::DeepSeekUserBalance => fetch_deepseek_user_balance(api_key, base_url).await, + BalanceApi::OpenRouterCredits => fetch_openrouter_credits(api_key, base_url).await, + BalanceApi::SiliconFlowUserInfo => { + fetch_siliconflow_user_info(api_key, base_url, provider).await + } + } +} + +async fn fetch_deepseek_user_balance( api_key: &str, base_url: &str, ) -> Option { let url = format!("{}/user/balance", base_url.trim_end_matches('/')); + let body: crate::pricing::BalanceResponse = balance_get_json(api_key, &url).await?; + body.balance_infos.into_iter().next() +} + +#[derive(serde::Deserialize)] +struct OpenRouterCreditsResponse { + data: OpenRouterCreditsData, +} + +#[derive(serde::Deserialize)] +struct OpenRouterCreditsData { + total_credits: f64, + total_usage: f64, +} + +fn openrouter_remaining_credits(total_credits: f64, total_usage: f64) -> f64 { + (total_credits - total_usage).max(0.0) +} + +async fn fetch_openrouter_credits( + api_key: &str, + base_url: &str, +) -> Option { + let url = format!("{}/credits", base_url.trim_end_matches('/')); + let body: OpenRouterCreditsResponse = balance_get_json(api_key, &url).await?; + let remaining = openrouter_remaining_credits(body.data.total_credits, body.data.total_usage); + Some(crate::pricing::BalanceInfo { + currency: "USD".to_string(), + total_balance: format!("{remaining:.2}"), + topped_up_balance: format!("{:.2}", body.data.total_credits), + granted_balance: String::new(), + }) +} + +#[derive(serde::Deserialize)] +struct SiliconFlowUserInfo { + data: Option, +} + +#[derive(serde::Deserialize)] +struct SiliconFlowUserData { + #[serde(default, alias = "totalBalance")] + total_balance: Option, + #[serde(default, alias = "chargeBalance")] + charge_balance: Option, + #[serde(default)] + balance: Option, +} + +async fn fetch_siliconflow_user_info( + api_key: &str, + base_url: &str, + provider: ApiProvider, +) -> Option { + let url = format!("{}/user/info", base_url.trim_end_matches('/')); + let body: SiliconFlowUserInfo = balance_get_json(api_key, &url).await?; + let data = body.data?; + let total = data + .total_balance + .as_deref() + .or(data.balance.as_deref()) + .map(str::trim) + .filter(|value| !value.is_empty())?; + let currency = if provider == ApiProvider::SiliconflowCn { + "CNY" + } else { + "USD" + }; + Some(crate::pricing::BalanceInfo { + currency: currency.to_string(), + total_balance: total.to_string(), + topped_up_balance: data.charge_balance.unwrap_or_default(), + granted_balance: String::new(), + }) +} + +async fn balance_get_json(api_key: &str, url: &str) -> Option { let client = &*BALANCE_CLIENT; let response = client .get(url) @@ -181,17 +297,62 @@ pub(crate) async fn fetch_deepseek_balance( ); return None; } - let body: crate::pricing::BalanceResponse = response.json().await.ok()?; - // Return the first balance entry (typically the user's primary currency). - body.balance_infos.into_iter().next() + response.json().await.ok() } -pub(crate) fn should_fetch_deepseek_balance(app: &App) -> bool { +pub(crate) fn should_fetch_provider_balance(app: &App) -> bool { app.status_items.contains(&StatusItem::Balance) - && matches!( - app.api_provider, - ApiProvider::Deepseek | ApiProvider::DeepseekCN - ) + && crate::config::provider_has_balance_api(app.api_provider) +} + +/// Kick a background remaining-credit fetch for the live route. +/// +/// `force` skips the status-item gate (used by `/balance`). Providers without +/// a known endpoint clear the parked chip so a previous route cannot linger. +pub(crate) fn schedule_balance_fetch(app: &mut App, api_key: &str, base_url: &str, force: bool) { + if !crate::config::provider_has_balance_api(app.api_provider) { + if let Ok(mut guard) = app.balance_cell.lock() { + *guard = None; + } + return; + } + if !force && !should_fetch_provider_balance(app) { + return; + } + if api_key.trim().is_empty() { + return; + } + let cooldown_ok = force + || app + .last_balance_fetch + .is_none_or(|t| t.elapsed() >= BALANCE_FETCH_COOLDOWN); + if !cooldown_ok { + return; + } + app.last_balance_fetch = Some(Instant::now()); + let cell = app.balance_cell.clone(); + let provider = app.api_provider; + let api_key = api_key.to_string(); + let base_url = base_url.to_string(); + tokio::spawn(async move { + if let Some(info) = fetch_provider_balance(provider, &api_key, &base_url).await + && let Ok(mut guard) = cell.lock() + { + *guard = Some(info); + } + }); +} + +#[cfg(test)] +pub(crate) fn openrouter_credits_from_json(json: &str) -> Option { + let body: OpenRouterCreditsResponse = serde_json::from_str(json).ok()?; + let remaining = openrouter_remaining_credits(body.data.total_credits, body.data.total_usage); + Some(crate::pricing::BalanceInfo { + currency: "USD".to_string(), + total_balance: format!("{remaining:.2}"), + topped_up_balance: format!("{:.2}", body.data.total_credits), + granted_balance: String::new(), + }) } /// Route text from either clipboard transport into the canonical provider diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 7791aefc71..23da8e8cd3 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -21574,7 +21574,7 @@ fn default_footer_excludes_provider_specific_diagnostic_chips() { ); assert!( !items.contains(&crate::config::StatusItem::Balance), - "balance is DeepSeek-only and should not crowd the default footer for non-DeepSeek users" + "balance is an opt-in prepaid chip and should not crowd the default footer" ); assert!( items.contains(&crate::config::StatusItem::Cache), @@ -21589,27 +21589,43 @@ fn default_footer_excludes_provider_specific_diagnostic_chips() { // ── Balance footer chip tests ───────────────────────────────────── #[test] -fn should_fetch_deepseek_balance_requires_balance_status_item() { +fn should_fetch_provider_balance_requires_balance_status_item() { let mut app = create_test_app(); app.api_provider = ApiProvider::Deepseek; app.status_items = crate::config::StatusItem::default_footer(); - assert!(!should_fetch_deepseek_balance(&app)); + assert!(!should_fetch_provider_balance(&app)); app.status_items.push(crate::config::StatusItem::Balance); - assert!(should_fetch_deepseek_balance(&app)); + assert!(should_fetch_provider_balance(&app)); } #[test] -fn should_fetch_deepseek_balance_requires_deepseek_provider() { +fn should_fetch_provider_balance_covers_prepaid_providers() { let mut app = create_test_app(); app.status_items = vec![crate::config::StatusItem::Balance]; + app.api_provider = ApiProvider::Ollama; + assert!(!should_fetch_provider_balance(&app)); + app.api_provider = ApiProvider::Openrouter; - assert!(!should_fetch_deepseek_balance(&app)); + assert!(should_fetch_provider_balance(&app)); + + app.api_provider = ApiProvider::Siliconflow; + assert!(should_fetch_provider_balance(&app)); app.api_provider = ApiProvider::DeepseekCN; - assert!(should_fetch_deepseek_balance(&app)); + assert!(should_fetch_provider_balance(&app)); +} + +#[test] +fn openrouter_credits_map_remaining_usd() { + let info = + openrouter_credits_from_json(r#"{"data":{"total_credits":50.0,"total_usage":12.345}}"#) + .expect("openrouter credits JSON"); + assert_eq!(info.currency, "USD"); + assert_eq!(info.total_balance, "37.66"); + assert_eq!(info.chip_label().as_deref(), Some("$37.66")); } /// Regression for issue #244: visible session spend must not decrease. diff --git a/crates/tui/src/tui/underwater.rs b/crates/tui/src/tui/underwater.rs index a7df2d4ed0..0d94e241db 100644 --- a/crates/tui/src/tui/underwater.rs +++ b/crates/tui/src/tui/underwater.rs @@ -1137,6 +1137,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/crates/tui/src/tui/views/status_picker.rs b/crates/tui/src/tui/views/status_picker.rs index b6314aff27..f004f8b7c6 100644 --- a/crates/tui/src/tui/views/status_picker.rs +++ b/crates/tui/src/tui/views/status_picker.rs @@ -406,11 +406,13 @@ mod tests { } #[test] - fn balance_excluded_for_non_deepseek_provider() { + fn balance_offered_for_prepaid_providers_and_hidden_for_local() { let active = StatusItem::default_footer(); - let view = StatusPickerView::new(&active, ApiProvider::Openrouter, Locale::En); - assert!(!view.rows.contains(&StatusItem::Balance)); - assert!(view.rows.contains(&StatusItem::Mode)); + let openrouter = StatusPickerView::new(&active, ApiProvider::Openrouter, Locale::En); + assert!(openrouter.rows.contains(&StatusItem::Balance)); + let ollama = StatusPickerView::new(&active, ApiProvider::Ollama, Locale::En); + assert!(!ollama.rows.contains(&StatusItem::Balance)); + assert!(ollama.rows.contains(&StatusItem::Mode)); } #[test] diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 55ca21ddf0..9652c7c063 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -2541,6 +2541,49 @@ Delivery is best-effort: failures are logged and dropped, never retried into the agent loop, and a failing webhook never blocks the local file append. +## Control Socket (`[control_socket]`) + +Per-session control surface for supervised operation: with the feature +enabled, the interactive TUI binds one unix domain socket per *running* +session at `//control.sock` (mode `0600`; +`` is the same directory the session store uses, typically +`~/.codewhale/sessions`). The socket is removed with the session's +artifact directory, and a stale socket left by a crashed process is taken +over by the next launch. Unset or `enabled = false` = the feature is +**off** (the default) and behavior is unchanged. Unix-only; on other +platforms the key parses but no socket is bound. + +```toml +[control_socket] +enabled = false # default: OFF +``` + +The socket speaks newline-framed JSON-RPC, one request per connection: +write one request line, read one response line, close. + +```json +{"id":"1","method":"message","params":{"text":"hello"}} +{"id":"2","method":"interrupt","params":{}} +{"id":"3","method":"relaunch","params":{}} +{"id":"4","method":"status","params":{}} +``` + +- `message` — delivers `text` as a structured user message through the + ordinary composer dispatch path; dispatched immediately when idle, + queued when a turn is in flight (the response's `delivery` field says + which). +- `interrupt` — the Esc-shaped cancel of the active turn; `cancelled` + reports whether active work was in flight. +- `relaunch` — routed through the `/relaunch` slash-command path (same + save-and-resume handoff, no separate mechanics). +- `status` — answers `turn_state` (`idle` / `in_progress` / `waiting`) and + `goal` (`objective`, `status`, `paused`). + +Success responses echo the request id with a `type`-tagged result; +failures carry `error.code` (`invalid_request`, `command_error`, +`timeout`, `server_unavailable`). Requests are bounded at 1 MiB per line +and a handler that does not answer within 5 s is reported as `timeout`. + ## Tool Catalog Codewhale loads a small core native tool catalog by default and leaves less 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 5c132120fc..a0a17f0b1b 100644 --- a/scripts/command-migration-topology.json +++ b/scripts/command-migration-topology.json @@ -7,7 +7,6 @@ "crates/tui/src/commands/groups/utility/attachment.rs", "crates/tui/src/commands/groups/utility/automation.rs", "crates/tui/src/commands/groups/utility/jobs.rs", - "crates/tui/src/commands/groups/utility/loop_cmd.rs", "crates/tui/src/commands/groups/utility/mcp.rs", "crates/tui/src/commands/groups/utility/network.rs", "crates/tui/src/commands/groups/utility/task.rs", @@ -284,7 +283,6 @@ "core", "debug", "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 54d40143ab..191147ef86 100644 --- a/scripts/test_check_command_migration_manifest.py +++ b/scripts/test_check_command_migration_manifest.py @@ -362,11 +362,11 @@ def test_topology_artifact_is_sorted_unique(self) -> None: frontier = doc["frontier"] self.assertEqual(frontier, sorted(frontier)) self.assertEqual(len(frontier), len(set(frontier))) - # FEAT-018 removed utility, FEAT-019 removed memory, and FEAT-021 removed project - # from the frontier; the remaining 6 groups stay pending. + # FEAT-018 removed utility, FEAT-019 removed memory, FEAT-021 removed project, + # and FEAT-022 removed skills; the remaining five groups stay pending. self.assertEqual( set(frontier), - {"plugins", "skills", "session", "config", "debug", "core"}, + {"plugins", "session", "config", "debug", "core"}, ) @@ -513,6 +513,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"])