Fix CLI commands silently exiting 0 on failure

.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.

.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

.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))

.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

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'",

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)

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)

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)

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)

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)

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)

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

tests/cli/__init__.py

@@ -0,0 +1,3 @@
+# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team <reprostim@repronim.org>
+#
+# SPDX-License-Identifier: MIT

tests/cli/test_cmd_list_displays.py

@@ -0,0 +1,51 @@
+# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team <reprostim@repronim.org>
+#
+# 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

tests/cli/test_cmd_monitor_displays.py

@@ -0,0 +1,57 @@
+# SPDX-FileCopyrightText: 2020-2026 ReproNim ReproStim Team <reprostim@repronim.org>
+#
+# 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