Skip to content

fix(cli): guided error for --epochs 0/negative instead of raw traceback#148

Closed
ptlnextdoor wants to merge 1 commit into
robocurve:mainfrom
ptlnextdoor:fix/145-guided-epochs-error
Closed

fix(cli): guided error for --epochs 0/negative instead of raw traceback#148
ptlnextdoor wants to merge 1 commit into
robocurve:mainfrom
ptlnextdoor:fix/145-guided-epochs-error

Conversation

@ptlnextdoor

Copy link
Copy Markdown

Summary

--epochs 0 (or negative) crashes both run and eval-set with a raw traceback instead of the guided SystemExit every other invalid CLI input gets.

replace(task, epochs=...) reruns Task.__post_init__, which raises ConfigError when the count is below 1. Neither _cmd_run nor _cmd_eval_set caught it — same bug class already fixed for config-file kwargs in #47.

Changes

  • Adds _apply_epochs_or_exit, mirroring the existing _resolve_or_exit pattern: catches ConfigError and re-raises as SystemExit(str(exc)).
  • Both call sites (_cmd_run, _cmd_eval_set) now go through it instead of a bare dataclasses.replace.
  • Drops the now-unused local replace imports in both functions.
  • Updates test_cli_run_closes_embodiment_when_validation_raises (was asserting the raw ConfigError leaked, which was the bug) and adds test_cli_eval_set_negative_epochs_is_a_guided_error for the eval-set path, which had no coverage before.

Test plan

  • uv run ruff check .
  • uv run ruff format --check .
  • uv run mypy (strict)
  • uv run pytest --cov — 724 passed, 100% line+branch coverage
  • uv run pre-commit run --all-files
  • Manually confirmed inspect-robots run --task cubepick-reach --policy scripted --embodiment cubepick --epochs 0 now exits with the guided message instead of a traceback

Fixes #145

replace(task, epochs=...) reruns Task.__post_init__, which raises
ConfigError for a count below 1. Neither _cmd_run nor _cmd_eval_set
caught it, so the CLI's only unvalidated flag path crashed with a raw
traceback instead of the guided SystemExit every other invalid input
gets (same bug class fixed for config kwargs in robocurve#47).

Adds _apply_epochs_or_exit, used by both call sites, mirroring the
_resolve_or_exit pattern already used for registry resolution errors.

Fixes robocurve#145
Copilot AI review requested due to automatic review settings July 19, 2026 20:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jeqcho

jeqcho commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this ptlnextdoor, and sorry for the awkward outcome: #146 was opened a day earlier by the contributor who had claimed #145 in the issue thread, so it got priority and just merged. Closing this one as a duplicate.

That said, your PR got real things right that the merged one didn't. _apply_epochs_or_exit mirroring _resolve_or_exit is the shape this code should have, down to the import hygiene (moving replace into the helper, Task under TYPE_CHECKING). The pytest.raises(match=...) exact-message pinning is stronger than the substring asserts that landed. And you correctly spotted that the old close-embodiment test was asserting the bug itself.

I opened #152 to bring exactly those pieces into main, and your branch is most of the way there already. If you want to send it, it's yours; comment on #152 to claim it. Hope to see more from you either way.

@jeqcho jeqcho closed this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] --epochs 0 (or negative) crashes with a raw traceback instead of a guided error

3 participants