Repository navigation
feat(v1): robot.tool and sim.tool, one agent tool generated from a device's verbs - #4778
Conversation
d6b3002 to
171e1af
Compare
5ff6d4d to
50604f4
Compare
307493a to
dff1af6
Compare
dff1af6 to
e55d77a
Compare
b7ea2cb to
119ab73
Compare
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose spec is derived from the manifest's rpcs (describe, observe, act, run, stop, hold, estop; resume/calibrate are refused with PermissionError before any lookup) and, for the world, from Simulation's public methods whose parameters all cross the wire. Schemas come from an ast pass over each signature, so stop_when/stop_event fall out and policy becomes a spec string (as_policy now accepts str | PathLike via the factory). Dispatch rewrites an agent's act to confirm_act and validates arguments against the declared rpc first, so confirmed=/confirm= from the model are rejected rather than honoured (decision 7 of #4749). from_agent() is a ContextVar the run loop reads once per _loop, choosing confirm_act for every step; start copies the context into the Task thread so a background run is gated the same way. lockstep moves to run/lockstep.py and Robot.run/Robot.start become the loop functions themselves, so the manifest and the tool see the full keyword set with no **kw. strands is imported only inside tools/agent.py behind the new [tools] extra. The diff matches the title and description; the drawings/changelog fragment are in place.
What's good
- Gate semantics are pinned by tests on a fake bus: refused
actwrites zero goal frames;confirmed=Truefrom the model is rejected as an unknown argument ofact; an agentrunwithconfirm=asks once per step;calibrate/resumeput no byte on the bus. - Argument validation runs against
handler(device, rpc)(the declared verb) before anything moves, which is what keeps theconfirm_act-only keywords off the model's surface. start()lost its**kwsink and spells everyrunkeyword;test_robot_run_has_the_run_loops_own_signature...keeps the two in lockstep.- Lazy
strandsimport verified in a fresh interpreter; import-linter contracts updated and kept (7/7);mypy,ruffclean; full unit suite green (367 passed, 47 skipped) on a fresh env with[dev,mujoco,tools];scene.py --all --verifyreports no drift.
Verification suggestions
tests/tools/test_sim_tool.py::test_an_agent_builds_a_scene_steps_and_renders_an_image_blockneeds an offscreen GL backend on a display-less box (MUJOCO_GL=eglorosmesa); with the default GLFW it fails onX11: Failed to open display, which is the environment, not the PR.- Spot-check the generated
runschema and confirm it is intended thatrecordandpushare model-callable:python -c "from strands_robots import Robot; from strands_robots.tools import tool_spec; import json; print(json.dumps(tool_spec(Robot('so101'))['inputSchema']['json']['properties']['run']['properties'], indent=1))". Today a tool call canrun(policy=..., record='hf://owner/name', push=True)with the process's ambient HF credentials and no gate on that path (the operator gate covers motion only). If that is not the intent,parameters(..., exclude=)already exists intools/schema.pyand is currently unused.
|
Merged |
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input schema is {rpc: <enum>, <rpc>: {args}}, generated by an ast pass over each verb's signature (tools/schema.py), so stop_when/stop_event/callables fall out, Action becomes a per-joint number object, and policy becomes a spec string (as_policy now routes str | PathLike through the factory). Dispatch (tools/call.py) validates the model's arguments against the declared rpc first and only then swaps act for confirm_act, so confirmed=/confirm= from a model are refused as unknown arguments rather than honoured; record/push are refused as developer-only; resume/calibrate are never offered; hold on a body with a gate asks the operator before torque goes on. from_agent() is a ContextVar the run loop samples once per _loop/_lockstep to choose confirm_act, and start() copies the context into the Task thread. lockstep moves to run/lockstep.py, Robot.run/Robot.start are the loop functions themselves (no **kw sink), and the world gains pose(), live positions in describe(), remove() of objects/cameras, typed vectors via geometry.vec, and run({id: spec}). strands is imported only inside tools/agent.py behind the new [tools] extra. The diff matches the title and description; changelog fragment and drawings are in place.
Reviewed the PR-specific diff at 67511f44; head befc3f1d is that commit plus a dev merge (#4788) with no PR-specific changes. Spot-checked locally: tests/tools 40 passed / 1 skipped; whole tests/ green apart from Isaac EULA-gated environment errors; import strands_robots pulls neither strands nor PIL; every new module imports alone in a fresh interpreter; the three prior-review threads (record/push model-callable, CodeQL cyclic-import notes) are resolved in this head or answered on the thread.
What's good
- The operator gate is enforced structurally, not by trust: argument validation runs against
handler(device, rpc)beforetarget()'sconfirm_actsubstitution, so no tool argument can stand in for the operator (pinned on a fake bus intests/tools/test_tool_gate.pywith zero goal frames on refusal). - Tool errors are reported to the model as
status: errortext, never swallowed into a default action (AGENTS.md > Key Conventions 5/6). start()lost its**kwsink so the manifest and the tool see the full keyword set, andcontextvars.copy_context()keeps a background run gated.reset(seed=)is removed loudly (TypeError), not silently; no in-tree caller.
Verification suggestions
- The description says 12 actions on
sim.tool; the generated list is 14 (add_robot, add_object, camera, contacts, control_step, describe, pose, randomize, remove, render, reset, run, set_pose, step). Print it withpython -c "from strands_robots.simulation import Simulation; from strands_robots.tools.world import WorldDevice; print([r.name for r in WorldDevice(Simulation()).manifest.rpcs])"and confirmcontacts/control_stepare meant to be model-callable. holdthrough the tool feeds the measured pose to the full safety chain, so on a real arm resting slightly past a declared joint limit it is refused byJointLimitValidatorrather than holding where it is; worth a spot-check on hardware that this is the intended fail-closed posture.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input schema is {rpc: <enum>, <rpc>: {args}}, generated by an ast pass over each verb's signature (tools/schema.py) so callables/events fall off the wire, Action becomes a per-joint number object, and a policy becomes a spec string (as_policy now accepts str | PathLike via the factory). Dispatch (tools/call.py) refuses resume/calibrate with PermissionError before any lookup, refuses record/push by name, validates arguments against the declared rpc before swapping act for confirm_act, and gates hold through gate() so torque never comes on without the operator. run/source.py adds a from_agent() contextvar that the run loop reads to pick confirm_act over act on every step, and start() copies the context into the Task thread. Robot.run/Robot.start become the loop's own functions (explicit keywords, no **kw), lockstep moves to run/lockstep.py, and the world grows pose(body), live poses in describe(), vec()-validated fixed-length vectors, remove of objects/cameras, randomize returning what it drew, and reset(keyframe=) refusing an unknown keyframe (seed= dropped, loudly). strands is imported only inside .tool.
What's good
- The gate semantics hold on every path a model can reach:
act->confirm_act,rungated per step viais_agent(), backgroundstartcarries the context to its thread,holdgated,resume/calibratenever resolved,confirm/confirmedrefused as tool arguments.sim.toolcannot touch hardware (locksteprejects a robot not in the world;add_robotexposes placement args only). - Checked locally against the head tree: every new module imports alone in a fresh interpreter,
pytest tests/toolsis green (40 passed, 1 skipped), and the spec probes above confirm 7 robot actions / 14 world verbs withclose,add,manual_time,default,unique_idand thetoolproperty itself excluded. - Scene edits keep the rollback contract (
_growpops on a failed rebuild), and malformed model inputs (../names, missing files, negativestep, a list forpolicies) come back as error text rather than escaping the tool.
Verification suggestions
- The PR is currently
DIRTYagainstdev: #4789 landed after the merge base and edits theexcept PolicyFaultclauses in both_loopand the old in-file_lockstepto also catchNonFiniteTarget. When resolving, make sure the catch lands inrun/lockstep.py(where_lockstepnow lives) as well asrun/loop.py, then re-runpytest tests/tools tests/test_run.py tests/test_operator_gate.py. - Optional spot-check of the thread hand-off:
with from_agent(): task = real.start(policy, steps=3)on a fake-bus robot withconfirm=Noneshould raiseRequestRefusedattask.stop()and leave zero goal frames on the bus (covered bytest_a_background_start_inside_an_agent_call_carries_the_gate_to_its_thread).
befc3f1 to
ac8a5de
Compare
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input schema is {rpc: <enum>, <rpc>: {args}}, generated by an ast pass over each verb's signature (tools/schema.py) so callables and events fall off the wire, Action becomes a per-joint number object and policy a spec string (as_policy now routes str | PathLike through factory.make). tools/call.py validates a model's arguments against the declared rpc before swapping act for confirm_act, refuses resume/calibrate outright, refuses record/push by name, and gates hold through gate(). run/source.py adds the from_agent() contextvar the loop samples once per _loop/_lockstep, and start() copies the context into its Task thread. Robot.run/Robot.start become the loop functions themselves (explicit keywords, no **kw), lockstep moves to run/lockstep.py, and the world grows pose(), live poses in describe(), vec()-typed vectors, remove of objects/cameras, randomize returning what it drew, reset(keyframe=) refusing an unknown keyframe (seed= dropped loudly), and run({id: spec}). strands is imported only inside tools/agent.py behind the new [tools] extra. Diff matches the title and description; changelog fragment and drawings are in place. Head ac8a5de0 is a single commit on top of current dev (#4789 included; the NonFiniteTarget catch is present in run/lockstep.py as well as run/loop.py).
What's good
- The motion gate is structural: argument validation runs against
handler(device, rpc)first, soconfirm=/confirmed=from the model are unknown arguments rather than honoured; a refusedact,run,startorholdon a fake-bus robot puts zero goal frames on the bus (tests/tools/test_tool_gate.py). is_agent()is read once per loop andcontextvars.copy_context()carries it into theTaskthread, so a background run is gated the same as a foreground one.- Checked on the head tree: every new module imports alone in a fresh interpreter (
robot.run.loop,robot.run.lockstep,robot.robot,tools,tools.agent,simulation),pytest tests/tools40 passed / 1 skipped, the rest oftests/green apart from 5torchcodecshared-library failures that are the box, not the PR;ruff check,ruff format --checkclean;mypyclean apart from a pre-existingyamlstub note inpolicy/lerobot/contract.py. - Adversarial
sim.toolinputs (size=[0,0]camera then render, negative/strstep, negative object size,/etc/...xmlas a robot name, a non-MJCFpath, reversedrandomizerange, unknown keyframe/body) all come back asstatus: errortext; nothing escapes the tool and nothing segfaults.
Verification suggestions
- The inline comment is about the
run.policyspec on a hardware robot; the shape is reproducible without a server:python -c "from strands_robots.policy.factory import make; print(make('zenoh://203.0.113.9:7447/x'))"(with the[lerobot]extra) shows the form is accepted with no gate on the host, andstrands_robots/tools/call.py::dispatchnever looks at the value.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input schema is {rpc: <enum>, <rpc>: {args}}, generated by an ast pass over each verb's annotations (tools/schema.py) so callables and events fall off the wire, Action becomes a per-joint number object and a policy a spec string (as_policy now routes str | PathLike through factory.make). tools/call.py::dispatch is the single choke point: resume/calibrate are refused as operator-only, record/push and a policy naming a network address are refused as the developer's, act is rewritten to confirm_act, hold is gated at the measured pose, and the whole call runs under from_agent() (a ContextVar) so run's loop and lockstep write through confirm_act on every step, with start copying the context into its thread. Robot.run/Robot.start become the loop's own functions so the spec reads their full signatures; lockstep moves to run/lockstep.py; Simulation gains pose(), live poses in describe(), remove of objects and cameras, a Literal shape enum, vec()-validated vectors, randomize returning its draws, reset(keyframe=) validation (dropping the unused seed=, which now fails loudly), and run() keyed by robot id. strands and pillow form the new [tools] extra, imported only when a tool is built.
Verified on the head commit: pytest tests 426 passed / 48 skipped (the tools suite 42 passed / 1 skipped), ruff and mypy clean, all 7 import-linter contracts kept, and no heavy module in sys.modules after import strands_robots. The earlier policy="zenoh://..." concern is closed by the REMOTE_SPEC check that runs before as_policy()/attach, pinned by test_an_agent_never_chooses_where_the_cameras_stream.
What's good
- The gate is structural rather than per-action:
from_agent()+is_agent()at the write site meansact,run,lockstepand a backgroundstartall inherit the operator gate without each tool action re-implementing it, andtest_tool_gate.pypins it against a fake bus with zero goal frames written on refusal. - The spec is derived, not maintained:
test_a_manifest_change_changes_the_tool_with_no_tool_code_editand theNotOnTheWirerefusal for a required un-wireable parameter make the "new rpc = new action" claim testable, andtest_the_models_own_claim_is_not_a_confirmationcloses the obviousconfirmed=Truesmuggling path. - Lazy import is pinned in a fresh interpreter (
test_tool_imports.py), so the[tools]extra stays optional for core users. - Changelog fragment present;
loop.pystays under the layout cap by splittinglockstepout rather than trimming it.
Verification suggestions
- The one gate CI cannot run:
STRANDS_ROBOTS_LIVE=1 pytest tests/tools/test_live_model.pywith Bedrock credentials, to confirm a real model still produces theobservethenactordering against the current spec wording.
…vice's rpcs Part 8 of the v1 stack, rebased onto dev as one commit (the branch's history is the squash-merge the owner would make): - tools: a device's strands tool spec, generated from its rpcs and their signatures - robot: .tool on Robot and Simulation; an agent's run passes the safety chain and the operator gate on every step - docs(drawings): d08, the verbs become one tool - tools: the world tool reads live poses, types every vector, runs robots in lock-step by id (run/lockstep.py) - fix: an agent's hold asks the operator first; record= and push= are never a model's On dev adf9db7: NonFiniteTarget is a fault in lockstep too; _sceneops._grow and the flattened Contact() keep every module under 250 lines.
… network address (zenoh://...) is the developer's to pass, refused before attach
1750161 to
9062902
Compare
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input is {rpc: <enum>, <rpc>: {args}}, with each rpc's parameter object generated by an ast pass over the method's annotations (tools/schema.py), so callables, events and live objects fall off the wire, Action becomes a per-joint number object and a policy becomes a spec string (as_policy now routes str | PathLike through factory.make). dispatch (tools/call.py) is the single choke point: resume/calibrate are operator-only, record/push developer-only, an agent's act is rewritten to confirm_act, hold on a gated robot asks the operator first, and a zenoh:// policy spec is refused before attach. from_agent() (run/source.py) is a ContextVar the run loop reads once to pick confirm_act over act, and start() now copies the context into the task's thread. Robot.run/Robot.start become the loop's own functions so the spec reads their full signatures; lockstep moves to run/lockstep.py and Simulation.run accepts ids as keys. On the world side, vec() in geometry.py makes every vector argument typed and length-checked, remove takes an object or camera, pose(body) and describe() report live poses, randomize returns what it drew, and reset drops seed and refuses an unknown keyframe. Tests cover the spec shape, the gate on a fake-bus SO-101 (act/run/start/hold/calibrate/resume), the record/push and network-address refusals, world verbs, and a scripted-model agent turn; tests/tools is green at the head (42 passed, 1 skipped).
What's good
- The gate test matrix is the right one for a model-in-front-of-hardware component: every refusal asserts zero goal frames on the fake bus, and
confirmed=/confirm=from the model are rejected atdecodebecause the declaredactsignature, notconfirm_act, is what the arguments are checked against. start()carryingcopy_context()into theTaskthread closes the obvious hole where a background run would silently fall back toact.strandsandPILare imported only when a tool is built, pinned by a fresh-interpreter test, and the import-linter contracts are updated rather than loosened.
…point, not one argument name
dispatch gathers both shapes (robot.tool's policy= and sim.tool's
policies={id: spec}) and refuses a spec naming a network address before
anything is built; bind() refuses the same under from_agent(), so every
current and future entry point (lock-step, start, a fleet verb) is covered.
The rule (REMOTE_SPEC) lives in run/source.py with is_agent(). Pinned in
test_an_agent_never_chooses_where_a_worlds_cameras_stream: sim.tool run with
policies={a: zenoh://...} is an error naming the developer, attach is never
reached, and sim.run({...}) under from_agent() is a PermissionError.
# Conflicts: # pyproject.toml
|
Head |
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Adds strands_robots/tools/ and the robot.tool / sim.tool properties: one strands PythonAgentTool per device whose input is {rpc: <enum>, <rpc>: {args}}, with each rpc's parameter object generated by an ast pass over the method's annotations (tools/schema.py) so callables, events and live objects fall off the wire, Action becomes a per-joint number object and a policy a spec string (as_policy now routes str | PathLike through factory.make). resume/calibrate are never offered, record/push are refused, and a policy spec naming a network address is refused both in dispatch and in bind() under from_agent(). Inside the tool an act becomes confirm_act, hold is gated on the measured pose, and run/lock-step write through confirm_act when is_agent(); start copies the contextvar into its thread. Robot.run/start become the loop's own functions (no **kw swallowing), lockstep moves to run/lockstep.py, Simulation gains pose(), live poses in describe(), remove() of objects/cameras, randomize() returning what it drew, reset(keyframe=) validation (and drops the unused seed= loudly), and run() keyed by robot id; vectors go through geometry.vec so a string or wrong-length vector is refused by name. strands is imported only in tools/agent.py on first .tool access, behind the new [tools] extra.
Both prior [MUST FIX] threads on tools/call.py (model-chosen remote policy spec via policy= and via policies={id: spec}) are resolved at this head: the refusal gathers both shapes before target() and bind() refuses the same under from_agent(), each pinned by a test with ZenohPolicy.attach spied. No new blockers found.
What's good
- Fail-closed by construction:
dispatchvalidates against the declared rpc's signature before substituting the gated form, so a model'sconfirmed=True/confirm=Trueis refused as an unknown argument rather than honoured. _SceneOps.lockis anRLock, so the new nested_grow()underadd()'s lock is safe;_growrolls the scene back on a failed rebuild.- At this head on a MuJoCo-only box:
pytest tests/tools43 passed / 1 skipped;pytest tests(Isaac files excluded, EULA-gated here) 661 passed / 5 skipped;ruff check,ruff format --checkandmypy strands_robotsclean.
Verification suggestions
STRANDS_ROBOTS_LIVE=1 pytest tests/tools/test_live_model.pywith Bedrock credentials, to confirm the generated schema is accepted end-to-end by a real model (the scripted-model tests do not exercise provider-side schema validation, e.g.enumcontainingnullalongsidetype: ["string", "null"]).- Note the manifest's
RPCSdoes not includestart, sorobot.toolexposes 7 actions (describe, observe, act, run, stop, hold, estop); the description's "run/start" wording refers to thefrom_agent()propagation, whichtest_a_background_start_inside_an_agent_call_carries_the_gate_to_its_threadcovers directly.
Part 8 of #4749 · roadmap #3818
In one paragraph
Before this, giving an agent a robot meant hand-writing a tool spec and keeping it in sync with the API by hand. Now
robot.toolandsim.toolare each a single strandsAgentToolgenerated from the device's own verbs: onerpcselector plus one parameter object per rpc, whose JSON schema is read straight from each method's Python signature (anastpass, so nested generics and unions are exact and a parameter that cannot cross a wire is left out or, if required, refused). A new rpc is a new tool action with nothing hand-written. A command from a tool is treated as an agent's: insidefrom_agent()anactis dispatched throughconfirm_act, so the safety chain and the operator gate run on every step, including a backgroundrun/startwhose context travels to its thread;resumeandcalibratestay the operator's and a model never sees them.strandsis imported only when a tool is built (the[tools]extra).What it looks like
Rendered headless in MuJoCo as agent-style
acttool calls land through the gated dispatch path; shoulder_pan swings across 0.9 rad each cycle.Two so101 arms driven in lock-step by one
sim.toolruncall per frame ({"a": ..., "b": ...}), rendered headless in MuJoCo. Measured in the same run: pans 0.603 / -0.603 rad after 1.6 s; the cube dropped from z 0.40 anddescribe()andpose("cube")both read z 0.025.Measured
pytest tests/toolspytest tests/tools/test_world_verbs.pydevMUJOCO_GL=egl pytest testsSTRANDS_ROBOTS_LIVE=1 pytest tests/tools/test_live_model.pypytest tests/tools/test_tool_gate.pypytest tests/test_run.py tests/recording tests/test_operator_gate.pyruff check; ruff format --check; mypy strands_robots; lint-importspytest tests/test_layout.pytool spec: 7 actions on robot.tool, 14 on sim.tool.
Under the hood
Landed from the reference
robots-v1-2(part 8's files per its SNAPSHOT.md):tools/{schema,spec,wire,world,call,agent}.py,robot/run/source.py,tests/tools/. What differs from the reference: the reference's run loop had been reshaped around an earlierdev; here the gate and thefrom_agentcontext are added to the loop as it stands ondev(events,record=), andlockstepis split intorun/lockstep.pysoloop.pystays under the 250-line layout cap.Robot.run/Robot.startbecome the loop's own functions (robot first) so the tool spec reads their full signatures; a policy spec string is accepted wherever a policy is.The real turn's tool calls:
Files: 7 new in
strands_robots/tools/,robot/run/{source,lockstep}.py;robot.py,simulation.py,run/loop.py,run/__init__.py,policy/callable.py,pyproject.tomltouched. Package ~430 lines, tests ~550, in line with the reference module.Refs #3818