Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 64 additions & 48 deletions crates/tower-cmd/src/apps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,36 @@ pub fn apps_cmd() -> Command {
.subcommand(Command::new("list").about("List all of your apps"))
.subcommand(
Command::new("show")
.allow_external_subcommands(true)
.override_usage("tower apps show [OPTIONS] <APP_NAME>")
.after_help("Example: tower apps show hello-world")
.arg(
Arg::new("app_name")
.value_parser(value_parser!(String))
.index(1)
.required(true)
.help("Name of the app"),
)
.about("Show the details about an app in Tower"),
)
.subcommand(
Command::new("logs")
.arg(
Arg::new("app_name")
.value_parser(value_parser!(String))
.index(1)
.required(true)
.help("app_name#run_number"),
)
.arg(
Arg::new("run_number")
.value_parser(value_parser!(i64))
.index(2),
)
.arg(
Arg::new("follow")
.short('f')
.long("follow")
.help("Follow logs in real time")
.action(clap::ArgAction::SetTrue),
)
.allow_external_subcommands(true)
.override_usage("tower apps logs [OPTIONS] <APP_NAME>#<RUN_NUMBER>")
.after_help("Example: tower apps logs hello-world#11")
.about("Get the logs from a previous Tower app run"),
)
.subcommand(
Expand All @@ -54,15 +67,29 @@ pub fn apps_cmd() -> Command {
)
.subcommand(
Command::new("delete")
.allow_external_subcommands(true)
.override_usage("tower apps delete [OPTIONS] <APP_NAME>")
.after_help("Example: tower apps delete hello-world")
.arg(
Arg::new("app_name")
.value_parser(value_parser!(String))
.index(1)
.required(true)
.help("Name of the app"),
)
.about("Delete an app in Tower"),
)
}

pub async fn do_logs(config: Config, cmd: &ArgMatches) {
let (name, seq) = extract_app_name_and_run("logs", cmd.subcommand());
let app_name_raw = cmd.get_one::<String>("app_name").expect("app_name is required");
let (name, seq) = if let Some((name, num_str)) = app_name_raw.split_once('#') {
let num = num_str.parse::<i64>().unwrap_or_else(|_| output::die("Run number must be a number"));
(name.to_string(), num)
} else {
let num = match cmd.get_one::<i64>("run_number").copied() {
Some(n) => n,
None => latest_run_number(&config, app_name_raw).await,
};
(app_name_raw.clone(), num)
};
Comment on lines +83 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reject ambiguous run-number input forms.

If users pass both app#run and positional run_number, the positional value is silently ignored. It’s better to fail fast with a clear message.

💡 Suggested guard
 let (name, seq) = if let Some((name, num_str)) = app_name_raw.split_once('#') {
+    if cmd.get_one::<i64>("run_number").is_some() {
+        output::die(
+            "Provide run number either as 'app_name#run_number' or as a separate positional run_number, not both.",
+        );
+    }
     let num = num_str.parse::<i64>().unwrap_or_else(|_| output::die("Run number must be a number"));
     (name.to_string(), num)
 } else {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/tower-cmd/src/apps.rs` around lines 83 - 92, In the let (name, seq) =
... block, guard against providing both app#run and the positional run_number by
detecting when app_name_raw.split_once('#') yields Some(...) and
cmd.get_one::<i64>("run_number").is_some(); in that case call output::die with a
clear error like "Ambiguous run number: specify either app#run or the positional
run_number, not both" so the code fails fast; otherwise preserve the existing
behavior that uses the embedded # value or falls back to
cmd.get_one::<i64>("run_number")/latest_run_number(&config, app_name_raw).await
as currently implemented.

let follow = cmd.get_one::<bool>("follow").copied().unwrap_or(false);

if follow {
Expand All @@ -78,7 +105,7 @@ pub async fn do_logs(config: Config, cmd: &ArgMatches) {
}

pub async fn do_show(config: Config, cmd: &ArgMatches) {
let name = extract_app_name("show", cmd.subcommand());
let name = cmd.get_one::<String>("app_name").expect("app_name is required");

match api::describe_app(&config, &name).await {
Ok(app_response) => {
Expand Down Expand Up @@ -191,46 +218,20 @@ pub async fn do_create(config: Config, args: &ArgMatches) {
}

pub async fn do_delete(config: Config, cmd: &ArgMatches) {
let name = extract_app_name("delete", cmd.subcommand());
let name = cmd.get_one::<String>("app_name").expect("app_name is required");

output::with_spinner("Deleting app", api::delete_app(&config, &name)).await;
output::with_spinner("Deleting app", api::delete_app(&config, name)).await;
}

/// Extract app name and run number from command
fn extract_app_name_and_run(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> (String, i64) {
if let Some((name, _)) = cmd {
if let Some((name, num)) = name.split_once('#') {
return (
name.to_string(),
num.parse::<i64>().unwrap_or_else(|_| {
output::die("Run number must be an actual number");
}),
);
}

let line = format!(
"Run number is required. Example: tower apps {} <app name>#<run number>",
subcmd
);
output::die(&line);
}
let line = format!(
"App name is required. Example: tower apps {} <app name>#<run number>",
subcmd
);
output::die(&line)
}

fn extract_app_name(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> String {
if let Some((name, _)) = cmd {
return name.to_string();
async fn latest_run_number(config: &Config, name: &str) -> i64 {
match api::describe_app(config, name).await {
Ok(resp) => resp.runs
.iter()
.map(|r| r.number)
.max()
.unwrap_or_else(|| output::die(&format!("No runs found for app '{}'", name))),
Err(err) => output::tower_error_and_die(err, "Fetching app details failed"),
}

let line = format!(
"App name is required. Example: tower apps {} <app name>",
subcmd
);
output::die(&line);
}

const FOLLOW_BACKOFF_INITIAL: Duration = Duration::from_millis(500);
Expand Down Expand Up @@ -539,9 +540,24 @@ mod tests {
assert_eq!(cmd, "logs");
assert_eq!(sub_matches.get_one::<bool>("follow"), Some(&true));
assert_eq!(
sub_matches.subcommand().map(|(name, _)| name),
sub_matches.get_one::<String>("app_name").map(|s| s.as_str()),
Some("hello-world#11")
);
assert_eq!(sub_matches.get_one::<i64>("run_number"), None);
}

#[test]
fn test_separate_run_number_parsing() {
let matches = apps_cmd()
.try_get_matches_from(["apps", "logs", "hello-world", "11"])
.unwrap();
let (_, sub_matches) = matches.subcommand().unwrap();

assert_eq!(
sub_matches.get_one::<String>("app_name").map(|s| s.as_str()),
Some("hello-world")
);
assert_eq!(sub_matches.get_one::<i64>("run_number"), Some(&11));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion crates/tower-cmd/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ impl App {
}
Some(("deploy", args)) => deploy::do_deploy(sessionized_config, args).await,
Some(("package", args)) => package::do_package(sessionized_config, args).await,
Some(("run", args)) => run::do_run(sessionized_config, args, args.subcommand()).await,
Some(("run", args)) => run::do_run(sessionized_config, args).await,
Some(("teams", sub_matches)) => {
let teams_command = sub_matches.subcommand();

Expand Down
4 changes: 3 additions & 1 deletion crates/tower-cmd/src/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ impl TowerService {
}

#[tool(
description = "Run on Tower cloud. Prerequisites: Towerfile, tower_apps_create, tower_deploy."
description = "Run an app on Tower cloud. Pass parameters as a JSON object, e.g. {\"key\": \"value\"}. Prerequisites: Towerfile, tower_apps_create, tower_deploy."
)]
async fn tower_run_remote(
&self,
Expand Down Expand Up @@ -983,6 +983,8 @@ Rules:
- Use tower_file_update/add_parameter to modify Towerfiles (never edit TOML directly)
- DO NOT add hatchling/setuptools to pyproject.toml - Tower handles deployment
- Tower apps need: pyproject.toml (deps only), Python code, Towerfile
- Pass run parameters as a JSON object {\"key\": \"value\"}, not as CLI flags
- There is no --param or --params flag

Use tower_workflow_help for the complete workflow.".to_string()),
}
Expand Down
116 changes: 95 additions & 21 deletions crates/tower-cmd/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,19 @@ use tower_runtime::subprocess::SubprocessBackend;

pub fn run_cmd() -> Command {
Command::new("run")
.allow_external_subcommands(true)
.after_help(
"Examples:\n \
tower run Run app from ./Towerfile (remote)\n \
tower run --local Run app from ./Towerfile (local)\n \
tower run my-app Run a deployed app by name\n \
tower run -p key=value Pass a parameter to the run\n \
tower run my-app -p key=value Run a named app with parameters",
)
.arg(
Arg::new("app_name")
.help("Name of a deployed app to run (uses ./Towerfile if omitted)")
.index(1),
)
.arg(
Arg::new("dir")
.long("dir")
Expand Down Expand Up @@ -65,11 +77,26 @@ pub fn run_cmd() -> Command {
.about("Run your code in Tower or locally")
}

pub async fn do_run(config: Config, args: &ArgMatches, cmd: Option<(&str, &ArgMatches)>) {
if let Err(e) = do_run_inner(config, args, cmd).await {
pub async fn do_run(config: Config, args: &ArgMatches) {
if let Err(e) = do_run_inner(config, args).await {
match e {
Error::ApiRunError { source } => {
output::tower_error_and_die(source, "Scheduling run failed");
Error::ApiRunError { ref source } => {
let is_not_found = matches!(
source,
tower_api::apis::Error::ResponseError(resp) if resp.status == reqwest::StatusCode::NOT_FOUND
);
if is_not_found {
output::error("App not found. It may not exist or hasn't been deployed yet.");
output::write("\nTo fix this:\n");
output::write(" 1. Check your app exists: tower apps list\n");
output::write(" 2. Deploy your app: tower deploy\n");
output::write(" 3. Then run it: tower run\n");
std::process::exit(1);
}
if let Error::ApiRunError { source } = e {
output::tower_error_and_die(source, "Scheduling run failed");
}
unreachable!();
}
_ => {
output::die(&e.to_string());
Expand All @@ -83,9 +110,8 @@ pub async fn do_run(config: Config, args: &ArgMatches, cmd: Option<(&str, &ArgMa
pub async fn do_run_inner(
config: Config,
args: &ArgMatches,
cmd: Option<(&str, &ArgMatches)>,
) -> Result<(), Error> {
let res = get_run_parameters(args, cmd);
let res = get_run_parameters(args);

// We always expect there to be an environment due to the fact that there is a
// default value.
Expand Down Expand Up @@ -467,18 +493,15 @@ fn handle_run_completion(res: Result<Run, oneshot::error::RecvError>) -> Result<
}
}

/// get_run_parameters takes care of all the hairy bits around digging about in the `clap`
/// internals to figure out what the user is requesting. In the end, it determines if we are meant
/// to do a local run or a remote run, and it determines the path to the relevant Towerfile that
/// should be loaded.
/// Extracts the local/remote flag, Towerfile directory, parameters, and optional app name
/// from the parsed CLI args.
fn get_run_parameters(
args: &ArgMatches,
cmd: Option<(&str, &ArgMatches)>,
) -> Result<(bool, PathBuf, HashMap<String, String>, Option<String>), config::Error> {
let local = *args.get_one::<bool>("local").unwrap();
let path = resolve_path(args);
let params = parse_parameters(args);
let app_name = get_app_name(cmd);
let app_name = args.get_one::<String>("app_name").cloned();

Ok((local, path, params, app_name))
}
Expand Down Expand Up @@ -528,14 +551,6 @@ fn resolve_path(args: &ArgMatches) -> PathBuf {
}
}

/// get_app_name is a helper function that will extract the app name from the `clap` arguments if
fn get_app_name(cmd: Option<(&str, &ArgMatches)>) -> Option<String> {
match cmd {
Some((name, _)) if !name.is_empty() => Some(name.to_string()),
_ => None,
}
}

/// get_secrets_inner manages the process of getting secrets from the Tower server in a way that can be
/// used by the local runtime during local app execution. Returns API errors for spinner handling.
async fn get_secrets(
Expand Down Expand Up @@ -799,3 +814,62 @@ fn monitor_run_completion(config: &Config, run: &Run) -> oneshot::Receiver<Run>

rx
}

#[cfg(test)]
mod tests {
use super::run_cmd;

fn parse(args: &[&str]) -> Result<clap::ArgMatches, clap::Error> {
let mut full = vec!["run"];
full.extend_from_slice(args);
run_cmd().try_get_matches_from(full)
}

#[test]
fn app_name_parsed_as_positional_arg() {
let m = parse(&["my-app"]).unwrap();
assert_eq!(m.get_one::<String>("app_name").map(|s| s.as_str()), Some("my-app"));
}

#[test]
fn no_app_name_is_fine() {
let m = parse(&[]).unwrap();
assert_eq!(m.get_one::<String>("app_name"), None);
}

#[test]
fn unknown_flags_are_rejected() {
let err = parse(&["--param", "x=y"]).unwrap_err();
let msg = err.to_string();
assert!(msg.contains("--param"), "should mention the bad flag: {msg}");
assert!(msg.contains("--parameter"), "should suggest the correct flag: {msg}");
}

#[test]
fn help_flag_is_not_swallowed_as_app_name() {
let err = parse(&["--help"]).unwrap_err();
assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp);
}

#[test]
fn help_after_app_name_still_shows_help() {
let err = parse(&["my-app", "--help"]).unwrap_err();
assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp);
}

#[test]
fn parameters_after_app_name() {
let m = parse(&["my-app", "-p", "key=val"]).unwrap();
assert_eq!(m.get_one::<String>("app_name").map(|s| s.as_str()), Some("my-app"));
let params: Vec<&String> = m.get_many::<String>("parameters").unwrap().collect();
assert_eq!(params, vec!["key=val"]);
}

#[test]
fn parameters_before_app_name() {
let m = parse(&["-p", "key=val", "my-app"]).unwrap();
assert_eq!(m.get_one::<String>("app_name").map(|s| s.as_str()), Some("my-app"));
let params: Vec<&String> = m.get_many::<String>("parameters").unwrap().collect();
assert_eq!(params, vec!["key=val"]);
}
}
Loading