Fix: concurrent tool execution fails with ProcessDriver due to $this capture and circular references#1009
Open
vinitkadam03 wants to merge 4 commits intoprism-php:mainfrom
Open
Conversation
- Updated `CallsTools` trait to rename and enhance the `groupToolCallsByConcurrency` method to `resolveToolCalls`, which now returns structured pairs of tools and tool calls. - Modified the execution logic to handle resolved tool calls, ensuring proper error handling for unresolvable tools. - Adjusted the `handle` method in `Tool` class to correctly unwrap closures for named argument support. - Updated tests to reflect changes in method names and structure.
…tests - Updated the `Tool` class to initialize the `$fn` property as `null` by default. - Added a check in the `using` method to return `$this` if the provided function is the instance itself, preventing circular references. - Improved the `handle` method to throw an exception if no valid function is defined. - Introduced new tests for handling invokable subclasses, ensuring they work correctly with and without explicit function assignments.
Tool::handle() now auto-detects __invoke() on subclasses, making the explicit ->using($this) call redundant. Removing it also eliminates the circular reference that broke SerializableClosure.
- Introduced a new method `resolveHandler()` to encapsulate the logic for determining the callable handler for the tool. - Simplified the `handle()` method by delegating handler resolution to `resolveHandler()`, improving readability and maintainability. - Enhanced error handling for cases where no valid handler is defined, ensuring consistent behavior across invokable subclasses.
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
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.
Summary
Fixes #893 — concurrent tool execution via Laravel's
Concurrency::run()fails because closures capture$this(the handler object), causingSerializableClosureto serialize the entire application container. This also fixes the->using($this)circular reference issue for class-based invokable tools.Changes
CallsToolstrait — eliminate$thiscapture in concurrent closuresgroupToolCallsByConcurrencywithresolveToolCallswhich resolves each tool exactly once upfront and returns{tool, toolCall}pairs. Unresolvable tools carry the caught exception forward instead of being re-resolved inside the closure.executeToolCallwithstaticmethodexecuteResolvedToolCallthat receives the pre-resolved Tool directly — no$this, no$toolsarray in the serialization boundary.static fn()callingself::executeResolvedToolCall(), preventing the handler (and its transitive references to the Laravel container) from being serialized.Toolclass — fix circular reference and named argument handlingusing()now detects$fn === $thisand skips the assignment, preventing theTool -> $fn -> Toolcircular reference that breaksSerializableClosure.handle()auto-detects__invoke()on subclasses when$fnis null, so invokable tool classes work with or without->using($this).handle()unwrapsSerializableClosure\Serializers\Nativewrappers viagetClosure()before invocation, restoring PHP 8 named argument forwarding thatNative::__invoke()breaks.Fixes
TypeError: Cannot assign SerializableClosure\Serializers\Native to property ...(e.g. Scramble'sGeneratorConfig::$uiRoute)Error: Cannot modify readonly property ...(e.g. Spatie LaravelData'sDataType::$type)Tool -> $fn -> Toolcircular reference with class-based invokable tools using->using($this)Test plan
CallsToolsConcurrentTest— sequential, concurrent, mixed, error handling, groupingCallsToolsTest— all tool execution and event generation pathsToolTestandToolErrorHandlingTest— all tool behaviorConcurrentToolExecutionTest— concurrent flag behavior->using($this)works without circular referenceusing()at all