fix(cli): scope upload-only options (volcengine#2073)

Author: ZaynJarvis

crates/ov_cli/src/client.rs

@@ -578,13 +578,24 @@ impl HttpClient {
         data: &str,
         wait: bool,
         timeout: Option<f64>,
+        show_progress: bool,
+        verbose: bool,
     ) -> Result<serde_json::Value> {
         let path_obj = Path::new(data);
 
         if path_obj.exists() {
             if path_obj.is_dir() {
-                let zip_file = self.zip_directory(path_obj, None)?;
-                let temp_file_id = self.upload_temp_file(zip_file.path()).await?;
+                let zip_file = if show_progress {
+                    self.zip_directory_with_progress(path_obj, verbose, None)?
+                } else {
+                    self.zip_directory(path_obj, None)?
+                };
+                let temp_file_id = if show_progress {
+                    self.upload_temp_file_with_progress(zip_file.path(), verbose)
+                        .await?
+                } else {
+                    self.upload_temp_file(zip_file.path()).await?
+                };
 
                 let body = serde_json::json!({
                     "temp_file_id": temp_file_id,
@@ -597,7 +608,12 @@ impl HttpClient {
                     .post_with_timeout("/api/v1/skills", &body, dynamic_timeout)
                     .await
             } else if path_obj.is_file() {
-                let temp_file_id = self.upload_temp_file(path_obj).await?;
+                let temp_file_id = if show_progress {
+                    self.upload_temp_file_with_progress(path_obj, verbose)
+                        .await?
+                } else {
+                    self.upload_temp_file(path_obj).await?
+                };
 
                 let body = serde_json::json!({
                     "temp_file_id": temp_file_id,

crates/ov_cli/src/commands/resources.rs

@@ -58,10 +58,14 @@ pub async fn add_skill(
     data: &str,
     wait: bool,
     timeout: Option<f64>,
+    show_progress: bool,
+    verbose: bool,
     format: OutputFormat,
     compact: bool,
 ) -> Result<()> {
-    let result = client.add_skill(data, wait, timeout).await?;
+    let result = client
+        .add_skill(data, wait, timeout, show_progress, verbose)
+        .await?;
 
     if !wait && matches!(format, OutputFormat::Table) {
         eprintln!("Note: Skill is being processed in the background.");

crates/ov_cli/src/handlers.rs

@@ -134,6 +134,8 @@ pub async fn handle_add_skill(
         &data,
         wait,
         timeout,
+        ctx.should_show_progress(),
+        ctx.is_verbose(),
         ctx.output_format,
         ctx.compact,
     )

crates/ov_cli/src/main.rs

@@ -8,7 +8,7 @@ mod output;
 mod tui;
 mod utils;
 
-use clap::{ArgAction, Parser, Subcommand};
+use clap::{ArgAction, Args, Parser, Subcommand};
 use config::Config;
 use error::Result;
 use output::OutputFormat;
@@ -140,26 +140,77 @@ struct Cli {
     #[arg(long = "agent-id", global = true)]
     agent_id: Option<String>,
 
-    /// Use root API key for admin privileges
-    #[arg(long, global = true)]
+    /// Use root API key for admin commands
+    #[arg(long)]
     sudo: bool,
 
-    /// Show upload progress (overrides config file)
-    #[arg(long, global = true)]
+    /// Show upload progress (legacy pre-command placement; prefer command-local --progress)
+    #[arg(long, hide = true)]
     progress: bool,
 
-    /// Disable upload progress (overrides config file)
-    #[arg(long = "no-progress", global = true, conflicts_with = "progress")]
+    /// Disable upload progress (legacy pre-command placement; prefer command-local --no-progress)
+    #[arg(long = "no-progress", hide = true, conflicts_with = "progress")]
     no_progress: bool,
 
-    /// Enable verbose output (overrides config file)
-    #[arg(short, long, global = true)]
+    /// Enable upload diagnostics (legacy pre-command placement; prefer command-local --verbose)
+    #[arg(short, long, hide = true)]
     verbose: bool,
 
     #[command(subcommand)]
     command: Commands,
 }
 
+#[derive(Args, Debug, Clone, Copy, Default)]
+struct UploadCliOptions {
+    /// Show local file upload progress (overrides config file)
+    #[arg(long, conflicts_with = "no_progress")]
+    progress: bool,
+
+    /// Disable local file upload progress (overrides config file)
+    #[arg(long = "no-progress", conflicts_with = "progress")]
+    no_progress: bool,
+
+    /// Print extra diagnostics during local file upload
+    #[arg(short, long)]
+    verbose: bool,
+}
+
+impl UploadCliOptions {
+    fn is_set(self) -> bool {
+        self.progress || self.no_progress || self.verbose
+    }
+
+    fn merged_with_legacy(self, legacy: Self) -> Self {
+        Self {
+            progress: self.progress || (!self.no_progress && legacy.progress),
+            no_progress: self.no_progress || (!self.progress && legacy.no_progress),
+            verbose: self.verbose || legacy.verbose,
+        }
+    }
+
+    fn show_progress_override(self) -> Option<bool> {
+        if self.progress {
+            Some(true)
+        } else if self.no_progress {
+            Some(false)
+        } else {
+            None
+        }
+    }
+
+    fn verbose_override(self) -> Option<bool> {
+        if self.verbose { Some(true) } else { None }
+    }
+}
+
+impl CliContext {
+    fn with_upload_options(mut self, options: UploadCliOptions) -> Self {
+        self.show_progress = options.show_progress_override();
+        self.verbose = options.verbose_override();
+        self
+    }
+}
+
 // Commands are organized with category tags in their doc comments.
 //
 // # Command Tagging System
@@ -218,6 +269,8 @@ enum Commands {
         /// Watch interval in minutes for automatic resource monitoring (0 = no monitoring)
         #[arg(long, default_value = "0")]
         watch_interval: f64,
+        #[command(flatten)]
+        upload_options: UploadCliOptions,
     },
     /// [Data] Add a skill into OpenViking
     AddSkill {
@@ -229,6 +282,8 @@ enum Commands {
         /// Wait timeout in seconds
         #[arg(long)]
         timeout: Option<f64>,
+        #[command(flatten)]
+        upload_options: UploadCliOptions,
     },
     /// [Data] List directory contents
     #[command(alias = "list")]
@@ -625,6 +680,23 @@ impl Commands {
             _ => false,
         }
     }
+
+    fn supports_upload_options(&self) -> bool {
+        matches!(self, Self::AddResource { .. } | Self::AddSkill { .. })
+    }
+}
+
+fn legacy_upload_option_error(
+    options: UploadCliOptions,
+    command: &Commands,
+) -> Option<&'static str> {
+    if options.is_set() && !command.supports_upload_options() {
+        Some(
+            "--progress, --no-progress, and --verbose are only supported for add-resource and add-skill",
+        )
+    } else {
+        None
+    }
 }
 
 #[derive(Subcommand)]
@@ -1005,17 +1077,15 @@ async fn main() {
 
     let output_format = cli.output;
     let compact = cli.compact;
-
-    // Determine show_progress override:
-    let show_progress = if cli.progress {
-        Some(true)
-    } else if cli.no_progress {
-        Some(false)
-    } else {
-        None
+    let legacy_upload_options = UploadCliOptions {
+        progress: cli.progress,
+        no_progress: cli.no_progress,
+        verbose: cli.verbose,
     };
-    // Determine verbose override:
-    let verbose = if cli.verbose { Some(true) } else { None };
+    if let Some(message) = legacy_upload_option_error(legacy_upload_options, &cli.command) {
+        eprintln!("Error: {}", message);
+        std::process::exit(2);
+    }
 
     let ctx = match CliContext::new(
         output_format,
@@ -1024,8 +1094,8 @@ async fn main() {
         cli.user.clone(),
         cli.agent_id.clone(),
         cli.sudo,
-        show_progress,
-        verbose,
+        None,
+        None,
     ) {
         Ok(ctx) => ctx,
         Err(e) => {
@@ -1064,7 +1134,10 @@ async fn main() {
             exclude,
             no_directly_upload_media,
             watch_interval,
+            upload_options,
         } => {
+            let ctx =
+                ctx.with_upload_options(upload_options.merged_with_legacy(legacy_upload_options));
             handlers::handle_add_resource(
                 path,
                 to,
@@ -1088,7 +1161,12 @@ async fn main() {
             data,
             wait,
             timeout,
-        } => handlers::handle_add_skill(data, wait, timeout, ctx).await,
+            upload_options,
+        } => {
+            let ctx =
+                ctx.with_upload_options(upload_options.merged_with_legacy(legacy_upload_options));
+            handlers::handle_add_skill(data, wait, timeout, ctx).await
+        }
         Commands::Relations { uri } => handlers::handle_relations(uri, ctx).await,
         Commands::Link {
             from_uri,
@@ -1306,11 +1384,14 @@ async fn main() {
 
 #[cfg(test)]
 mod tests {
-    use super::{Cli, CliContext, Commands, PrivacyCommands, preprocess_privacy_args};
+    use super::{
+        Cli, CliContext, Commands, PrivacyCommands, UploadCliOptions, legacy_upload_option_error,
+        preprocess_privacy_args,
+    };
     use crate::config::Config;
     use crate::handlers;
     use crate::output::OutputFormat;
-    use clap::Parser;
+    use clap::{CommandFactory, Parser};
     use std::ffi::OsString;
 
     #[test]
@@ -1332,6 +1413,115 @@ mod tests {
         assert_eq!(cli.agent_id.as_deref(), Some("assistant-1"));
     }
 
+    #[test]
+    fn cli_tree_help_hides_upload_and_admin_only_flags() {
+        let err = Cli::command()
+            .try_get_matches_from(["ov", "tree", "--help"])
+            .expect_err("help should exit through clap error");
+        let help = err.to_string();
+
+        assert!(!help.contains("--progress"));
+        assert!(!help.contains("--no-progress"));
+        assert!(!help.contains("--verbose"));
+        assert!(!help.contains("--sudo"));
+    }
+
+    #[test]
+    fn cli_add_resource_help_shows_upload_flags() {
+        let err = Cli::command()
+            .try_get_matches_from(["ov", "add-resource", "--help"])
+            .expect_err("help should exit through clap error");
+        let help = err.to_string();
+
+        assert!(help.contains("--progress"));
+        assert!(help.contains("--no-progress"));
+        assert!(help.contains("--verbose"));
+    }
+
+    #[test]
+    fn cli_add_skill_help_shows_upload_flags() {
+        let err = Cli::command()
+            .try_get_matches_from(["ov", "add-skill", "--help"])
+            .expect_err("help should exit through clap error");
+        let help = err.to_string();
+
+        assert!(help.contains("--progress"));
+        assert!(help.contains("--no-progress"));
+        assert!(help.contains("--verbose"));
+    }
+
+    #[test]
+    fn cli_tree_rejects_upload_and_admin_only_flags_after_subcommand() {
+        assert!(Cli::try_parse_from(["ov", "tree", "viking://", "--progress"]).is_err());
+        assert!(Cli::try_parse_from(["ov", "tree", "viking://", "--no-progress"]).is_err());
+        assert!(Cli::try_parse_from(["ov", "tree", "viking://", "--verbose"]).is_err());
+        assert!(Cli::try_parse_from(["ov", "tree", "viking://", "--sudo"]).is_err());
+    }
+
+    #[test]
+    fn cli_parses_upload_flags_on_upload_commands() {
+        let add_resource =
+            Cli::try_parse_from(["ov", "add-resource", "./README.md", "--progress", "--verbose"])
+                .expect("add-resource upload flags should parse");
+        match add_resource.command {
+            Commands::AddResource { upload_options, .. } => {
+                assert!(upload_options.progress);
+                assert!(upload_options.verbose);
+            }
+            _ => panic!("expected add-resource command"),
+        }
+
+        let add_skill = Cli::try_parse_from(["ov", "add-skill", "./skill", "--no-progress"])
+            .expect("add-skill upload flags should parse");
+        match add_skill.command {
+            Commands::AddSkill { upload_options, .. } => {
+                assert!(upload_options.no_progress);
+            }
+            _ => panic!("expected add-skill command"),
+        }
+    }
+
+    #[test]
+    fn cli_keeps_legacy_pre_command_upload_flags() {
+        let cli = Cli::try_parse_from([
+            "ov",
+            "--progress",
+            "--verbose",
+            "add-resource",
+            "./README.md",
+        ])
+        .expect("legacy pre-command upload flags should still parse");
+
+        assert!(cli.progress);
+        assert!(cli.verbose);
+    }
+
+    #[test]
+    fn legacy_pre_command_upload_flags_only_allow_upload_commands() {
+        let upload_options = UploadCliOptions {
+            progress: true,
+            no_progress: false,
+            verbose: false,
+        };
+
+        let tree = Cli::try_parse_from(["ov", "--progress", "tree", "viking://"])
+            .expect("hidden legacy flag still parses before runtime validation");
+        assert!(legacy_upload_option_error(upload_options, &tree.command).is_some());
+
+        let add_resource =
+            Cli::try_parse_from(["ov", "--progress", "add-resource", "./README.md"])
+                .expect("legacy pre-command upload flags should parse for add-resource");
+        assert!(legacy_upload_option_error(upload_options, &add_resource.command).is_none());
+    }
+
+    #[test]
+    fn cli_parses_sudo_before_admin_command() {
+        let cli = Cli::try_parse_from(["ov", "--sudo", "admin", "list-accounts"])
+            .expect("pre-command sudo should parse");
+
+        assert!(cli.sudo);
+    }
+
     #[test]
     fn cli_context_overrides_identity_from_cli_flags() {
         let config = Config {