.NET: Project ToolExecution events as FunctionCallContent/FunctionResultContent in GitHubCopilotAgent streaming - #6228

Pull request overview

This PR fixes GitHubCopilotAgent’s streaming event projection so Copilot SDK tool execution events are surfaced as semantic FunctionCallContent / FunctionResultContent updates rather than opaque AIContent, preventing models from misinterpreting successful tool calls as “incomplete” and fabricating timeouts (fixes #5897).

Changes:

• Add streaming dispatch cases for ToolExecutionStartEvent and ToolExecutionCompleteEvent.
• Project tool start/complete events into FunctionCallContent / FunctionResultContent, including argument parsing via ParseArguments.
• Add unit tests covering tool start/complete projections and error paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Automated Code Review

Reviewers: 4 | Confidence: 87%

✓ Correctness

The implementation correctly maps ToolExecutionStartEvent and ToolExecutionCompleteEvent to FunctionCallContent and FunctionResultContent respectively. Type alignment with the SDK (Arguments is System.Object, Success is bool, constructors match) is verified. Null-handling logic is sound: null-conditional operators correctly lift Success to bool?, and fallback values are properly applied throughout. The ParseArguments helper robustly handles string JSON, JsonElement, IDictionary, and fallback cases. No correctness issues found.

✓ Security Reliability

The implementation correctly maps ToolExecutionStartEvent and ToolExecutionCompleteEvent to FunctionCallContent/FunctionResultContent. JSON parsing in ParseArguments is well-guarded with try/catch for JsonException, null handling is comprehensive via null-conditional operators, and fallback paths ensure no unhandled failure modes. No injection risks, resource leaks, or unsafe deserialization patterns identified. The triming/AOT suppressions are a documented conscious trade-off.

✓ Test Coverage

Test coverage for the new tool execution event projection is solid, covering happy paths, null data, null arguments, invalid JSON, error cases, and multiple arguments. One notable gap: the success-with-null-result edge case (Success=true but Result is null) is untested, which would produce a null FunctionResultContent.Result rather than a fallback message. The ParseArguments helper also has defensive branches for JsonElement and IDictionary inputs that aren't exercised, though these may not be reachable depending on the SDK's property types.

✓ Design Approach

I did not find a design-approach issue in this diff. The change addresses the confirmed gap directly by projecting Copilot tool execution events into FunctionCallContent/FunctionResultContent, and the new tests cover the intended success, failure, null-data, and malformed-argument cases without contradicting nearby contracts I inspected.

Automated review by giles17's agents

Automated Code Review

Reviewers: 4 | Confidence: 91%

✓ Correctness

The changes correctly add handling for non-generic IDictionary types (like Hashtable) and empty/whitespace string arguments in the ParseArguments method. The ordering of type checks is correct: IDictionary<string, object?> → IDictionary → string → JsonElement → fallback. No correctness bugs found.

✓ Security Reliability

The changes add defensive argument parsing for non-generic dictionaries and empty/whitespace strings. The code is well-structured with proper null handling, try-catch around JSON deserialization, and no new security or reliability concerns introduced. The IDictionary enumeration is safe, no resource leaks or injection risks are present, and the existing JsonException handling prevents crashes from malformed input.

✓ Test Coverage

Test coverage for the new production code paths is solid. Both new branches in ParseArguments (non-generic IDictionary handling and string.IsNullOrWhiteSpace guard) have dedicated tests with meaningful assertions. The added tests also backfill coverage for pre-existing paths (JsonElement happy path, IDictionary<string, object?> direct pass-through, null Result on successful completion). All assertions verify specific values rather than just absence of exceptions. No blocking test coverage gaps found.

✗ Design Approach

I found one design issue in the new non-generic IDictionary fallback. The change fixes the generic-invariance gap for string-keyed dictionaries, but it also broadens acceptance to arbitrary dictionary keys by stringifying them, which can silently rewrite malformed inputs instead of preserving or rejecting them. I did not find other design-approach problems in the added null/whitespace and JsonElement handling.

Flagged Issues

• dotnet/src/Microsoft.Agents.AI.GitHub.Copilot/GitHubCopilotAgent.cs:434 stringifies arbitrary IDictionary keys via ToString(), which silently rewrites invalid inputs (e.g., keys 1 and "1" collapse to one entry; a null key becomes ""). This broadens acceptance beyond the repo's existing string-keyed dictionary pattern instead of validating that keys are actually strings.

Automated review by giles17's agents

Automated Code Review

Reviewers: 4 | Confidence: 94% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach

Automated review by giles17's agents