-
Notifications
You must be signed in to change notification settings - Fork 827
Add TextContent.Annotations #6619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for content annotations across AI abstractions and OpenAI clients, introducing new annotation types, serialization support, client mapping, and tests.
- Introduce
AIAnnotation
and derivedCitationAnnotation
with serialization metadata - Add
Annotations
property toTextContent
and update JSON metadata - Map provider annotations in OpenAI clients and adjust coalescing logic
- Extend tests to cover annotation serialization and client integration
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/AIAnnotation.cs | Add base annotation type with JSON polymorphic support |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationAnnotation.cs | Define citation-specific annotation properties |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/TextContent.cs | Add Annotations property to text content |
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs | Update coalescing to skip annotated content |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponseChatClient.cs | Map service output annotations into TextContent |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIChatClient.cs | Attach message-level annotations in chat responses |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantChatClient.cs | Handle streaming text annotations from assistants |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Microsoft.Extensions.AI.Abstractions.json | Update API metadata for new annotation types |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Utilities/AIJsonUtilitiesTests.cs | Adjust test for null‐ignoring during serialization |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/*.cs | Add unit tests for annotation roundtrips |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs | Add integration test for web search annotations |
test/Libraries/Microsoft.Extensions.AI.Integration.Tests/ChatClientIntegrationTests.cs | Refactor _chatClient field to ChatClient property |
Comments suppressed due to low confidence (5)
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs:23
- Consider adding a streaming version of this annotation test (e.g., using GetStreamingResponseAsync) to ensure annotations are correctly surfaced in streaming scenarios as well.
[ConditionalFact]
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantChatClient.cs:218
- Add unit tests for the OpenAIAssistantChatClient to validate that
TextAnnotation
mappings (placeholder, start/end index, tool names) are correctly applied in streaming responses.
if (mcu.TextAnnotation is { } tau)
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantChatClient.cs:218
- [nitpick] The variable name
tau
is not immediately descriptive. Consider renaming it totextAnnotation
for clarity.
if (mcu.TextAnnotation is { } tau)
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIChatClient.cs:490
- [nitpick] Consider replacing the LINQ
OfType<>().FirstOrDefault()
call with a simple loop to find the firstTextContent
to avoid potential overhead in high-throughput scenarios.
TextContent? annotationContent = returnMessage.Contents.OfType<TextContent>().FirstOrDefault();
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs:236
- [nitpick] The nested
TryAsCoalescable
helper improves clarity but could be extracted as a private method or moved outside the loop for readability and easier unit testing.
static bool TryAsCoalescable(AIContent content, [NotNullWhen(true)] out TContent? coalescable)
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/AIAnnotation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationSourceLocation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationSourceLocation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationSourceLocation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationSourceLocation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CitationSourceLocation.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantChatClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I added some questions/comments about bits where I think the meaning may be hard to interpret but the broad concept makes sense and feels valuable.
Note that there's a variety of things OpenAI doesn't support or expose that others do, e.g. Google, Anthropic, and AWS all support including snippets of the cited content; I've included such things here, as well.
That sounds wise to me. Including a "snippet" is extremely helpful to the app developer - maybe OpenAI (or its client library) will add that later. It's good we're not restricted to a pure lowest-common-denominator set of information and instead are modelling what's common across a broad range of providers.
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/AIAnnotation.cs
Show resolved
Hide resolved
Worth checking the W3C Annotation Model Could be helpful on defining the specialized annotations which in this document are named like
|
I've addressed the feedback. @rogerbarreto and @SteveSandersonMS, could you both please take another look? Including at my naming choices as part of addressing Roger's feedback. @eiriktsarpalis, can you also review please? Thanks. |
@stephentoub Looks good to me. I see you've ended up going for the most general design. Given that it's a relatively advanced feature, that seems a reasonable tradeoff even if people have to do a bit of casting to access character indices. Moving annotations to |
Adds support to M.E.AI.Abstractions for annotations.
Some design notes:
Microsoft Reviewers: Open in CodeFlow