Repository navigation
[dotnet-port-api] Add Response.ModelID - #1156
PratikDhanave (PratikDhanave) wants to merge 5 commits into
Conversation
Response and ResponseUpdate now carry ModelID, the identifier of the model that produced the response, matching M.E.AI ChatResponse/ChatResponseUpdate (ProcessUpdate folds ModelId). Response.Update folds it like ResponseID, and ToUpdates round-trips it. The OpenAI (chat + responses), Anthropic, and Gemini providers populate it from the model on the response. ConversationID is intentionally not added: in Go the conversation identity lives on Session.ServiceID, so a separate field would be redundant. Implements microsoft#1140.
github-actions
Bot
added
area:agent
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Streaming model IDs and metadata-only round-tripping remain incomplete.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Adds provider-agnostic ModelID fields to agent responses and response updates, with provider mappings and folding support.
Changes:
- Adds and propagates
ModelID. - Surfaces model IDs across OpenAI, Anthropic, and Gemini providers.
- Adds response and OpenAI chat tests.
| File | Reviewed changes |
|---|---|
provider/openaiprovider/responses.go |
Non-streaming model ID is populated; streaming updates still omit it. |
provider/openaiprovider/chat.go |
Surfaces the model ID for chat responses. |
provider/openaiprovider/chat_test.go |
Tests OpenAI chat model ID exposure. |
provider/geminiprovider/agent.go |
Non-streaming model ID is populated; streaming updates still omit it. |
provider/anthropicprovider/agent.go |
Non-streaming model ID is populated; streaming updates still omit it. |
agent/response.go |
Adds and folds ModelID; metadata-only updates omit it during round-tripping. |
agent/response_test.go |
Tests model ID folding and round-tripping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| AgentID: resp.AgentID, | ||
| MessageID: msg.ID, | ||
| ResponseID: resp.ID, | ||
| ModelID: resp.ModelID, |
| Role: message.RoleAssistant, | ||
| MessageID: resp.ID, | ||
| ResponseID: resp.ID, | ||
| ModelID: string(resp.Model), |
| yield(&agent.ResponseUpdate{ | ||
| Contents: responseContents, | ||
| Role: message.RoleAssistant, | ||
| ModelID: resp.ModelVersion, |
|
|
||
| currentUpdate := &agent.ResponseUpdate{ | ||
| ResponseID: resp.ID, | ||
| ModelID: resp.Model, |
github-actions
Bot
added
kind:code
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · copilot · auto · 98.2 AIC · ⌖ 5.96 AIC · ⊞ 9.2K
|
|
||
| // ModelID is the identifier of the model that produced this response, when | ||
| // the provider supplies it. It is empty otherwise. | ||
| ModelID string `json:",omitzero"` |
There was a problem hiding this comment.
This PR adds ModelID to agent.Response/agent.ResponseUpdate (the Go equivalent of upstream AgentResponse/AgentResponseUpdate, not ChatResponse/ChatResponseUpdate). Upstream, ModelId/model exists only on the lower-level ChatResponse/ChatResponseUpdate types:
- .NET:
Microsoft.Agents.AI.Abstractions/AgentResponse.csandAgentResponseUpdate.cshave noModelIdproperty (properties areAgentId,ResponseId,ContinuationToken,CreatedAt,FinishReason,Usage,RawRepresentation,AdditionalProperties).ModelIdis folded only inAIAgentChatClient.CloneWithConversationIdonChatResponse(dotnet/src/Microsoft.Agents.AI/ChatClient/AIAgentChatClient.cs:368), and referenced inAgentResponseUpdateTests.ConstructorWithChatResponseUpdateRoundtrips(dotnet/tests/Microsoft.Agents.AI.Abstractions.UnitTests/AgentResponseUpdateTests.cs:42) as aChatResponseUpdatefield that is not asserted to roundtrip ontoAgentResponseUpdate. - Python:
AgentResponse.__init__/AgentResponseUpdate.__init__(python/packages/core/agent_framework/_types.py:2848and:3134) take nomodel/model_idparameter, while the siblingChatResponse.__init__/ChatResponseUpdate.__init__(same file,:2449and:2735) do.
So both upstream implementations deliberately keep model identity at the chat-client layer and omit it from the agent-level response contract. Adding ModelID unconditionally to Go's agent.Response/ResponseUpdate (the AgentResponse analog) is a divergence from that design, not merely an idiomatic Go difference — it's a new agent-level field neither upstream implementation exposes.
Suggested resolution: either (a) confirm with maintainers this is an intentional Go-specific enhancement over the agent-level contract (and document that divergence), or (b) if a ModelID surface is wanted, scope it to a chat-level type analogous to ChatResponse/ChatResponseUpdate if/when Go introduces one, rather than agent.Response.
Quim Muntal (qmuntal)
left a comment
Quim Muntal (qmuntal)
left a comment
There was a problem hiding this comment.
Fix review comments.
# Conflicts: # agent/response.go # provider/openaiprovider/responses.go
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
proxy.golang.orgstorage.googleapis.comsum.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"
- "storage.googleapis.com"
- "sum.golang.org"See Network Configuration for more information.
Generated by Go API Consistency Review Agent for #1156 · copilot · auto · 125.5 AIC · ⌖ 7.69 AIC · ⊞ 13.9K
|
|
||
| // ModelID is the identifier of the model that produced this response, when | ||
| // the provider supplies it. It is empty otherwise. | ||
| ModelID string `json:",omitzero"` |
There was a problem hiding this comment.
Parity concern (unresolved from prior review): ModelID added at the agent-response layer, not the chat-response layer
Upstream .NET and Python deliberately keep model identity on the lower-level chat-response contract and omit it from the agent-level response contract that Go's agent.Response/ResponseUpdate mirror:
- .NET:
Microsoft.Agents.AI.Abstractions/AgentResponse.csandAgentResponseUpdate.cshave noModelIdproperty (members areAgentId,ResponseId,ContinuationToken,CreatedAt,FinishReason,Usage,RawRepresentation,AdditionalProperties).ModelIdis folded only ontoChatResponseinAIAgentChatClient.CloneWithConversationId(dotnet/src/Microsoft.Agents.AI/ChatClient/AIAgentChatClient.cs:368).AgentResponseUpdateTests.ConstructorWithChatResponseUpdateRoundtrips(dotnet/tests/Microsoft.Agents.AI.Abstractions.UnitTests/AgentResponseUpdateTests.cs:42) exercisesModelIdas aChatResponseUpdatefixture field but never asserts it round-trips ontoAgentResponseUpdate. - Python:
AgentResponse.__init__/AgentResponseUpdate.__init__(python/packages/core/agent_framework/_types.py:2848,:3134) take nomodel/model_idparameter, while siblingChatResponse.__init__/ChatResponseUpdate.__init__(same file,:2449,:2735) do.
Go has no chat-level response type distinct from agent.Response, so this PR's choice to add ModelID at the agent layer is a real API-shape divergence from both upstream implementations, not just an idiomatic naming difference. Suggest either explicitly confirming with maintainers that this is an intentional Go-specific widening of the agent-level contract (and documenting the divergence in the doc comment), or deferring the field to a future chat-level type if/when Go introduces one.
|
|
||
| currentUpdate := &agent.ResponseUpdate{ | ||
| ResponseID: resp.ID, | ||
| ModelID: resp.Model, |
There was a problem hiding this comment.
ModelID is only propagated on non-streaming responses across three providers
This PR populates ResponseUpdate.ModelID from the final/non-streaming response object in responsesProcessResponse (here), provider/anthropicprovider/agent.go (non-streaming branch, resp.Model), and provider/geminiprovider/agent.go (non-streaming branch, resp.ModelVersion). None of the corresponding streaming paths set it:
responsesProcessStreamingUpdate(provider/openaiprovider/responses.go) builds every streamingagent.ResponseUpdateviacreateUpdate, which never setsModelID, even thoughResponseCreatedEvent/ResponseCompletedEventcarryevent.Response.Model.- The Anthropic streaming loop (
provider/anthropicprovider/agent.go, around thestream.Next()loop) never readsaccumulated.Modelorevent.Message.Modelinto the yielded updates. - The Gemini streaming loop (
provider/geminiprovider/agent.go, around the per-chunkyield) never readsresp.ModelVersioninto the yielded updates.
Upstream M.E.AI sets ChatResponseUpdate.ModelId consistently for both streaming and non-streaming chat responses (the same ModelId property backs both ChatResponse and ChatResponseUpdate, and provider adapters populate it per-chunk). As written, collecting a streaming run via ResponseStream.Collect on any of these three providers silently produces an empty Response.ModelID, even though the PR's non-streaming tests pass. Suggest populating ModelID in each streaming update from the per-chunk/per-event model field, matching the non-streaming branches added in this PR.
There was a problem hiding this comment.
Addressed — ModelID is now set on the streaming updates for all three providers: OpenAI Responses threads the model through responsesStreamState from the lifecycle events' Response.Model; Anthropic reads it from the message_start Message.Model; Gemini reads each chunk's ModelVersion (and the final usage update). Added TestStreamingModelIDSurfaced to lock in the Collect() round-trip.
API consistency review
Changed Go contract
Upstream evidence reviewed
AssessmentThe new Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
github-actions
Bot
added
the
parity-approved
There was a problem hiding this comment.
🟡 Changes recommended
A model-only response round-trip creates an empty assistant message instead of preserving an empty message list.
6 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| // ModelID is the identifier of the model that produced this response, when | ||
| // the provider supplies it. It is empty otherwise. | ||
| ModelID string `json:",omitzero"` |
| } | ||
|
|
||
| if hasAdditionalProperties || resp.ContinuationToken != "" { | ||
| if hasAdditionalProperties || resp.ContinuationToken != "" || resp.ModelID != "" { |
Implements #1140.
M.E.AI
ChatResponse/ChatResponseUpdatecarry aModelIdthatProcessUpdatefolds onto the response. Go had no canonical, provider-agnostic way to read which model produced a response — it was only surfaced ad hoc via providerAdditionalProperties.Change
ModelIDtoagent.Responseandagent.ResponseUpdate.Response.Update(likeResponseID/FinishReason) and propagate it inToUpdates.ConversationIDfrom the issue is intentionally omitted: Go conversation identity lives onSession.ServiceID, so a separate field would be redundant.Tests
TestResponse_Update_ModelID: ModelID folds from a later update and round-trips throughToUpdates.TestChatModelIDSurfaced: the OpenAI chat provider surfaces the response model onResponse.ModelID.