.NET: Add declarative HttpRequestAction sample#5572
Conversation
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 86%
✓ Correctness
The PR introduces a
MergeForLastMessageextension method to preserve original text content while keeping server-side media references when settingSystem.LastMessage. It also reordersExecuteAsyncinRootExecutorto run afterSetLastMessageAsync, adds a guard inWorkflowRunnerto prevent re-processing already-responded-to external requests during resume, and adds anInvokeHttpRequestsample. The core logic changes are correct: theMergeForLastMessagemerge strategy is sound, theRootExecutorreordering is safe because generatedExecuteAsyncimplementations only initialize variables (independent ofSystem.LastMessage) and still complete before child actions execute viaSendResultMessageAsync, and theWorkflowRunnerguard correctly skips replayed requests by comparingRequestId. One test claims to exercise a fallback path but doesn't actually reach it due toChatMessageconstructor behavior.
✓ Security Reliability
This PR adds HttpRequestAction support (sample + factory plumbing), introduces a MergeForLastMessage extension to preserve original user text while keeping server-side media references after round-tripping, reorders RootExecutor so System.LastMessage is set before workflow actions execute, and adds a guard in WorkflowRunner to prevent duplicate request processing. All changes are internal or shared-sample code operating on trusted data within the workflow pipeline. No injection risks, resource leaks, missing input validation, or unhandled failure modes were identified. The MergeForLastMessage method mutates the round-tripped message's Contents list in place, but this is safe since the object is freshly returned from CreateMessageAsync and consumed only once. The unit tests thoroughly cover the merge behavior.
✓ Test Coverage
The new
MergeForLastMessageextension method has thorough unit test coverage with 9 test cases covering null input, text replacement, server media preservation, multiple text slots, fallback paths, server property preservation, and empty contents. However, there are two notable test coverage gaps: (1) theWorkflowRunner.csbehavioral change that filters duplicateRequestInfoEventrequest IDs has zero test coverage, and (2) theRootExecutor.csreordering — movingExecuteAsyncafterSetLastMessageAsync— has no test that specifically validates the new execution order matters (existing workflow tests use mocks that return identical text, making the merge a no-op). TheWorkflowRunnerandWorkflowFactoryare shared sample utilities without a test project, which partially explains their lack of tests. Overall, the unit tests for the core new logic are well-structured and meaningful.
✓ Design Approach
The workflow/runtime changes mostly head in the right direction, but the new
MergeForLastMessagestrategy is too narrow for multimodal input. It restores missing text by appending it after provider-returned content, which can reorder text relative to media and change the meaning ofSystem.LastMessagewhen that message is later reused as agent input.
Suggestions
- Consider adding a workflow-level test where the mock's
CreateMessageAsyncreturns a message with stripped/altered text, then assert thatSystem.LastMessage.Textstill reflects the original user input. The existingDeclarativeWorkflowTest.CreateMockProviderreturns identical text, making the merge a no-op and leaving theRootExecutorreordering—the key behavioral fix—effectively unvalidated at the integration level.
Automated review by peibekwe's agents
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new declarative InvokeHttpRequest workflow sample and fixes a regression where System.LastMessage could lose user-authored text after AgentProvider round-tripping, while preserving server-side media references.
Changes:
- Adds
InvokeHttpRequestdeclarative workflow sample (YAML + runner) demonstratingHttpRequestAction. - Introduces
ChatMessage.MergeForLastMessage(...)and wires it into executors to preserve original user text inSystem.LastMessagewhile keeping server media references. - Updates workflow execution plumbing to pass through an
IHttpRequestHandlerand adjusts request tracking behavior inWorkflowRunner.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.UnitTests/Extensions/ChatMessageExtensionsTests.cs | Adds unit tests covering merge semantics for System.LastMessage. |
| dotnet/src/Shared/Workflows/Execution/WorkflowRunner.cs | Adjusts how request info updates externalResponse based on request id. |
| dotnet/src/Shared/Workflows/Execution/WorkflowFactory.cs | Adds HttpRequestHandler wiring into workflow options for HttpRequestAction. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Kit/RootExecutor.cs | Stores merged last message (original text + server media) before executing workflow. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/DeclarativeWorkflowExecutor.cs | Stores merged last message (original text + server media) for declarative execution path. |
| dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Extensions/ChatMessageExtensions.cs | Implements MergeForLastMessage merging logic. |
| dotnet/samples/03-workflows/Declarative/InvokeHttpRequest/Program.cs | New sample host wiring up DefaultHttpRequestHandler and running the workflow. |
| dotnet/samples/03-workflows/Declarative/InvokeHttpRequest/InvokeHttpRequest.yaml | New workflow using HttpRequestAction + follow-up agent loop. |
| dotnet/samples/03-workflows/Declarative/InvokeHttpRequest/InvokeHttpRequest.csproj | New sample project file. |
| dotnet/agent-framework-dotnet.slnx | Adds the new sample project and minor ordering tweaks. |
Description
Adds a new declarative
InvokeHttpRequestsample demonstratingHttpRequestActionwith a multi-turn conversation. Also fixes regression onSystem.LastMessagebehavior where user-authored text gets lost in state afterAgentProviderround-tripping strips or altersTextContent.Contribution Checklist