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 all commits
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 |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // | ||
| // 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 eventvisibility keeps runner-only event forwarding decisions out of | ||
| // event payloads and persisted session data. | ||
| package eventvisibility | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| ) | ||
|
|
||
| type contextKey struct{} | ||
| type hiddenEvents struct { | ||
| mu sync.Mutex | ||
| ids map[string]struct{} | ||
| } | ||
|
|
||
| // WithCapture creates a run-scoped registry for events that must be processed | ||
| // internally without being forwarded to the caller. Derived contexts share it; | ||
| // a new call creates an independent registry. Unconsumed entries live only as | ||
| // long as the run's context. | ||
| func WithCapture(ctx context.Context) context.Context { | ||
| return context.WithValue(ctx, contextKey{}, &hiddenEvents{ids: make(map[string]struct{})}) | ||
| } | ||
|
|
||
| // Hide marks id for internal processing. It reports false if there is no | ||
| // runner capture registry or id is empty, so standalone producers can retain | ||
| // their normal filtering behavior. Concurrent calls are safe. | ||
| func Hide(ctx context.Context, id string) bool { | ||
| state, _ := ctx.Value(contextKey{}).(*hiddenEvents) | ||
| if state == nil || id == "" { | ||
| return false | ||
| } | ||
| state.mu.Lock() | ||
| defer state.mu.Unlock() | ||
| state.ids[id] = struct{}{} | ||
| return true | ||
| } | ||
|
|
||
| // TakeHidden consumes the forwarding decision for the original event ID before | ||
| // event plugins can replace it. Missing registries and IDs return false. | ||
| // Concurrent calls are safe. | ||
| func TakeHidden(ctx context.Context, id string) bool { | ||
| state, _ := ctx.Value(contextKey{}).(*hiddenEvents) | ||
| if state == nil { | ||
| return false | ||
| } | ||
| state.mu.Lock() | ||
| defer state.mu.Unlock() | ||
| _, hidden := state.ids[id] | ||
| delete(state.ids, id) | ||
| return hidden | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // | ||
| // 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 eventvisibility | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sync" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestHiddenEventsAreRunScoped(t *testing.T) { | ||
| parent := context.Background() | ||
| require.False(t, Hide(parent, "event")) | ||
| require.False(t, TakeHidden(parent, "event")) | ||
| ctx := WithCapture(parent) | ||
| require.False(t, Hide(ctx, "")) | ||
| child, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
| require.True(t, Hide(child, "event")) | ||
| other := WithCapture(ctx) | ||
| require.False(t, TakeHidden(other, "event"), "nested runner runs must not consume their parent's decisions") | ||
| require.True(t, TakeHidden(ctx, "event")) | ||
| require.False(t, TakeHidden(ctx, "event"), "consumed decisions must be released") | ||
| } | ||
|
|
||
| func TestHiddenEventsAcrossConcurrentNodes(t *testing.T) { | ||
| ctx := WithCapture(context.Background()) | ||
| var wg sync.WaitGroup | ||
| for i := 0; i < 20; i++ { | ||
| wg.Add(1) | ||
| go func(i int) { | ||
| defer wg.Done() | ||
| Hide(ctx, fmt.Sprint(i)) | ||
| }(i) | ||
| } | ||
| wg.Wait() | ||
| for i := 0; i < 20; i++ { | ||
| require.True(t, TakeHidden(ctx, fmt.Sprint(i))) | ||
| } | ||
| } |
| 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,24 @@ 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
+1210
to
+1214
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] 新增的强制等待需要兼容 Graph 对中间节点消息的过滤 当 Runner 配置 approval Reviewer,图结构为 已用相同的单轮 mock 模型和两个工具调用对比:在无 context deadline 的情况下,修改前 cbd89b0 执行两个工具,随后工具结果确认等待 5 秒超时、记录警告并继续;当前 68a6ad9 在工具调用事件确认处等待 5 秒后报错,工具执行数为 0。Graph 丢失确认的底层缺陷原本就存在,本 PR 新增的回归是把失败提前到审批之前并终止工具处理。 建议让内部会话写入及完成确认链路独立于对外消息过滤,并补充“非终点 AgentNode + terminal-only + Reviewer”的回归测试。仅对被丢弃事件直接发送确认,会继续缺少当前审批上下文,无法满足本次修复的目标。 |
||
| } | ||
| // Snapshot before emission transfers ownership to downstream event plugins. | ||
| completionID := agent.GetAppendEventNoticeKey(llmResponseEvent.ID) | ||
| emitCtx := p.ctx | ||
| var emitSpan oteltrace.Span | ||
| emitStarted := false | ||
|
|
@@ -1209,9 +1226,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, | ||
| completionID, flowEventWaitTimeout(p.ctx)) | ||
| } | ||
| 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( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 4567
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 14808
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 7422
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 12006
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 9791
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 7604
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 1742
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 4927
Script executed:
Repository: trpc-group/trpc-agent-go
Length of output: 2287
Keep completion-required tool-call responses visible to the runner under GraphTerminalMessagesOnly.
When a nonterminal AgentNode child emits a tool-call response, the Graph message filter drops it without notifying completion. The child LLM flow then blocks in
emitLLMResponseuntil the completion timeout, so tool execution stalls and the node fails. Make the filter hand this event to the runner for persistence, or keep it out of the Graph stream without dropping its completion path.中文
当 GraphTerminalMessagesOnly 隐藏非终端 AgentNode 的输出时,需要保留完成确认的工具调用响应。
非终端 AgentNode 子流程发出工具调用响应时,Graph 消息过滤器会在未通知完成的情况下丢弃该事件。子 LLM 流程随后在
emitLLMResponse中阻塞,直到完成等待超时。因此工具执行停止,节点失败。应让过滤器把该事件交给 Runner 持久化,或在不丢弃完成路径的前提下将其排除在 Graph 输出流之外。