improve: stricter input validation and default loopback in gr00t_inference#90
Open
cagataycali wants to merge 3 commits intostrands-labs:mainfrom
Open
improve: stricter input validation and default loopback in gr00t_inference#90cagataycali wants to merge 3 commits intostrands-labs:mainfrom
cagataycali wants to merge 3 commits intostrands-labs:mainfrom
Conversation
…rence
Improvements to the gr00t_inference tool:
1. Input validation for all user-supplied parameters:
- data_config and embodiment_tag validated against strict alphanumeric
patterns (they are enumerable values from the docstring).
- checkpoint_path and trt_engine_path reject shell metacharacters,
null bytes, and '..' traversal components.
- container_name validated against Docker naming rules.
- dtype values checked against explicit allowlists.
- Port range validated (1-65535).
2. Default host changed from 0.0.0.0 to 127.0.0.1 (loopback):
- Inference services should default to localhost-only binding.
- Users can still explicitly pass host='0.0.0.0' when network
access is needed.
3. Process verification for stop action:
- Added _is_gr00t_process() to verify a PID belongs to a GR00T
inference process before sending signals.
- Host-system fallback now uses pgrep -f with the inference_service
pattern instead of lsof (which matches any process on the port).
awsarron
requested changes
Apr 3, 2026
Member
awsarron
left a comment
awsarron
left a comment
There was a problem hiding this comment.
would be good to add tests for these cases. gr00t_inference doesn't have any test coverage at the moment
Encapsulate all input validation (data_config, embodiment_tag, container_name, paths, dtypes, port range) into a single validate_inputs() function. This: 1. Keeps the tool function focused on orchestration 2. Makes validation independently testable 3. Raises ValueError consistently (no mixed return-dict errors) Tests: 15 new tests covering every validation branch.
Member
Author
Status Check1 unresolved thread — but the fix was already implemented in commit @awsarron's nit ("extract validation into single function") was addressed: all validation logic is now in a dedicated CI: ✅ All checks passing @awsarron could you resolve the thread and re-review? This is a small, self-contained improvement. 🤖 Pipeline analysis by AI agent. Strands Agents. Feedback welcome! |
45 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the
gr00t_inferencetool with comprehensive input validation, a safer default bind address, and more precise process management.Changes
1. Input validation for all user-supplied parameters
data_configandembodiment_tag: Validated against strict^[a-z][a-z0-9_]+$patterns — these are enumerable values fromdata_configs.json.checkpoint_pathandtrt_engine_path: Reject shell metacharacters (;,|,$, backticks, etc.), null bytes, and..path traversal.container_name: Validated against Docker's container naming rules.vit_dtype,llm_dtype,dit_dtype): Checked against explicit allowlists (fp16,fp8,nvfp4).2. Default host changed from
0.0.0.0→127.0.0.1Inference services should not bind to all interfaces by default. Users can still explicitly pass
host='0.0.0.0'when remote access is needed.3. Process verification in stop action
_is_gr00t_process()that inspects/proc/<pid>/cmdlineto confirm a PID belongs to a GR00T inference process before sending signals.pgrep -f inference_service.py(matches the actual process) instead oflsof -t -i:<port>(which matches any process on that port).Motivation
As an agent tool,
gr00t_inferencereceives string parameters that ultimately get interpolated intosubprocess.run()calls and Docker exec commands. Validating these inputs up front is both a robustness improvement (fail fast with clear errors) and defense-in-depth against accidental misuse.