Skip to content

fix(cli): exit non-zero when configure receives invalid API URL - #3356

Open
Alan5168 wants to merge 1 commit into
vectorize-io:mainfrom
Alan5168:fix/cli-configure-url-exit-code
Open

fix(cli): exit non-zero when configure receives invalid API URL#3356
Alan5168 wants to merge 1 commit into
vectorize-io:mainfrom
Alan5168:fix/cli-configure-url-exit-code

Conversation

@Alan5168

Copy link
Copy Markdown
Contributor

Summary

hindsight configure --api-url <invalid> prints an error message but returns exit code 0. Scripts and CI pipelines relying on the exit status cannot detect the failure.

Root cause

The URL validation block in handle_configure() calls ui::print_error! and then return Ok(()), so the error is logged but the process exits successfully.

Fix

Replace the silent success with anyhow::bail!, which lets the top-level error handler print the message and exit with status 1 — consistent with every other error path in the CLI.

Tests

Added two integration tests in cli_profile.rs:

  • configure_rejects_invalid_url_with_nonzero_exit — verifies exit code != 0 and error message on stderr for a URL missing the scheme
  • configure_accepts_valid_http_and_https_urls — regression: both http:// and https:// URLs still save successfully

All 13 tests in cli_profile pass:

cargo test --manifest-path hindsight-cli/Cargo.toml --test cli_profile
test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Scope

Only touches the configure command's URL validation. No change to URL parsing logic, config saving, or any other command.

…d API URL

Previously, `hindsight configure --api-url <invalid>` printed an error
but returned exit code 0 because the validation block called
`print_error!` followed by `return Ok(())`. Scripts and CI pipelines
relying on the exit code could not detect the failure.

Replace the silent success with `anyhow::bail!` so the top-level error
handler prints the message and exits with status 1, matching the
behavior of every other CLI error path.

Add two integration tests:
- configure rejects URLs missing http(s):// with non-zero exit
- configure accepts both http:// and https:// URLs

@koriyoshi2041 koriyoshi2041 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at 924aa3b1. Propagating the validation failure through anyhow::bail! gives the CLI the expected non-zero status while keeping the error on stderr. The added tests cover both rejection and the existing HTTP/HTTPS success paths. Local cargo test --test cli_profile passed, and git diff --check origin/main...HEAD is clean.

@handnewb handnewb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Small focused fix — CLI should exit non-zero on configuration error, not silently return 0. Good test coverage with the new cli_profile test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants