fix: guard --epochs 0/negative with a guided error (#145)

[nk-pavan-official]

Summary
--epochs 0 (or negative) crashed with a raw Python traceback instead of a clean CLI error. Fixing that.
Changes

_cmd_run: wrapped the replace(task, epochs=...) call in try/except for ConfigError, exits clean instead of crashing
_cmd_eval_set: same fix, but had to turn the list comprehension into a loop so I could catch the error per task and say which one failed
Added 2 tests in test_registry_cli.py for both call sites (0, -1, -5)
Updated after review: retyped patched as list[Task] (dropped a stale type: ignore), updated an existing test to expect SystemExit instead of the raw error, added a CHANGELOG entry

Checklist

• pre-commit install done; hooks pass (or ran pre-commit run --all-files)
• Tests added/updated (and the CubePick mock exercises the change where applicable)
• Coverage stays at 100% (pytest --cov)
• ruff check . and ruff format --check . pass
• mypy passes (strict)
• CHANGELOG.md updated under "Unreleased"
• Public API changes are reflected in inspect_robots.__all__ and the API-snapshot test
• Core stays NumPy-only (new deps are optional extras, lazily imported)

Related

Closes #145

[Tushar2604]

Thanks for taking this on.

I pulled the branch and ran the actual gates rather than going off the checklist. Note: locally, mypy fails, there's no CHANGELOG diff, and an existing test fails — the checklist marks those as done, so this likely just needs a re-run after the fixes below.

mypy --strict fails
src\inspect_robots\cli.py:1021: error: Unused "type: ignore" comment [unused-ignore]
tasks was already list[Any] pre-change, so no suppression is needed. Fix: drop the ignore, retype patched as list[Task].

Breaks test_cli_run_closes_embodiment_when_validation_raises
Still asserts pytest.raises(ConfigError) for --epochs 0; this PR correctly raises SystemExit now, so it fails (1 failed, 711 passed locally). Fix: update to pytest.raises(SystemExit), then re-run the full suite since this touches a shared error path.

No CHANGELOG entry
Please add a ### Fixed entry under Unreleased (#47 is a good template).

Minor, not blocking
f"--epochs: {exc}" reads a little rough grammatically — not asking for a change.

One more ask: could you paste the before/after CLI output for run --epochs 0 and eval-set --epochs -1? Clearest confirmation it behaves as expected end to end.

@jeqcho could you take a look when you have a moment? The approach looks right to me and I ran the gates locally to flag the above, but I'll leave the actual direction/merge call to you.

[nk-pavan-official]

Thanks for taking this on.

I pulled the branch and ran the actual gates rather than going off the checklist. Note: locally, mypy fails, there's no CHANGELOG diff, and an existing test fails — the checklist marks those as done, so this likely just needs a re-run after the fixes below.

mypy --strict fails src\inspect_robots\cli.py:1021: error: Unused "type: ignore" comment [unused-ignore] tasks was already list[Any] pre-change, so no suppression is needed. Fix: drop the ignore, retype patched as list[Task].

Breaks test_cli_run_closes_embodiment_when_validation_raises Still asserts pytest.raises(ConfigError) for --epochs 0; this PR correctly raises SystemExit now, so it fails (1 failed, 711 passed locally). Fix: update to pytest.raises(SystemExit), then re-run the full suite since this touches a shared error path.

No CHANGELOG entry Please add a ### Fixed entry under Unreleased (#47 is a good template).

Minor, not blocking f"--epochs: {exc}" reads a little rough grammatically — not asking for a change.

One more ask: could you paste the before/after CLI output for run --epochs 0 and eval-set --epochs -1? Clearest confirmation it behaves as expected end to end.

@jeqcho could you take a look when you have a moment? The approach looks right to me and I ran the gates locally to flag the above, but I'll leave the actual direction/merge call to you.

[nk-pavan-official]

Fixed all three. Retyped patched as list[Task] and dropped the stale type: ignore, updated the test to expect SystemExit with a message check, and added a CHANGELOG entry under Fixed matching #47's format. Full suite passes now - 728 passed, 2 skipped (unrelated, missing rerun-sdk), 100% coverage, mypy clean.

$ inspect-robots run --task cubepick-reach --policy scripted --embodiment cubepick --epochs 0
--epochs: Epochs count must be >= 1, got 0

$ inspect-robots eval-set --task cubepick-reach --policy scripted --embodiment cubepick --epochs -1
--epochs (task 'cubepick-reach'): Epochs count must be >= 1, got -1

[jeqcho]

Thanks Pavan, this is ready. A few things stood out beyond the fix itself.

The test coverage is thorough where it counts: parametrized over 0 and negatives at both call sites, asserting the flag name, the offending value, and the task name all show up in the message. Strengthening test_cli_run_closes_embodiment_when_validation_raises with a message assertion was a good catch too, since that test guards the embodiment-close invariant and was previously asserting the buggy behavior.

Also appreciated how you handled review: all three findings fixed within hours, with before/after CLI output pasted as proof, and a CHANGELOG entry matching the #47 format.

Merging. One cosmetic thing I'll fold into a follow-up rather than block on: --epochs: Epochs count must be >= 1, got 0 doubles up a bit; something like --epochs must be >= 1, got 0 reads cleaner. There is also an open idea to extract the two inline try/excepts into a shared helper next to _resolve_or_exit. If either interests you, they are yours.

[jeqcho]

Merged, thanks Pavan. The follow-ups from my review are now written up in #152 if you want a second one; it builds directly on what you landed here. Either way, glad to have you contributing.

[nk-pavan-official]

Thanks Jay, appreciate it. Will check out #152, that refactor idea sounds fun