Skip to content

fix(cli): guard --max-action-delta against invalid explicit values#155

Merged
jeqcho merged 2 commits into
robocurve:mainfrom
Tushar2604:fix/max-action-delta-validation
Jul 21, 2026
Merged

fix(cli): guard --max-action-delta against invalid explicit values#155
jeqcho merged 2 commits into
robocurve:mainfrom
Tushar2604:fix/max-action-delta-validation

Conversation

@Tushar2604

Copy link
Copy Markdown
Contributor

Summary

An explicit invalid --max-action-delta silently weakened guardrails instead of erroring.

The bug:

  • _build_guardrails wraps DeltaLimitApprover(space, max_delta=max_action_delta) in a blanket except ValueError and degrades to a stderr warning for any failure.
  • That degrade-per-component design is correct for a derived default an embodiment's space can't support — there was never a working default to lose.
  • It is not correct for an explicitly passed, malformed value (negative, zero, non-finite). That is a CLI input error, not an embodiment limitation, but it hit the same blanket handler and got downgraded to a warning.
  • Result: the operator asks for a tighter per-step limit, mistypes it, and the run proceeds on real hardware with clamp-only guardrails and exit code 0 — silently, unless they are watching stderr.

Changes

  • Validate args.max_action_delta in _check_shared_run_conflicts, which already runs before component resolution in both run and eval-set, and already houses the sibling --disable-guardrails conflict check. A non-finite or non-positive value now exits with a guided SystemExit before anything resolves or energizes.
  • No change needed in _build_guardrails or DeltaLimitApproverargparse's type=float means the CLI value is always a plain float, so the other failure mode in _validate_max_delta (array broadcast) can only be reached via the Python API, not this flag.
  • Derived-limit degradation is untouched: an embodiment with no declared bounds and no explicit --max-action-delta still warns and continues, exactly as before. Verified manually and confirmed the existing test_cli_degraded_guardrails_warn_but_run still passes unchanged.
  • Added parametrized tests for both run and eval-set (-1, 0, inf, nan).
  • Added a CHANGELOG.md entry under UnreleasedFixed.

Checklist

  • pre-commit hooks pass — pre-commit run --all-files shells out to uv, not on PATH here; ran ruff check ., ruff format --check ., mypy, and pytest --cov directly, plus the full suite on a Linux checkout.
  • Tests added/updated — 8 new parametrized cases (4 values × run/eval-set).
  • Coverage stays at 100% — verified on Linux: 100.00% total, 738 passed.
  • ruff check . and ruff format --check . pass
  • mypy passes (strict) — full run, 63 source files.
  • CHANGELOG.md updated under "Unreleased" — Fixed entry.
  • Public API unaffected — _check_shared_run_conflicts is a private CLI function.
  • Core stays NumPy-only — no new deps.

Note: I did not run the docs-build job locally (it needs a full Node/npm toolchain for the Docusaurus site). Confirmed by inspection this diff is out of scope for it — no changes to docs/, website/, or any public docstring, and CHANGELOG.md is not referenced by the site build.

Related

Closes #154.

Tushar2604 and others added 2 commits July 21, 2026 14:08
An explicit --max-action-delta that is non-finite or non-positive was
caught by _build_guardrails's degrade-per-component except ValueError and
downgraded to a stderr warning, so the run proceeded on clamp-only
guardrails instead of the clamp + delta-limit chain the operator asked
for. That degrade path is correct for a *derived* default an embodiment's
space can't support; it should not also swallow a malformed explicit
value, which is a CLI input error.

- Validate args.max_action_delta in _check_shared_run_conflicts, which
  already runs before component resolution in both run and eval-set and
  already houses the sibling --disable-guardrails conflict check. A
  non-finite or non-positive value now exits with a guided SystemExit
  before anything resolves or energizes.
- argparse's type=float means the CLI value is always a plain float, so
  DeltaLimitApprover._validate_max_delta's other failure mode (array
  broadcast) can only be reached via the Python API, not this flag; no
  change needed there.
- Derived-limit degradation (no explicit flag, an embodiment declaring no
  bounds) is untouched: verified manually and via the existing
  test_cli_degraded_guardrails_warn_but_run, which passes no explicit
  --max-action-delta and still warns-and-continues.
- Add parametrized tests for run and eval-set (-1, 0, inf, nan) and a
  CHANGELOG entry under Unreleased -> Fixed.
@jeqcho
jeqcho merged commit 4db20b8 into robocurve:main Jul 21, 2026
18 checks passed
@jeqcho

jeqcho commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Merged, thank you @Tushar2604. Two things stood out. Putting the guard in _check_shared_run_conflicts instead of _build_guardrails was the right architectural read: the catch-and-warn path exists for derived defaults an embodiment can't support, and your fix closes the explicit-input hole without touching that behavior (leaving test_cli_degraded_guardrails_warn_but_run untouched made that easy to confirm). And the parametrized tests covering -1, 0, inf, and nan on both run and eval-set pin the contract exactly where the bug lived.

Good hunting on this one. More where it came from is always welcome.

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] An explicit invalid --max-action-delta silently weakens guardrails instead of erroring

2 participants