feat: return to first class clap handling of sub commands#213
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/tower-cmd/src/schedules.rs (1)
73-80: Consider aligning delete arg naming with update (id_or_name).
updateacceptsid_or_name, whiledeletestill usesschedule_id. Unifying names would make the CLI surface more consistent.♻️ Suggested consistency refactor
- Arg::new("schedule_id") + Arg::new("id_or_name") .value_parser(value_parser!(String)) .index(1) .required(true) - .help("The schedule ID to delete"), + .help("ID or name of the schedule to delete"), - let schedule_id = args.get_one::<String>("schedule_id").expect("schedule_id is required"); + let id_or_name = args.get_one::<String>("id_or_name").expect("id_or_name is required"); - api::delete_schedule(&config, schedule_id), + api::delete_schedule(&config, id_or_name), - output::success(&format!("Schedule {} deleted", schedule_id)); + output::success(&format!("Schedule {} deleted", id_or_name));Also applies to: 191-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/schedules.rs` around lines 73 - 80, Rename the delete command's argument from "schedule_id" to match update's "id_or_name" to keep CLI consistent: update the Arg::new("schedule_id") in the Command::new("delete") block to Arg::new("id_or_name"), adjust its value_parser/index/required/help text accordingly (e.g., "The schedule ID or name to delete"), and make the same change for the other delete occurrence referenced around the 191-199 region so both delete argument definitions mirror the update argument name "id_or_name".crates/tower-cmd/src/run.rs (1)
83-99: SimplifyApiRunErrorhandling by bindingsourceonce.Line 96 re-matches the same variant already matched at Line 83, and Line 99 adds unnecessary control-flow noise. You can keep behavior identical with a single binding.
♻️ Proposed simplification
- Error::ApiRunError { ref source } => { + Error::ApiRunError { source } => { let is_not_found = matches!( - source, + &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::tower_error_and_die(source, "Scheduling run failed"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/run.rs` around lines 83 - 99, The Error::ApiRunError arm should bind source once and reuse it instead of re-matching; change the pattern to Error::ApiRunError { source } => { let is_not_found = matches!(&source, tower_api::apis::Error::ResponseError(resp) if resp.status == reqwest::StatusCode::NOT_FOUND); if is_not_found { ... std::process::exit(1); } output::tower_error_and_die(source, "Scheduling run failed"); } so you compute the NOT_FOUND check by matching on &source and then call output::tower_error_and_die with the moved source, removing the redundant if let and unreachable!.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tower-cmd/src/apps.rs`:
- Around line 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.
In `@crates/tower-cmd/src/secrets.rs`:
- Around line 191-196: The current parsing of secret_name_arg using
split_once('/') accepts empty parts (e.g., "/secret" or "env/") and forwards
invalid environment/name values; update the logic around
secret_name_arg.split_once('/') to validate that both env and name are non-empty
strings before using them (reference secret_name_arg, environment, name, and
args.get_one), and if either part is empty return or surface a clear error (or
fall back to the existing args.get_one path) so you don't call the API with
invalid values.
In `@tests/integration/features/steps/cli_steps.py`:
- Around line 146-155: Formatting check is failing for the test function
step_output_should_show_app_not_found_with_guidance; run the Black formatter on
the file that contains this function (or run black .) to apply consistent
formatting, fix any spacing/trailing-whitespace issues around the function and
its docstring, then add and commit the reformatted file so CI's Black check
passes.
---
Nitpick comments:
In `@crates/tower-cmd/src/run.rs`:
- Around line 83-99: The Error::ApiRunError arm should bind source once and
reuse it instead of re-matching; change the pattern to Error::ApiRunError {
source } => { let is_not_found = matches!(&source,
tower_api::apis::Error::ResponseError(resp) if resp.status ==
reqwest::StatusCode::NOT_FOUND); if is_not_found { ... std::process::exit(1); }
output::tower_error_and_die(source, "Scheduling run failed"); } so you compute
the NOT_FOUND check by matching on &source and then call
output::tower_error_and_die with the moved source, removing the redundant if let
and unreachable!.
In `@crates/tower-cmd/src/schedules.rs`:
- Around line 73-80: Rename the delete command's argument from "schedule_id" to
match update's "id_or_name" to keep CLI consistent: update the
Arg::new("schedule_id") in the Command::new("delete") block to
Arg::new("id_or_name"), adjust its value_parser/index/required/help text
accordingly (e.g., "The schedule ID or name to delete"), and make the same
change for the other delete occurrence referenced around the 191-199 region so
both delete argument definitions mirror the update argument name "id_or_name".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
crates/tower-cmd/src/apps.rscrates/tower-cmd/src/lib.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/run.rscrates/tower-cmd/src/schedules.rscrates/tower-cmd/src/secrets.rscrates/tower-cmd/src/teams.rstests/integration/features/cli_runs.featuretests/integration/features/steps/cli_steps.py
Comment on lines
+83
to
+92
| 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) | ||
| }; |
There was a problem hiding this comment.
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.
Comment on lines
+191
to
+196
| let (environment, name) = if let Some((env, name)) = secret_name_arg.split_once('/') { | ||
| (env.to_string(), name.to_string()) | ||
| } else { | ||
| let env = args.get_one::<String>("environment").expect("environment has default"); | ||
| (env.clone(), secret_name_arg.clone()) | ||
| }; |
There was a problem hiding this comment.
Validate environment/name shorthand before calling the API.
split_once('/') currently allows empty parts (e.g., /secret or env/). That sends invalid values downstream and returns less helpful errors.
💡 Suggested fix
let secret_name_arg = args.get_one::<String>("secret_name").expect("secret_name is required");
let (environment, name) = if let Some((env, name)) = secret_name_arg.split_once('/') {
+ if env.is_empty() || name.is_empty() {
+ output::die("Invalid secret identifier. Use 'secret_name' or 'environment/secret_name'.");
+ }
(env.to_string(), name.to_string())
} else {
let env = args.get_one::<String>("environment").expect("environment has default");
(env.clone(), secret_name_arg.clone())
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (environment, name) = if let Some((env, name)) = secret_name_arg.split_once('/') { | |
| (env.to_string(), name.to_string()) | |
| } else { | |
| let env = args.get_one::<String>("environment").expect("environment has default"); | |
| (env.clone(), secret_name_arg.clone()) | |
| }; | |
| let (environment, name) = if let Some((env, name)) = secret_name_arg.split_once('/') { | |
| if env.is_empty() || name.is_empty() { | |
| output::die("Invalid secret identifier. Use 'secret_name' or 'environment/secret_name'."); | |
| } | |
| (env.to_string(), name.to_string()) | |
| } else { | |
| let env = args.get_one::<String>("environment").expect("environment has default"); | |
| (env.clone(), secret_name_arg.clone()) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-cmd/src/secrets.rs` around lines 191 - 196, The current parsing
of secret_name_arg using split_once('/') accepts empty parts (e.g., "/secret" or
"env/") and forwards invalid environment/name values; update the logic around
secret_name_arg.split_once('/') to validate that both env and name are non-empty
strings before using them (reference secret_name_arg, environment, name, and
args.get_one), and if either part is empty return or surface a clear error (or
fall back to the existing args.get_one path) so you don't call the API with
invalid values.
Comment on lines
+146
to
+155
| @step('the output should show "App not found" with guidance') | ||
| def step_output_should_show_app_not_found_with_guidance(context): | ||
| """Verify 404 error shows helpful guidance instead of a bare error""" | ||
| output = context.cli_output | ||
| assert ( | ||
| red_color_code in output | ||
| ), f"Expected red color codes in output, got: {output}" | ||
| assert "App not found" in output, f"Expected 'App not found' in output, got: {output}" | ||
| assert "tower deploy" in output, f"Expected 'tower deploy' guidance in output, got: {output}" | ||
|
|
There was a problem hiding this comment.
Black formatting check is failing and blocks CI.
The pipeline reports one Python file needs reformatting. Please run Black on this file/update before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/features/steps/cli_steps.py` around lines 146 - 155,
Formatting check is failing for the test function
step_output_should_show_app_not_found_with_guidance; run the Black formatter on
the file that contains this function (or run black .) to apply consistent
formatting, fix any spacing/trailing-whitespace issues around the function and
its docstring, then add and commit the reformatted file so CI's Black check
passes.
Specifically to improve error messages for run params, and to give guidance for what to do when something goes wrong. Additionally, moving back to clap means we get stuff like still working for free (this was confusing claude, and also users).
socksy
force-pushed
the
feature/improve-run-params-errors
branch
from
2194e34 to
c0f642b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Building on the work from #206, improve the handling of sub commands. Specifically allowing
--helpaftertower run foo-app, giving better error messages when doing things liketower run --param foo=bar(where--paramshould either be-por--parameter), and make sub command handling and documentation consistent.Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation