diff --git a/.ai/cli/exit-codes-spec.md b/.ai/cli/exit-codes-spec.md new file mode 100644 index 00000000..e22d288b --- /dev/null +++ b/.ai/cli/exit-codes-spec.md @@ -0,0 +1,83 @@ +# CLI Exit Code Propagation Specification + +## Overview + +Reported from real-data testing: on Linux, `reprostim bids-inject` did not +return a non-zero process exit code on failure, even though its underlying +`do_main()` returned a non-zero result. Callers relying on `$?` (cron jobs, +`datalad containers-run`, shell wrappers such as +`code/reprostim-bids-inject`) could not detect failures. + +## Root cause + +Click's `BaseCommand.main()` runs in `standalone_mode=True` by default (this +is what the `reprostim` console-script entry point — and `python -m +reprostim`, both routing through `cli/entrypoint.py::main` — actually use). +In that mode, `main()` invokes the command callback, captures its return +value, but **discards it**: unless the callback raises an exception or +explicitly calls `ctx.exit(code)` (or `sys.exit(code)`), Click calls +`ctx.exit()` with no argument at the end, which defaults to code `0`. + +Concretely: a `@click.command()` callback that ends with `return res` — where +`res` is a non-zero int meant to signal failure — has **no effect on the +process exit code**. The process always exits `0` in standalone mode unless +an exception propagates out of the callback. Verified empirically with a +minimal reproduction (`return 42` from a bare Click command still yields +shell `$? == 0`). + +The correct pattern is `ctx.exit(res)`, which raises `click.exceptions.Exit` +— an exception Click's `main()` explicitly catches and turns into +`sys.exit(e.exit_code)`. + +## Audit results across `src/reprostim/cli/` + +| Command module | Before | After | +|---|---|---| +| `cmd_bids_inject.py` | `return res` | `ctx.exit(res)` | +| `cmd_list_displays.py` | `return res` | `ctx.exit(res)` | +| `cmd_monitor_displays.py` | `return res` | `ctx.exit(res)` | +| `cmd_qr_parse.py` | `return res` | `ctx.exit(res)` | +| `cmd_split_video.py` | `return res` | `ctx.exit(res)` | +| `cmd_timesync_stimuli.py` | `return -1` (on `do_init` failure) and `return res` (final) | `ctx.exit(1)` and `ctx.exit(res)` | +| `cmd_bids_inject_sidecar.py` | already `ctx.exit(res)` | unchanged | +| `cmd_video_audit.py` | already `ctx.exit(1)` / `ctx.exit(rv)` | unchanged | +| `cmd_detect_noscreen.py` | `_main_exit()` helper already calls `sys.exit(code)` directly (the `return code` after it is dead code, but harmless — `sys.exit` raises before it's reached) | unchanged | +| `cmd_echo.py` | no error path (always succeeds) | unchanged | + +All 6 affected commands share the same architectural pattern: the Click +callback lazily imports and calls a `do_main(...)`-style function from the +corresponding implementation module, gets back an `int` result, and must +propagate it as the process exit code. + +`cmd_timesync_stimuli.py` also had a latent secondary bug on the same error +path: `logger.error()` was called with no message argument, which raises +`TypeError` (stdlib `logging.Logger.error` requires a `msg` positional arg) +instead of logging and returning cleanly. Fixed alongside the exit-code fix +since it's on the exact path being corrected: `logger.error("do_init(...) +failed")`. + +## Known non-bug / out of scope + +`cmd_list_displays.py` and `cmd_monitor_displays.py` compute `res: int = 0` +as a hardcoded local — `do_list_displays()`/`do_monitor_displays()` in +`capture/disp_mon.py` don't return a status at all (implicit `None`). So +today these two commands can never actually produce a non-zero `res` via the +`ctx.exit(res)` path; a failure inside `do_list_displays`/`do_monitor_displays` +still surfaces correctly as a non-zero exit only because an *uncaught +exception* propagates out of the callback (unrelated to the `return res` vs +`ctx.exit(res)` bug — exceptions always worked). The `ctx.exit(res)` fix is +applied for consistency and to be forward-compatible if these functions ever +gain a real return-code contract, but wiring an actual result code through +`disp_mon.py` is out of scope for this fix. + +## Verification approach + +Regular unit tests that call `do_main()` directly (bypassing Click) cannot +catch this class of bug — `do_main()` genuinely returns the right int; the +bug is entirely in how the Click callback wrapper handles that int. Tests +must go through `click.testing.CliRunner().invoke(cmd, args)` and assert on +`result.exit_code`, with the underlying `do_main` (or `do_init`) mocked to +return a specific non-zero value, to prove the value actually reaches the +process exit code. This was the exact gap found in the existing test suite: +every existing `CliRunner`-based test mocked `do_main` with `return_value=0` +only — none exercised the non-zero path through the CLI layer. diff --git a/.ai/cli/exit-codes-tasks.md b/.ai/cli/exit-codes-tasks.md new file mode 100644 index 00000000..1c1902fc --- /dev/null +++ b/.ai/cli/exit-codes-tasks.md @@ -0,0 +1,51 @@ +# CLI Exit Code Propagation Task List + +Tracks implementation progress against [exit-codes-spec.md](exit-codes-spec.md). + +--- + +## Fixes + +- [x] `src/reprostim/cli/cmd_bids_inject.py` — `return res` → `ctx.exit(res)` +- [x] `src/reprostim/cli/cmd_list_displays.py` — `return res` → `ctx.exit(res)` +- [x] `src/reprostim/cli/cmd_monitor_displays.py` — `return res` → `ctx.exit(res)` +- [x] `src/reprostim/cli/cmd_qr_parse.py` — `return res` → `ctx.exit(res)` +- [x] `src/reprostim/cli/cmd_split_video.py` — `return res` → `ctx.exit(res)` +- [x] `src/reprostim/cli/cmd_timesync_stimuli.py` — `return -1` → `ctx.exit(1)` (`do_init` + failure path) and `return res` → `ctx.exit(res)` (final); also fixed the adjacent + `logger.error()` call (was missing its required `msg` argument, which raised `TypeError` + instead of logging) +- [x] Confirmed already correct, no change needed: `cmd_bids_inject_sidecar.py` + (`ctx.exit(res)`), `cmd_video_audit.py` (`ctx.exit(1)` / `ctx.exit(rv)`), + `cmd_detect_noscreen.py` (`_main_exit()` helper calls `sys.exit(code)` directly), + `cmd_echo.py` (no error path) + +## Tests + +- [x] `tests/qr/test_parse.py::test_cli_nonzero_do_main_result_propagated_to_exit_code` — + mocks `do_main` to return `7`, asserts `CliRunner` `result.exit_code == 7` +- [x] `tests/video/test_split.py::test_cli_nonzero_do_main_result_propagated_to_exit_code` — + mocks `do_main` to return `(5, [])`, asserts `result.exit_code == 5` +- [x] `tests/bids/test_inject.py` — new CLI test section added (none existed before, despite + this being the module where the bug was found in production): + `test_cli_help_renders_without_error`, `test_cli_missing_videos_option_nonzero_exit`, + `test_cli_nonzero_do_main_result_propagated_to_exit_code` (mocks `do_main` → `3`, asserts + `result.exit_code == 3`), `test_cli_zero_do_main_result_exits_zero` +- [x] `tests/cli/` package created (no CLI-level tests existed at all for these 3 commands): + - [x] `test_cmd_list_displays.py` — `--help`, success (mocked `do_list_displays`), option + forwarding, exception-from-implementation still exits non-zero + - [x] `test_cmd_monitor_displays.py` — same shape as above for `do_monitor_displays` + - [x] `test_cmd_timesync_stimuli.py` — `--help`, + `test_cli_do_init_failure_exits_nonzero` (regression test for the `return -1` bug), + `test_cli_nonzero_do_main_result_propagated_to_exit_code` (regression test for the + `return res` bug), baseline success case +- [x] Full suite run (`pytest tests/`): 733 passed, 1 pre-existing unrelated failure + (`tests/qr/test_parse.py::test_do_main_qrdet_missing_packages_returns_error` — a + `torch.overrides` double-docstring `RuntimeError` in this environment; confirmed via + `git stash` that it fails identically on unmodified code, unrelated to this change) + +## Docs + +- [x] `.ai/cli/exit-codes-spec.md` created +- [x] `.ai/cli/exit-codes-tasks.md` created (this file) +- [x] `.ai/context.md` — cli/ section updated to link this spec diff --git a/.ai/context.md b/.ai/context.md index 5ccbfa24..c6e535c4 100644 --- a/.ai/context.md +++ b/.ai/context.md @@ -30,7 +30,10 @@ BIDS Integration & Documentation Main Python package with CLI tools and analysis utilities. **Key Modules:** -- **cli/** - Command-line interface (Click-based with DYMGroup for suggestions) +- **cli/** - Command-line interface (Click-based with DYMGroup for suggestions). All command + callbacks must propagate their `do_main()`-style result via `ctx.exit(res)`, never a plain + `return res` — Click's standalone-mode `main()` silently discards a callback's return value + (see [cli/exit-codes-spec.md](cli/exit-codes-spec.md), [cli/exit-codes-tasks.md](cli/exit-codes-tasks.md)) - `entrypoint.py` - Main CLI dispatcher - `cmd_qr_parse.py` - Parse QR codes from `.mkv` videos (PARSE/INFO modes) (see [qr/parse-spec.md](qr/parse-spec.md)) - `cmd_timesync_stimuli.py` - PsychoPy integration for QR/audio code generation (see [qr/timesync-stimuli-tasks.md](qr/timesync-stimuli-tasks.md)) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index c8ebacb4..83433a1f 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -58,7 +58,7 @@ jobs: - name: Install reprostim run: | - hatch run pip install -e . + hatch run pip install -e ".[disp_mon]" hatch build - name: Run pytest diff --git a/pyproject.toml b/pyproject.toml index 257b877a..4b1675e4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -59,6 +59,7 @@ audio = [ # displays monitor optional dependencies # platform specific, Linux and macOS supported disp_mon = [ + "psutil>=5.9.0", "pygame>=2.6.1", "pyglet>=1.5.27", "pyudev>=0.23.0 ; sys_platform == 'linux'", diff --git a/src/reprostim/cli/cmd_bids_inject.py b/src/reprostim/cli/cmd_bids_inject.py index fd9465d1..613a4aac 100644 --- a/src/reprostim/cli/cmd_bids_inject.py +++ b/src/reprostim/cli/cmd_bids_inject.py @@ -250,4 +250,4 @@ def bids_inject( click.echo( f"Command 'bids-inject' completed in {elapsed_sec} sec, exit code {res}" ) - return res + ctx.exit(res) diff --git a/src/reprostim/cli/cmd_list_displays.py b/src/reprostim/cli/cmd_list_displays.py index d3ce63cd..b13f6c9a 100644 --- a/src/reprostim/cli/cmd_list_displays.py +++ b/src/reprostim/cli/cmd_list_displays.py @@ -42,4 +42,4 @@ def list_displays(ctx, provider: str, fmt: str): logger.debug(f"reprostim list-displays script finished: {res}") logger.debug(f"Exit on : {datetime.now()}") logger.debug(f"Exit code : {res}") - return res + ctx.exit(res) diff --git a/src/reprostim/cli/cmd_monitor_displays.py b/src/reprostim/cli/cmd_monitor_displays.py index dca9e639..b8f4ca78 100644 --- a/src/reprostim/cli/cmd_monitor_displays.py +++ b/src/reprostim/cli/cmd_monitor_displays.py @@ -102,4 +102,4 @@ def monitor_displays( logger.debug(f"reprostim monitor-displays script finished: {res}") logger.debug(f"Exit on : {datetime.now()}") logger.debug(f"Exit code : {res}") - return res + ctx.exit(res) diff --git a/src/reprostim/cli/cmd_qr_parse.py b/src/reprostim/cli/cmd_qr_parse.py index 3e436fe8..6317b103 100644 --- a/src/reprostim/cli/cmd_qr_parse.py +++ b/src/reprostim/cli/cmd_qr_parse.py @@ -160,4 +160,4 @@ def qr_parse( elapsed_sec = round(time.time() - start_time_sec, 1) logger.debug(f"Command 'qr-parse' completed in {elapsed_sec} sec, exit code {res}") - return res + ctx.exit(res) diff --git a/src/reprostim/cli/cmd_split_video.py b/src/reprostim/cli/cmd_split_video.py index 50cb5084..249501fb 100644 --- a/src/reprostim/cli/cmd_split_video.py +++ b/src/reprostim/cli/cmd_split_video.py @@ -245,4 +245,4 @@ def split_video( click.echo( f"Command 'split-video' completed in {elapsed_sec} sec, exit code {res}" ) - return res + ctx.exit(res) diff --git a/src/reprostim/cli/cmd_timesync_stimuli.py b/src/reprostim/cli/cmd_timesync_stimuli.py index 18a66eb5..e5a8eb4c 100644 --- a/src/reprostim/cli/cmd_timesync_stimuli.py +++ b/src/reprostim/cli/cmd_timesync_stimuli.py @@ -187,8 +187,8 @@ def timesync_stimuli( os.environ["REPROSTIM_AUDIO_LIB"] = audio_lib if not do_init(output): - logger.error() - return -1 + logger.error("do_init(...) failed") + ctx.exit(1) res = do_main( mode, @@ -220,4 +220,4 @@ def timesync_stimuli( logger.info(f"reprostim timesync-stimuli script finished: {res}") logger.info(f"Exit on : {datetime.now()}") logger.info(f"Exit code : {res}") - return res + ctx.exit(res) diff --git a/tests/bids/test_inject.py b/tests/bids/test_inject.py index e5e789b3..65f0c0a9 100644 --- a/tests/bids/test_inject.py +++ b/tests/bids/test_inject.py @@ -15,6 +15,7 @@ from zoneinfo import ZoneInfo, ZoneInfoNotFoundError import pytest +from click.testing import CliRunner from reprostim.bids.inject import ( _REPROSTIM_COLS, @@ -50,6 +51,7 @@ dt_utc_to_bids, dt_utc_to_reprostim, ) +from reprostim.cli.cmd_bids_inject import bids_inject from reprostim.video.audit import AudioInfo, VideoInfo from reprostim.video.split import SplitResult @@ -1659,3 +1661,55 @@ def test_scans_tsv_dry_run_does_not_modify_file(tmp_path): ) assert scans_tsv.read_text(encoding="utf-8") == original_content + + +# =========================================================================== +# CLI tests (Click CliRunner) for cmd_bids_inject +# =========================================================================== + + +def test_cli_help_renders_without_error(): + """--help exits with code 0 and produces output.""" + result = CliRunner().invoke(bids_inject, ["--help"]) + assert result.exit_code == 0 + assert "Usage" in result.output + + +def test_cli_missing_videos_option_nonzero_exit(tmp_path): + """Omitting the required -f/--videos option produces a non-zero exit.""" + scans_tsv = _copy_bids_fixture(tmp_path) + result = CliRunner().invoke(bids_inject, [str(scans_tsv)]) + assert result.exit_code != 0 + + +def test_cli_nonzero_do_main_result_propagated_to_exit_code(tmp_path): + """A non-zero do_main() result must become the process exit code. + + Regression test: the command used to `return res` from the Click + callback, which Click's standalone-mode main() silently discards + (the process exits 0 no matter what `res` was, unless an exception + is raised or ctx.exit()/sys.exit() is called explicitly). This is + the exact bug reported from a real Linux run of bids-inject. + """ + scans_tsv = _copy_bids_fixture(tmp_path) + videos_tsv = _write_videos_tsv(tmp_path, _VA_V1) + + with patch("reprostim.bids.inject.do_main", return_value=3): + result = CliRunner().invoke( + bids_inject, + [str(scans_tsv), "-f", videos_tsv], + ) + assert result.exit_code == 3 + + +def test_cli_zero_do_main_result_exits_zero(tmp_path): + """A zero do_main() result still exits 0 (baseline success case).""" + scans_tsv = _copy_bids_fixture(tmp_path) + videos_tsv = _write_videos_tsv(tmp_path, _VA_V1) + + with patch("reprostim.bids.inject.do_main", return_value=0): + result = CliRunner().invoke( + bids_inject, + [str(scans_tsv), "-f", videos_tsv], + ) + assert result.exit_code == 0 diff --git a/tests/cli/__init__.py b/tests/cli/__init__.py new file mode 100644 index 00000000..882830f8 --- /dev/null +++ b/tests/cli/__init__.py @@ -0,0 +1,3 @@ +# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team +# +# SPDX-License-Identifier: MIT diff --git a/tests/cli/test_cmd_list_displays.py b/tests/cli/test_cmd_list_displays.py new file mode 100644 index 00000000..8cc6122f --- /dev/null +++ b/tests/cli/test_cmd_list_displays.py @@ -0,0 +1,51 @@ +# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team +# +# SPDX-License-Identifier: MIT + +"""CLI tests (Click CliRunner) for reprostim.cli.cmd_list_displays.""" + +from unittest.mock import patch + +from click.testing import CliRunner + +from reprostim.cli.cmd_list_displays import list_displays + + +def test_cli_help_renders_without_error(): + """--help exits with code 0 and produces output.""" + result = CliRunner().invoke(list_displays, ["--help"]) + assert result.exit_code == 0 + assert "Usage" in result.output + + +def test_cli_success_exits_zero(): + """A successful run (do_list_displays raises nothing) exits 0.""" + with patch("reprostim.capture.disp_mon.do_list_displays") as mock_dld: + result = CliRunner().invoke(list_displays, []) + mock_dld.assert_called_once() + assert result.exit_code == 0 + + +def test_cli_options_forwarded(): + """-p/--provider and -f/--format are forwarded to do_list_displays.""" + with patch("reprostim.capture.disp_mon.do_list_displays") as mock_dld: + CliRunner().invoke(list_displays, ["-p", "pygame", "-f", "text"]) + args = mock_dld.call_args.args + assert args[0].value == "pygame" + assert args[1] == "text" + + +def test_cli_exception_from_implementation_exits_nonzero(): + """An exception raised inside do_list_displays surfaces as a non-zero exit. + + Unlike the `return res` bug (a *silently discarded* success/failure + signal), an uncaught exception has always correctly propagated to a + non-zero exit code — this test pins down that this remains true after + the `ctx.exit(res)` fix. + """ + with patch( + "reprostim.capture.disp_mon.do_list_displays", + side_effect=RuntimeError("boom"), + ): + result = CliRunner().invoke(list_displays, []) + assert result.exit_code != 0 diff --git a/tests/cli/test_cmd_monitor_displays.py b/tests/cli/test_cmd_monitor_displays.py new file mode 100644 index 00000000..12af15bd --- /dev/null +++ b/tests/cli/test_cmd_monitor_displays.py @@ -0,0 +1,57 @@ +# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team +# +# SPDX-License-Identifier: MIT + +"""CLI tests (Click CliRunner) for reprostim.cli.cmd_monitor_displays.""" + +from unittest.mock import patch + +from click.testing import CliRunner + +from reprostim.cli.cmd_monitor_displays import monitor_displays + + +def test_cli_help_renders_without_error(): + """--help exits with code 0 and produces output.""" + result = CliRunner().invoke(monitor_displays, ["--help"]) + assert result.exit_code == 0 + assert "Usage" in result.output + + +def test_cli_success_exits_zero(): + """A successful run (do_monitor_displays raises nothing) exits 0.""" + with patch("reprostim.capture.disp_mon.do_monitor_displays") as mock_dmd: + result = CliRunner().invoke(monitor_displays, []) + mock_dmd.assert_called_once() + assert result.exit_code == 0 + + +def test_cli_options_forwarded(): + """CLI options are forwarded positionally to do_monitor_displays.""" + with patch("reprostim.capture.disp_mon.do_monitor_displays") as mock_dmd: + CliRunner().invoke( + monitor_displays, + ["-p", "quartz", "-t", "5", "-w", "10", "-n", "Built-in*", "-i", "1"], + ) + args = mock_dmd.call_args.args + assert args[0].value == "quartz" + assert args[1] == 5 + assert args[2] == 10 + assert args[3] == "Built-in*" + assert args[4] == "1" + + +def test_cli_exception_from_implementation_exits_nonzero(): + """An exception raised inside do_monitor_displays surfaces as a non-zero exit. + + Unlike the `return res` bug (a *silently discarded* success/failure + signal), an uncaught exception has always correctly propagated to a + non-zero exit code — this test pins down that this remains true after + the `ctx.exit(res)` fix. + """ + with patch( + "reprostim.capture.disp_mon.do_monitor_displays", + side_effect=RuntimeError("boom"), + ): + result = CliRunner().invoke(monitor_displays, []) + assert result.exit_code != 0 diff --git a/tests/cli/test_cmd_timesync_stimuli.py b/tests/cli/test_cmd_timesync_stimuli.py new file mode 100644 index 00000000..adad679b --- /dev/null +++ b/tests/cli/test_cmd_timesync_stimuli.py @@ -0,0 +1,60 @@ +# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team +# +# SPDX-License-Identifier: MIT + +"""CLI tests (Click CliRunner) for reprostim.cli.cmd_timesync_stimuli.""" + +from unittest.mock import patch + +from click.testing import CliRunner + +from reprostim.cli.cmd_timesync_stimuli import timesync_stimuli + + +def test_cli_help_renders_without_error(): + """--help exits with code 0 and produces output.""" + result = CliRunner().invoke(timesync_stimuli, ["--help"]) + assert result.exit_code == 0 + assert "Usage" in result.output + + +def test_cli_do_init_failure_exits_nonzero(): + """do_init() returning False must produce a non-zero exit code. + + Regression test: this path used to `return -1` from the Click + callback, which Click's standalone-mode main() silently discards + (the process exits 0 regardless of the returned value unless an + exception is raised or ctx.exit()/sys.exit() is called explicitly). + """ + with ( + patch("reprostim.qr.timesync_stimuli.do_init", return_value=False), + patch("reprostim.qr.timesync_stimuli.do_main") as mock_do_main, + ): + result = CliRunner().invoke(timesync_stimuli, []) + mock_do_main.assert_not_called() + assert result.exit_code != 0 + + +def test_cli_nonzero_do_main_result_propagated_to_exit_code(): + """A non-zero do_main() result must become the process exit code. + + Regression test: the command used to `return res` from the Click + callback at the very end, silently discarded by Click's standalone + main() the same way as the `return -1` case above. + """ + with ( + patch("reprostim.qr.timesync_stimuli.do_init", return_value=True), + patch("reprostim.qr.timesync_stimuli.do_main", return_value=4), + ): + result = CliRunner().invoke(timesync_stimuli, []) + assert result.exit_code == 4 + + +def test_cli_success_exits_zero(): + """do_init() True and do_main() returning 0 exits 0 (baseline).""" + with ( + patch("reprostim.qr.timesync_stimuli.do_init", return_value=True), + patch("reprostim.qr.timesync_stimuli.do_main", return_value=0), + ): + result = CliRunner().invoke(timesync_stimuli, []) + assert result.exit_code == 0 diff --git a/tests/qr/test_parse.py b/tests/qr/test_parse.py index 688f9b69..c5b8bf38 100644 --- a/tests/qr/test_parse.py +++ b/tests/qr/test_parse.py @@ -721,3 +721,17 @@ def test_cli_invalid_path(cli_runner, tmp_path): """CLI exits non-zero for a path that does not exist.""" result = cli_runner.invoke(qr_parse_cmd, [str(tmp_path / "missing.mkv")]) assert result.exit_code != 0 + + +def test_cli_nonzero_do_main_result_propagated_to_exit_code(cli_runner, tmp_path): + """A non-zero do_main() result must become the process exit code. + + Regression test: the command used to `return res` from the Click + callback, which Click's standalone-mode main() silently discards + (the process exits 0 no matter what `res` was, unless an exception + is raised or ctx.exit()/sys.exit() is called explicitly). + """ + video = _video(tmp_path) + with patch("reprostim.qr.parse.do_main", return_value=7): + result = cli_runner.invoke(qr_parse_cmd, [str(video)]) + assert result.exit_code == 7 diff --git a/tests/video/test_split.py b/tests/video/test_split.py index 79f362a2..c0c890f5 100644 --- a/tests/video/test_split.py +++ b/tests/video/test_split.py @@ -15,7 +15,9 @@ from unittest.mock import MagicMock, patch import pytest +from click.testing import CliRunner +from reprostim.cli.cmd_split_video import split_video from reprostim.video.audit import VaRecord from reprostim.video.split import ( BufferPolicy, @@ -834,10 +836,6 @@ def test_do_main_specs_per_spec_failure_continues_processing(mock_csd, mock_sv): # CLI tests (Click CliRunner) for cmd_split_video # =========================================================================== -from click.testing import CliRunner # noqa: E402 - -from reprostim.cli.cmd_split_video import split_video # noqa: E402 - @pytest.fixture() def input_video(tmp_path: Path) -> str: @@ -1382,3 +1380,26 @@ def test_cli_verbose_emits_completed_message(input_video): ], ) assert "completed" in result.output.lower() + + +def test_cli_nonzero_do_main_result_propagated_to_exit_code(input_video): + """A non-zero do_main() result must become the process exit code. + + Regression test: the command used to `return res` from the Click + callback, which Click's standalone-mode main() silently discards + (the process exits 0 no matter what `res` was, unless an exception + is raised or ctx.exit()/sys.exit() is called explicitly). + """ + with patch("reprostim.video.split.do_main", return_value=(5, [])): + result = CliRunner().invoke( + split_video, + [ + "-i", + input_video, + "-o", + "out.mkv", + "--spec", + "2024-02-02T17:30:00/PT3M", + ], + ) + assert result.exit_code == 5