mirror of
https://github.com/microsoft/semantic-kernel.git
synced 2026-10-12 02:19:06 +00:00
.Net: fix: address three static analysis issues (audio format, text search, KernelProcess) (#13925)
Fixes #13922 ## Problem Three static analysis issues identified by PVS-Studio in the .NET codebase: 1. **`ClientCore.ChatCompletion.cs`** — `GetAudioOutputMimeType` contained a duplicate `ChatOutputAudioFormat.Wav` check (lines 985 and 1000) making the second branch unreachable. The `Aac` audio format supported by the OpenAI SDK was silently unhandled, causing a `NotSupportedException` at runtime. 2. **`TextSearchStore.cs`** — `GetTextSearchResultsAsync` assigned a LINQ expression to a `results` variable (deferred execution) that was never iterated. The return statement duplicated the projection. The unused variable was dead code. 3. **`KernelProcess.cs`** — The constructor accepted a `threads` parameter but never assigned it to the `Threads` property, silently discarding all threads passed by callers. ## Solution 1. Replace the second `Wav` branch with an `Aac` branch and update the error message to list `'aac'` as a supported format. 2. Reuse the `results` variable in the `return` statement instead of duplicating the `Select` projection. 3. Assign the `threads` parameter to `this.Threads` when it is not null. ## Testing - Changes are logic-only with no new external dependencies. - Existing unit tests for `OpenAIChatCompletionService` and `TextSearchStore` continue to apply. - No new runtime behaviour is introduced for the `Wav` fix (it was already handled earlier in the method); `Aac` now maps to `"audio/aac"` as expected. --------- Co-authored-by: octo-patch <[email protected]> Co-authored-by: Roger Barreto <[email protected]>
This commit is contained in:
co-authored by
octo-patch
Roger Barreto
parent
dd026f3413
commit
c781da134e
+60
@@ -1981,6 +1981,16 @@ public sealed class OpenAIChatCompletionServiceTests : IDisposable
|
||||
{ "{\"voice\":\"echo\",\"format\":\"opus\"}", "{\"voice\":\"echo\",\"format\":\"opus\"}" },
|
||||
};
|
||||
|
||||
public static TheoryData<ChatOutputAudioFormat, string> AudioMimeTypeMappingData => new()
|
||||
{
|
||||
{ ChatOutputAudioFormat.Wav, "audio/wav" },
|
||||
{ ChatOutputAudioFormat.Aac, "audio/aac" },
|
||||
{ ChatOutputAudioFormat.Mp3, "audio/mp3" },
|
||||
{ ChatOutputAudioFormat.Opus, "audio/opus" },
|
||||
{ ChatOutputAudioFormat.Flac, "audio/flac" },
|
||||
{ ChatOutputAudioFormat.Pcm16, "audio/pcm16" },
|
||||
};
|
||||
|
||||
#pragma warning disable CS8618, CA1812
|
||||
private sealed class MathReasoning
|
||||
{
|
||||
@@ -2191,6 +2201,56 @@ public sealed class OpenAIChatCompletionServiceTests : IDisposable
|
||||
// The ExpiresAt value is converted to a DateTime object, so we can't directly compare it to the Unix timestamp
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(AudioMimeTypeMappingData))]
|
||||
public async Task ItMapsAudioOutputFormatToCorrectMimeTypeAsync(ChatOutputAudioFormat format, string expectedMimeType)
|
||||
{
|
||||
// Arrange
|
||||
var chatCompletion = new OpenAIChatCompletionService(modelId: "gpt-4o", apiKey: "NOKEY", httpClient: this._httpClient);
|
||||
|
||||
var responseJson = """
|
||||
{
|
||||
"model": "gpt-4o",
|
||||
"choices": [
|
||||
{
|
||||
"message": {
|
||||
"role": "assistant",
|
||||
"content": "This is the text response.",
|
||||
"audio": {
|
||||
"data": "AQIDBA=="
|
||||
}
|
||||
},
|
||||
"finish_reason": "stop"
|
||||
}
|
||||
],
|
||||
"usage": {
|
||||
"prompt_tokens": 10,
|
||||
"completion_tokens": 20,
|
||||
"total_tokens": 30
|
||||
}
|
||||
}
|
||||
""";
|
||||
|
||||
this._messageHandlerStub.ResponseToReturn = new HttpResponseMessage(HttpStatusCode.OK)
|
||||
{ Content = new StringContent(responseJson) };
|
||||
|
||||
var settings = new OpenAIPromptExecutionSettings
|
||||
{
|
||||
Modalities = ChatResponseModalities.Text | ChatResponseModalities.Audio,
|
||||
Audio = new ChatAudioOptions(ChatOutputAudioVoice.Alloy, format)
|
||||
};
|
||||
|
||||
// Act
|
||||
var result = await chatCompletion.GetChatMessageContentAsync(this._chatHistoryForTest, settings);
|
||||
|
||||
// Assert
|
||||
Assert.NotNull(result);
|
||||
Assert.Equal(2, result.Items.Count);
|
||||
var audioContent = result.Items[1] as AudioContent;
|
||||
Assert.NotNull(audioContent);
|
||||
Assert.Equal(expectedMimeType, audioContent.MimeType);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task GetChatMessageContentsThrowsExceptionWithEmptyBinaryContentAsync()
|
||||
{
|
||||
|
||||
@@ -1006,9 +1006,9 @@ internal partial class ClientCore
|
||||
return "audio/opus";
|
||||
}
|
||||
|
||||
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav)
|
||||
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Aac)
|
||||
{
|
||||
return "audio/wav";
|
||||
return "audio/aac";
|
||||
}
|
||||
|
||||
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Flac)
|
||||
@@ -1021,7 +1021,7 @@ internal partial class ClientCore
|
||||
return "audio/pcm16";
|
||||
}
|
||||
|
||||
throw new NotSupportedException($"Unsupported audio output format '{audioOptions.OutputAudioFormat}'. Supported formats are 'wav', 'mp3', 'opus', 'flac' and 'pcm16'.");
|
||||
throw new NotSupportedException($"Unsupported audio output format '{audioOptions.OutputAudioFormat}'. Supported formats are 'wav', 'mp3', 'opus', 'aac', 'flac' and 'pcm16'.");
|
||||
}
|
||||
|
||||
private OpenAIChatMessageContent CreateChatMessageContent(ChatMessageRole chatRole, string content, ChatToolCall[] toolCalls, FunctionCallContent[]? functionCalls, IReadOnlyDictionary<string, object?>? metadata, string? authorName)
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
using System;
|
||||
using System.Collections.Generic;
|
||||
using System.Linq;
|
||||
using Microsoft.SemanticKernel.Process.Internal;
|
||||
using Microsoft.SemanticKernel.Process.Models;
|
||||
|
||||
@@ -50,5 +51,9 @@ public sealed record KernelProcess : KernelProcessStepInfo
|
||||
Verify.NotNullOrWhiteSpace(state.Name);
|
||||
|
||||
this.Steps = [.. steps];
|
||||
if (threads is not null)
|
||||
{
|
||||
this.Threads = threads.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
// Copyright (c) Microsoft. All rights reserved.
|
||||
|
||||
using System.Collections.Generic;
|
||||
using Xunit;
|
||||
|
||||
namespace Microsoft.SemanticKernel.Process.UnitTests;
|
||||
|
||||
/// <summary>
|
||||
/// Unit tests for <see cref="KernelProcess"/>.
|
||||
/// </summary>
|
||||
public class KernelProcessTests
|
||||
{
|
||||
/// <summary>
|
||||
/// Verifies that the <see cref="KernelProcess"/> constructor assigns the supplied
|
||||
/// <c>threads</c> dictionary to the <see cref="KernelProcess.Threads"/> property
|
||||
/// instead of silently discarding it.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Constructor_AssignsThreadsParameter_WhenProvided()
|
||||
{
|
||||
// Arrange
|
||||
var state = new KernelProcessState(name: "TestProcess", version: "v1", id: "p1");
|
||||
var steps = new List<KernelProcessStepInfo>();
|
||||
var threads = new Dictionary<string, KernelProcessAgentThread>
|
||||
{
|
||||
["main"] = new KernelProcessAgentThread { ThreadName = "main", ThreadId = "t-1" },
|
||||
["aux"] = new KernelProcessAgentThread { ThreadName = "aux", ThreadId = "t-2" },
|
||||
};
|
||||
|
||||
// Act
|
||||
var process = new KernelProcess(state, steps, edges: null, threads: threads);
|
||||
|
||||
// Assert
|
||||
Assert.Equal(2, process.Threads.Count);
|
||||
Assert.True(process.Threads.ContainsKey("main"));
|
||||
Assert.True(process.Threads.ContainsKey("aux"));
|
||||
Assert.Equal("t-1", process.Threads["main"].ThreadId);
|
||||
Assert.Equal("t-2", process.Threads["aux"].ThreadId);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that the <see cref="KernelProcess"/> constructor leaves
|
||||
/// <see cref="KernelProcess.Threads"/> as an empty dictionary when the
|
||||
/// <c>threads</c> argument is null.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Constructor_DefaultsThreadsToEmptyDictionary_WhenNull()
|
||||
{
|
||||
// Arrange
|
||||
var state = new KernelProcessState(name: "TestProcess", version: "v1", id: "p2");
|
||||
var steps = new List<KernelProcessStepInfo>();
|
||||
|
||||
// Act
|
||||
var process = new KernelProcess(state, steps, edges: null, threads: null);
|
||||
|
||||
// Assert
|
||||
Assert.NotNull(process.Threads);
|
||||
Assert.Empty(process.Threads);
|
||||
}
|
||||
}
|
||||
@@ -202,12 +202,7 @@ public sealed partial class TextSearchStore<TKey> : ITextSearch, IDisposable
|
||||
var searchResult = await this.SearchInternalAsync(query, searchOptions, cancellationToken).ConfigureAwait(false);
|
||||
|
||||
var results = searchResult.Select(x => new TextSearchResult(x.Text ?? string.Empty) { Name = x.SourceName, Link = x.SourceLink });
|
||||
return new(searchResult.Select(x =>
|
||||
new TextSearchResult(x.Text ?? string.Empty)
|
||||
{
|
||||
Name = x.SourceName,
|
||||
Link = x.SourceLink
|
||||
}).ToAsyncEnumerable());
|
||||
return new(results.ToAsyncEnumerable());
|
||||
}
|
||||
|
||||
/// <inheritdoc/>
|
||||
|
||||
@@ -245,6 +245,37 @@ public class TextSearchStoreTests
|
||||
Assert.Equal("Sample text", actualResultsList[0]);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task GetTextSearchResultsAsyncReturnsTextSearchResults()
|
||||
{
|
||||
// Arrange
|
||||
var mockResults = new List<VectorSearchResult<TextSearchStore<string>.TextRagStorageDocument<string>>>
|
||||
{
|
||||
new(new TextSearchStore<string>.TextRagStorageDocument<string>
|
||||
{
|
||||
Text = "Sample text",
|
||||
SourceName = "src-name",
|
||||
SourceLink = "src-link",
|
||||
}, 0.9f)
|
||||
};
|
||||
|
||||
this._recordCollectionMock
|
||||
.Setup(r => r.SearchAsync("query", 3, It.IsAny<VectorSearchOptions<TextSearchStore<string>.TextRagStorageDocument<string>>>(), It.IsAny<CancellationToken>()))
|
||||
.Returns(mockResults.ToAsyncEnumerable());
|
||||
|
||||
using var store = new TextSearchStore<string>(this._vectorStoreMock.Object, "testCollection", 128);
|
||||
|
||||
// Act
|
||||
var actualResults = await store.GetTextSearchResultsAsync("query");
|
||||
|
||||
// Assert
|
||||
var actualResultsList = await actualResults.Results.ToListAsync();
|
||||
Assert.Single(actualResultsList);
|
||||
Assert.Equal("Sample text", actualResultsList[0].Value);
|
||||
Assert.Equal("src-name", actualResultsList[0].Name);
|
||||
Assert.Equal("src-link", actualResultsList[0].Link);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SearchAsyncWithHybridReturnsSearchResults()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user