Skip to content

[dotnet-port-fixes] Preserve tool approval message ordering #1012

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.

Sign up for GitHub

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
Quim Muntal (qmuntal) wants to merge 1 commit into
base: main
Choose a base branch
from
+186 −34
Open

[dotnet-port-fixes] Preserve tool approval message ordering #1012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 62 additions & 29 deletions agent/harness/toolautocall/autocall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ func (f *autocall) Run(next agent.RunFunc, ctx context.Context, messages []*mess
}
var messagesCloned bool
session, _ := agent.GetOption(opts, agent.WithSession)
serviceID, _ := agent.GetOption(opts, agent.WithServiceID)
serviceManagedHistory := serviceID != "" || session.ServiceID() != ""
yieldUpdate := func(update *agent.ResponseUpdate) bool {
if !f.disableApprovalResponseBinding && update != nil {
if err := recordPendingApprovalRequests(session, update.Contents); err != nil {
Expand Down Expand Up @@ -232,8 +234,9 @@ func (f *autocall) Run(next agent.RunFunc, ctx context.Context, messages []*mess
// the inner client had returned them directly.
var notInvokedMsgs []toolApprovalResultWithRequestMessage
var preDownstreamCallHistory []*message.Message
var approvedResultInsertIdx int
var err error
messages, preDownstreamCallHistory, notInvokedMsgs, err = f.processToolApprovalResponses(ctx, messages, toolMsgID, funcCallFallbackMsgID)
messages, preDownstreamCallHistory, notInvokedMsgs, approvedResultInsertIdx, err = f.processToolApprovalResponses(ctx, messages, toolMsgID, funcCallFallbackMsgID, serviceManagedHistory)
if err != nil {
yield(nil, err)
return
Expand All @@ -252,7 +255,7 @@ func (f *autocall) Run(next agent.RunFunc, ctx context.Context, messages []*mess
}
if newMsg != nil {
opts = updateOptionsForNextIteration(opts)
messages = append(messages, newMsg)
messages = slices.Insert(messages, approvedResultInsertIdx, newMsg)
newMsg.ID = toolMsgID
if !yield(convertToolResultMsgToUpdate(newMsg, toolMsgID), nil) {
return
Expand Down Expand Up @@ -677,29 +680,37 @@ func markServerHandledFunctionCalls(updates []*agent.ResponseUpdate, functionCal
func hasAnyApprovalContent(msgs []*message.Message) bool {
approvalResponseNeedsProcessing := approvalResponseNeedsProcessingByRequestID(msgs)
return slices.ContainsFunc(msgs, func(m *message.Message) bool {
if m == nil || m.Contents == nil {
return messageHasFunctionApproval(m, approvalResponseNeedsProcessing)
})
}

func messageHasFunctionApproval(msg *message.Message, approvalResponseNeedsProcessing map[string]bool) bool {
return msg != nil && slices.ContainsFunc(msg.Contents, func(content message.Content) bool {
return functionApprovalNeedsProcessing(content, approvalResponseNeedsProcessing)
})
}

func messageContainsOnlyFunctionApprovals(msg *message.Message, approvalResponseNeedsProcessing map[string]bool) bool {
return msg != nil && len(msg.Contents) > 0 && !slices.ContainsFunc(msg.Contents, func(content message.Content) bool {
return !functionApprovalNeedsProcessing(content, approvalResponseNeedsProcessing)
})
}

func functionApprovalNeedsProcessing(content message.Content, approvalResponseNeedsProcessing map[string]bool) bool {
switch content := content.(type) {
case *message.ToolApprovalRequestContent:
if content == nil {
return false
}
return slices.ContainsFunc(m.Contents, func(c message.Content) bool {
switch c := c.(type) {
case *message.ToolApprovalRequestContent:
if c == nil {
return false
}
if responseNeedsProcessing, hasResponse := approvalResponseNeedsProcessing[c.RequestID]; hasResponse {
return responseNeedsProcessing
}
return approvalToolCallNeedsProcessing(c.ToolCall)
case *message.ToolApprovalResponseContent:
if c == nil {
return false
}
return approvalToolCallNeedsProcessing(c.ToolCall)
default:
return false
}
})
})
if responseNeedsProcessing, hasResponse := approvalResponseNeedsProcessing[content.RequestID]; hasResponse {
return responseNeedsProcessing
}
return approvalToolCallNeedsProcessing(content.ToolCall)
case *message.ToolApprovalResponseContent:
return content != nil && approvalToolCallNeedsProcessing(content.ToolCall)
default:
return false
}
}

func approvalToolCallNeedsProcessing(toolCall message.ToolCallContent) bool {
Expand Down Expand Up @@ -1193,15 +1204,36 @@ func (f *autocall) createFunctionResultContent(result functionInvocationResult)
// - Recreates tool call content for any ToolApprovalResponseContent that hasn't been handled yet.
// - Generates failed FunctionResultContent for any rejected function tool call.
// - Adds all the new content items to originalMessages and returns them as the pre-invocation history.
func (f *autocall) processToolApprovalResponses(ctx context.Context, msgs []*message.Message, toolMsgID, fallbackMsgID string) ([]*message.Message, []*message.Message, []toolApprovalResultWithRequestMessage, error) {
func (f *autocall) processToolApprovalResponses(ctx context.Context, msgs []*message.Message, toolMsgID, fallbackMsgID string, serviceManagedHistory bool) ([]*message.Message, []*message.Message, []toolApprovalResultWithRequestMessage, int, error) {
approvalResponseNeedsProcessing := approvalResponseNeedsProcessingByRequestID(msgs)
lastApprovalIdx := -1
for i, msg := range slices.Backward(msgs) {
if messageHasFunctionApproval(msg, approvalResponseNeedsProcessing) {
lastApprovalIdx = i
break
}
}
trailingMessageCount := 0
if lastApprovalIdx >= 0 {
trailingMessageCount = len(msgs) - (lastApprovalIdx + 1)
if !messageContainsOnlyFunctionApprovals(msgs[lastApprovalIdx], approvalResponseNeedsProcessing) {
trailingMessageCount++
}
}

// Extract any approval responses where we need to execute or reject the function calls.
// The original messages are also modified to remove all approval requests and responses.
msgs, approvals, rejections, err := f.extractAndRemoveToolApprovalRequestsAndResponses(ctx, msgs)
if err != nil {
return nil, nil, nil, err
return nil, nil, nil, 0, err
}
insertIdx := len(msgs) - trailingMessageCount
// Wrap the tool call content in message(s).
preDownstreamCallHistory := convertToToolCallContentMessages(append(rejections, approvals...), fallbackMsgID)
if !serviceManagedHistory && len(preDownstreamCallHistory) > 0 {
msgs = slices.Insert(msgs, insertIdx, preDownstreamCallHistory...)
insertIdx += len(preDownstreamCallHistory)
}
// Generate failed function result contents for any rejected requests and wrap it in a message.
rejectedFunctionContent := f.generateRejectedFunctionResults(ctx, rejections)
var rejectedPreDownstreamCallResultsMsgs *message.Message
Expand All @@ -1212,13 +1244,14 @@ func (f *autocall) processToolApprovalResponses(ctx context.Context, msgs []*mes
Contents: rejectedFunctionContent,
}
}
// Add generated tool call and function result content to the pre-downstream-call history so they can be returned to the caller as part of the next response.
// Also, add them into the original messages list so that they are passed to the inner client and can be used to generate a result.
// Add generated function result content to the pre-downstream-call history so it can be returned to the caller as part of the next response.
// Also, insert it at the approval anchor so it stays ahead of trailing caller messages.
if rejectedPreDownstreamCallResultsMsgs != nil {
preDownstreamCallHistory = append(preDownstreamCallHistory, rejectedPreDownstreamCallResultsMsgs)
msgs = append(msgs, rejectedPreDownstreamCallResultsMsgs)
msgs = slices.Insert(msgs, insertIdx, rejectedPreDownstreamCallResultsMsgs)
insertIdx++
}
return msgs, preDownstreamCallHistory, approvals, nil
return msgs, preDownstreamCallHistory, approvals, insertIdx, nil
}

type toolApprovalResultWithRequestMessage struct {
Expand Down
129 changes: 124 additions & 5 deletions agent/harness/toolautocall/autocall_approval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ func invokeAndAssertApproval(t *testing.T, tools []tool.Tool, input []*message.M
invokeAndAssertApprovalWithAgent(t, runner.Run, tools, input, expectedOutput, additionalTools)
}

// invokeAndAssertApprovalWithAgent performs streaming test execution
// invokeAndAssertApprovalWithAgent performs streaming test execution against a
// provider-managed conversation. Client-managed history is covered separately.
func invokeAndAssertApprovalWithAgent(t *testing.T, next agent.RunFunc,
tools []tool.Tool, input []*message.Message,
expectedOutput []*agent.ResponseUpdate, additionalTools []tool.Tool,
Expand All @@ -62,7 +63,7 @@ func invokeAndAssertApprovalWithAgent(t *testing.T, next agent.RunFunc,
ctx := t.Context()

// Build options
var opts []agent.Option
opts := []agent.Option{agent.WithServiceID("test-conversation")}
for _, tool := range tools {
opts = append(opts, agent.WithTool(tool))
}
Expand Down Expand Up @@ -473,6 +474,124 @@ func TestFunctionInvoking_CanDisableApprovalNotRequiredBypassing(t *testing.T) {
}
}

func TestFunctionInvoking_ApprovedResultPrecedesTrailingMessageWithServiceManagedHistory(t *testing.T) {
request := &message.ToolApprovalRequestContent{
RequestID: "ficc_callId1",
ToolCall: &message.FunctionCallContent{CallID: "callId1", Name: "Func1"},
}
input := []*message.Message{
{Role: message.RoleAssistant, Contents: message.Contents{request}},
message.New(request.CreateResponse(true, "")),
message.NewText("keep the answer concise"),
}
expected := []*message.Message{
{Role: message.RoleTool, Contents: message.Contents{
&message.FunctionResultContent{CallID: "callId1", Result: "Result 1"},
}},
message.NewText("keep the answer concise"),
}

runner := &agenttest.Runner{
Responses: agenttest.NewResponseBuilder(expectedMessages(t, expected...)).
Add(&agent.ResponseUpdate{Role: message.RoleAssistant, Contents: message.Contents{
&message.TextContent{Text: "done"},
}}).
Build(),
}
for _, err := range toolautocall.New(toolautocall.Config{NewID: func() string { return "" }}).Run(
runner.Run,
t.Context(),
input,
agent.WithServiceID("conversation-1"),
agent.WithTool(tool.ApprovalRequiredFunc(createFunc1())),
) {
if err != nil {
t.Fatal(err)
}
}
}

func TestFunctionInvoking_ApprovedCallAndResultPrecedeTrailingMessageWithClientManagedHistory(t *testing.T) {
request := &message.ToolApprovalRequestContent{
RequestID: "ficc_callId1",
ToolCall: &message.FunctionCallContent{CallID: "callId1", Name: "Func1"},
}
input := []*message.Message{
message.NewText("hello"),
{Role: message.RoleAssistant, ID: "response-1", Contents: message.Contents{request}},
message.New(request.CreateResponse(true, "")),
message.NewText("keep the answer concise"),
}
expected := []*message.Message{
message.NewText("hello"),
{Role: message.RoleAssistant, ID: "response-1", Contents: message.Contents{
&message.FunctionCallContent{CallID: "callId1", Name: "Func1", InformationalOnly: true},
}},
{Role: message.RoleTool, Contents: message.Contents{
&message.FunctionResultContent{CallID: "callId1", Result: "Result 1"},
}},
message.NewText("keep the answer concise"),
}

runner := &agenttest.Runner{
Responses: agenttest.NewResponseBuilder(expectedMessages(t, expected...)).
Add(&agent.ResponseUpdate{Role: message.RoleAssistant, Contents: message.Contents{
&message.TextContent{Text: "done"},
}}).
Build(),
}
for _, err := range toolautocall.New(toolautocall.Config{NewID: func() string { return "" }}).Run(
runner.Run,
t.Context(),
input,
agent.WithTool(tool.ApprovalRequiredFunc(createFunc1())),
) {
if err != nil {
t.Fatal(err)
}
}
}

func TestFunctionInvoking_ApprovedCallAndResultPrecedeResidualContentWithClientManagedHistory(t *testing.T) {
request := &message.ToolApprovalRequestContent{
RequestID: "ficc_callId1",
ToolCall: &message.FunctionCallContent{CallID: "callId1", Name: "Func1"},
}
input := []*message.Message{
message.NewText("hello"),
{Role: message.RoleAssistant, ID: "response-1", Contents: message.Contents{request}},
message.New(request.CreateResponse(true, ""), &message.TextContent{Text: "please proceed"}),
}
expected := []*message.Message{
message.NewText("hello"),
{Role: message.RoleAssistant, ID: "response-1", Contents: message.Contents{
&message.FunctionCallContent{CallID: "callId1", Name: "Func1", InformationalOnly: true},
}},
{Role: message.RoleTool, Contents: message.Contents{
&message.FunctionResultContent{CallID: "callId1", Result: "Result 1"},
}},
message.NewText("please proceed"),
}

runner := &agenttest.Runner{
Responses: agenttest.NewResponseBuilder(expectedMessages(t, expected...)).
Add(&agent.ResponseUpdate{Role: message.RoleAssistant, Contents: message.Contents{
&message.TextContent{Text: "done"},
}}).
Build(),
}
for _, err := range toolautocall.New(toolautocall.Config{NewID: func() string { return "" }}).Run(
runner.Run,
t.Context(),
input,
agent.WithTool(tool.ApprovalRequiredFunc(createFunc1())),
) {
if err != nil {
t.Fatal(err)
}
}
}

// TestFunctionInvoking_AllFunctionCallsReplacedWithApprovalsWhenAllRequireApproval tests that
// all function calls are replaced with approval requests when all functions require approval
func TestFunctionInvoking_AllFunctionCallsReplacedWithApprovalsWhenAllRequireApproval(t *testing.T) {
Expand Down Expand Up @@ -717,13 +836,13 @@ func TestFunctionInvoking_PreservesNilToolCallApprovalContentWhenProcessingOther

expectedDownstreamAgentInput := []*message.Message{
message.New(&message.TextContent{Text: "hello"}),
{Role: message.RoleTool, Contents: []message.Content{
&message.FunctionResultContent{CallID: "callId1", Result: "Result 1"},
}},
message.New(
&message.ToolApprovalRequestContent{RequestID: "missing-request-tool-call"},
&message.ToolApprovalResponseContent{RequestID: "missing-response-tool-call", Approved: true},
),
{Role: message.RoleTool, Contents: []message.Content{
&message.FunctionResultContent{CallID: "callId1", Result: "Result 1"},
}},
}

invokeAndAssertApproval(t, tools, input, downstreamAgentOutput, expectedOutput, expectedDownstreamAgentInput, nil)
Expand Down
Loading