Surface chat system_fingerprint and logprobs as response metadata - #1104
PratikDhanave (PratikDhanave) wants to merge 1 commit into
Conversation
The Chat path set ResponseID, FinishReason, and usage but never populated AdditionalProperties, dropping the response system_fingerprint and the per-choice logprobs that a caller requesting logprobs would expect back. The Responses path already populates AdditionalProperties, so this was also a chat-vs-responses asymmetry. Populate AdditionalProperties with SystemFingerprint (when present) and Logprobs (when the choice carries any) on both the non-streaming and streaming paths, matching the Python client which surfaces both as chat response metadata.
There was a problem hiding this comment.
🟡 Changes recommended
Streaming logprobs may be overwritten across chunks, and this path lacks coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Surfaces Chat response system_fingerprint and logprobs metadata through AdditionalProperties.
Changes:
- Propagates metadata for streaming and non-streaming responses.
- Adds tests for metadata exposure.
File summaries
| File | Summary |
|---|---|
provider/openaiprovider/chat.go |
Populates Chat response metadata. |
provider/openaiprovider/chat_test.go |
Tests metadata propagation. |
Review details
Suppressed comments (2)
provider/openaiprovider/chat.go:227
- The new streaming
Logprobsbranch is not exercised: the added streaming test only sends/assertsSystemFingerprint, while all existing streaming fixtures uselogprobs: null. Add a streamed chunk containing content or refusal logprobs and assert that the yielded update (and collected message) preserves theLogprobsmetadata, so this path cannot regress unnoticed.
if logprobs := chunk.Choices[0].Logprobs; len(logprobs.Content) > 0 || len(logprobs.Refusal) > 0 {
additionalProperties = map[string]any{"Logprobs": logprobs}
}
provider/openaiprovider/chat_test.go:457
- This streaming test only sends
system_fingerprint; it never supplies a non-empty chunklogprobsvalue, so it would pass if the new streaming Logprobs handling were removed or broken. Add a chunk with content/refusal logprobs and assert that the streamed update (and, if collection is supported, the collected response) preserves them.
func TestChatResponseMetadataSurfaced_Streaming(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "text/event-stream")
_, _ = io.WriteString(w, "data: {\"id\":\"chatcmpl-md\",\"object\":\"chat.completion.chunk\",\"created\":1727888631,\"model\":\"gpt-4o-mini\",\"system_fingerprint\":\"fp_test\",\"choices\":[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"content\":\"ok\"},\"finish_reason\":\"stop\"}]}\n\n")
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if logprobs := chunk.Choices[0].Logprobs; len(logprobs.Content) > 0 || len(logprobs.Refusal) > 0 { | ||
| additionalProperties = map[string]any{"Logprobs": logprobs} |
|
Scope: user-visible behavior (bug fix) Changed Go contract: No exported API added/changed. Upstream evidence reviewed:
Result: Aligned. The Go change closes a genuine chat-vs-responses asymmetry (the Responses path already surfaced comparable metadata via
|
The Chat path built its
ResponseUpdatewithResponseID/FinishReason/usage but never setAdditionalProperties, so two pieces of response metadata were dropped:system_fingerprint(on the completion / chunk), andlogprobs(returned when a caller requests them viaLogprobs/TopLogprobs).The Python client carries both as chat response metadata (
_get_metadata_from_chat_response→system_fingerprint,_get_metadata_from_chat_choice→logprobs). The Responses path in this provider already populatesAdditionalProperties, so this was also a chat-vs-responses asymmetry — the existing chat fixtures even containsystem_fingerprint/logprobsvalues that nothing surfaced.Change
AdditionalPropertieswithSystemFingerprint(when non-empty) andLogprobs(when the choice carries content/refusal logprobs) on both the non-streaming and streaming paths. Keys follow the PascalCase convention already used by the Responses path helper (responsesPopulateAdditionalProperties).Tests
TestChatResponseMetadataSurfaced_NonStreamingand_Streaming: assertSystemFingerprint(and, non-streaming,Logprobs) reach the messageAdditionalProperties. Both fail before the change (no message carries any), pass after.