fix(policies/groot): a reply the client cannot read names the peer, not the codec - #3677
Merged
cagataycali merged 2 commits intoSep 15, 2026
Merged
Conversation
…ot the codec Gr00tInferenceClient.call_endpoint decoded every reply frame ungraded. One wire fault - something on the port that is not the msgpack REQ/REP GR00T policy server - reached the caller as three different outcomes, two of them silent, none naming the host, the port or the request answered: an undecodable frame raised the codec's own ExtraData; a msgpack str was returned as the declared dict and failed one frame later in the policy on .items(); an int died on the 'error' in response membership test. Grade the reply at a _decode_reply seam that knows the peer and the endpoint, in the shape the moveit2 client now uses, with one difference the reference server forces: a list is admitted, because PolicyServer packs get_action's (action, info) tuple as a 2-element list. get_action then grades that envelope itself rather than returning whatever list arrived as the action dict. The 'error' field is read only off a map, which is the reference client's own guard. MsgSerializer.from_bytes is annotated Any, which is what unpackb returns.
yinsong1986
approved these changes
Sep 15, 2026
yinsong1986
left a comment
yinsong1986
left a comment
Contributor
There was a problem hiding this comment.
Summary
Grades the GR00T ZMQ client's reply direction at a single _decode_reply seam: bytes that are not msgpack and values that decode to neither a map nor a list are both refused with a ConnectionError naming the peer URI, the endpoint answered, the problem in the codec's own words, and the frame's opening bytes - replacing three divergent pre-fix outcomes (a bare ExtraData, a TypeError from the membership test, and a scalar silently returned as the declared dict). Unlike the MoveIt2 sibling (#3676), a list is deliberately admitted at the seam because the reference server's get_action returns (action, info) as a 2-element list; get_action then grades that envelope itself instead of returning whatever list arrived. The error-field read is now guarded by isinstance(response, dict), matching the reference client, and MsgSerializer.from_bytes is honestly annotated -> Any.
What's good
- Verified locally at head
64ea1062: the new suite (33 cells) and the fulltests/policies/grootdirectory (427 passed) are green, ruff is clean, and the relevant whole-tree graders (docstring xrefs,Raises:completeness, test-name, ASCII) all pass. - Controls pin the unchanged contract precisely:
{"error": ...}keeps itsRuntimeError(and is asserted not to be aConnectionError),(action, info)still unpacks, the legacy bare action map still returns, a list reply is not indexed forerror,pingstill absorbs and returnsFalse, andGr00tPolicy.resetstill continues while logging the peer. ConnectionErrorwithout a private subclass is justified in the docstring by the actual catch topology (pingandresetabsorb by contract; nothing else between the seam and the caller catchesOSError), and theexcept (TypeError, ValueError)tuple has no member covering another, per AGENTS.md > Review Learnings (#86) > Exception Clauses Must Be Narrow.- Changelog fragment named for the PR number, ASCII-only user-facing strings, no host paths, scope disciplined to the one seam plus its tests.
cagataycali
enabled auto-merge (squash)
September 15, 2026 13:10
github-project-automation
Bot
moved this from In review
to Done
in Strands Labs - Robots
3 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.
What
Gr00tInferenceClient.call_endpointdecoded every reply frame ungraded. One wire fault - something on the port that is not the msgpack REQ/REP GR00T policy server - reached the caller as three different outcomes, two of them silent, none naming the host, the port or the request answered. Measured through the public client againstmainat53e37a1:ExtraData: unpack(b) received extra data.ConnectionError: GR00T policy server at tcp://127.0.0.1:5555 answered 'get_action' with an unreadable reply: not msgpack (ExtraData: unpack(b) received extra data.); it beginsb'<html>502 Bad Gateway</html>'...ExtraDatab"ERROR"sentinelExtraData42TypeError: argument of type 'int' is not iterableexpected a msgpack map or list, got int'ok'str 'ok'as the declareddictexpected a msgpack map or list, got str'internal error'TypeError: string indices must be integersexpected a msgpack map or list, got str[1, 2]throughget_action1as the action dictConnectionError: ... answered 'get_action' with a list this client cannot read as an action chunk: expected (action, info) ... got a list of 2 whose elements are ['int', 'int']This is the GR00T sibling of #3676 (MoveIt2), found by reading the other
MsgSerializer.from_bytes(self.socket.recv())site in the package. Same seam, same report shape, one contract difference the reference server forces (below).Why
The reference client (
gr00t.policy.server_client.PolicyClient) already refuses one wrong-peer frame by name - a bareb"ERROR"gets "Make sure we are running the correct policy server" - and reads theerrorfield only off a reply thatisinstance(response, dict). Our client had neither guard:"error" in responseis a membership test, so a string answered itFalsewithout raising and was returned as the declareddict, to fail one frame later inGr00tPolicy._unpack_service_actionswithAttributeError: 'str' object has no attribute 'items'; anintraisedTypeErrorfrom the test itself; a string containing"error"took the server-error branch and raisedTypeError: string indices must be integers.Why a list is admitted here where #3676 refuses one.
PolicyServer.runpacks each handler's return value as-is, andget_actionreturns(action, info), which msgpack carries as a 2-element list - so the GR00T reply contract is map or list, and the reference client'sisinstance(response, dict) and "error" in responseis what lets that list through._decode_replygrades the wire (one msgpack value, map or list);get_actionthen grades the envelope it alone understands (2 elements, first a map, or a bare legacy action map) instead of returning whatever list arrived. That second door was the same silent half at a different depth:[1, 2]returned1, and any other length returned the list itself, each to fail on.items()later.ConnectionErrorneeds no private subclass: nothing between the seam and the caller catchesOSError.pingstill absorbs it (any failure means "not reachable"), andGr00tPolicy.resetstill logs it and continues - what changes is that the INFO line now names the peer and'reset'where it loggedunpack(b) received extra data.MsgSerializer.from_bytesis annotated-> Any, which is whatunpackbreturns.Tests
New
tests/policies/groot/test_unreadable_reply_names_the_peer.py: 21 fail pre-fix, 33 pass after. Tables over 6 undecodable frames (including the legacyb"ERROR"sentinel), 5 decodable scalars and 4 malformed action envelopes; cells pin that both wire doors report in one class, that the endpoint is named rather than assumed, that the codec's own words are quoted, that the cause is preserved, and that the envelope report names element types rather than rendering an array. Controls pin the unchanged contract: a map round-trips,{"error": ...}still raisesRuntimeError,(action, info)still unpacks, a bare action map still returns, a list reply is not indexed forerror,pingstill returnsFalse, andresetstill continues.10/10 mutants caught (map-or-list guard removed, cause dropped, endpoint / uri / frame preview / codec detail / decoded type / remedy dropped, envelope refusal removed, dict-guard on the
errortest dropped), 1-14 cells each.Gate:
ruff check+format --checkclean,mypy0 issues ingroot/client.py(0 onmaintoo), 653 passed acrosstests/policies/groot+ the two ZMQ-domain siblings. Whole-tree roster (145 graders): 4169 passed; every failure/error ismujoco/cv2/serialabsent from the venv or the utf8 subprocess cell unable to import the uninstalled package - the same 3 fail on unmodifiedmainin the same venv, delta zero.Composition with #3577.
check_merge_base_overlap.py --all-openpairs this with #3577 by name (its test reachesstrands_robots.policies.groot.policy, which #3577 edits; neither edits a path the other does).git merge-tree --write-treeof the two heads: 0 conflicts;tests/policies/groot/on the composed tree: 425 passed, 2 skipped, including all 33 cells here.LOC: +148/-10 production, +250 tests.