feat(advisors): enable recursive advisor execution with two new built-in advisors #4622
Conversation
…-in advisors Add support for recursive (repetitive) advisor execution patterns, allowing advisors to re-invoke themselves or the remaining chain multiple times. This enables advanced patterns like retry logic, iterative refinement, and multi-pass processing. - Add AdvisorUtils.copyChainAfterAdvisor() utility to enable recursive chain invocation - Implement ToolCallAdvisor for recursive tool calling with configurable tool execution - Implement StructuredOutputValidationAdvisor for recursive output validation with retry logic - Add MCP JSON Jackson2 dependency for JSON schema validation - Add test suites for both new advisors and utility methods - Add documentation for recursive advisor patterns Signed-off-by: Christian Tzolov <[email protected]>
…idation feedback loop Refactor retry logic to provide validation error feedback to LLM for self-correction. Extract validation into separate method and augment prompts with error messages on retry. Add comprehensive test coverage for various validation scenarios including nested objects, lists, malformed JSON, and type mismatches. Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
Implements return direct functionality allowing tools to bypass the LLM and return results directly to clients. Adds null safety checks for chatResponse and comprehensive test coverage. Signed-off-by: Christian Tzolov <[email protected]>
| Assert.notNull(outputType, "outputType must not be null"); | ||
| Assert.isTrue(advisorOrder > BaseAdvisor.HIGHEST_PRECEDENCE && advisorOrder < BaseAdvisor.LOWEST_PRECEDENCE, | ||
| "advisorOrder must be between HIGHEST_PRECEDENCE and LOWEST_PRECEDENCE"); | ||
| Assert.isTrue(repeatAttempts >= 0, "repeatAttempts must be greater than or equal to 0"); |
There was a problem hiding this comment.
I think a value of zero effectively makes this infinite, given the do {} while() construct. Is this what we want?
There was a problem hiding this comment.
If the repeatAttempts is set to 0 this effectively mean never repeat.
E.g. the loop is count = 0; do { count++ ...} while (count <= maxAttempt); and if the maxAttempt is 0 the loop should run only once?
|
|
||
| this.advisorOrder = advisorOrder; | ||
|
|
||
| this.jsonvalidator = new DefaultJsonSchemaValidator(); |
There was a problem hiding this comment.
Can be initialised above, and maybe even made static AFAICT
There was a problem hiding this comment.
Ah, I wonted to make the objectmaper configurable and aligned with the default mapper used in spring ai (JsonParser.getObjectMapper()) but forgot to add it.
Will update the code
|
|
||
| ChatClientResponse chatClientResponse = null; | ||
|
|
||
| var repeatCounter = new AtomicInteger(0); |
There was a problem hiding this comment.
you're right. Not sure why i thought that some concurrency can occur. will make it int
| * @return a new CallAdvisorChain containing all advisors after the specified advisor | ||
| * @throws IllegalArgumentException if the specified advisor is not part of the chain | ||
| */ | ||
| public static CallAdvisorChain copyChainAfterAdvisor(CallAdvisorChain callAdvisorChain, CallAdvisor after) { |
There was a problem hiding this comment.
Shouldn't this be a method on CallAdvisorChain instead? I feel like it would be more readable
|
|
||
| if (toolExecutionResult.returnDirect()) { | ||
| // Interupt the tool calling loop and return the tool execution result | ||
| // directly to the client application instead of returning it tothe |
There was a problem hiding this comment.
| // directly to the client application instead of returning it tothe | |
| // directly to the client application instead of returning it to the |
| // Interupt the tool calling loop and return the tool execution result | ||
| // directly to the client application instead of returning it tothe | ||
| // LLM. | ||
| isToolCall = false; |
There was a problem hiding this comment.
This should be changed to a break statement IMO
|
|
||
| = Recursive Advisors | ||
|
|
||
| == What is a Recursive Advisor |
There was a problem hiding this comment.
| == What is a Recursive Advisor | |
| == What is a Recursive Advisor? |
| * Implementing Evaluation logic with modifications to the request | ||
| * Implementing retry logic with modifications to the request | ||
|
|
||
| The `AdvisorUtils.copyChainAfterAdvisor()` method is the key utility that enables recursive advisor patterns. |
There was a problem hiding this comment.
Change here if you change this to be an instance method on chain
…ain interface - Add CallAdvisorChain.copy(CallAdvisor after) method to replace AdvisorUtils.copyChainAfterAdvisor() - Implement copy() method in DefaultAroundAdvisorChain - Update StructuredOutputValidationAdvisor and ToolCallAdvisor to use new copy() API - Add ObjectMapper parameter support to StructuredOutputValidationAdvisor for custom JSON processing - Improve ToolCallAdvisor return direct logic using break instead of flag - Move tests from AdvisorUtilsTests to DefaultAroundAdvisorChainTests - Update documentation to reflect API changes This refactoring improves the API design by moving chain manipulation logic closer to where it belongs (on the chain itself) rather than in a utility class. Signed-off-by: Christian Tzolov <[email protected]>
|
Thank you the review and great feedback @ericbottard |
Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
2 participants
Add support for recursive (repetitive) advisor execution patterns, allowing advisors to re-invoke themselves or the remaining chain multiple times. This enables advanced patterns like retry logic, iterative refinement, and multi-pass processing.
Depends on: modelcontextprotocol/java-sdk#617
TODO: The ToolCallAdvisor doesn't implement streaming tool-calling model at the moment. Perhaps we can address this in a follow up issues along with auto-config integrations for the ToolCallAdvisor.