Skip to content

[dotnet-code] Extract portable message unwrap helper - #1108

Merged
Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-code-message-router-unwrap-20260918222921-0aed06660944a6f8
Sep 22, 2026
Merged

Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-code-message-router-unwrap-20260918222921-0aed06660944a6f8

Conversation

@michelle-clayton-work

Copy link
Copy Markdown
Contributor

Tip

Your pull request is ready to create! 🎉 ✅

Everything is OK—the changes have been pushed to branch dotnet-code-message-router-unwrap-20260918222921-0aed06660944a6f8. Please review the changes, including any protected files, before creating the pull request.

Create the pull request

The original pull request description is below.


Summary

Extracts the workflow message router's PortableValue unwrapping into a small unexported helper. This keeps Go routing behavior the same while making the route flow more structurally similar to the .NET MessageRouter.RouteMessageAsync conversion step, which should make future .NET-to-Go comparisons easier.

.NET Reference

  • dotnet/src/Microsoft.Agents.AI.Workflows/Execution/MessageRouter.cs - unwraps PortableValue messages to the registered runtime type before handler lookup.

Public API and Behavior

No public Go API changed. No intentional behavior change was made.

Tests

  • gofmt -w workflow/route.go
  • go test ./workflow

Notes

Rejected candidates:

  • dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/WorkflowModel.cs - no current Go declarative workflow counterpart was found, so a structural cleanup would be speculative.
  • dotnet/tests/Microsoft.Agents.AI.Workflows.Declarative.Mcp.UnitTests/DefaultMcpToolHandlerTests.cs - nearest Go MCP areas are provider/tool-facing and broader than the requested tiny portability cleanup.
  • Open approved [dotnet-code] PR found: [dotnet-port-api] Add compaction-backed history provider #1092, covering compaction rather than this workflow router area. Two lower-integrity PR search results were filtered by the environment and were not inspected.

The upstream .NET git fetch was blocked by the environment network boundary, so the .NET reference was sampled/read via the read-only GitHub MCP CLI instead.

Closes #1106

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 19, 2026 19:09
@github-actions github-actions Bot added area:workflow Changes files in the workflow area size:small At most 30 changed lines across at most 2 files labels Sep 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Pass PortableValue by pointer or otherwise propagate cache mutations to preserve existing behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Extracts portable message unwrapping into an unexported workflow router helper.

Changes:

  • Replaces inline unwrapping with unwrapPortableMessage.
  • Preserves registered and polymorphic routing paths.
  • No public API changes.
File Summary
workflow/​route.go Extracts portable message unwrapping. Moderate issue: passing PortableValue by value prevents cache updates from propagating.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread workflow/route.go Outdated
} else if value := pvalue.Any(); value != nil && pvalue.TypeID.MatchPolymorphic(reflect.TypeOf(value)) {
msg = value
}
msg = mr.unwrapPortableMessage(pvalue)
@github-actions github-actions Bot added the kind:code Changes production behavior or code label Sep 19, 2026
@github-actions

This comment has been minimized.

@michelle-clayton-work

Copy link
Copy Markdown
Contributor Author

Copilot address PR feedback

Copilot AI and others added 2 commits September 21, 2026 19:21
Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>
Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>

Copilot AI commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Copilot address PR feedback

Addressed in 46af8bb (with the pointer-cache requirement documented in 34cd37d). Added a regression assertion for the delayed catch-all path; go test ./workflow passes.

@github-actions github-actions Bot added the kind:tests Changes tests, fixtures, or test infrastructure label Sep 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Scope: internal-only

Changed Go contract: None. messageRouter.unwrapPortableMessage is an unexported helper on an unexported type; no exported functions, methods, types, fields, constants, or options were added, removed, or changed. PortableValue.Delayed() already existed prior to this PR and is unchanged in signature.

Upstream evidence reviewed: dotnet/src/Microsoft.Agents.AI.Workflows/Execution/MessageRouter.cs — the PR description cites this as the structural model for the extracted conversion step (unwrap PortableValue/registered runtime type before handler dispatch). No equivalent Go-specific "delayed deserialization cache" concept exists in .NET (which does not defer JSON decoding the same way), so no direct upstream symbol maps to the cache-preservation bugfix itself; this is a Go-implementation-specific correctness detail.

Result: aligned (out of scope for parity concerns)

Summary: This PR (1) extracts the existing portable-value unwrap logic in routeMessage into a small helper unwrapPortableMessage, mirroring the .NET MessageRouter conversion step structurally, and (2) fixes a bug where the unwrap path previously returned a stale copy of pvalue instead of the copy with its lazily-populated cache field, so catch-all handlers now receive a PortableValue that reports Delayed() == false once decoded. Both changes are confined to internal/unexported code paths (workflow/route.go) plus a test assertion addition (workflow/route_test.go) verifying the cache fix. No exported Go API surface changed, so public-api-change is not applicable, and no examples were touched. No cross-language parity issue is introduced since the affected mechanism (delayed JSON deserialization caching) is Go-internal plumbing without a corresponding .NET/Python public contract.

Generated by Go API Consistency Review Agent for #1108 · copilot · auto · 34.8 AIC · ⌖ 6.95 AIC · ⊞ 9.2K · ◷

@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Sep 22, 2026
Merged via the queue into main with commit fb340a0 Sep 22, 2026
28 checks passed
@qmuntal
Quim Muntal (qmuntal) deleted the dotnet-code-message-router-unwrap-20260918222921-0aed06660944a6f8 branch September 22, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:workflow Changes files in the workflow area kind:code Changes production behavior or code kind:tests Changes tests, fixtures, or test infrastructure size:small At most 30 changed lines across at most 2 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[dotnet-code] Extract portable message unwrap helper

4 participants