refactor: decouple SwarmAI engine from domain via ExecutionStrategy behaviour (#784)#902
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
…iour Implements frontman-ai#784 — replaces the closure-based tool_executor interface with a composable {module, opts} ExecutionStrategy behaviour. SwarmAI gains: - SwarmAi.ExecutionStrategy behaviour (4 callbacks) - SwarmAi.DefaultExecutionStrategy (batteries-included + use macro) - SwarmAi.Runtime.deliver_tool_result/5 and await_tool_result/3 - Internal tool result Registry inside Runtime's supervision subtree Domain loses: - FrontmanServer.ToolCallRegistry (removed from application.ex) - Execution.notify_tool_result/4 (replaced by Runtime.deliver_tool_result) - :notified/:no_executor leak through Tasks.add_tool_result Domain gains: - FrontmanServer.Tasks.Execution.AgentStrategy (plain struct, testable) - Clean separation: Tasks.add_tool_result returns {:ok, interaction} only Files: 5 new, 8 modified (4 swarm_ai + 4 frontman_server)
…tration, dead code removal
Critical fixes found during audit:
1. deliver_tool_result used {:tool_result_delivered, ...} but PE expects {:tool_result, ...} — unified to {:tool_result, ...}
2. start_mcp_tool_mfa was not registering PE's pid in Runtime.ToolRegistry — deliver_tool_result would silently return :no_executor
3. execution.ex still passed strategy: opt but Runtime only understands tool_executor: — bridged via AgentStrategy.init + closure adapter
4. Removed dead encode_result_for_swarm function (orphaned by notify_tool_result removal)
SBALAVIGNESH123
force-pushed
the
feat/784-execution-strategy-boundary
branch
from
8113a1c to
5f3cbb8
Compare
|
I have read the CLA Document and I hereby sign the CLA |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review caught that process_result: nil dropped maybe_enrich_with_images for screenshot tools. PE's build_await_result needs the MFA to convert base64 image data URLs into ContentPart.image parts that the LLM can process. Added AgentStrategy.make_mcp_tool_result/4 and wired it in both the adapter closure (execution.ex) and the legacy executor (build_legacy_executor).
|
Good catch, fixed in 0f98445. The process_result MFA was nil which skipped maybe_enrich_with_images for MCP screenshot tools. Now wired to AgentStrategy.make_mcp_tool_result/4 which runs the image enrichment pipeline, matching the old ToolExecutor behaviour exactly. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
MCP.parse_tool_result can return maps when tool output is valid JSON. maybe_enrich_with_images has a 'when is_binary(content)' guard that skips image extraction for non-string content. The old notify_tool_result called encode_result_for_swarm before sending. Added encode_for_delivery/1 in TaskChannel to JSON-encode map results at the boundary before delivery to PE.
|
Fixed in bbd460b. Added encode_for_delivery/1 in TaskChannel that JSON-encodes map results before passing to deliver_tool_result. MCP.parse_tool_result returns maps for valid JSON tool output, but maybe_enrich_with_images needs a binary string to match its is_binary guard. This matches the old encode_result_for_swarm behaviour that was previously called inside notify_tool_result. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e return
ToolExecutor is dead code (replaced by AgentStrategy) but must compile
cleanly. Updated pattern match from {:ok, _interaction, _executor_status}
to {:ok, _interaction} to match the new add_tool_result return type.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
execute_mcp_tool published the tool call before registering in the ToolRegistry, creating a race where the client could respond before we were listening. Fixed by inlining the receive and registering BEFORE publish, matching the pattern in start_mcp_tool_mfa. Uses raw receive instead of await_tool_result to avoid double Registry.register (await_tool_result registers internally).
|
Hey , this PR is ready for review. It addresses #784 by moving the tool result Registry and execution lifecycle entirely into SwarmAI, which cleans up the domain boundary that was discussed in the issue. The old ToolExecutor is left in place intentionally so nothing breaks — the plan is to remove it in a follow-up once the existing tests are migrated to the new API. All of Devin's findings have been addressed and resolved. Happy to squash the fixup commits before merge if you'd prefer a cleaner history. Let me know if anything needs changes. |
|
@BlueHotDog would appreciate your eyes on this when you get a chance. All Devin review findings have been resolved. |
1 participant
This addresses the architectural coupling identified in #784 where the SwarmAI engine and the application domain were tightly bound through opaque closure-based tool execution interfaces. The core problem was that FrontmanServer owned process registries and routing logic that conceptually belonged inside the Swarm runtime, making it impossible to test execution strategies in isolation or swap them without touching domain code.
The fix introduces a formal ExecutionStrategy OTP behaviour in the swarm_ai app with four callbacks: init, execute_tool, on_deadline, and an optional should_continue?. This follows the {module, opts} convention used by Oban and Broadway, where callers pass a strategy as {MyStrategy, opts} and the engine handles lifecycle. A DefaultExecutionStrategy ships as a batteries-included base that blocks via await_tool_result for MCP tools, and a use macro lets consumers override individual callbacks while inheriting sensible defaults.
On the infrastructure side, the tool result Registry has been moved from FrontmanServer.ToolCallRegistry into SwarmAI's own supervision subtree. The Runtime module gains deliver_tool_result/5 and await_tool_result/3 as the public API for result routing, replacing the old pattern where the domain layer owned process rendezvous. This means the Swarm engine now fully owns the mechanics of tool execution, timeouts, and result delivery.
On the domain side, a new AgentStrategy struct replaces the old ToolExecutor closure builder. It implements the ExecutionStrategy behaviour and handles backend tool dispatch, MCP tool routing via await_tool_result, image enrichment, error reporting to Sentry, and sub-agent compatibility. The Tasks.add_tool_result function now returns a clean {:ok, interaction} without leaking executor lifecycle state like :notified or :no_executor. TaskChannel calls deliver_tool_result directly after persisting results.
The old ToolExecutor module is intentionally left in place for reference but is no longer called from any production code path. Existing tests that reference the old ToolCallRegistry are known tech debt and will be migrated in a follow-up. Three new test files cover the behaviour contracts, the deliver/await Registry mechanics, and AgentStrategy's init and timeout policies as a pure struct without needing a running runtime.