[dotnet-port-fixes] Execute invocable tool siblings - #863
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the agent/harness/toolautocall middleware to correctly handle mixed assistant turns that contain both (a) invocable local function calls and (b) declaration-only (non-invocable) schema tool calls. The middleware now executes the invocable siblings, leaves declaration-only calls actionable for the caller, and only marks the executed local calls as informational-only, with a regression test covering the mixed-call ordering.
Changes:
- Added an execution-plan step to split function calls into “processable now” vs “terminate after processing” to avoid bypassing invocable siblings.
- Updated the main loop to use the plan for termination decisions and for selecting which calls to execute/mark informational-only.
- Added a regression test for mixed invocable + non-invocable schema tool calls to ensure invocable siblings still execute.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| agent/harness/toolautocall/autocall.go | Builds and uses a function-call execution plan so invocable siblings execute even when non-invocable schema tools are present. |
| agent/harness/toolautocall/autocall_test.go | Adds regression coverage ensuring mixed-call ordering executes the invocable tool and leaves declaration-only calls actionable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
Quim Muntal (qmuntal)
left a comment
There was a problem hiding this comment.
microsoft/agent-framework#7388 implements a new feature, it doesn't fix a bug, but the Go PR landed as part of the dotnet-port-fixes. AFAIU, this shouldn't have happened. Also, the port seems incomplete, as the .NET feature is opt-in.
|
Quim Muntal (@qmuntal) opened up #872 to improve the wording of the workflow prompts |
|
Quim Muntal (@qmuntal) I added the public experimental Config.EnableExecutableFunctionBypassing option, defaulting to false, with tests for both disabled and enabled behavior |
…-calls-d7ea5478fe3d9fd5
Go API Consistency ReviewScopeThis PR adds Upstream referenceThe PR is described as a port of microsoft/agent-framework#7388, commit Parity finding — behavioral divergence (requires confirmation)The upstream .NET feature (
The Go implementation uses a different strategy: invocable siblings are executed immediately in the same turn, and only declaration-only calls are returned to the caller. There is no session-state storage or cross-turn re-injection. This means the observable semantics differ: in .NET, tool results from invocable calls appear in the next turn's history; in Go, they appear in the current turn's tool result messages. In multi-turn or stateful scenarios this difference may affect conversation coherence. What needs resolution
This review does not block correctness of the opt-in guard added in this PR (which is a clear improvement over the prior unconditional behavior). The concern is semantic alignment with upstream for anyone relying on parity between SDKs.
|
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent for #863 · sonnet46 · 39.5 AIC · ⌖ 4.99 AIC · ⊞ 6K
| if !f.enableExecutableFunctionBypassing { | ||
| return functionCallExecutionPlan{terminate: true} | ||
| } | ||
| terminate = true |
There was a problem hiding this comment.
Parity gap vs upstream .NET — execution strategy diverges from InvocableFunctionBypassingChatClient
The upstream .NET commit aaaa56bc solves the mixed-call problem via a store-and-defer approach: InvocableFunctionBypassingChatClient removes invocable calls from the current response, stores them in AgentSessionStateBag, and returns only declaration-only calls to the caller. On the next request, the stored calls are re-injected as pre-approved ToolApprovalResponseContent so FunctionInvokingChatClient reconstructs and executes them.
The Go implementation here takes a different approach: it executes invocable siblings immediately in the same turn.
| Concern | .NET EnableInvocableFunctionBypassing |
Go EnableExecutableFunctionBypassing |
|---|---|---|
| Invocable call timing | Deferred to next request via session state | Executed immediately in current turn |
| Session state used | Yes (AgentSessionStateBag) |
No |
| Re-injection mechanism | Pre-approved ToolApprovalResponseContent |
N/A |
| Experimental gate | [Experimental] attribute |
opt-in field (no experimental marker) |
This is a meaningful behavioral divergence. The PR description calls it a port of #7388, but the execution model is structurally different. Please either:
- Confirm this is an intentional, language-idiomatic divergence and document it in
docs/dotnet-go-sdk-feature-comparison.md, or - Align with the upstream store-and-defer model if cross-turn session-state persistence is required for correctness in multi-turn scenarios.
Summary
Updated
agent/harness/toolautocallso invocable local tool calls still run when the same assistant turn also includes declaration-only schema tools. The middleware now executes the invocable siblings, leaves declaration-only calls actionable for the caller, and marks only the executed local calls informational-only. Added a regression test covering mixed-call ordering.Ported .NET PRs
aaaa56bc60b6703b305664f196e74b284e425918, based ondotnet/src/Microsoft.Agents.AI/ChatClient/InvocableFunctionBypassingChatClient.csanddotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/InvocableFunctionBypassingChatClientTests.cs.Breaking Changes
No.
Tests and Examples
go test ./agent/harness/toolautocallTestFunctionInvoking_MixedInvocableAndNonInvocableSchemaToolExecutesInvocableSiblingNotes
agent/harness/toolautocall; no exported Go API changed.TerminateOnUnknownCallsbehavior for unknown tools and only realigned mixed invocable/declaration-only schema-tool handling.Closes #842