From 65f071921f6a84647f0a54d29f8db16f93ae4f81 Mon Sep 17 00:00:00 2001 From: bnavetta Date: Tue, 19 May 2026 13:09:10 +0000 Subject: [PATCH 1/2] Make agent run skill flag repeatable Co-Authored-By: Oz --- app/src/ai/agent_sdk/driver.rs | 48 ++++- app/src/ai/agent_sdk/mod.rs | 182 +++++++++++++----- app/src/ai/skills/resolve_skill_spec.rs | 33 +++- app/src/ai/skills/skill_manager_tests.rs | 58 ++++++ crates/warp_cli/src/agent.rs | 10 +- crates/warp_cli/src/lib_tests.rs | 50 ++++- .../repeatable-oz-agent-run-skill/PRODUCT.md | 45 +++++ specs/repeatable-oz-agent-run-skill/TECH.md | 56 ++++++ 8 files changed, 411 insertions(+), 71 deletions(-) create mode 100644 specs/repeatable-oz-agent-run-skill/PRODUCT.md create mode 100644 specs/repeatable-oz-agent-run-skill/TECH.md diff --git a/app/src/ai/agent_sdk/driver.rs b/app/src/ai/agent_sdk/driver.rs index ba2ccadc0f..fb5fe55a0e 100644 --- a/app/src/ai/agent_sdk/driver.rs +++ b/app/src/ai/agent_sdk/driver.rs @@ -47,7 +47,7 @@ use crate::ai::skills::{ SkillWatcher, }; use crate::ai::{ - agent::conversation::AIConversationId, + agent::{conversation::AIConversationId, InvokeSkillUserQuery}, agent_sdk::driver::harness::{ harness_model_env_vars, task_env_vars, HarnessCleanupDisposition, HarnessKind, HarnessRunner, ResumePayload, SavePoint, ThirdPartyHarness, @@ -375,6 +375,11 @@ struct GlobalSkillResolution { pub enum AgentRunPrompt { /// Prompt is provided locally (already resolved to a plain string). Local(String), + /// A local Oz run that invokes a skill using the skill-specific API input. + LocalSkill { + skill: ParsedSkill, + user_query: Option, + }, /// Server resolves prompt from the task's stored prompt. /// Used when task_id is provided without an explicit prompt. ServerSide { @@ -1780,6 +1785,19 @@ impl AgentDriver { } = global_skill_resolution; Self::load_environment_skills(&foreground, environment_skill_repos).await; Self::load_global_skills(&foreground, global_skill_specs, global_skill_repos).await; + foreground + .spawn(|_, ctx| { + // In all CLI runs, we should include all loaded skills from any source: + // - The `--skill` flag + // - Global skills + // - Skills from cloned repos + // Unlike in the desktop app, there aren't multiple terminal sessions with + // different skill sets that we need to filter out. + SkillManager::handle(ctx).update(ctx, |manager, _| { + manager.set_cloud_environment(true); + }); + }) + .await?; } let (task_id_for_refresh, ai_client_for_refresh) = foreground @@ -1932,6 +1950,13 @@ impl AgentDriver { Option, ) = match prompt { AgentRunPrompt::Local(text) => (Cow::Borrowed(text), None, None, None), + AgentRunPrompt::LocalSkill { skill, user_query } => { + let prompt = match user_query { + Some(user_query) => format!("{}\n\n{}", skill.content, user_query.query), + None => skill.content.clone(), + }; + (Cow::Owned(prompt), None, None, None) + } AgentRunPrompt::ServerSide { skill, attachments_dir, @@ -2472,6 +2497,27 @@ impl AgentDriver { .update(ctx, |input, ctx| input.input_enter(ctx)); } } + AgentRunPrompt::LocalSkill { skill, user_query } => { + if FeatureFlag::AgentView.is_enabled() { + terminal.enter_agent_view( + None, + restored_conversation_id, + AgentViewEntryOrigin::Cli, + ctx, + ); + } + + terminal.ai_controller().update(ctx, |controller, ctx| { + controller.send_ai_input_with_context( + |context| AIAgentInput::InvokeSkill { + context, + skill: skill.clone(), + user_query: user_query.clone(), + }, + ctx, + ); + }); + } AgentRunPrompt::ServerSide { skill, attachments_dir, diff --git a/app/src/ai/agent_sdk/mod.rs b/app/src/ai/agent_sdk/mod.rs index 9102c05e4c..cb007f8f02 100644 --- a/app/src/ai/agent_sdk/mod.rs +++ b/app/src/ai/agent_sdk/mod.rs @@ -11,7 +11,7 @@ use crate::ai::agent::api::convert_conversation::{ convert_conversation_data_to_ai_conversation, RestorationMode, }; use crate::ai::agent::api::ServerConversationToken; -use crate::ai::agent::conversation::AIConversationId; +use crate::ai::agent::{conversation::AIConversationId, InvokeSkillUserQuery}; use crate::ai::agent_sdk::driver::harness::{harness_kind, HarnessKind}; use crate::ai::agent_sdk::driver::{AgentDriverOptions, AgentRunPrompt, Task}; use crate::ai::agent_sdk::mcp_config::build_mcp_servers_from_specs; @@ -24,6 +24,7 @@ use crate::server::server_api::ai::AIClient; use crate::workflows::workflow::Workflow; use ai::api_keys::{ApiKeyManager, AwsCredentialsRefreshStrategy}; use anyhow::Context; +use futures::future::try_join_all; use warp_cli::{ agent::{AgentCommand, AgentProfileCommand, OutputFormat}, artifact::ArtifactCommand, @@ -40,7 +41,7 @@ use warp_cli::{ task::{MessageCommand, TaskCommand}, CliCommand, GlobalOptions, }; -use warp_core::features::FeatureFlag; +use warp_core::{features::FeatureFlag, safe_info}; use warp_isolation_platform::IsolationPlatformError; #[cfg(not(target_family = "wasm"))] use warp_logging::log_file_path; @@ -64,7 +65,7 @@ use warp_graphql::object_permissions::OwnerType; use crate::ai::attachment_utils::attachments_download_dir; use crate::ai::skills::{ - clone_repo_for_skill, resolve_skill_spec, ResolveSkillError, ResolvedSkill, + clone_repo_for_skill, resolve_skill_spec, ResolveSkillError, ResolvedSkill, SkillManager, }; pub(crate) use driver::harness::{task_env_vars, validate_cli_installed, ClaudeHarness}; @@ -248,7 +249,7 @@ fn run_agent( "unexpected argument '--conversation' found" )); } - if args.skill.is_some() && !FeatureFlag::OzPlatformSkills.is_enabled() { + if !args.skill.is_empty() && !FeatureFlag::OzPlatformSkills.is_enabled() { return Err(anyhow::anyhow!("unexpected argument '--skill' found")); } if args.harness != Harness::Oz && !FeatureFlag::AgentHarness.is_enabled() { @@ -393,23 +394,53 @@ fn build_merged_config_and_task( // Keep the task config snapshot aligned with the effective model selection. merged_config.model_id = model_override.clone().map(|id| id.to_string()); - // Combine base_prompt with user prompt locally. - let local_prompt = match (merged_config.base_prompt.as_deref(), prompt) { - (Some(base_prompt), Some(Prompt::PlainText(user_prompt))) => { - Prompt::PlainText(format!("{base_prompt}\n\n{user_prompt}")) - } - (Some(base_prompt), None) => { - // Skill-only invocation: use skill instructions as the prompt - Prompt::PlainText(base_prompt.to_string()) - } - (_, Some(p)) => p.clone(), - (None, None) => { - return Err(anyhow::anyhow!(AgentDriverError::InvalidRuntimeState)); + let prompt = if args.harness == Harness::Oz { + if let Some(skill) = resolved_skill { + let user_query = prompt + .as_ref() + .map(|prompt| { + resolve_prompt(prompt, ctx).map(|query| InvokeSkillUserQuery { + query, + referenced_attachments: Default::default(), + }) + }) + .transpose()?; + + AgentRunPrompt::LocalSkill { + skill: skill.parsed_skill.clone(), + user_query, + } + } else { + let local_prompt = match (merged_config.base_prompt.as_deref(), prompt) { + (Some(base_prompt), Some(Prompt::PlainText(user_prompt))) => { + Prompt::PlainText(format!("{base_prompt}\n\n{user_prompt}")) + } + (Some(base_prompt), None) => Prompt::PlainText(base_prompt.to_string()), + (_, Some(p)) => p.clone(), + (None, None) => { + return Err(anyhow::anyhow!(AgentDriverError::InvalidRuntimeState)); + } + }; + AgentRunPrompt::Local(resolve_prompt(&local_prompt, ctx)?) } + } else { + // Third-party harnesses do not use Warp's InvokeSkill input type, so keep passing + // skill instructions as prompt text for harness-native execution. + let local_prompt = match (merged_config.base_prompt.as_deref(), prompt) { + (Some(base_prompt), Some(Prompt::PlainText(user_prompt))) => { + Prompt::PlainText(format!("{base_prompt}\n\n{user_prompt}")) + } + (Some(base_prompt), None) => Prompt::PlainText(base_prompt.to_string()), + (_, Some(p)) => p.clone(), + (None, None) => { + return Err(anyhow::anyhow!(AgentDriverError::InvalidRuntimeState)); + } + }; + AgentRunPrompt::Local(resolve_prompt(&local_prompt, ctx)?) }; let task = Task { - prompt: AgentRunPrompt::Local(resolve_prompt(&local_prompt, ctx)?), + prompt, model: model_override, profile: args.profile.clone(), mcp_specs: runtime_mcp_specs, @@ -744,49 +775,95 @@ impl AgentDriverRunner { .map_err(|_| AgentDriverError::TeamMetadataRefreshTimeout) } - /// Resolve the skill spec from args, if one was provided. + /// Resolve the skill specs from args, if any were provided. /// - /// In sandboxed mode with a fully-qualified spec (org + repo), the repo is - /// cloned first since it may not exist locally. Otherwise we resolve directly - /// against the local filesystem. - async fn resolve_skill( + /// In sandboxed mode with fully-qualified specs (org + repo), the + /// repos are cloned first since they may not exist locally. Otherwise we + /// resolve directly against the local filesystem. Every resolved skill is + /// also registered with `SkillManager` so the agent can read it later. + async fn resolve_skills( foreground: &ModelSpawner, args: &RunAgentArgs, working_dir: &Path, - ) -> Result, AgentDriverError> { + ) -> Result, AgentDriverError> { if !FeatureFlag::OzPlatformSkills.is_enabled() { - return Ok(None); + return Ok(Vec::new()); + } + if args.skill.is_empty() { + return Ok(Vec::new()); + } + let skill_specs = args.skill.clone(); + let mut repos_to_clone = Vec::new(); + if args.sandboxed { + for skill_spec in &skill_specs { + let (Some(org), Some(repo_name)) = (&skill_spec.org, &skill_spec.repo) else { + continue; + }; + repos_to_clone.push((org.clone(), repo_name.clone())); + } + repos_to_clone.sort(); + repos_to_clone.dedup(); } - let Some(skill_spec) = args.skill.clone() else { - return Ok(None); - }; - // In sandboxed mode with a fully-qualified spec, clone the repo first. - let needs_clone = args.sandboxed && skill_spec.org.is_some() && skill_spec.repo.is_some(); - if needs_clone { - let org = skill_spec.org.as_ref().expect("org checked above"); - let repo_name = skill_spec.repo.as_ref().expect("repo checked above"); - log::info!("Cloning {org}/{repo_name} for skill resolution in sandboxed mode"); - clone_repo_for_skill(org, repo_name, working_dir) - .await + if !repos_to_clone.is_empty() { + let repos_for_log = repos_to_clone + .iter() + .map(|(org, repo_name)| format!("{org}/{repo_name}")) + .collect::>() + .join(", "); + safe_info!( + safe: ( + "Cloning {} repo(s) for skill resolution in sandboxed mode", + repos_to_clone.len() + ), + full: ("Cloning repos for skill resolution in sandboxed mode: {repos_for_log}") + ); + + let clone_futures = repos_to_clone + .into_iter() + .map(|(org, repo_name)| async move { + clone_repo_for_skill(&org, &repo_name, working_dir) + .await + .map_err(|err| { + AgentDriverError::SkillResolutionFailed(format_skill_resolution_error( + err, + )) + }) + }); + try_join_all(clone_futures).await?; + } + + let mut resolved_skills = Vec::with_capacity(skill_specs.len()); + for skill_spec in skill_specs { + let working_dir_buf = working_dir.to_path_buf(); + let skill = foreground + .spawn(move |_, ctx| resolve_skill_spec(&skill_spec, &working_dir_buf, ctx)) + .await? .map_err(|err| { AgentDriverError::SkillResolutionFailed(format_skill_resolution_error(err)) })?; + log::debug!( + "Resolved skill '{}' from {}", + skill.name, + skill.skill_path.display() + ); + resolved_skills.push(skill); + } + if !resolved_skills.is_empty() { + let skills = resolved_skills + .iter() + .map(|skill| skill.parsed_skill.clone()) + .collect::>(); + foreground + .spawn(move |_, ctx| { + SkillManager::handle(ctx).update(ctx, |manager, _| { + manager.handle_skills_added(skills); + }); + }) + .await?; } - let working_dir_buf = working_dir.to_path_buf(); - let skill = foreground - .spawn(move |_, ctx| resolve_skill_spec(&skill_spec, &working_dir_buf, ctx)) - .await? - .map_err(|err| { - AgentDriverError::SkillResolutionFailed(format_skill_resolution_error(err)) - })?; - log::debug!( - "Resolved skill '{}' from {}", - skill.name, - skill.skill_path.display() - ); - Ok(Some(skill)) + Ok(resolved_skills) } /// Build the AgentDriverOptions and Task, handling task creation or existing task setup. @@ -808,20 +885,21 @@ impl AgentDriverRunner { } .map_err(AgentDriverError::ConfigBuildFailed)?; - // Resolve the skill, if we have one - let resolved_skill = Self::resolve_skill(foreground, &args, &working_dir).await?; + // Resolve and register any explicitly requested skills. Only the first is invoked. + let resolved_skills = Self::resolve_skills(foreground, &args, &working_dir).await?; + let invoked_skill = resolved_skills.first().cloned(); // Extract variables we want to use later before moving args into the closure let task_id_str = args.task_id.clone(); let prompt = args.prompt_arg.to_prompt(); - let skill = args.skill.clone(); + let skill = args.invoked_skill().cloned(); // Build the AgentConfigSnapshot, Task, and AgentDriverOptions let prompt_clone = prompt.clone(); let (merged_config, mut task, mut driver_options) = foreground .spawn(move |_, ctx| -> anyhow::Result<_> { let (merged_config, task) = - build_merged_config_and_task(&args, &resolved_skill, &prompt_clone, ctx)?; + build_merged_config_and_task(&args, &invoked_skill, &prompt_clone, ctx)?; let task_id = args.task_id.as_ref().and_then(|s| s.parse().ok()); let should_share = (args.share.is_shared() || args.task_id.is_some()) diff --git a/app/src/ai/skills/resolve_skill_spec.rs b/app/src/ai/skills/resolve_skill_spec.rs index c4f3bf1bfb..9e1bddf27c 100644 --- a/app/src/ai/skills/resolve_skill_spec.rs +++ b/app/src/ai/skills/resolve_skill_spec.rs @@ -19,6 +19,7 @@ use ai::skills::{ use command::blocking::Command; use command::r#async::Command as AsyncCommand; use warp_cli::skill::SkillSpec; +use warp_core::{safe_debug, safe_info}; use warp_util::local_or_remote_path::LocalOrRemotePath; use warpui::AppContext; use warpui::SingletonEntity as _; @@ -154,9 +155,12 @@ pub async fn clone_repo_for_skill( // Check if target already exists. if target_dir.exists() { if target_dir.join(".git").is_dir() { - log::info!( - "Target directory {} already exists and appears to be a git repo, skipping clone", - target_dir.display() + safe_info!( + safe: ("Target repo for skill resolution already exists, skipping clone"), + full: ( + "Target directory {} already exists and appears to be a git repo, skipping clone", + target_dir.display() + ) ); return Ok(()); } @@ -171,11 +175,19 @@ pub async fn clone_repo_for_skill( }); } - log::info!("Cloning {} into {}", repo_url, target_dir.display()); - log::debug!( - "[GIT OPERATION] resolve_skill_spec.rs clone_repo_for_skill git clone {} {}", - repo_url, - target_dir.display() + safe_info!( + safe: ("Cloning repo for skill resolution"), + full: ("Cloning {} into {}", repo_url, target_dir.display()) + ); + safe_debug!( + safe: ( + "[GIT OPERATION] resolve_skill_spec.rs clone_repo_for_skill git clone " + ), + full: ( + "[GIT OPERATION] resolve_skill_spec.rs clone_repo_for_skill git clone {} {}", + repo_url, + target_dir.display() + ) ); let output = AsyncCommand::new("git") @@ -200,7 +212,10 @@ pub async fn clone_repo_for_skill( }); } - log::info!("Successfully cloned {org}/{repo}"); + safe_info!( + safe: ("Successfully cloned repo for skill resolution"), + full: ("Successfully cloned {org}/{repo}") + ); Ok(()) } diff --git a/app/src/ai/skills/skill_manager_tests.rs b/app/src/ai/skills/skill_manager_tests.rs index 5d3166bb03..30fc9d7582 100644 --- a/app/src/ai/skills/skill_manager_tests.rs +++ b/app/src/ai/skills/skill_manager_tests.rs @@ -4,11 +4,69 @@ use ai::skills::{ParsedSkill, SkillProvider, SkillScope}; use repo_metadata::{repositories::DetectedRepositories, DirectoryWatcher, RepoMetadataModel}; use std::collections::{HashMap, HashSet}; use std::fs; +use std::path::PathBuf; use tempfile::TempDir; use warp_core::channel::ChannelState; use warpui::App; use watcher::HomeDirectoryWatcher; +// ============================================================================ +// Tests for explicit skill registration +// ============================================================================ + +#[test] +fn handle_skills_added_registers_multiple_explicit_skills() { + let temp = TempDir::new().unwrap(); + let repo = dunce::canonicalize(temp.path()).unwrap().join("repo"); + fs::create_dir_all(&repo).unwrap(); + + let primary_path = repo.join(".agents/skills/primary/SKILL.md"); + let helper_path = repo.join(".agents/skills/helper/SKILL.md"); + let skills = vec![ + ParsedSkill { + name: "primary".to_string(), + description: "Primary skill".to_string(), + path: primary_path.clone(), + content: "# Primary".to_string(), + line_range: None, + provider: SkillProvider::Agents, + scope: SkillScope::Project, + }, + ParsedSkill { + name: "helper".to_string(), + description: "Helper skill".to_string(), + path: helper_path.clone(), + content: "# Helper".to_string(), + line_range: None, + provider: SkillProvider::Agents, + scope: SkillScope::Project, + }, + ]; + + App::test((), |mut app| async move { + app.add_singleton_model(DirectoryWatcher::new); + app.add_singleton_model(|_| DetectedRepositories::default()); + app.add_singleton_model(RepoMetadataModel::new); + app.add_singleton_model(HomeDirectoryWatcher::new_for_test); + app.add_singleton_model(WarpManagedPathsWatcher::new_for_testing); + let skill_manager_handle = app.add_singleton_model(SkillManager::new); + + skill_manager_handle.update(&mut app, |manager, _ctx| { + manager.handle_skills_added(skills); + }); + + let registered_paths = skill_manager_handle.read(&app, |manager, _ctx| { + ( + manager.skill_paths_by_name("primary"), + manager.skill_paths_by_name("helper"), + ) + }); + + assert_eq!(registered_paths.0, vec![primary_path]); + assert_eq!(registered_paths.1, vec![helper_path]); + }); +} + // ============================================================================ // Tests for get_skills_for_working_directory subdirectory scoping // ============================================================================ diff --git a/crates/warp_cli/src/agent.rs b/crates/warp_cli/src/agent.rs index 7b92711e15..f63a87db75 100644 --- a/crates/warp_cli/src/agent.rs +++ b/crates/warp_cli/src/agent.rs @@ -267,11 +267,12 @@ pub struct RunAgentArgs { /// If a repo is specified, searches only that repo. If org is also specified, /// validates the repo's git remote matches the expected org. /// - /// When used with --prompt, the skill provides the base context and the prompt is the task. + /// When used with --prompt, the first skill provides the base context and the prompt is the task. + /// Repeat this flag to make additional skills available to the agent. /// /// To automate a skill on a schedule, use `oz schedule create --skill `. #[arg(long = "skill", value_name = "SPEC")] - pub skill: Option, + pub skill: Vec, /// Name for this agent task. #[arg(long = "name", short = 'n')] @@ -357,6 +358,11 @@ impl RunAgentArgs { specs.extend(self.mcp_servers.iter().cloned().map(MCPSpec::Uuid)); specs } + + /// Return the first `--skill` value, which is the skill invoked for this run. + pub fn invoked_skill(&self) -> Option<&SkillSpec> { + self.skill.first() + } } #[derive(Debug, Clone, Args)] diff --git a/crates/warp_cli/src/lib_tests.rs b/crates/warp_cli/src/lib_tests.rs index 02420084e1..90498420f8 100644 --- a/crates/warp_cli/src/lib_tests.rs +++ b/crates/warp_cli/src/lib_tests.rs @@ -375,7 +375,7 @@ fn agent_run_accepts_prompt_only() { assert_eq!(run_args.prompt_arg.prompt.as_deref(), Some("hello")); assert!(run_args.prompt_arg.saved_prompt.is_none()); - assert!(run_args.skill.is_none()); + assert!(run_args.skill.is_empty()); assert!(run_args.task_id.is_none()); } @@ -392,7 +392,7 @@ fn agent_run_accepts_saved_prompt_only() { assert!(run_args.prompt_arg.prompt.is_none()); assert_eq!(run_args.prompt_arg.saved_prompt.as_deref(), Some("sp-123")); - assert!(run_args.skill.is_none()); + assert!(run_args.skill.is_empty()); assert!(run_args.task_id.is_none()); } @@ -408,10 +408,46 @@ fn agent_run_accepts_skill_only() { }; assert!(run_args.prompt_arg.prompt.is_none()); - assert!(run_args.skill.is_some()); + assert_eq!(run_args.skill.len(), 1); + assert_eq!( + run_args.invoked_skill().unwrap().skill_identifier, + "my-skill" + ); assert!(run_args.task_id.is_none()); } +#[test] +fn agent_run_accepts_multiple_skills() { + let args = Args::try_parse_from([ + "warp", + "agent", + "run", + "--skill", + "primary", + "--skill", + "repo:helper", + "--prompt", + "do stuff", + ]) + .unwrap(); + + let Some(Command::CommandLine(boxed_cmd)) = args.command else { + panic!("Expected `warp agent run` command"); + }; + let CliCommand::Agent(AgentCommand::Run(run_args)) = boxed_cmd.as_ref() else { + panic!("Expected `warp agent run` command"); + }; + + assert_eq!(run_args.skill.len(), 2); + assert_eq!(run_args.skill[0].skill_identifier, "primary"); + assert_eq!(run_args.skill[1].repo.as_deref(), Some("repo")); + assert_eq!(run_args.skill[1].skill_identifier, "helper"); + assert_eq!( + run_args.invoked_skill().unwrap().skill_identifier, + "primary" + ); +} + #[test] fn agent_run_accepts_task_id_only() { let args = Args::try_parse_from(["warp", "agent", "run", "--task-id", "tid-456"]).unwrap(); @@ -424,7 +460,7 @@ fn agent_run_accepts_task_id_only() { }; assert!(run_args.prompt_arg.prompt.is_none()); - assert!(run_args.skill.is_none()); + assert!(run_args.skill.is_empty()); assert_eq!(run_args.task_id.as_deref(), Some("tid-456")); } @@ -443,7 +479,7 @@ fn agent_run_accepts_prompt_and_skill() { }; assert_eq!(run_args.prompt_arg.prompt.as_deref(), Some("do stuff")); - assert!(run_args.skill.is_some()); + assert_eq!(run_args.skill.len(), 1); } #[test] @@ -467,7 +503,7 @@ fn agent_run_accepts_saved_prompt_and_skill() { }; assert_eq!(run_args.prompt_arg.saved_prompt.as_deref(), Some("sp-1")); - assert!(run_args.skill.is_some()); + assert_eq!(run_args.skill.len(), 1); } #[test] @@ -519,7 +555,7 @@ fn agent_run_accepts_skill_and_task_id() { }; assert!(run_args.prompt_arg.prompt.is_none()); - assert!(run_args.skill.is_some()); + assert_eq!(run_args.skill.len(), 1); assert_eq!(run_args.task_id.as_deref(), Some("tid-1")); } diff --git a/specs/repeatable-oz-agent-run-skill/PRODUCT.md b/specs/repeatable-oz-agent-run-skill/PRODUCT.md new file mode 100644 index 0000000000..e389d023dd --- /dev/null +++ b/specs/repeatable-oz-agent-run-skill/PRODUCT.md @@ -0,0 +1,45 @@ +# Repeatable `oz agent run --skill` Product Spec +## Summary +`oz agent run --skill ` should accept the flag multiple times. Each provided skill should be resolved with the same behavior as the existing single-skill flag, and every resolved skill should be available to the running Oz agent through the normal skill system. Only the first skill should be treated as the invoked skill for the run. +## Problem +Today, users can provide one `--skill` to set the initial skill instructions for an Oz agent run. Some workflows need one primary invoked skill plus additional skills available for the agent to read later. Users currently cannot express that directly in a single command without relying on environment-wide skill discovery or modifying prompts manually. +## Goals +- Allow multiple `--skill ` flags on `oz agent run`. +- Resolve every provided skill using the same lookup, qualification, cloning, and error behavior that the single flag already uses. +- Make every resolved skill available in the run's `SkillsManager`. +- Submit only the first resolved skill as the invoked skill. +- Preserve existing behavior for commands that provide zero or one `--skill`. +## Non-goals +- Changing `oz agent run-cloud --skill` behavior. +- Changing skill resolution precedence or accepted skill spec formats. +- Combining multiple skill instruction bodies into the initial prompt. +- Changing the skill file format or the displayed skill-invoked output. +## Figma / design references +Figma: none provided. +## User Experience +1. A user can run `oz agent run --skill primary --skill helper --prompt "Do the task"`. +2. The command parser accepts repeated `--skill` flags and preserves their order. +3. The first skill in the command is the invoked skill. Its instructions are used exactly where the previous single `--skill` instructions were used: + - as the base prompt for a local prompt run, + - as the skill-only prompt for a skill-only run, + - as the server-side attached skill for a `--task-id` run. +4. Additional skills are not treated as invoked skills. They do not replace the run name, base prompt, task creation prompt, server-side attached skill, or skill-invoked output. +5. Every provided skill spec is resolved before the run starts. If any skill fails to resolve, the command fails with the same style of error the single-skill path already reports. +6. Each resolved skill is registered with the run's `SkillsManager` so it is available through the agent's normal skill-listing and read-skill behavior. +7. Skill resolution order is deterministic and follows the CLI order. The first successfully resolved skill remains the invoked skill even when later skills have different names, providers, or qualified repo specs. +8. Existing commands with a single `--skill` keep their current behavior and output. +9. Existing commands without `--skill` keep their current prompt validation behavior. +## Success Criteria +1. `oz agent run --skill a --skill b --prompt "..."` parses successfully. +2. All provided skill specs are resolved using the existing resolver. +3. All resolved skills are inserted into `SkillsManager`. +4. Only the first resolved skill is passed into the existing invoked-skill path. +5. Single-skill and no-skill command behavior remains unchanged. +6. Tests cover parser order, multi-skill resolution, `SkillsManager` population, and first-skill-only invoked behavior. +## Validation +- Add CLI parsing coverage for repeated `--skill` preserving order. +- Add Rust unit coverage around resolving multiple skills from disk and registering all of them with `SkillsManager`. +- Add or update task/config tests so only the first skill contributes invoked-skill configuration. +- Run targeted Rust tests for `warp_cli` and the touched agent SDK modules. +## Open questions +- None. diff --git a/specs/repeatable-oz-agent-run-skill/TECH.md b/specs/repeatable-oz-agent-run-skill/TECH.md new file mode 100644 index 0000000000..8374f2197c --- /dev/null +++ b/specs/repeatable-oz-agent-run-skill/TECH.md @@ -0,0 +1,56 @@ +# Repeatable `oz agent run --skill` Technical Spec +## Problem +`RunAgentArgs` currently stores `--skill` as `Option`, and the local agent setup resolves at most one skill before building the task. The implementation must preserve that first skill as the invoked skill while still resolving and registering every repeated skill so the Oz harness sees them in the normal `SkillsManager` context. +## Relevant code +- `crates/warp_cli/src/agent.rs:260` - `RunAgentArgs.skill` defines the local `oz agent run --skill` CLI field. +- `crates/warp_cli/src/lib_tests.rs:397` - parser tests lock in accepted prompt/skill combinations. +- `app/src/ai/agent_sdk/mod.rs:230` - feature-gate handling rejects `--skill` when Oz platform skills are disabled. +- `app/src/ai/agent_sdk/mod.rs:329` - `build_merged_config_and_task` consumes the resolved invoked skill. +- `app/src/ai/agent_sdk/mod.rs:741` - `AgentDriverRunner::resolve_skill` resolves the single CLI skill. +- `app/src/ai/agent_sdk/mod.rs:811` - `build_driver_options_and_task` resolves skill before task/config construction. +- `app/src/ai/skills/resolve_skill_spec.rs:132` - existing resolver implements skill spec lookup behavior. +- `app/src/ai/skills/skill_manager.rs:372` - `SkillManager::handle_skills_added` registers parsed skills by path and name. +- `app/src/ai/agent_sdk/driver_tests.rs:580` - existing skill-loading tests verify `SkillsManager` contents. +## Current state +The parser accepts only one local `--skill` because `RunAgentArgs.skill` is an `Option`. Agent setup clones sandboxed fully qualified skill repos only for that one spec, resolves it with `resolve_skill_spec`, and passes the resulting `Option` into config/task construction. Environment and global skills are loaded later in `AgentDriver::run_internal`, but the explicitly invoked skill is not explicitly added to `SkillsManager` by the setup path. +## Proposed changes +1. Change only local `RunAgentArgs.skill` to `Vec` with append semantics. Leave `RunCloudArgs.skill` as `Option` because repeatable behavior is scoped to `oz agent run`. +2. Update local prompt-group, feature-gate, and prompt-source checks from `is_some()` to `!is_empty()`. +3. Add a helper on `RunAgentArgs` such as `invoked_skill(&self) -> Option<&SkillSpec>` to centralize first-skill selection. Use it anywhere the code needs the historical single-skill behavior, including task creation prompt text. +4. Replace `AgentDriverRunner::resolve_skill` with a multi-skill resolver that: + - returns `Vec`, + - iterates `args.skill` in CLI order, + - applies the same sandboxed `org/repo` clone rule before resolving each skill, + - calls `resolve_skill_spec` for each spec with the same working directory and error formatting. +5. Add all resolved skills to `SkillsManager` after resolution and before task construction. This should call `SkillManager::handle_skills_added` with the resolved skills' `parsed_skill` values. The manager's path/name maps already deduplicate by path, so repeated registration is safe. +6. Continue passing only `resolved_skills.first().cloned()` into `build_merged_config_and_task` and `build_server_side_task`. Those functions should not concatenate multiple skill instruction bodies. +7. Update CLI tests and add focused app-unit coverage. Prefer small helper tests over end-to-end UI tests where possible. +## End-to-end flow +1. Clap parses repeated `--skill` values into `RunAgentArgs.skill` in command-line order. +2. Local agent setup canonicalizes the working directory. +3. Setup resolves each skill spec in order, cloning qualified sandboxed repos when needed. +4. Setup registers every resolved parsed skill with `SkillsManager`. +5. Setup selects the first resolved skill as `invoked_skill`. +6. Existing config/task construction uses `invoked_skill` exactly like the old single resolved skill. +7. `AgentDriver::run_internal` starts the Oz harness with the invoked skill prompt and a `SkillsManager` that also includes any additional resolved skills. +## Risks and mitigations +- Risk: secondary skills accidentally change the prompt or run name. + - Mitigation: keep existing config/task functions single-skill and pass only the first resolved skill. +- Risk: feature-gate checks accidentally allow hidden `--skill` usage. + - Mitigation: update all local checks to use `!args.skill.is_empty()`. +- Risk: resolving the same repo-qualified skill multiple times clones redundantly. + - Mitigation: existing `clone_repo_for_skill` skips targets that already exist as git repos; no new clone cache is needed. +- Risk: third-party harness behavior changes unexpectedly. + - Mitigation: this change only impacts local `RunAgentArgs`; `run-cloud` remains unchanged, and existing Oz-only skill loading gates stay in place. +## Testing and validation +- `crates/warp_cli/src/lib_tests.rs`: assert repeated `--skill` parses in order. +- `app/src/ai/agent_sdk/mod.rs` or adjacent tests: verify helper behavior selects only the first skill for invoked-skill code paths. +- `app/src/ai/agent_sdk/driver_tests.rs`: verify registering resolved CLI skills inserts all parsed skills into `SkillsManager`. +- Run: + - `cargo nextest run -p warp_cli agent_run_accepts_multiple_skills` + - `cargo nextest run -p warp ` + - `cargo fmt` +## Follow-ups +- Consider whether `oz agent run-cloud --skill` should become repeatable separately if cloud task creation needs the same primary-plus-supporting-skill behavior. +## Parallelization +Parallel sub-agents are not proposed. The parser, resolver, config, and tests are tightly coupled and fit in a small local change; splitting the work would add merge overhead without reducing meaningful wall-clock time. From 61a00f75db1cf68e241760eb5931ff60b04ae4d3 Mon Sep 17 00:00:00 2001 From: Oz Date: Wed, 20 May 2026 10:36:28 +0100 Subject: [PATCH 2/2] Handle mixed path separators on Windows --- app/src/ai/skills/file_watchers/utils.rs | 80 ++++++++----------- .../ai/skills/file_watchers/utils_tests.rs | 9 +++ 2 files changed, 43 insertions(+), 46 deletions(-) diff --git a/app/src/ai/skills/file_watchers/utils.rs b/app/src/ai/skills/file_watchers/utils.rs index 4d664d4852..39c8c78784 100644 --- a/app/src/ai/skills/file_watchers/utils.rs +++ b/app/src/ai/skills/file_watchers/utils.rs @@ -1,14 +1,9 @@ -use std::{ - collections::HashSet, - path::{Path, PathBuf}, - sync::LazyLock, -}; +use std::path::{Path, PathBuf}; use ai::skills::{ home_skills_path, read_skills, ParsedSkill, SkillProvider, SKILL_PROVIDER_DEFINITIONS, }; use anyhow::Error; -use regex::Regex; use repo_metadata::{local_model::GetContentsArgs, RepoContent, RepoMetadataModel}; use warpui::AppContext; @@ -69,52 +64,45 @@ pub fn is_skill_file(path: &Path) -> bool { extract_skill_parent_directory(path).is_ok() } -static SKILL_PROVIDER_PATHS: LazyLock> = LazyLock::new(|| { - // Collect the skill provider paths from the definitions - SKILL_PROVIDER_DEFINITIONS - .iter() - .map(|p| p.skills_path.to_string_lossy().to_string()) - .collect() -}); - -// Pattern: {prefix}/{provider_path}/{skill-name}/SKILL.md -// where provider_path is 2 parts (e.g., ".agents/skills") and skill-name is 1 part -#[cfg(not(target_os = "windows"))] -static SKILL_FILE_PATTERN: LazyLock = LazyLock::new(|| { - Regex::new(r"(.+)/([^/]+/[^/]+)/[^/]+/SKILL\.md$") - .expect("Failed to compile skill file pattern") -}); - -// On windows, the path separator is \ -#[cfg(target_os = "windows")] -static SKILL_FILE_PATTERN: LazyLock = LazyLock::new(|| { - Regex::new(r"(.+)\\([^\\]+\\[^\\]+)\\[^\\]+\\SKILL\.md$") - .expect("Failed to compile skill file pattern") -}); - pub fn extract_skill_parent_directory(path: &Path) -> Result { - let is_warp_home_skill = path - .file_name() - .and_then(|name| name.to_str()) - .is_some_and(|name| name == "SKILL.md") - && path - .parent() - .and_then(Path::parent) - .is_some_and(|parent| warp_managed_skill_dirs().iter().any(|dir| parent == dir)); - if is_warp_home_skill { + if path.file_name().and_then(|name| name.to_str()) != Some("SKILL.md") { + return Err(anyhow::anyhow!("Not a skill path: {}", path.display())); + } + + // Find the provider-specific skills directory. For example, if `path` is + // `path/to/project/.claude/skills/my-skill/SKILL.md`, `provider_skills_dir` + // is `path/to/project/.claude/skills`. + let Some(provider_skills_dir) = path.parent().and_then(Path::parent) else { + return Err(anyhow::anyhow!("Not a skill path: {}", path.display())); + }; + + if warp_managed_skill_dirs() + .iter() + .any(|dir| provider_skills_dir == dir) + { return dirs::home_dir() .ok_or_else(|| anyhow::anyhow!("Home directory not available for {}", path.display())); } - let path_str = path.to_string_lossy(); - - if let Some(captures) = SKILL_FILE_PATTERN.captures(&path_str) { - if let Some(provider_path) = captures.get(2) { - if SKILL_PROVIDER_PATHS.contains(provider_path.as_str()) { - if let Some(parent_directory) = captures.get(1) { - return Ok(PathBuf::from(parent_directory.as_str())); - } + + for provider in SKILL_PROVIDER_DEFINITIONS.iter() { + if !provider_skills_dir.ends_with(&provider.skills_path) { + continue; + } + + let mut parent_directory = provider_skills_dir; + for _ in provider.skills_path.components() { + if let Some(parent) = parent_directory.parent() { + parent_directory = parent; + } else { + return Err(anyhow::anyhow!("Not a skill path: {}", path.display())); } } + + if parent_directory.as_os_str().is_empty() { + return Err(anyhow::anyhow!("Not a skill path: {}", path.display())); + } + + return Ok(parent_directory.to_path_buf()); } Err(anyhow::anyhow!("Not a skill path: {}", path.display())) diff --git a/app/src/ai/skills/file_watchers/utils_tests.rs b/app/src/ai/skills/file_watchers/utils_tests.rs index 0efbeca294..c7cdf3e292 100644 --- a/app/src/ai/skills/file_watchers/utils_tests.rs +++ b/app/src/ai/skills/file_watchers/utils_tests.rs @@ -214,6 +214,15 @@ fn extract_skill_parent_directory_different_providers() { } } +#[cfg(target_os = "windows")] +#[test] +fn extract_skill_parent_directory_handles_mixed_windows_separators() { + let parent_directory = std::path::PathBuf::from(r"C:\repo"); + let skill_path = std::path::PathBuf::from(r"C:\repo\.agents/skills/my-skill/SKILL.md"); + let result = extract_skill_parent_directory(&skill_path); + assert_eq!(result.ok(), Some(parent_directory)); +} + #[test] fn extract_skill_parent_directory_returns_none_for_non_skill() { let Some(home_dir) = dirs::home_dir() else {