Skip to content

fix(cli): exit non-zero on red error paths - #166

Merged
Paul-Kyle merged 1 commit into
phasespace-labs:mainfrom
usmamalik345:fix/164-cli-error-exit-codes
Aug 30, 2026
Merged

fix(cli): exit non-zero on red error paths#166
Paul-Kyle merged 1 commit into
phasespace-labs:mainfrom
usmamalik345:fix/164-cli-error-exit-codes

Conversation

@usmamalik345

Copy link
Copy Markdown
Contributor

Summary

  • CLI red-error handlers that previously printed and returned (exit 0) now raise SystemExit(1), matching the house idiom in review.py.
  • Covered the handlers listed in fix(cli): error handlers across nine files print and return, leaving exit code 0 #164 (return-paths and fall-offs).
  • Left alone: yellow "reindex already running" (HTTP 409) — informational, not a hard failure.
  • Added regression tests patterned on tests/test_cli_stop.py.

Fixes #164

Test plan

  • pytest tests/test_cli_error_exit_codes.py tests/test_cli_stop.py (17 passed)
  • Spot-check: force an API failure on palinode depends --unblocked / palinode trace … and confirm non-zero exit

Assisted-by: Cursor

CLI handlers that printed a red error and then returned left the process
at exit code 0, so automation could not detect failure. Raise
SystemExit(1) on those paths (house idiom). Keep the yellow
reindex-already-running HTTP 409 path as exit 0.

Fixes phasespace-labs#164

Co-authored-by: Cursor <cursoragent@cursor.com>
@Paul-Kyle

Copy link
Copy Markdown
Member

Complete, and the part I want to call out is the 409. The issue said that sweep was a floor rather than a proof and asked you to judge each site — you left "reindex already running" at exit 0, pinned it with a test, and said why in both the issue and the PR instead of leaving it to be found in the diff. That is the whole reason it was help wanted and not a first issue.

Also right, and easy to get wrong: the raise in depends.py is inside the except rather than at the return below it, which would have caught the success path too. And you left the ten bare click.Abort() calls alone — those are #162's, and staying out of them keeps whoever takes it from rebasing.

Merging. Thanks for a strong first contribution.

@Paul-Kyle
Paul-Kyle merged commit 5ead732 into phasespace-labs:main Aug 30, 2026
9 checks passed
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.

fix(cli): error handlers across nine files print and return, leaving exit code 0

3 participants