Repository navigation
feat(vended-tools): add swarm tool in Python [replica of harness-sdk PR 4922] - #33
poshinchen wants to merge 10 commits into
Conversation
…ke swarm call stream
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v4-reason (optimized) review — run af1746, Opus 4.8
Assessment: comment
Assessment: Comment
Solid, well-tested addition. The swarm tool is a clean shim over the SDK Swarm with a thoughtful authority-mode schema, depth-limiting, and good test coverage (schema building, spec resolution, limits forwarding, depth exhaustion, and an end-to-end child-stop case).
Review Categories
- Consistency: Preset ordering differs between the schema enum (
sorted) and the description (insertion order);mcp_serversis the only axis not normalized fromNoneto a default. - Side effects:
_resolve_specsmutates the model-supplied input dicts in place. - Robustness: Non-completion errors embed the full (potentially large)
SwarmResultin the exception message. - API process: Confirm
api/needs-reviewsign-off for the new publicmake_swarmsurface.
Nice work on the depth-propagation design and the comprehensive, shape-level test assertions.
| def _build_description(base: str, presets: Mapping[str, Preset]) -> str: | ||
| """Append preset information to the tool description so the model knows about them.""" | ||
| if not presets: | ||
| return base |
There was a problem hiding this comment.
Important
Issue: _build_agent_item_schema lists agent_type enum values via sorted(presets), but _build_description lists the available roles via presets.items() (insertion order). When these two orderings differ, the enum shown in the schema and the human-readable role list in the description are presented in a different order, which is confusing for the model and for anyone debugging the tool spec.
Suggestion: Use a single, consistent ordering for both. For example, sort in _build_description as well:
| return base | |
| roles = "\n".join(f"- {name}: {presets[name].description or name}" for name in sorted(presets)) |
| default_preset = next(iter(presets)) | ||
|
|
||
| if isinstance(mcp_servers, Choice): | ||
| if not mcp_servers.options: |
There was a problem hiding this comment.
Important
Issue: mcp_servers is the only axis not normalized to a default when None is passed. instructions defaults to Open() and model defaults to Inherit(), but mcp_servers is left as None and passed through to _resolve_specs/_build_agent_item_schema as-is. This asymmetry means the default swarm instance has mcp_servers=None while the others have concrete axis objects, which is surprising and forces downstream None handling. Was leaving it None intentional (i.e. "no MCP axis at all" differs from Inherit())?
Suggestion: If None and Inherit() are meant to behave identically, normalize mcp_servers here (mcp_servers = mcp_servers if mcp_servers is not None else Inherit()) for consistency; otherwise document the distinction in the docstring.
| raise ValueError(f"Agent at index {index} must be a dict, got {type(entry).__name__}") | ||
| name = entry.get("name") | ||
| if not isinstance(name, str) or not name.strip(): | ||
| raise ValueError(f"Agent at index {index} must have a non-empty 'name' string") |
There was a problem hiding this comment.
Important
Issue: _resolve_specs mutates the caller's input in place via entry["name"] = name. The agents list comes directly from the model's tool input (tool_context's parsed arguments). Mutating caller-supplied dicts is a side effect that can surprise callers and complicate debugging/logging of the original request.
Suggestion: Build a resolved copy instead of mutating the input, e.g. pass {**entry, "name": name} into _resolve_spec rather than reassigning entry["name"].
| f"Swarm stopped before completing (status={result.status.value}, " | ||
| f"{result.execution_count} iterations). Partial output:\n{result}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Suggestion
Issue: On non-completion the error embeds the full result object via f-string (Partial output:\n{result}). Depending on SwarmResult.__str__, this could be very large (every node's full output) and is raised as a RuntimeError message, which may be logged/surfaced verbatim to the model and users.
Suggestion: Consider bounding or summarizing the partial output included in the exception message, similar to how task[:120] is truncated for the start log.
|
Live Strandly's actual comments on the original PR 4922 (sanitized copy: mentions escaped, links backticked) Review (CHANGES_REQUESTED):
Review (COMMENTED):
Review (COMMENTED):
Review (APPROVED):
Review (CHANGES_REQUESTED):
Review (APPROVED):
Inline at
Inline at
Inline at
Inline at
Inline at
Inline at
Inline at
Inline at
|
GitHub Actions
Failure
Documentation Preview FailedThe documentation deployment encountered an error. Please check the deployment logs for more details. |
Replica of
https://github.com/strands-agents/harness-sdk/pull/4922(backticked: no cross-reference). Below: the v4-reason optimized review and a sanitized copy of live Strandly's actual comments.