Repository navigation
plugin/guardrail/approval: synchronize tool-call context before review #2669
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ import ( | |
| "trpc.group/trpc-go/trpc-agent-go/internal/modelcontext" | ||
| imodelrequest "trpc.group/trpc-go/trpc-agent-go/internal/modelrequest" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/responseusage" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/state/barrier" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/state/steer" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/state/summaryfork" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/state/summaryinject" | ||
|
|
@@ -1002,11 +1003,16 @@ func (p *streamingResponseProcessor) process( | |
| } | ||
| return true | ||
| } | ||
| llmResponseEvent := p.emitLLMResponse( | ||
| llmResponseEvent, err := p.emitLLMResponse( | ||
| eventInvocation, | ||
| response, | ||
| traceDetails, | ||
| ) | ||
| if err != nil { | ||
| *p.err = err | ||
| responseErr = err | ||
| return false | ||
| } | ||
| p.lastEvent = llmResponseEvent | ||
| if p.tracker != nil { | ||
| p.tracker.SetLastEvent(p.lastEvent) | ||
|
|
@@ -1191,13 +1197,22 @@ func (p *streamingResponseProcessor) emitLLMResponse( | |
| eventInvocation *agent.Invocation, | ||
| response *model.Response, | ||
| traceDetails bool, | ||
| ) *event.Event { | ||
| ) (*event.Event, error) { | ||
| llmResponseEvent := p.flow.createLLMResponseEvent( | ||
| eventInvocation, | ||
| p.currentInvocation, | ||
| response, | ||
| p.llmRequest, | ||
| ) | ||
| // Agent wrappers forward events asynchronously. BeforeTool reviewers need | ||
| // this response in the session before callbacks run or parallel tools clone | ||
| // the session. Wait for this event's acknowledgement, not a channel drain. | ||
| waitForPersistence := barrier.Enabled(eventInvocation) && | ||
| barrier.ToolCallPersistenceRequired(eventInvocation) && | ||
| !response.IsPartial && response.IsToolCallResponse() | ||
| if waitForPersistence { | ||
| llmResponseEvent.RequiresCompletion = true | ||
|
Comment on lines
+1213
to
+1214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| emitCtx := p.ctx | ||
| var emitSpan oteltrace.Span | ||
| emitStarted := false | ||
|
|
@@ -1209,9 +1224,21 @@ func (p *streamingResponseProcessor) emitLLMResponse( | |
| latencyResponseAttrs(response)..., | ||
| ) | ||
| } | ||
| agent.EmitEvent(emitCtx, eventInvocation, p.eventChan, llmResponseEvent) | ||
| finishLatencySpan(emitSpan, emitStarted, nil) | ||
| return llmResponseEvent | ||
| err := agent.EmitEvent(emitCtx, eventInvocation, p.eventChan, llmResponseEvent) | ||
| if !waitForPersistence { | ||
| // Preserve emission behavior for runs without this synchronization need. | ||
| finishLatencySpan(emitSpan, emitStarted, nil) | ||
| return llmResponseEvent, nil | ||
| } | ||
| if err == nil { | ||
| err = eventInvocation.AddNoticeChannelAndWait(p.ctx, | ||
| agent.GetAppendEventNoticeKey(llmResponseEvent.ID), flowEventWaitTimeout(p.ctx)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] 保留 OnEvent 替换前的完成通知义务
已用同一个仅替换完整工具调用事件的插件做对照:修改前 cbd89b0 正常完成两个工具调用;当前 68a6ad9 在等待确认时超时,两个工具均未执行。元数据补齐遗漏原本就存在,但工具调用回复在本 PR 之前没有这个等待依赖,因此这一插件组合的执行失败是新增的回归。 建议 Runner 在执行事件插件前保留原始事件的完成通知要求和 ID,并在替换事件处理结束后通知原生产者;同时补充 Reviewer 与 OnEvent 替换事件组合的回归测试。只补齐 bool 还需考虑替换事件自带新 ID 时,确认仍必须对应原生产者等待的 ID。 |
||
| } | ||
| finishLatencySpan(emitSpan, emitStarted, err) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("wait for tool-call event persistence: %w", err) | ||
| } | ||
| return llmResponseEvent, nil | ||
| } | ||
|
|
||
| func (p *streamingResponseProcessor) traceChat( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // | ||
| // Tencent is pleased to support the open source community by making trpc-agent-go available. | ||
| // | ||
| // Copyright (C) 2026 Tencent. All rights reserved. | ||
| // | ||
| // trpc-agent-go is licensed under the Apache License Version 2.0. | ||
| // | ||
|
|
||
| package llmflow | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| oteltrace "go.opentelemetry.io/otel/trace" | ||
| "trpc.group/trpc-go/trpc-agent-go/agent" | ||
| "trpc.group/trpc-go/trpc-agent-go/event" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/flow" | ||
| "trpc.group/trpc-go/trpc-agent-go/internal/state/barrier" | ||
| "trpc.group/trpc-go/trpc-agent-go/model" | ||
| ) | ||
|
|
||
| type persistenceObserver struct { | ||
| called atomic.Bool | ||
| acknowledged *atomic.Bool | ||
| beforeAcknowledgement atomic.Bool | ||
| } | ||
|
|
||
| func (p *persistenceObserver) ProcessResponse(context.Context, *agent.Invocation, *model.Request, *model.Response, chan<- *event.Event) { | ||
| p.called.Store(true) | ||
| p.beforeAcknowledgement.Store(!p.acknowledged.Load()) | ||
| } | ||
|
|
||
| func TestToolCallPersistenceBeforeResponseProcessors(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| required, consumer, partial, toolCall, cancel bool | ||
| wantWait bool | ||
| }{ | ||
| {name: "acknowledged", required: true, consumer: true, toolCall: true, wantWait: true}, | ||
| {name: "canceled before acknowledgement", required: true, consumer: true, toolCall: true, cancel: true, wantWait: true}, | ||
| {name: "no requirement", consumer: true, toolCall: true}, | ||
| {name: "standalone flow", required: true, toolCall: true}, | ||
| {name: "partial tool call", required: true, consumer: true, partial: true, toolCall: true}, | ||
| {name: "text response", required: true, consumer: true}, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| inv := agent.NewInvocation() | ||
| if tc.required { | ||
| barrier.RequireToolCallPersistence(inv) | ||
| } | ||
| if tc.consumer { | ||
| barrier.Enable(inv) | ||
| } | ||
| var acknowledged atomic.Bool | ||
| post := &persistenceObserver{acknowledged: &acknowledged} | ||
| f := New(nil, []flow.ResponseProcessor{post}, Options{}) | ||
| msg := model.NewAssistantMessage("Inspect the file.") | ||
| if tc.toolCall { | ||
| msg.ToolCalls = []model.ToolCall{{ID: "call", Function: model.FunctionDefinitionParam{Name: "read", Arguments: []byte(`{}`)}}} | ||
| } | ||
| rsp := &model.Response{IsPartial: tc.partial, Choices: []model.Choice{{Message: msg}}} | ||
| out := make(chan *event.Event, 1) | ||
| var processErr error | ||
| p := newStreamingResponseProcessor(f, ctx, inv, nil, &model.Request{}, out, | ||
| oteltrace.SpanFromContext(ctx), false, &processErr) | ||
| done := make(chan bool, 1) | ||
| go func() { done <- p.process(rsp) }() | ||
| var evt *event.Event | ||
| select { | ||
| case evt = <-out: | ||
| case <-ctx.Done(): | ||
| t.Fatal(ctx.Err()) | ||
| } | ||
| require.Equal(t, tc.wantWait, evt.RequiresCompletion) | ||
| if tc.wantWait { | ||
| require.False(t, post.called.Load()) | ||
| if tc.cancel { | ||
| cancel() | ||
| } else { | ||
| acknowledged.Store(true) | ||
| require.NoError(t, inv.NotifyCompletion(ctx, agent.GetAppendEventNoticeKey(evt.ID))) | ||
| } | ||
| } | ||
| select { | ||
| case ok := <-done: | ||
| if tc.cancel { | ||
| require.False(t, ok) | ||
| require.ErrorIs(t, processErr, context.Canceled) | ||
| require.False(t, post.called.Load(), "review and tool processors must not run after synchronization fails") | ||
| } else { | ||
| require.True(t, ok) | ||
| require.NoError(t, processErr) | ||
| require.True(t, post.called.Load()) | ||
| require.Equal(t, !tc.wantWait, post.beforeAcknowledgement.Load()) | ||
| } | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatal("response processing did not finish") | ||
| } | ||
| }) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.