fix: non-zero exit code when a workflow run ends failed or aborted#2959
fix: non-zero exit code when a workflow run ends failed or aborted#2959doquanghuy wants to merge 1 commit into
Conversation
workflow run and workflow resume printed Status: failed (or emitted the --json payload) and exited 0, so scripts and CI could not rely on the process exit code. Map terminal outcomes: failed|aborted -> 1, completed|paused -> 0, on both the text and --json paths. The previous exit-0-on-failed behavior was pinned by test_workflow_run_failing_yaml_without_project; the pin is updated to the new contract. Fixes github#2958
|
@mnriem when you have a moment, would appreciate a review — happy to adjust anything. |
There was a problem hiding this comment.
⚠️ Not ready to approve
The new tests use run: "true" / run: "false" which will fail on Windows under shell=True, and the resume exit-code behavior changed by this PR is not covered by an end-to-end test.
Pull request overview
This PR updates the Specify CLI workflow commands so workflow run / workflow resume return a non-zero process exit code when the workflow outcome ends in a terminal failure (failed / aborted), making the CLI easier to use from scripts and CI without parsing stdout.
Changes:
- Added a single status→exit-code helper (
failed/aborted→1,completed/paused→0). - Applied the mapping to the epilogues of
workflow runandworkflow resumefor both text output and--json. - Updated/added tests to reflect and validate the new exit-code contract.
File summaries
| File | Description |
|---|---|
src/specify_cli/__init__.py |
Introduces _run_outcome_exit_code() and uses it to exit with the workflow outcome’s status code for run/resume (text + --json). |
tests/test_workflows.py |
Adds new CLI-level exit-code tests for workflow run (but currently uses non-portable shell commands). |
tests/test_workflow_run_without_project.py |
Updates an existing pinned assertion to expect exit code 1 for a failed workflow run. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| steps: | ||
| - id: fine | ||
| type: shell | ||
| run: "true" |
| steps: | ||
| - id: boom | ||
| type: shell | ||
| run: "false" |
| class TestWorkflowRunExitCodes: | ||
| """CLI-level tests for the run/resume process exit codes.""" | ||
|
|
Description
Fixes #2958.
workflow runandworkflow resumeended with exit code 0 even when the run finishedStatus: failed, so scripts/CI had to parse stdout to detect failure. This PR adds a single status→exit-code mapping (_run_outcome_exit_code:failed/aborted→ 1;completed/paused→ 0 — paused is a waiting state, not a failure) and applies it at the four epilogues (run/resume × text/--json). The--jsonpayload itself is unchanged — it still prints first; only the process exit code changes.Behavior change, flagged plainly: the old behavior was deliberately pinned by
tests/test_workflow_run_without_project.py::test_workflow_run_failing_yaml_without_project(exit_code == 0asserted on a failed run). That pin is updated to the new contract with an explanatory comment. Anyone scripting against exit-0-on-failed would be affected — the issue discusses the trade-off; happy to gate this behind a flag instead if you'd rather not change the default.Testing
uv sync && uv run pytest— full suite 3728 passedTestWorkflowRunExitCodes(3 CLI-level tests): failed→1 (text +--json), completed→0; the two failure-path tests are red against currentmain, green with the fix (verified both directions)uvx ruff check src/— cleanuv run specify --helpCliRunner)AI Disclosure
Code, tests, and this description were authored with AI assistance (Claude); verified by running the repo's test suite and ruff locally in both red (unfixed) and green (fixed) directions.