Conversation
* feat(params): add secret parameter support * Rename secret parameter field to hidden
bradhe
requested review from
codingcyclist,
Copilot,
giray123,
jo-sm,
konstantinoscs,
sammuti and
socksy
📝 WalkthroughWalkthroughBumps versions to 0.3.50, adds a serde-default Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR releases version 0.3.50 of the Tower CLI, implementing several bug fixes and a new hidden parameters feature. The changes improve the CLI's robustness when handling signals and broken pipes, fix issues with schedule command argument parsing, and add support for marking parameters as hidden to prevent their values from being displayed in schedule listings.
Changes:
- Added hidden parameter support across API models, towerfile configuration, and MCP tools
- Fixed schedule update/delete commands by replacing external subcommand pattern with explicit positional arguments
- Restored default SIGINT behavior in Python CLI entrypoint to fix ctrl+c handling
- Added graceful broken pipe handling to prevent panics when piping CLI output
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump to 0.3.50 |
| pyproject.toml | Version bump to 0.3.50 |
| Cargo.toml | Version bump to 0.3.50 |
| Cargo.lock | Version bump to 0.3.50 for all workspace crates |
| src/tower/cli.py | Added SIGINT signal handler to restore default behavior |
| tests/tower/test_cli.py | Added test coverage for SIGINT signal handling |
| crates/tower/src/lib.rs | Code formatting improvements for method chains |
| crates/tower-cmd/src/schedules.rs | Fixed update/delete commands to use positional args, improved parse_parameters, added comprehensive tests |
| crates/tower-cmd/src/output.rs | Added write_to_stdout/stderr helpers with broken pipe handling |
| crates/tower-cmd/src/mcp.rs | Added masking of hidden parameter values in schedule listings |
| crates/tower-cmd/src/api.rs | Added hidden field to RunParameter creation, explicitly set update_schedule fields |
| crates/tower-api/src/models/run_parameter.rs | Added hidden field with serde default |
| crates/tower-api/src/models/parameter.rs | Added hidden field with serde default |
| crates/config/src/towerfile.rs | Added hidden field to Parameter struct with tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/config/src/towerfile.rs (1)
122-130: Consider adding ahiddenparameter toadd_parameter()for API completeness.Currently,
add_parameter()always setshidden: false. If programmatic creation of hidden parameters is needed in the future, this method would need modification.💡 Optional enhancement
- pub fn add_parameter(&mut self, name: String, description: String, default: String) { + pub fn add_parameter(&mut self, name: String, description: String, default: String, hidden: bool) { self.parameters.push(Parameter { name, description, default, - hidden: false, + hidden, }); }This would require updating callers to pass
falsefor the new parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/config/src/towerfile.rs` around lines 122 - 130, Update the add_parameter method to accept a hidden: bool argument and use it when constructing the Parameter so callers can create hidden parameters; specifically change the signature of add_parameter (currently pub fn add_parameter(&mut self, name: String, description: String, default: String)) to include hidden: bool and set Parameter { name, description, default, hidden } accordingly, then update all callers to pass false where they previously relied on the default non-hidden behavior.crates/tower-cmd/src/api.rs (2)
881-886: Same suggestion applies here.Consider using
RunParameter::new(key, value)for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/api.rs` around lines 881 - 886, Replace the inline construction of RunParameter in the iterator map with the canonical constructor: change the closure that currently returns RunParameter { name: key, value, hidden: false } to call RunParameter::new(key, value) so the mapping uses the struct's constructor for consistency with other call sites (refer to RunParameter::new and the map producing RunParameter instances).
843-848: Consider usingRunParameter::new()constructor.The struct literal works, but using the constructor would be more maintainable if the struct evolves (fewer places to update).
♻️ Suggested refactor
let run_parameters = parameters.map(|params| { params .into_iter() - .map(|(key, value)| RunParameter { - name: key, - value, - hidden: false, - }) + .map(|(key, value)| RunParameter::new(key, value)) .collect() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/api.rs` around lines 843 - 848, Replace the inline struct literal used in the map with the RunParameter constructor to centralize construction logic: in the closure mapping (where RunParameter { name: key, value, hidden: false } is created) call RunParameter::new(key, value) (or RunParameter::new(key, value, false) if the constructor requires an explicit hidden flag) so future field changes only need updates in the RunParameter::new implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/config/src/towerfile.rs`:
- Around line 122-130: Update the add_parameter method to accept a hidden: bool
argument and use it when constructing the Parameter so callers can create hidden
parameters; specifically change the signature of add_parameter (currently pub fn
add_parameter(&mut self, name: String, description: String, default: String)) to
include hidden: bool and set Parameter { name, description, default, hidden }
accordingly, then update all callers to pass false where they previously relied
on the default non-hidden behavior.
In `@crates/tower-cmd/src/api.rs`:
- Around line 881-886: Replace the inline construction of RunParameter in the
iterator map with the canonical constructor: change the closure that currently
returns RunParameter { name: key, value, hidden: false } to call
RunParameter::new(key, value) so the mapping uses the struct's constructor for
consistency with other call sites (refer to RunParameter::new and the map
producing RunParameter instances).
- Around line 843-848: Replace the inline struct literal used in the map with
the RunParameter constructor to centralize construction logic: in the closure
mapping (where RunParameter { name: key, value, hidden: false } is created) call
RunParameter::new(key, value) (or RunParameter::new(key, value, false) if the
constructor requires an explicit hidden flag) so future field changes only need
updates in the RunParameter::new implementation.
ℹ️ 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 ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/config/src/towerfile.rscrates/tower-api/src/models/parameter.rscrates/tower-api/src/models/run_parameter.rscrates/tower-cmd/src/api.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/output.rscrates/tower-cmd/src/schedules.rscrates/tower/src/lib.rspyproject.tomlsrc/tower/cli.pytests/tower/test_cli.py
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).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/tower-cmd/src/secrets.rs (1)
76-91: Consider clarifying argument precedence in help text.When a user provides both
environment/secret_nameformat and the-eflag (e.g.,tower secrets delete prod/my-secret -e staging), the environment from the slash-separated format takes precedence and the-eflag is silently ignored. This could lead to unexpected deletions.Consider either:
- Adding a warning when both are provided but conflict, or
- Improving the help text to clarify precedence
📝 Suggested help text improvement
.arg( Arg::new("secret_name") .value_parser(value_parser!(String)) .index(1) .required(true) - .help("secret name, or environment/secret_name"), + .help("Secret name, or environment/name (e.g., 'my-secret' or 'prod/my-secret'). If environment is included, the -e flag is ignored."), )🤖 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 76 - 91, The help text and behavior are ambiguous when both the positional "secret_name" contains an environment prefix (e.g., "prod/my-secret") and the "-e/--environment" flag is supplied; update the UX by either (A) making the help text for Arg::new("secret_name") and Arg::new("environment") explicitly state that a slash-separated "environment/secret_name" takes precedence over the -e flag, or (B) add a runtime check after parsing (where you process the parsed "secret_name" and "environment") that detects a conflicting environment in the positional (contains '/') vs the -e flag and emits a clear warning or returns an error; reference the Arg names "secret_name" and "environment" when implementing the check or updating the help strings so callers know which value wins.crates/tower-cmd/src/schedules.rs (2)
177-177: Avoidexpectpanics in command handlers.Line 177 and Line 191 currently panic on missing args. Clap enforces these in normal CLI flow, but handlers are safer and easier to reuse/test when they fail gracefully instead of panicking.
Proposed non-panicking extraction
pub async fn do_update(config: Config, args: &ArgMatches) { - let id_or_name = args.get_one::<String>("id_or_name").expect("id_or_name is required"); + let Some(id_or_name) = args.get_one::<String>("id_or_name") else { + output::die("Missing required argument: id_or_name"); + return; + }; let cron = args.get_one::<String>("cron"); let parameters = parse_parameters(args); output::with_spinner( "Updating schedule", api::update_schedule(&config, id_or_name, cron, parameters), @@ pub async fn do_delete(config: Config, args: &ArgMatches) { - let schedule_id = args.get_one::<String>("schedule_id").expect("schedule_id is required"); + let Some(schedule_id) = args.get_one::<String>("schedule_id") else { + output::die("Missing required argument: schedule_id"); + return; + }; output::with_spinner( "Deleting schedule", api::delete_schedule(&config, schedule_id),Also applies to: 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/schedules.rs` at line 177, Replace the panic-prone args.get_one(...).expect(...) calls by handling the Option and returning a graceful error; specifically, change the extraction of id_or_name (the let id_or_name = args.get_one::<String>("id_or_name").expect(...)) to check args.get_one("id_or_name").ok_or_else(|| /* user-friendly error */) and propagate that error from the command handler (or map it into the handler's Result type); do the same for the other args.get_one call referenced at the same area so the handler returns an Err with a clear message instead of panicking.
204-237: Consider centralizing parameter parsing to avoid drift.This parser is largely duplicated from
crates/tower-cmd/src/run.rs(parse logic and validation messages). A shared helper would reduce maintenance risk and keep behavior consistent.🤖 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 204 - 237, The parameter parsing logic in parse_parameters is duplicated with the one in run.rs; extract this logic into a single shared helper (e.g., parse_key_value_parameters or parse_parameters_shared) in a common module (lib or utils) that takes &ArgMatches and returns Option<HashMap<String,String>>, preserving the current validation and output::error messages for empty key and missing '=' cases; then replace the body of parse_parameters in schedules.rs and the duplicate function in run.rs to call this new shared helper to avoid drift and keep behavior consistent.
🤖 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 82-92: Validate and fail fast when parsing app_name_raw: if
app_name_raw.split_once('#') returns Some((name, num_str)) ensure name is
non-empty and num_str parses to a positive i64 (use output::die with a clear
message on failure), and if the positional run_number flag
(cmd.get_one::<i64>("run_number")) is also supplied treat that as a conflicting
input and call output::die rather than silently preferring the embedded form; in
the else branch ensure the chosen run number from cmd.get_one or
latest_run_number(&config, app_name_raw).await is > 0 and call output::die on
invalid/non-positive values so app_name_raw, split_once, latest_run_number, and
output::die are the clear anchors for the changes.
In `@crates/tower-cmd/src/run.rs`:
- Around line 83-100: The ApiRunError arm re-matches `e` and leaves an
`unreachable!()` panic risk; instead, use the already-bound `source` from
`Error::ApiRunError { ref source }` to decide the path: detect the NOT_FOUND
case as you do, otherwise call `output::tower_error_and_die(source, "Scheduling
run failed")` directly and remove the secondary `if let Error::ApiRunError {
source } = e` and the `unreachable!()`; this simplifies `Error::ApiRunError`
handling and eliminates the panic path.
In `@crates/tower-cmd/src/teams.rs`:
- Line 91: Replace the panic-style expect call when retrieving the team name
with the module's standard error handling: call
args.get_one::<String>("team_name") and if it returns None, invoke output::error
with a descriptive message (e.g., "team_name is required") and then call
std::process::exit(1); otherwise proceed using the obtained value (the `name`
variable). This mirrors the existing pattern used elsewhere in this file (see
other uses of output::error and std::process::exit) and keeps behavior
consistent without panicking.
---
Nitpick comments:
In `@crates/tower-cmd/src/schedules.rs`:
- Line 177: Replace the panic-prone args.get_one(...).expect(...) calls by
handling the Option and returning a graceful error; specifically, change the
extraction of id_or_name (the let id_or_name =
args.get_one::<String>("id_or_name").expect(...)) to check
args.get_one("id_or_name").ok_or_else(|| /* user-friendly error */) and
propagate that error from the command handler (or map it into the handler's
Result type); do the same for the other args.get_one call referenced at the same
area so the handler returns an Err with a clear message instead of panicking.
- Around line 204-237: The parameter parsing logic in parse_parameters is
duplicated with the one in run.rs; extract this logic into a single shared
helper (e.g., parse_key_value_parameters or parse_parameters_shared) in a common
module (lib or utils) that takes &ArgMatches and returns
Option<HashMap<String,String>>, preserving the current validation and
output::error messages for empty key and missing '=' cases; then replace the
body of parse_parameters in schedules.rs and the duplicate function in run.rs to
call this new shared helper to avoid drift and keep behavior consistent.
In `@crates/tower-cmd/src/secrets.rs`:
- Around line 76-91: The help text and behavior are ambiguous when both the
positional "secret_name" contains an environment prefix (e.g., "prod/my-secret")
and the "-e/--environment" flag is supplied; update the UX by either (A) making
the help text for Arg::new("secret_name") and Arg::new("environment") explicitly
state that a slash-separated "environment/secret_name" takes precedence over the
-e flag, or (B) add a runtime check after parsing (where you process the parsed
"secret_name" and "environment") that detects a conflicting environment in the
positional (contains '/') vs the -e flag and emits a clear warning or returns an
error; reference the Arg names "secret_name" and "environment" when implementing
the check or updating the help strings so callers know which value wins.
ℹ️ 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
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tower-cmd/src/mcp.rs
Comment on lines
+82
to
+92
| 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) | ||
| }; |
There was a problem hiding this comment.
Harden logs parsing with upfront input validation.
Line 83–92 can currently accept invalid inputs like #11 (empty app name) and non-positive run numbers (0, -1), and it silently prioritizes app_name#run_number over positional run_number when both are provided. Fail fast here for clearer CLI behavior.
Suggested patch
pub async fn do_logs(config: Config, cmd: &ArgMatches) {
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 has_positional_run = cmd.get_one::<i64>("run_number").is_some();
+ let (name, seq) = if let Some((name, num_str)) = app_name_raw.split_once('#') {
+ if has_positional_run {
+ output::die("Specify run number either as app_name#run_number or as run_number, not both");
+ }
+ if name.trim().is_empty() {
+ output::die("App name cannot be empty");
+ }
let num = num_str.parse::<i64>().unwrap_or_else(|_| output::die("Run number must be a number"));
+ if num <= 0 {
+ output::die("Run number must be greater than 0");
+ }
(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,
};
+ if app_name_raw.trim().is_empty() {
+ output::die("App name cannot be empty");
+ }
+ if num <= 0 {
+ output::die("Run number must be greater than 0");
+ }
(app_name_raw.clone(), num)
};📝 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 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) | |
| }; | |
| let app_name_raw = cmd.get_one::<String>("app_name").expect("app_name is required"); | |
| let has_positional_run = cmd.get_one::<i64>("run_number").is_some(); | |
| let (name, seq) = if let Some((name, num_str)) = app_name_raw.split_once('#') { | |
| if has_positional_run { | |
| output::die("Specify run number either as app_name#run_number or as run_number, not both"); | |
| } | |
| if name.trim().is_empty() { | |
| output::die("App name cannot be empty"); | |
| } | |
| let num = num_str.parse::<i64>().unwrap_or_else(|_| output::die("Run number must be a number")); | |
| if num <= 0 { | |
| output::die("Run number must be greater than 0"); | |
| } | |
| (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, | |
| }; | |
| if app_name_raw.trim().is_empty() { | |
| output::die("App name cannot be empty"); | |
| } | |
| if num <= 0 { | |
| output::die("Run number must be greater than 0"); | |
| } | |
| (app_name_raw.clone(), num) | |
| }; |
🤖 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 82 - 92, Validate and fail fast
when parsing app_name_raw: if app_name_raw.split_once('#') returns Some((name,
num_str)) ensure name is non-empty and num_str parses to a positive i64 (use
output::die with a clear message on failure), and if the positional run_number
flag (cmd.get_one::<i64>("run_number")) is also supplied treat that as a
conflicting input and call output::die rather than silently preferring the
embedded form; in the else branch ensure the chosen run number from cmd.get_one
or latest_run_number(&config, app_name_raw).await is > 0 and call output::die on
invalid/non-positive values so app_name_raw, split_once, latest_run_number, and
output::die are the clear anchors for the changes.
Comment on lines
+83
to
100
| 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!(); | ||
| } |
There was a problem hiding this comment.
Simplify ApiRunError handling and remove the unreachable!() path.
The current branch re-matches e and relies on unreachable!(). If downstream behavior changes, this can become a panic path. Handle source once and dispatch directly.
Proposed simplification
- 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 {
+ 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 {
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");
}📝 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
| 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!(); | |
| } | |
| 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 { | |
| 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); | |
| } | |
| 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 - 100, The ApiRunError arm
re-matches `e` and leaves an `unreachable!()` panic risk; instead, use the
already-bound `source` from `Error::ApiRunError { ref source }` to decide the
path: detect the NOT_FOUND case as you do, otherwise call
`output::tower_error_and_die(source, "Scheduling run failed")` directly and
remove the secondary `if let Error::ApiRunError { source } = e` and the
`unreachable!()`; this simplifies `Error::ApiRunError` handling and eliminates
the panic path.
|
|
||
| pub async fn do_switch(config: Config, args: &ArgMatches) { | ||
| let name = extract_team_name("switch", args.subcommand()); | ||
| let name = args.get_one::<String>("team_name").expect("team_name is required"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check if the file exists and get its size
wc -l crates/tower-cmd/src/teams.rs
# Read the file to see line 91 and surrounding context
cat -n crates/tower-cmd/src/teams.rsRepository: tower/tower-cli
Length of output: 4844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are other uses of expect() in this file to understand patterns
rg -n --type=rust 'expect\(' crates/tower-cmd/src/teams.rs
# Check how do_switch is called - look for the main command dispatcher
rg -n --type=rust -B5 'do_switch' crates/tower-cmd/src/Repository: tower/tower-cli
Length of output: 1053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around the do_switch call in lib.rs to understand the full flow
cat -n crates/tower-cmd/src/lib.rs | sed -n '170,190p'
# Also check if there are other expect() uses in the wider codebase that might show patterns
rg -n --type=rust 'expect\(' crates/tower-cmd/src/*.rs | head -20Repository: tower/tower-cli
Length of output: 3386
Replace expect() with graceful error handling to match file's established patterns.
Line 91 uses expect("team_name is required"), but the rest of this file uses output::error() followed by std::process::exit(1) for error handling (see lines 31-32, 107-108, 114-118). While clap's .required(true) validation prevents this from panicking in normal flow, the defensive pattern used elsewhere in teams.rs should be applied here for consistency.
Proposed fix
- let name = args.get_one::<String>("team_name").expect("team_name is required");
+ let Some(name) = args.get_one::<String>("team_name") else {
+ output::error("Missing required argument: team_name");
+ std::process::exit(1);
+ };📝 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 name = args.get_one::<String>("team_name").expect("team_name is required"); | |
| let Some(name) = args.get_one::<String>("team_name") else { | |
| output::error("Missing required argument: team_name"); | |
| std::process::exit(1); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tower-cmd/src/teams.rs` at line 91, Replace the panic-style expect
call when retrieving the team name with the module's standard error handling:
call args.get_one::<String>("team_name") and if it returns None, invoke
output::error with a descriptive message (e.g., "team_name is required") and
then call std::process::exit(1); otherwise proceed using the obtained value (the
`name` variable). This mirrors the existing pattern used elsewhere in this file
(see other uses of output::error and std::process::exit) and keeps behavior
consistent without panicking.
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
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.
Summary by CodeRabbit
New Features
Improvements
Tests