Fix simulator skill failure handling#414
Conversation
Greptile SummaryThis PR fixes simulator skill failure handling across five files: it adds proper validation so bad inputs return clean user-facing messages, marks replay skills with missing artifacts as in-training, fails manipulation skills fast when FK/IK topics are absent, removes the unimplemented
Confidence Score: 5/5Safe to merge — all changes are defensive additions that improve error handling without altering the happy path. The new validation paths only fire on bad inputs or missing runtime artifacts, leaving normal execution untouched. The
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[execute_callback] --> B{Parse JSON inputs}
B -- failure --> C[Return: Invalid inputs JSON]
B -- success --> D{inputs is dict?}
D -- no --> E[Return: Inputs must be JSON object]
D -- yes --> F{Lookup skill type}
F -- code_entry --> G[_execute_code_skill]
F -- physical_entry --> H[_execute_physical_skill]
F -- in_training_entry --> I[Return: Skill not executable yet]
F -- not found --> J[Return: Skill not available]
G --> K{_validate_skill_inputs}
K -- error --> L[Return: validation error message]
K -- ok --> M{_sim_manipulation_unavailable_reason}
M -- topics missing --> N[Return: Arm manipulation unavailable]
M -- ok --> O[Execute skill.execute inputs]
style C fill:#ffcccc
style E fill:#ffcccc
style I fill:#fff3cc
style J fill:#ffcccc
style L fill:#ffcccc
style N fill:#fff3cc
style O fill:#ccffcc
Reviews (3): Last reviewed commit: "refactor: simplify skill input validatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _validate_skill_inputs(self, skill_id: str, skill, inputs) -> str | None: | ||
| """Return a user-facing validation error, or None if inputs are usable.""" | ||
| if not isinstance(inputs, dict): | ||
| return "Skill inputs must be a JSON object." |
There was a problem hiding this comment.
isinstance(inputs, dict) guard inside _validate_skill_inputs is unreachable in the current call flow. execute_callback already aborts and returns before reaching _execute_code_skill (the only caller of this method) whenever inputs is not a dict, so the check here will never fire. If the function is meant to be callable from other contexts in the future, a docstring note would be clearer; otherwise the guard is dead code.
| def _validate_skill_inputs(self, skill_id: str, skill, inputs) -> str | None: | |
| """Return a user-facing validation error, or None if inputs are usable.""" | |
| if not isinstance(inputs, dict): | |
| return "Skill inputs must be a JSON object." | |
| def _validate_skill_inputs(self, skill_id: str, skill, inputs) -> str | None: | |
| """Return a user-facing validation error, or None if inputs are usable. | |
| ``inputs`` is guaranteed to be a dict by the time ``execute_callback`` | |
| dispatches here, but the guard is kept for safety in case this helper | |
| is ever called from a different context. | |
| """ | |
| if not isinstance(inputs, dict): | |
| return "Skill inputs must be a JSON object." |
| topic_names = {name for name, _types in self.get_topic_names_and_types()} | ||
| missing_topics = [name for name in ("/fk_pose", "/ik_solution") if name not in topic_names] |
There was a problem hiding this comment.
get_topic_names_and_types() call traverses the full ROS 2 graph on every manipulation skill invocation in simulator mode. If many manipulation skills fire in quick succession this could add latency. Caching the result for a short TTL would be more efficient, though correctness is unaffected since the check already returns None in non-sim mode.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
pick_socks,wave) as unavailable/in-training instead of silently hiding themopen_doorskill and stop the security guard directive from advertising or requesting itValidation
python3 -m py_compile ros2_ws/src/brain/brain_client/brain_client/skill_loader.py ros2_ws/src/brain/brain_client/brain_client/skills_action_server.py workspace/innate_skills/scan_for_objects.py workspace/innate_agents/security_guard_agent.pygit diff --checkrg -n "open_door|open door" workspace/innate_agents workspace/innate_skills ros2_ws/src/brain -Sreturns no matchesSecurityGuardAgent().get_skills()now returns onlyinnate-os/navigate_to_positionandinnate-os/send_emailopen_doorremoval with./innate sim up --once; action smoke matrix confirmed invalid inputs return clean validation messages, manipulation skills return the simulator FK/IK unavailable message,scan_for_objectsreturns missing Gemini key,wavereturns missing runtime artifacts, andhead_emotionsucceeds