Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
e0419d4
improve: stricter input validation and default loopback in gr00t_infe…
cagataycali Apr 1, 2026
0d9630f
style: apply ruff formatting
cagataycali Apr 1, 2026
bc3cc2e
refactor: extract validate_inputs() from gr00t_inference tool
cagataycali Apr 3, 2026
3c15558
fix: _is_gr00t_process now verifies port binding
cagataycali May 15, 2026
87b93a7
style: fix ruff formatting (extra blank line)
strands-agent May 16, 2026
3643fcc
fix: merge main, add torch mock seeding support for LIBERO tests
github-actions[bot] May 18, 2026
45d4256
fix: host-system fallback verifies port via /proc/<pid>/cmdline
cagataycali May 21, 2026
29ad002
fix: add word-boundary to pgrep port patterns in _stop_service
May 21, 2026
bceae06
fix: add explanatory comments to empty except blocks (CodeQL)
cagataycali May 21, 2026
5c83d6c
Merge remote-tracking branch 'origin/main' into improve/groot-input-v…
strands-agent May 22, 2026
9f365cf
fix: address PR #90 review feedback — host validation, narrow excepti…
cagataycali May 22, 2026
85fec29
chore: drop out-of-scope torch_mock comment (review feedback)
cagataycali May 22, 2026
af70002
fix: address review round-2 — narrow path denylist, suppress exceptio…
cagataycali May 22, 2026
71d71aa
fix: address review round-3 — revert host default to 0.0.0.0, accept …
cagataycali May 22, 2026
17f606f
style: fix ruff format for gr00t_inference and validation tests
cagataycali May 22, 2026
f448c50
fix: move _HOSTNAME_RE to module level, harden path traversal for bac…
cagataycali May 22, 2026
81bcee8
fix: address review round-4 — action allowlist, numeric host rejectio…
cagataycali May 22, 2026
ae4e609
fix: address yinsong1986 review — remove validate_inputs defaults, su…
cagataycali May 22, 2026
50fb2d7
fix: rename _read_only_actions → _port_only_actions, factor _PYTHON_P…
May 22, 2026
e332e01
fix: address review round-6 — loopback default, option-injection guar…
cagataycali May 22, 2026
4dbe73f
fix: N1.7 process identification, host=127.0.0.1 default, expanded va…
cagataycali May 22, 2026
64df853
fix: test timeout + host-path hygiene in gr00t validation tests
cagataycali May 22, 2026
e9a7375
fix: address review round-7 — sentinel host flip, hostname length cap…
cagataycali May 22, 2026
e8dd5d1
fix: remove duplicate comment line in gr00t_inference dispatch
cagataycali May 22, 2026
c3c02ef
fix: sentinel default for host — distinguish explicit from default
cagataycali May 22, 2026
a955680
fix: address review round-8 — restart host_was_explicit, volume colon…
cagataycali May 22, 2026
9def51f
fix: remove duplicate TestReviewRound8Fixes class + ruff format
cagataycali May 22, 2026
3b5039b
fix: address review round-9 — container_name on image-only branch, sy…
cagataycali May 22, 2026
8a54f0c
fix: use 'from None' to suppress exception chain in host validation (…
May 22, 2026
6cbff72
fix: address review round-10 — remove backslash from _SHELL_META, raw…
cagataycali May 22, 2026
8dd443e
fix: narrow remaining except Exception clauses, remove emoji, fix CHA…
cagataycali May 22, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,89 @@
All notable behavioural changes to `strands-robots` are logged here. Follows
[Keep a Changelog](https://keepachangelog.com/) conventions.

## Unreleased - #90 (gr00t_inference validation hardening)

### Added

- ``validate_inputs()`` centralises all parameter validation with action-aware
scoping: read-only actions (``find_containers``, ``list``, ``status``,
``stop``) only validate ``port``/``host``/``protocol``; mutating actions
(``start``, ``restart``, ``lifecycle``) validate the full parameter surface.
- ``host`` parameter now accepts both IP addresses and RFC-952 hostnames
(e.g. ``localhost``, ``host.docker.internal``). All-numeric strings that
fail ``ipaddress.ip_address()`` (e.g. ``127.0.01``, ``999.999.999.999``)
are rejected as obvious IP typos.
- ``protocol`` validation moved into ``validate_inputs()`` (previously
hand-rolled outside the helper, breaking the single-entry-point contract).
- ``pgrep`` patterns now match both ``--port N`` and ``--port=N`` forms,
preventing silent miss when the service is started with the ``=`` syntax.
- Integration tests that invoke ``gr00t_inference()`` end-to-end and assert
that invalid inputs are caught (pins the ``try/except ValueError`` wiring).
- End-to-end regression test for ``_stop_service`` cross-port-kill scenario:
verifies that a process on port 8000 is NOT killed when stopping port 80.
- Exception clauses narrowed throughout: ``_is_gr00t_process`` / ``_is_gr00t_host_process``
use ``(OSError, subprocess.SubprocessError, UnicodeDecodeError)``;
``_list_running_services``/``_is_service_running`` use ``OSError``;
``_stop_service`` uses ``(OSError, subprocess.SubprocessError)``;
``_start_service`` uses ``(OSError, RuntimeError)``.
Only ``_download_checkpoint`` retains ``except Exception`` (``# noqa: BLE001``)
because huggingface_hub raises varied, opaque exception types.
- ``action`` parameter validated against a complete allowlist of 10 valid
actions; unknown actions get a clear error with the valid set listed.
- ``image_name``, ``volumes``, and ``container_command`` parameters are now
validated (Docker image reference, path traversal, shell metacharacters).
- ``pgrep`` pattern factored into ``_PGREP_INFERENCE_PORT_FMT`` module-level
constant — single source of truth across all 4 usage sites.
- ``_PGREP_INFERENCE_PORT_FMT`` and ``_is_gr00t_*_process`` now match both
N1.5/N1.6 (``inference_service.py``) and N1.7 (``gr00t.eval.run_gr00t_server``)
entry-points. Closes the N1.7 stop/status identification gap.
- All exception types in process-probe helpers now log (WARNING for
PermissionError, DEBUG for other OSError/SubprocessError/UnicodeDecodeError).

### Changed

- ``validate_inputs()`` parameters are now all required (no defaults).
``gr00t_inference()`` is the single source of truth for default values;
the validator no longer duplicates them (prevents silent drift).
- ``_DOCKER_IMAGE_RE`` extended to support private-registry references with
port numbers (e.g. ``localhost:5000/myorg/img:tag``).
- ``_is_gr00t_process`` / ``_is_gr00t_host_process`` now require ``--port``
in the process cmdline to match — prevents false-killing editors or
log-tailers that happen to touch ``inference_service.py``.
- ``PermissionError`` in process probes now logs at WARNING level instead
of being silently swallowed.
- **BREAKING** Default ``host`` changed from ``0.0.0.0`` to ``127.0.0.1``
(loopback-only, per AGENTS.md). Container actions (``start``/``restart``/
``lifecycle``) auto-flip to ``0.0.0.0`` internally since Docker's
``-p {port}:{port}`` publish requires bind-all inside the container.
**Migration:** if your downstream connects from another host, pass
``host="0.0.0.0"`` explicitly.
- Host-system fallback (``pgrep``) is documented as Linux-only. Non-Linux
platforms will see "No service running" rather than silently succeeding.

### Fixed

- Duplicate ``torch_mock.manual_seed`` assignment in ``tests/mocks/torch_mock.py``.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ### Fixed bullet says "Default host remains 0.0.0.0 (no breaking change)", which directly contradicts the PR title ("default loopback in gr00t_inference") and the PR Summary section ("Default host changed to 127.0.0.1"). The actual diff keeps host: str = "0.0.0.0" everywhere, so this CHANGELOG bullet describes the truth -- but then the PR title and Summary need to be updated, otherwise the merged-commit log message and the gh pr view output will permanently disagree with the code. Two minutes to fix: edit the PR title to drop "default loopback" and rewrite the Summary's bullet 6 to match this CHANGELOG entry.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in ae4e609 — CHANGELOG restructured. The host default info is now under ### Notes (not ### Fixed) since no behaviour was actually fixed:

Default host remains 0.0.0.0 (no breaking change). The Docker container's -p {port}:{port} publish requires the service to bind all interfaces inside the container.

PR title already reads improve: stricter input validation in gr00t_inference (no "default loopback").


🤖 AI agent response. Strands Agents.

- Option-injection guard: ``repo_url``, ``repo_tag``, ``policy_name`` starting
with ``-`` are rejected (prevents git/docker flag injection via subprocess argv).
- Host-system fallback (pgrep) now returns a clear error on non-Linux platforms
instead of silently reporting success.
- All-numeric hostname guard narrowed to multi-label patterns only — single-label
numerics (e.g. ``123``) are valid per RFC-1123.

### Notes

- Host validation is **broader** than before for hostnames (RFC-952 names like
``localhost`` and ``host.docker.internal`` now pass) but **stricter** for
IP-like typos (all-numeric labels like ``127.0.01`` are rejected).
- Validation scope covers ``port``, ``host``, ``protocol``, ``data_config``,
``embodiment_tag``, ``container_name``, TRT dtypes, ``checkpoint_path``,
``trt_engine_path``, ``image_name``, ``volumes``, and ``container_command``.
Parameters ``repo_url``, ``repo_tag``, ``hf_repo``, ``policy_name`` are
NOT validated here — they flow into argv-style subprocess calls which
are not shell-injection-vulnerable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This entry directly contradicts the PR description (which advertises "Default host changed to 127.0.0.1" as a feature). The diff keeps host="0.0.0.0", so the CHANGELOG is correct, but please also (a) retract bullet 6 of the PR description and (b) update the PR title — default loopback is no longer true. Otherwise downstream readers (release notes, blog posts, agent prompts) will quote the title and be wrong. Consider also restructuring this bullet under ### Notes or ### Unchanged rather than ### Fixed — no behaviour was fixed here, the original plan was simply abandoned.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in ae4e609 — PR title already updated to improve: stricter input validation in gr00t_inference (no "default loopback"). CHANGELOG restructured with explicit ### Changed and ### Notes sections. The ### Fixed bullet about host remaining 0.0.0.0 is moved to ### Notes for clarity.


🤖 AI agent response. Strands Agents.


## Unreleased - #178 (LiberoOffScreenRenderEngine retired)

### Removed: ``LiberoOffScreenRenderEngine`` simulation backend (BREAKING)
Expand Down
Loading
Loading