Skip to content

fix: Incorrect order when Advisors have the same order #3461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

YunKuiLu
Copy link
Contributor

@YunKuiLu YunKuiLu commented Jun 6, 2025

If the order of Advisors is the same, then it should follow the order in which the parameters are passed in.

When I add new SimpleLoggerAdvisor(Ordered.LOWEST_PRECEDENCE) to defaultAdvisors. Since ChatModelCallAdvisor has the same order as SimpleLoggerAdvisor, it will result in the order of ChatModelCallAdvisor being before that of SimpleLoggerAdvisor after sorting.

In the end, even if SimpleLoggerAdvisor continued to execute, it would be of no use.

@markpollack
Copy link
Member

I think we should throw an exception, at at least log a warning, if anything would be short circuit. Probably in DefaultChatClient's

		private BaseAdvisorChain buildAdvisorChain() {
			// At the stack bottom add the model call advisors.
			// They play the role of the last advisors in the advisor chain.
			this.advisors.add(ChatModelCallAdvisor.builder().chatModel(this.chatModel).build());
			this.advisors.add(ChatModelStreamAdvisor.builder().chatModel(this.chatModel).build());

			return DefaultAroundAdvisorChain.builder(this.observationRegistry)
				.pushAll(this.advisors)
				.templateRenderer(this.templateRenderer)
				.build();
		}

@markpollack markpollack assigned markpollack and unassigned tzolov Jul 17, 2025
@YunKuiLu
Copy link
Contributor Author

YunKuiLu commented Jul 17, 2025

I think we should throw an exception, at at least log a warning, if anything would be short circuit. Probably in DefaultChatClient's

Would it be easier to check for the short circuit in this method?

public class DefaultAroundAdvisorChain implements BaseAdvisorChain {
...
  @Override
  public ChatClientResponse nextCall(ChatClientRequest chatClientRequest) {
    ...
    
    if (this.callAdvisors.isEmpty()) {
	    throw new IllegalStateException("No CallAdvisors available to execute");
    }
    
    var advisor = this.callAdvisors.pop();
    
    if (advisor instanceof ChatModelCallAdvisor && !this.callAdvisors.isEmpty()){
         // Short circuit detected
    }
    ... 
  }
...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants