.NET: Fix workflow session bug#7032
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a .NET workflow session issue where external request “envelope” payloads could fail to be recognized/rehydrated when the recorded request type’s assembly identity (notably version) no longer matches what’s loaded at runtime. The fix shifts envelope-type resolution to the live workflow port declarations and validates type identity via TypeId.IsMatch(...) (which normalizes away assembly version/culture/token data).
Changes:
- Updated
WorkflowSessionto resolve potential envelope payload types from the workflow’s live request ports (and only treat a payload as an envelope when the recordedTypeIdmatches the port’s declared request type). - Removed the previous
ResolveTypeLenient+ cache approach that relied on partial-name binding (Type.GetType(...)with a “simple” assembly name). - Consolidated and expanded unit tests to cover envelope resolution/matching across assembly-version and generic-argument version mutations, plus negative cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows/WorkflowSession.cs | Reworks envelope detection to use live workflow port types and TypeId.IsMatch(...); removes ResolveTypeLenient and the associated cache. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowSessionTests.cs | Adds updated unit coverage for envelope type resolution and envelope extraction based on port/type matching. |
| dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/WorkflowSessionResolveTypeLenientTests.cs | Deletes superseded tests that targeted the removed ResolveTypeLenient behavior. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90%
✓ Correctness
The PR replaces a static Type.GetType-based type resolution (ResolveTypeLenient) with a port-based resolution (ResolveEnvelopeType) that looks up the request type from the workflow's live port definitions. This is a sound architectural improvement: instead of resolving types by assembly-qualified name (which is fragile across version changes), the new code uses the port ID to find the port, then verifies the recorded TypeId matches the port's declared request type via TypeId.IsMatch (which ignores version/culture/publicKeyToken). The static-to-instance method conversions are correct and access this._workflow.Ports which is an init-only Dictionary<string, RequestPort> (read-only after construction, so thread-safe for concurrent reads). The ConcurrentDictionary cache removal is fine since the dictionary lookup + IsMatch is cheap. Tests comprehensively cover matching, version mutation, unknown port, type mismatch, and envelope detection scenarios. No correctness issues found.
✓ Security Reliability
This PR replaces Type.GetType-based dynamic type resolution (ResolveTypeLenient) with port-based type lookup (ResolveEnvelopeType), which is a net security improvement. The old approach could resolve arbitrary types from any loaded assembly given a controlled TypeId in a checkpointed payload; the new approach constrains resolution to types explicitly declared by the workflow's request ports. The TypeId.IsMatch method provides the same version-tolerant matching (comparing simple assembly names and normalized type names, ignoring version/culture/public key token). Methods that were previously static now correctly access this._workflow.Ports through instance dispatch. Test coverage is comprehensive, covering matching, version mutation, unknown port, and type mismatch scenarios. No security or reliability issues found.
✓ Test Coverage
The PR replaces the old
ResolveTypeLenientapproach (which usedType.GetTypewith partial-name binding) with a newResolveEnvelopeTypeapproach that resolves types from the live workflow ports. The test file was properly renamed and expanded. The new tests coverResolveEnvelopeTypewith 5 cases (happy path, assembly version mutation, generic argument version mutation, unknown port ID, type mismatch) andTryGetRequestEnvelopewith 3 cases (happy path, non-envelope port type, type mismatch). All key behavioral paths are tested with meaningful assertions. The only untested path is the second guard inTryGetRequestEnvelope(line 308) whereTryGetDataAssucceds but the result is not anIExternalRequestEnvelope, which is a minor gap since the first guard already filters non-envelope types. Overall, test coverage is adequate for this change.
✓ Failure Modes
This PR replaces a global Type.GetType-based type resolution (ResolveTypeLenient) with port-scoped resolution (ResolveEnvelopeType) that looks up the concrete type from the workflow's registered RequestPort dictionary. The new approach is safer: it only recognizes a request as an envelope when the port the request was sent through actually declares an envelope type, avoiding silent misresolution. TypeId.IsMatch handles version-mutation tolerance. The static ConcurrentDictionary cache is removed in favor of dictionary lookups on each call, which is a minor performance trade-off but not a failure mode. No silent failure paths, swallowed exceptions, or missing cleanup were introduced.
✗ Design Approach
The new port-based envelope resolution is consistent with checkpoint matching, but the added test suite now codifies a narrower contract than
IExternalRequestEnvelopecurrently documents. That leaves a real design mismatch for valid requests whose payload implements the envelope interface but whose port is declared as a broader assignable type.
Flagged Issues
- The new tests lock in "only when the port declares the envelope type" semantics, but
IExternalRequestEnvelopestill documents envelope handling based on the runtime payload itself (dotnet/src/Microsoft.Agents.AI.Workflows/IExternalRequestEnvelope.cs:22-27). BecauseExternalRequest.Createaccepts any payload assignable to the port type (dotnet/src/Microsoft.Agents.AI.Workflows/ExternalRequest.cs:49-60), a valid request such as aRequestPortdeclared asobjectcarrying an envelope instance would now silently lose envelope unwrapping/rewrapping behavior.
Automated review by peibekwe's agents
|
Flagged issue The new tests lock in "only when the port declares the envelope type" semantics, but Source: automated DevFlow PR review |
Motivation & Context
Fix workflow session bug.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.