Conversation
* feat: send X-Tower-Idempotency-Key on deploy Wire up the deploy command to send an idempotency key so consecutive deploys of unchanged source (e.g. to staging then production) collapse to a single AppVersion server-side instead of creating a new version each time. - Auto-populate the key from the git HEAD SHA when the working tree is clean; omit it on a dirty tree so provenance is never misrepresented. - Add --idempotency-key to override detection (useful for CI building outside a checkout) and --no-idempotency-key to opt out on a clean tree. - Print a hint when the server reuses an existing version so the user understands why no new version was created. - Apply the same git auto-detection to the MCP deploy tool. * test: add deploy idempotency key integration coverage BDD regression tests for the X-Tower-Idempotency-Key deploy behavior: explicit --idempotency-key, --no-idempotency-key opt-out, git auto-detect on a clean tree, header omission on a dirty tree, and the reuse hint on a repeat deploy with the same key. The mock API server now records the idempotency key seen on each deploy and reuses a stored (backdated) version when the same key recurs, exposing both via test-only inspection endpoints. * style: black-format deploy idempotency test files * chore: Cleanup to how the reuse hint is presented * chore: Don't really need to check output mode any longer * fix(test): match reuse-hint assertion to the new CLI wording Commit 560872b changed the deploy reuse hint from 'Reusing version ...' to 'No changes since commit ...' and updated the Rust unit test, but the behave step still asserted the old copy, failing the integration suite.
📝 WalkthroughWalkthroughThe PR replaces global CLI output state with ChangesCLI output and command wiring
Storage beta notices
Deploy idempotency
Binary build workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DeployPipeline
participant API
CLI->>DeployPipeline: resolve idempotency key
DeployPipeline->>API: upload package with optional key
API-->>DeployPipeline: return new or cached version
DeployPipeline-->>CLI: display result or reuse hint
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
.github/workflows/build-binaries.yml (1)
373-383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a native container step instead of nested
docker-run-action.GitHub-hosted
ubuntu-24.04-armrunners already provide Docker; runningaddnab/docker-run-actiontoapk add rustand test the wheel duplicates functionality the runner already offers (flagged by zizmor assuperfluous-actions). Acontainer:step (or a plainrun:withdocker run) would be simpler and avoid an extra third-party action dependency. This mirrors the pre-existingmusllinuxjob's pattern, so it's a pre-existing style choice being replicated rather than a new regression.♻️ Illustrative alternative using a job-level container
musllinux-arm-test: needs: musllinux-arm runs-on: ubuntu-24.04-arm container: image: python:${{ matrix.python-version }}-alpine steps: - run: | apk add rust python -m venv .venv .venv/bin/pip3 install dist/*.whl --force-reinstall .venv/bin/${{ env.EXECUTABLE_NAME }} --help🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-binaries.yml around lines 373 - 383, Replace the addnab/docker-run-action step named “Test wheel” for the aarch64 musllinux target with a native container-based or plain docker run approach, matching the existing musllinux job pattern. Preserve the Alpine image, workspace access, Rust installation, virtualenv setup, wheel installation, and executable help check while removing the third-party action dependency.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tower-cmd/src/beta.rs`:
- Line 26: Update the user-facing STORAGE_BETA_MESSAGE constant to correct the
misspelled “featues” to “features,” preserving the rest of the beta notice
unchanged.
In `@crates/tower-cmd/src/deploy.rs`:
- Around line 253-258: Update the reuse hint message in the deployment flow to
avoid assuming sent_key is a git commit; use neutral wording that identifies it
as the key from the last deploy while preserving the key value and deployment
date.
In `@crates/tower-cmd/src/output.rs`:
- Around line 118-128: Update the non-JSON branch of muted so the dimmed message
passed to write includes a terminating newline, while preserving the existing
JSON behavior and styling.
---
Nitpick comments:
In @.github/workflows/build-binaries.yml:
- Around line 373-383: Replace the addnab/docker-run-action step named “Test
wheel” for the aarch64 musllinux target with a native container-based or plain
docker run approach, matching the existing musllinux job pattern. Preserve the
Alpine image, workspace access, Rust installation, virtualenv setup, wheel
installation, and executable help check while removing the third-party action
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fa4df15-12cb-45a3-ab2b-2826f04efd03
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/build-binaries.ymlcrates/config/Cargo.tomlcrates/config/src/lib.rscrates/config/src/session.rscrates/tower-cmd/src/beta.rscrates/tower-cmd/src/catalogs.rscrates/tower-cmd/src/deploy.rscrates/tower-cmd/src/lib.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/output.rscrates/tower-cmd/src/util/deploy.rscrates/tower-cmd/src/util/git.rscrates/tower-cmd/src/util/mod.rsplugin/skills/tower/SKILL.mdtests/integration/features/cli_deploy_idempotency.featuretests/integration/features/steps/cli_steps.pytests/mock-api-server/main.py
| } | ||
| } | ||
|
|
||
| pub(crate) const STORAGE_BETA_MESSAGE: &str = "Tower Storage is in beta. Core functionality is stable, but some featues and interfaces might change before general availability."; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the typo in the beta notice.
The user-facing message says “featues”; change it to “features” before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/beta.rs` at line 26, Update the user-facing
STORAGE_BETA_MESSAGE constant to correct the misspelled “featues” to “features,”
preserving the rest of the beta notice unchanged.
Comment on lines
+253
to
+258
| Some(format!( | ||
| "No changes since commit {} (deployed on {})", | ||
| sent_key, | ||
| util::dates::format(created_at), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reuse hint wording assumes the key is a git commit.
"No changes since commit {sent_key} ..." is accurate when the key was auto-detected from git, but misleading when the user supplied an arbitrary value via --idempotency-key. Consider more neutral wording, e.g. "No changes since the last deploy with key {sent_key}", or branch the message based on whether the key came from git.
💬 Suggested tweak
Some(format!(
- "No changes since commit {} (deployed on {})",
+ "No changes since the last deploy with key `{}` (deployed on {})",
sent_key,
util::dates::format(created_at),
))📝 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
| Some(format!( | |
| "No changes since commit {} (deployed on {})", | |
| sent_key, | |
| util::dates::format(created_at), | |
| )) | |
| } | |
| Some(format!( | |
| "No changes since the last deploy with key `{}` (deployed on {})", | |
| sent_key, | |
| util::dates::format(created_at), | |
| )) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/deploy.rs` around lines 253 - 258, Update the reuse hint
message in the deployment flow to avoid assuming sent_key is a git commit; use
neutral wording that identifies it as the key from the last deploy while
preserving the key value and deployment date.
* refactor(cli): replace the output globals with an explicit `Out` Where CLI output went was controlled by two mutable globals: an OUTPUT_MODE and a CURRENT_SENDER that the MCP server set and cleared around each tool call. Every output:: function read them to choose between stdout, JSON, and the MCP peer, so a command never said where its own output went. Pass the destination in as a value instead: commands take an Out and the writing functions become methods on it. The CLI builds one over stdout; an MCP tool builds one whose writer forwards each line to that call's channel. The sender and the notification flag live on the value, not in globals. * chore: rustfmt util/git.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/tower-cmd/src/apps.rs (1)
407-412: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate error message across fatal/non-fatal branches.
out.error(&format!("Failed to stream run logs: {}", err))is emitted identically in both the fatal (return) and non-fatal (continue+backoff) branches. Hoisting it above theifavoids the duplication.♻️ Proposed refactor
Err(err) => { + out.error(&format!("Failed to stream run logs: {}", err)); if is_fatal_stream_error(&err) { - out.error(&format!("Failed to stream run logs: {}", err)); return; } - out.error(&format!("Failed to stream run logs: {}", err)); sleep(backoff).await; backoff = next_backoff(backoff); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tower-cmd/src/apps.rs` around lines 407 - 412, In the error handling branch for the stream log operation, hoist the shared `out.error` call above the `is_fatal_stream_error(&err)` check so the message is emitted once. Preserve the fatal branch’s immediate return and the non-fatal branch’s existing continue/backoff behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tower-cmd/src/apps.rs`:
- Around line 120-148: Update do_logs so the non-follow describe_run_logs call
does not discard Err results; route failures through the module’s established
output error-handling path, while preserving the existing log-line iteration for
successful responses and the follow_logs branch.
In `@crates/tower-cmd/src/mcp.rs`:
- Around line 814-817: Update the deployment flow around deploy_from_dir and
do_deploy_package to preserve the returned deployment outcome instead of sending
output to Out::sink(). Propagate the reuse result through the MCP tool response
so reused deployments include the idempotency reuse hint rather than always
returning the generic success message.
In `@crates/tower-cmd/src/output.rs`:
- Around line 191-353: Update package_error, config_error, and
tower_error_and_die so --json requests emit exactly one structured error through
the Format/Out JSON path before exiting, rather than human-readable text or
multiple documents. Route usage errors occurring before Out initialization to
stderr, while preserving existing human-readable behavior for non-JSON output
and authentication-specific messaging.
---
Nitpick comments:
In `@crates/tower-cmd/src/apps.rs`:
- Around line 407-412: In the error handling branch for the stream log
operation, hoist the shared `out.error` call above the
`is_fatal_stream_error(&err)` check so the message is emitted once. Preserve the
fatal branch’s immediate return and the non-fatal branch’s existing
continue/backoff behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ddf2a17-c402-4b7f-b64b-456bfc8d6f8c
📒 Files selected for processing (19)
crates/tower-cmd/src/apps.rscrates/tower-cmd/src/beta.rscrates/tower-cmd/src/catalogs.rscrates/tower-cmd/src/deploy.rscrates/tower-cmd/src/environments.rscrates/tower-cmd/src/lib.rscrates/tower-cmd/src/mcp.rscrates/tower-cmd/src/output.rscrates/tower-cmd/src/package.rscrates/tower-cmd/src/run.rscrates/tower-cmd/src/schedules.rscrates/tower-cmd/src/secrets.rscrates/tower-cmd/src/session.rscrates/tower-cmd/src/teams.rscrates/tower-cmd/src/util/apps.rscrates/tower-cmd/src/util/cmd.rscrates/tower-cmd/src/util/deploy.rscrates/tower-cmd/src/util/git.rscrates/tower-cmd/src/version.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/tower-cmd/src/util/deploy.rs
- crates/tower-cmd/src/beta.rs
- crates/tower-cmd/src/util/git.rs
- crates/tower-cmd/src/deploy.rs
Comment on lines
+120
to
148
| pub async fn do_logs(out: &output::Out, 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 num = num_str | ||
| .parse::<i64>() | ||
| .unwrap_or_else(|_| output::die("Run number must be a number")); | ||
| .unwrap_or_else(|_| out.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, | ||
| None => latest_run_number(out, &config, app_name_raw).await, | ||
| }; | ||
| (app_name_raw.clone(), num) | ||
| }; | ||
| let follow = cmd.get_one::<bool>("follow").copied().unwrap_or(false); | ||
|
|
||
| if follow { | ||
| follow_logs(config, name, seq).await; | ||
| follow_logs(out, config, name, seq).await; | ||
| return; | ||
| } | ||
|
|
||
| if let Ok(resp) = api::describe_run_logs(&config, &name, seq).await { | ||
| for line in resp.log_lines { | ||
| output::remote_log_event(&line); | ||
| out.remote_log_event(&line); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure and find relevant functions
ast-grep outline crates/tower-cmd/src/apps.rs --view expanded || true
echo '--- grep for key functions / patterns ---'
rg -n "do_logs|follow_logs|latest_run_number|tower_error_and_die|if let Ok\\(resp\\)" crates/tower-cmd/src/apps.rs
echo '--- relevant sections around do_logs ---'
sed -n '110,170p' crates/tower-cmd/src/apps.rs
echo '--- relevant sections around latest_run_number ---'
sed -n '250,340p' crates/tower-cmd/src/apps.rs
echo '--- relevant sections around follow_logs ---'
sed -n '340,430p' crates/tower-cmd/src/apps.rsRepository: tower/tower-cli
Length of output: 12864
Surface describe_run_logs errors in do_logs.
The non---follow branch drops Err from api::describe_run_logs, so a fetch failure exits cleanly with no output. Use the same error-handling path as the rest of this module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/apps.rs` around lines 120 - 148, Update do_logs so the
non-follow describe_run_logs call does not discard Err results; route failures
through the module’s established output error-handling path, while preserving
the existing log-line iteration for successful responses and the follow_logs
branch.
Comment on lines
+814
to
+817
| // The tool builds its own result message, so the deploy's own progress is discarded. | ||
| let out = crate::output::Out::sink(); | ||
| match deploy::deploy_from_dir( | ||
| &out, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the deployment reuse result.
Line 815 discards deploy_from_dir output, including deploy::do_deploy_package’s idempotency reuse hint. MCP therefore always returns “Deploy completed successfully” even when the server reused an existing version. Capture or return the deployment outcome and include it in the tool result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/mcp.rs` around lines 814 - 817, Update the deployment
flow around deploy_from_dir and do_deploy_package to preserve the returned
deployment outcome instead of sending output to Out::sink(). Propagate the reuse
result through the MCP tool response so reused deployments include the
idempotency reuse hint rather than always returning the generic success message.
Comment on lines
+191
to
353
| pub fn package_error(&self, err: tower_package::Error) { | ||
| let msg = match err { | ||
| tower_package::Error::NoManifest => "No manifest was found".to_string(), | ||
| tower_package::Error::InvalidManifest => { | ||
| "Invalid manifest was found or created".to_string() | ||
| } | ||
| tower_package::Error::InvalidPath => { | ||
| "There was a problem determining exactly where your Towerfile was stored on disk" | ||
| .to_string() | ||
| } | ||
| tower_package::Error::InvalidGlob { message } => { | ||
| format!("Invalid file glob pattern: {}", message) | ||
| } | ||
| tower_package::Error::InvalidTowerfile { message } => { | ||
| format!("Invalid Towerfile: {}", message) | ||
| } | ||
| tower_package::Error::MissingTowerfile => { | ||
| "No Towerfile was found in the target directory".to_string() | ||
| } | ||
| tower_package::Error::MissingRequiredAppField { field } => { | ||
| format!("Missing required app field `{}` in Towerfile", field) | ||
| } | ||
| tower_package::Error::Io { source } => format!("IO error: {}", source), | ||
| tower_package::Error::MissingScript { script } => { | ||
| format!("Script '{}' not found. Check that the 'script' field in your Towerfile points to a file that exists in your project.", script) | ||
| } | ||
| }; | ||
|
|
||
| pub fn title(text: &str) -> String { | ||
| text.bold().green().to_string() | ||
| } | ||
| let line = format!("{} {}\n", "Package error:".red(), msg); | ||
| self.write(&line); | ||
| } | ||
|
|
||
| pub fn placeholder(text: &str) -> String { | ||
| text.white().dimmed().italic().to_string() | ||
| } | ||
| pub fn config_error(&self, err: config::Error) { | ||
| let msg = match err { | ||
| config::Error::ConfigDirNotFound => "Config directory not found".to_string(), | ||
| config::Error::NoHomeDir => "No home directory found".to_string(), | ||
| config::Error::Io { ref source } => format!("IO error: {}", source), | ||
| config::Error::NoSession => "No session".to_string(), | ||
| config::Error::TeamNotFound { ref team_name } => { | ||
| format!("Team with name `{}` not found!", team_name) | ||
| } | ||
| config::Error::UnknownDescribeSessionValue { value: _ } => { | ||
| "An error occured while describing the session associated with the JWT you provided. Maybe your CLI is out of date?".to_string() | ||
| } | ||
| config::Error::DescribeSessionError { ref err } => { | ||
| format!("An error occured while describing the session associated with the JWT you provided: {}", err) | ||
| } | ||
| }; | ||
|
|
||
| pub fn paragraph(msg: &str) -> String { | ||
| msg.chars() | ||
| .collect::<Vec<char>>() | ||
| .chunks(78) | ||
| .map(|c| c.iter().collect::<String>()) | ||
| .map(|li| format!(" {}", li)) | ||
| .collect::<Vec<String>>() | ||
| .join("\n") | ||
| } | ||
| let line = format!("{} {}\n", "Config error:".red(), msg); | ||
| self.write(&line); | ||
| } | ||
|
|
||
| pub fn config_error(err: config::Error) { | ||
| let msg = match err { | ||
| config::Error::ConfigDirNotFound => "Config directory not found".to_string(), | ||
| config::Error::NoHomeDir => "No home directory found".to_string(), | ||
| config::Error::Io { ref source } => format!("IO error: {}", source), | ||
| config::Error::NoSession => "No session".to_string(), | ||
| config::Error::TeamNotFound { ref team_name } => { | ||
| format!("Team with name `{}` not found!", team_name) | ||
| } | ||
| config::Error::UnknownDescribeSessionValue { value: _ } => { | ||
| "An error occured while describing the session associated with the JWT you provided. Maybe your CLI is out of date?".to_string() | ||
| } | ||
| config::Error::DescribeSessionError { ref err } => { | ||
| format!("An error occured while describing the session associated with the JWT you provided: {}", err) | ||
| // Outputs both the model.detail and the model.errors fields in a human readable format. | ||
| fn output_full_error_details(&self, model: &ErrorModel) { | ||
| // Show the main detail message if available | ||
| if let Some(detail) = &model.detail { | ||
| self.write(&format!("\n{}\n", "Error details:".yellow())); | ||
| self.write(&format!("{}\n", detail.red())); | ||
| } | ||
|
|
||
| // Show any additional error details from the errors field | ||
| if let Some(errors) = &model.errors { | ||
| if !errors.is_empty() { | ||
| if model.detail.is_none() { | ||
| self.write(&format!("\n{}\n", "Error details:".yellow())); | ||
| } | ||
| for error in errors { | ||
| let msg = format!( | ||
| " • {}", | ||
| error.message.as_deref().unwrap_or("Unknown error") | ||
| ); | ||
| self.write(&format!("{}\n", msg.red())); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| let line = format!("{} {}\n", "Config error:".red(), msg); | ||
| write(&line); | ||
| } | ||
| fn output_response_content_error<T>(&self, err: ResponseContent<T>) { | ||
| // Attempt to deserialize the error content into an ErrorModel. | ||
| let error_model = match serde_json::from_str::<ErrorModel>(&err.content) { | ||
| Ok(model) => { | ||
| debug!("Error model (status: {}): {:?}", err.status, model); | ||
| model | ||
| } | ||
| Err(e) => { | ||
| debug!("Failed to parse error content as JSON: {}", e); | ||
| debug!("Raw error content: {}", err.content); | ||
| // Show the raw error content if JSON parsing fails | ||
| self.write(&format!("\n{}\n", "API Error:".yellow())); | ||
| self.write(&format!("{}\n", err.content.red())); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| pub fn write(msg: &str) { | ||
| if get_output_mode().is_mcp() { | ||
| let clean_msg = msg.trim_end().to_string(); | ||
| send_to_current_sender(clean_msg); | ||
| } else { | ||
| write_to_stdout(msg); | ||
| match err.status { | ||
| StatusCode::CONFLICT => { | ||
| self.output_full_error_details(&error_model); | ||
| } | ||
| StatusCode::UNPROCESSABLE_ENTITY => { | ||
| self.output_full_error_details(&error_model); | ||
| } | ||
| StatusCode::INTERNAL_SERVER_ERROR => { | ||
| self.error( | ||
| "The Tower API encountered an internal error. Maybe try again later on.", | ||
| ); | ||
| } | ||
| StatusCode::NOT_FOUND => { | ||
| self.output_full_error_details(&error_model); | ||
| } | ||
| StatusCode::UNAUTHORIZED => { | ||
| self.error( | ||
| "You aren't authorized to do that! Are you logged in? Run `tower login` to login.", | ||
| ); | ||
| } | ||
| _ => { | ||
| if error_model.detail.is_none() && error_model.errors.is_none() { | ||
| self.error("The Tower API returned an error that the Tower CLI doesn't know what to do with! Maybe try again in a bit."); | ||
| } | ||
| self.output_full_error_details(&error_model); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn error(msg: &str) { | ||
| if get_output_mode().is_json() { | ||
| let response = serde_json::json!({ | ||
| "result": "error", | ||
| "message": msg | ||
| }); | ||
| json(&response); | ||
| } else { | ||
| let line = format!("{} {}\n", "Oh no!".red(), msg); | ||
| write(&line); | ||
| fn tower_error<T>(&self, err: ApiError<T>) { | ||
| match err { | ||
| ApiError::ResponseError(resp) => { | ||
| self.output_response_content_error(resp); | ||
| } | ||
| ApiError::Reqwest(e) => { | ||
| debug!("Reqwest error: {:?}", e); | ||
| self.error("The Tower CLI wasn't able to talk to the Tower API! Are you offline? Try again later."); | ||
| } | ||
| ApiError::Serde(e) => { | ||
| debug!("Serde error: {:?}", e); | ||
| self.error("The Tower API returned something that the Tower CLI didn't understand. Maybe you need to upgrade Tower CLI?"); | ||
| } | ||
| ApiError::Io(e) => { | ||
| debug!("Io error: {:?}", e); | ||
| self.error("An error happened while talking to the Tower API. You can try that again in a bit."); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn runtime_error(err: tower_runtime::errors::Error) { | ||
| let line = format!("{} {}\n", "Runtime Error:".red(), err.to_string()); | ||
| write(&line); | ||
| } | ||
| /// Handles Tower API errors with context-specific authentication messages. | ||
| /// If the error is a 401 Unauthorized, provides a helpful message mentioning | ||
| /// the operation that failed and suggests running 'tower login'. | ||
| /// Always exits the process with error code 1. | ||
| pub fn tower_error_and_die<T>(&self, err: ApiError<T>, operation: &str) -> ! { | ||
| // Check if this is an authentication error | ||
| if let ApiError::ResponseError(ref resp) = err { | ||
| if resp.status == StatusCode::UNAUTHORIZED { | ||
| self.die(&format!( | ||
| "{} because you are not logged into Tower. Please run 'tower login' first.", | ||
| operation | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| // Outputs both the model.detail and the model.errors fields in a human readable format. | ||
| pub fn output_full_error_details(model: &ErrorModel) { | ||
| // Show the main detail message if available | ||
| if let Some(detail) = &model.detail { | ||
| write(&format!("\n{}\n", "Error details:".yellow())); | ||
| write(&format!("{}\n", detail.red())); | ||
| // Show the detailed error first | ||
| self.tower_error(err); | ||
| self.die(operation); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve a single parseable JSON error response.
These paths bypass Format, so API/package/config failures under --json emit human text or multiple documents. Render one structured error before exiting; send pre-Out usage errors to stderr.
Also applies to: 423-431, 603-611
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tower-cmd/src/output.rs` around lines 191 - 353, Update package_error,
config_error, and tower_error_and_die so --json requests emit exactly one
structured error through the Format/Out JSON path before exiting, rather than
human-readable text or multiple documents. Route usage errors occurring before
Out initialization to stderr, while preserving existing human-readable behavior
for non-JSON output and authentication-specific messaging.
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
4 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.
Summary by CodeRabbit
--idempotency-key,--no-idempotency-key, and auto-use of the current git commit SHA when the tree is clean.tower deployCLI reference with the new idempotency options.