From 9ad2fa456f3fedbe93b2973c5ef1dae9494ad273 Mon Sep 17 00:00:00 2001 From: Pavan Date: Sun, 19 Jul 2026 04:04:14 +0530 Subject: [PATCH 1/4] cli: guard --epochs 0/negative with a guided error instead of a traceback Fixes #145. Wraps the two replace(task, epochs=...) call sites in _cmd_run and _cmd_eval_set with a ConfigError catch, matching the existing _resolve_or_exit pattern. eval-set names the failing task. --- src/inspect_robots/cli.py | 17 +++++++++++++-- tests/test_registry_cli.py | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/inspect_robots/cli.py b/src/inspect_robots/cli.py index 383719d..8f4051b 100644 --- a/src/inspect_robots/cli.py +++ b/src/inspect_robots/cli.py @@ -885,7 +885,12 @@ def _cmd_run(args: argparse.Namespace) -> int: embodiment = resolved.embodiment try: if args.epochs is not None: - task = replace(task, epochs=args.epochs) + from inspect_robots.errors import ConfigError + + try: + task = replace(task, epochs=args.epochs) + except ConfigError as exc: + raise SystemExit(f"--epochs: {exc}") from exc _announce_components(resolved) approver = _build_and_announce_guardrails(args, embodiment.info.action_space) @@ -1005,7 +1010,15 @@ def _cmd_eval_set(args: argparse.Namespace) -> int: defaults = load_defaults(os.environ) tasks = [_resolve_or_exit("task", name) for name in task_names] if args.epochs is not None: - tasks = [replace(t, epochs=args.epochs) for t in tasks] + from inspect_robots.errors import ConfigError + + patched: list[object] = [] + for t in tasks: + try: + patched.append(replace(t, epochs=args.epochs)) + except ConfigError as exc: + raise SystemExit(f"--epochs (task {t.name!r}): {exc}") from exc + tasks = patched # type: ignore[assignment] resolved = _resolve_components(args, defaults) embodiment = resolved.embodiment diff --git a/tests/test_registry_cli.py b/tests/test_registry_cli.py index 0e09a14..a329eb7 100644 --- a/tests/test_registry_cli.py +++ b/tests/test_registry_cli.py @@ -379,6 +379,50 @@ def test_cli_run_epochs_fail_on_error_store_frames( assert list((tmp_path / "frames").rglob("*.npy")) # --store-frames streamed (per-run subdir) +@pytest.mark.parametrize("epochs_value", ["0", "-1", "-5"]) +def test_cli_run_zero_epochs_exits_with_guided_error(epochs_value: str) -> None: + """--epochs 0 / negative must produce a guided SystemExit, not a raw traceback (#145).""" + with pytest.raises(SystemExit) as excinfo: + main( + [ + "run", + "--task", + "cubepick-reach", + "--policy", + "scripted", + "--embodiment", + "cubepick", + "--epochs", + epochs_value, + ] + ) + message = str(excinfo.value) + assert "--epochs" in message + assert epochs_value in message + + +@pytest.mark.parametrize("epochs_value", ["0", "-1"]) +def test_cli_eval_set_zero_epochs_exits_with_guided_error(epochs_value: str) -> None: + """eval-set --epochs 0 / negative must produce a guided SystemExit naming the task (#145).""" + with pytest.raises(SystemExit) as excinfo: + main( + [ + "eval-set", + "cubepick-reach", + "--policy", + "scripted", + "--embodiment", + "cubepick", + "--epochs", + epochs_value, + ] + ) + message = str(excinfo.value) + assert "--epochs" in message + assert epochs_value in message + assert "cubepick-reach" in message # the failing task name is named + + def _register_task(name: str, *, num_scenes: int = 1, max_steps: int = 20) -> None: from inspect_robots.registry import task as task_decorator from inspect_robots.scene import Scene From 0c68c4ac84971574f93811f1959f36d2314b9c30 Mon Sep 17 00:00:00 2001 From: Pavan Date: Sun, 19 Jul 2026 15:13:04 +0530 Subject: [PATCH 2/4] address review: fix type hint, update test for SystemExit, add changelog --- src/inspect_robots/cli.py | 4 ++-- tests/test_registry_cli.py | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/inspect_robots/cli.py b/src/inspect_robots/cli.py index 8f4051b..8bef6ab 100644 --- a/src/inspect_robots/cli.py +++ b/src/inspect_robots/cli.py @@ -1012,13 +1012,13 @@ def _cmd_eval_set(args: argparse.Namespace) -> int: if args.epochs is not None: from inspect_robots.errors import ConfigError - patched: list[object] = [] + patched: list[Any] = [] for t in tasks: try: patched.append(replace(t, epochs=args.epochs)) except ConfigError as exc: raise SystemExit(f"--epochs (task {t.name!r}): {exc}") from exc - tasks = patched # type: ignore[assignment] + tasks = patched resolved = _resolve_components(args, defaults) embodiment = resolved.embodiment diff --git a/tests/test_registry_cli.py b/tests/test_registry_cli.py index a329eb7..0fba4d6 100644 --- a/tests/test_registry_cli.py +++ b/tests/test_registry_cli.py @@ -2508,8 +2508,11 @@ def test_cli_run_closes_embodiment_when_validation_raises( ) -> None: """A failure between resolving the embodiment and eval() (here: --epochs 0 raising ConfigError) must still close the embodiment — otherwise a bad flag - leaves real arms energized.""" - from inspect_robots.errors import ConfigError + leaves real arms energized. + + The fix converts the raw ConfigError into a guided SystemExit; the + important invariant is that close() still fires via the outer try/finally. + """ from inspect_robots.mock import CubePickEmbodiment closed: list[bool] = [] @@ -2522,7 +2525,7 @@ def close(self) -> None: monkeypatch.setitem(reg._FACTORIES["embodiment"], "tracked-cubepick", _Tracked) monkeypatch.setenv(ENV_POLICY, "scripted") monkeypatch.setenv(ENV_EMBODIMENT, "tracked-cubepick") - with pytest.raises(ConfigError): + with pytest.raises(SystemExit) as excinfo: main( [ "reach the cube", @@ -2534,6 +2537,7 @@ def close(self) -> None: str(tmp_path / "logs"), ] ) + assert "--epochs" in str(excinfo.value) assert closed == [True] From 746683c89da9ea9ffd564a8e07bf552372c68058 Mon Sep 17 00:00:00 2001 From: Pavan Date: Sun, 19 Jul 2026 15:24:41 +0530 Subject: [PATCH 3/4] tighten patched list type to list[Task], add changelog entry --- CHANGELOG.md | 5 +++++ src/inspect_robots/cli.py | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4b5aae..5003a63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,11 @@ All notable changes to this project are documented here. The format is based on ### Fixed +- **`--epochs 0` or a negative value now exits with a guided error instead of a + raw traceback** (#145). Both `inspect-robots run` and `inspect-robots eval-set` + catch the `ConfigError` raised by `Task`'s epoch validation and surface it + through the existing `_resolve_or_exit` pattern, matching how invalid + constructor kwargs are handled for config-file components (#47). - **Operator scoring no longer prompts twice for self-confirming embodiments** (#53). On interactive ad-hoc runs, definitive `success` or `failure` termination verdicts are adopted as the operator judgement, announced on the diff --git a/src/inspect_robots/cli.py b/src/inspect_robots/cli.py index 8bef6ab..7c3bd59 100644 --- a/src/inspect_robots/cli.py +++ b/src/inspect_robots/cli.py @@ -1011,8 +1011,9 @@ def _cmd_eval_set(args: argparse.Namespace) -> int: tasks = [_resolve_or_exit("task", name) for name in task_names] if args.epochs is not None: from inspect_robots.errors import ConfigError + from inspect_robots.task import Task - patched: list[Any] = [] + patched: list[Task] = [] for t in tasks: try: patched.append(replace(t, epochs=args.epochs)) From 021a3e94c5a057d0cd9eccf057ea00076fb3976e Mon Sep 17 00:00:00 2001 From: Pavan Date: Thu, 23 Jul 2026 20:22:35 +0530 Subject: [PATCH 4/4] refactor(cli): extract shared epochs validation helper --- CHANGELOG.md | 11 ++++++---- src/inspect_robots/cli.py | 44 ++++++++++++++++++++++++-------------- tests/test_registry_cli.py | 17 +++++++-------- 3 files changed, 43 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5003a63..6089411 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,10 +80,13 @@ All notable changes to this project are documented here. The format is based on ### Fixed - **`--epochs 0` or a negative value now exits with a guided error instead of a - raw traceback** (#145). Both `inspect-robots run` and `inspect-robots eval-set` - catch the `ConfigError` raised by `Task`'s epoch validation and surface it - through the existing `_resolve_or_exit` pattern, matching how invalid - constructor kwargs are handled for config-file components (#47). + raw traceback** (#145, #152). Both `inspect-robots run` and `inspect-robots + eval-set` share a single `_apply_epochs_or_exit` helper (next to + `_resolve_or_exit`) that catches the `ConfigError` from `Task`'s epoch + validation. The message is `--epochs must be >= 1, got N`; the `eval-set` + path appends `(task 'NAME')` to name the failing task. The duplicate + `ConfigError` import that previously appeared mid-function in both commands is + gone (#47). - **Operator scoring no longer prompts twice for self-confirming embodiments** (#53). On interactive ad-hoc runs, definitive `success` or `failure` termination verdicts are adopted as the operator judgement, announced on the diff --git a/src/inspect_robots/cli.py b/src/inspect_robots/cli.py index 7c3bd59..ffd5c37 100644 --- a/src/inspect_robots/cli.py +++ b/src/inspect_robots/cli.py @@ -71,6 +71,7 @@ from inspect_robots.rollout import TrialRecord from inspect_robots.scene import Scene from inspect_robots.spaces import Box + from inspect_robots.task import Task def _styled(text: str, code: str) -> str: @@ -478,6 +479,27 @@ def _resolve_or_exit( ) from exc +def _apply_epochs_or_exit(task: Task, epochs: int, /) -> Task: + """Return a copy of *task* with the epoch count overridden, or exit cleanly. + + Centralises the ``--epochs`` guard for both ``run`` and ``eval-set``: + a non-positive value raises ``ConfigError`` inside ``dataclasses.replace`` + and we surface it as a guided ``SystemExit`` — the same ``raise … from exc`` + idiom used by :func:`_resolve_or_exit` — instead of a raw traceback. + The task name is included in the message only when a ``task_label`` is + meaningful (i.e. the eval-set path); callers supply it via the wrapper + that knows the context. + """ + from dataclasses import replace + + from inspect_robots.errors import ConfigError + + try: + return replace(task, epochs=epochs) + except ConfigError: + raise SystemExit(f"--epochs must be >= 1, got {epochs}") from None + + def _build_guardrails( space: Box, max_action_delta: float | None ) -> tuple[Approver, list[str], list[str]]: @@ -835,8 +857,6 @@ def _build_and_announce_guardrails(args: argparse.Namespace, action_space: Box) def _cmd_run(args: argparse.Namespace) -> int: - from dataclasses import replace - from inspect_robots import eval from inspect_robots.logging import JsonLogSink from inspect_robots.scene import Scene @@ -885,12 +905,7 @@ def _cmd_run(args: argparse.Namespace) -> int: embodiment = resolved.embodiment try: if args.epochs is not None: - from inspect_robots.errors import ConfigError - - try: - task = replace(task, epochs=args.epochs) - except ConfigError as exc: - raise SystemExit(f"--epochs: {exc}") from exc + task = _apply_epochs_or_exit(task, args.epochs) _announce_components(resolved) approver = _build_and_announce_guardrails(args, embodiment.info.action_space) @@ -1000,8 +1015,6 @@ def _cmd_eval_set(args: argparse.Namespace) -> int: the embodiment once per task), the CLI resolves the embodiment exactly once for the whole set, so a real robot is not reconnected between tasks. """ - from dataclasses import replace - from inspect_robots import eval_set _check_shared_run_conflicts(args) @@ -1010,15 +1023,14 @@ def _cmd_eval_set(args: argparse.Namespace) -> int: defaults = load_defaults(os.environ) tasks = [_resolve_or_exit("task", name) for name in task_names] if args.epochs is not None: - from inspect_robots.errors import ConfigError - from inspect_robots.task import Task - patched: list[Task] = [] for t in tasks: try: - patched.append(replace(t, epochs=args.epochs)) - except ConfigError as exc: - raise SystemExit(f"--epochs (task {t.name!r}): {exc}") from exc + patched.append(_apply_epochs_or_exit(t, args.epochs)) + except SystemExit: + raise SystemExit( + f"--epochs must be >= 1, got {args.epochs} (task {t.name!r})" + ) from None tasks = patched resolved = _resolve_components(args, defaults) diff --git a/tests/test_registry_cli.py b/tests/test_registry_cli.py index 0fba4d6..7c46627 100644 --- a/tests/test_registry_cli.py +++ b/tests/test_registry_cli.py @@ -382,7 +382,10 @@ def test_cli_run_epochs_fail_on_error_store_frames( @pytest.mark.parametrize("epochs_value", ["0", "-1", "-5"]) def test_cli_run_zero_epochs_exits_with_guided_error(epochs_value: str) -> None: """--epochs 0 / negative must produce a guided SystemExit, not a raw traceback (#145).""" - with pytest.raises(SystemExit) as excinfo: + import re + + expected = f"--epochs must be >= 1, got {epochs_value}" + with pytest.raises(SystemExit, match=re.escape(expected)): main( [ "run", @@ -396,15 +399,15 @@ def test_cli_run_zero_epochs_exits_with_guided_error(epochs_value: str) -> None: epochs_value, ] ) - message = str(excinfo.value) - assert "--epochs" in message - assert epochs_value in message @pytest.mark.parametrize("epochs_value", ["0", "-1"]) def test_cli_eval_set_zero_epochs_exits_with_guided_error(epochs_value: str) -> None: """eval-set --epochs 0 / negative must produce a guided SystemExit naming the task (#145).""" - with pytest.raises(SystemExit) as excinfo: + import re + + expected = f"--epochs must be >= 1, got {epochs_value} (task 'cubepick-reach')" + with pytest.raises(SystemExit, match=re.escape(expected)): main( [ "eval-set", @@ -417,10 +420,6 @@ def test_cli_eval_set_zero_epochs_exits_with_guided_error(epochs_value: str) -> epochs_value, ] ) - message = str(excinfo.value) - assert "--epochs" in message - assert epochs_value in message - assert "cubepick-reach" in message # the failing task name is named def _register_task(name: str, *, num_scenes: int = 1, max_steps: int = 20) -> None: